Change glic status icon click and keyboard shortcut to toggle UI
Change clicking the status icon and invoking the global keyboard
shortcut to toggling the UI instead of just showing it. The behavior for
the menu item will still be the same - it will only show and not toggle.
Low-Coverage-Reason: TRIVIAL_CHANGE glic_background_mode_manager.cc was just changed to call a different function which is already being tested
Fixed: 392670699
Change-Id: Ie3503115a9b84906e7fd7460a8a4fa77099b55a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6215901
Reviewed-by: Ian Wells <iwells@chromium.org>
Reviewed-by: Alison Gale <agale@chromium.org>
Reviewed-by: Eshwar Stalin <estalin@chromium.org>
Commit-Queue: Charles Meng <charlesmeng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1416499}
diff --git a/chrome/browser/glic/BUILD.gn b/chrome/browser/glic/BUILD.gn
index 10e9c62..6b4788b1 100644
--- a/chrome/browser/glic/BUILD.gn
+++ b/chrome/browser/glic/BUILD.gn
@@ -223,6 +223,7 @@
":impl",
":test_support",
"//chrome/browser",
+ "//chrome/browser/glic/launcher",
"//chrome/browser/ui",
"//chrome/common",
"//chrome/test:test_support",
diff --git a/chrome/browser/glic/glic_keyed_service.cc b/chrome/browser/glic/glic_keyed_service.cc
index a9f348f2..62cf923 100644
--- a/chrome/browser/glic/glic_keyed_service.cc
+++ b/chrome/browser/glic/glic_keyed_service.cc
@@ -58,14 +58,15 @@
window_controller_->Shutdown();
}
-void GlicKeyedService::ToggleUI(BrowserWindowInterface* bwi) {
+void GlicKeyedService::ToggleUI(BrowserWindowInterface* bwi,
+ bool prevent_close) {
// Glic may be disabled for certain user profiles (the user is browsing in
// incognito or guest mode, policy, etc). In those cases, the entry points to
// this method should already have been removed.
CHECK(GlicEnabling::IsEnabledForProfile(profile_));
profile_manager_->SetActiveGlic(this);
- window_controller_->Toggle(bwi);
+ window_controller_->Toggle(bwi, prevent_close);
}
void GlicKeyedService::GuestAdded(content::WebContents* guest_contents) {
diff --git a/chrome/browser/glic/glic_keyed_service.h b/chrome/browser/glic/glic_keyed_service.h
index dd9b9b0..98d7b44 100644
--- a/chrome/browser/glic/glic_keyed_service.h
+++ b/chrome/browser/glic/glic_keyed_service.h
@@ -52,9 +52,10 @@
// KeyedService
void Shutdown() override;
- // Show, summon or activate the panel, or close it if it's already active.
- // If glic_button_view is non-null, attach the panel to that view's Browser.
- void ToggleUI(BrowserWindowInterface* bwi);
+ // Show, summon or activate the panel, or close it if it's already active and
+ // prevent_close is false. If glic_button_view is non-null, attach the panel
+ // to that view's Browser.
+ void ToggleUI(BrowserWindowInterface* bwi, bool prevent_close = false);
GlicMetrics* metrics() { return metrics_.get(); }
GlicWindowController& window_controller() { return *window_controller_; }
diff --git a/chrome/browser/glic/glic_window_controller.cc b/chrome/browser/glic/glic_window_controller.cc
index 90df0c40..aba29e4a 100644
--- a/chrome/browser/glic/glic_window_controller.cc
+++ b/chrome/browser/glic/glic_window_controller.cc
@@ -295,7 +295,8 @@
}
}
-void GlicWindowController::Toggle(BrowserWindowInterface* bwi) {
+void GlicWindowController::Toggle(BrowserWindowInterface* bwi,
+ bool prevent_close) {
// If `bwi` is non-null, the glic button was clicked on a specific window and
// glic should be attached to that window. Otherwise glic was invoked from the
// hotkey or other OS-level entrypoint.
@@ -329,6 +330,12 @@
}
}
+ auto maybe_close = [this, prevent_close] {
+ if (!prevent_close) {
+ Close();
+ }
+ };
+
// Pressing the button or the hotkey when the window is open, or waiting to
// load should close it. The latter is required because otherwise if there
// were an error loading the backend (or if it just took a long time) then the
@@ -347,7 +354,7 @@
// button click was eventually processed asynchronously after the button
// was obscured, or the user invokes the glic hotkey while glic is
// attached to the active window.
- Close();
+ maybe_close();
} else {
// Button clicked on a different browser: attach to that one.
AttachToBrowser(new_attached_browser);
@@ -363,7 +370,7 @@
if (attached_browser_) {
if (IsActive()) {
// Hotkey when glic active and attached: close.
- Close();
+ maybe_close();
return;
}
@@ -372,7 +379,7 @@
// Hotkey when glic inactive but attached to active browser: close.
// Note: this should not be possible, since if the attached browser is
// active, new_attached_browser must not have been null.
- Close();
+ maybe_close();
} else {
// Hotkey when neither attached browser nor glic are active: open
// detached.
@@ -382,7 +389,7 @@
}
// Hotkey invoked when glic is already detached.
- Close();
+ maybe_close();
} else if (state_ != State::kClosed) {
// Currently in the process of showing the widget, allow that to finish.
diff --git a/chrome/browser/glic/glic_window_controller.h b/chrome/browser/glic/glic_window_controller.h
index 531df7a..0740b12 100644
--- a/chrome/browser/glic/glic_window_controller.h
+++ b/chrome/browser/glic/glic_window_controller.h
@@ -75,8 +75,8 @@
~GlicWindowController() override;
// Show, summon, or activate the panel if needed, or close it if it's already
- // active.
- void Toggle(BrowserWindowInterface* browser);
+ // active and prevent_close is false.
+ void Toggle(BrowserWindowInterface* browser, bool prevent_close = false);
// Attaches glic to the last focused Chrome window.
void Attach();
diff --git a/chrome/browser/glic/glic_window_controller_interactive_uitest.cc b/chrome/browser/glic/glic_window_controller_interactive_uitest.cc
index f61b090..36f1278e 100644
--- a/chrome/browser/glic/glic_window_controller_interactive_uitest.cc
+++ b/chrome/browser/glic/glic_window_controller_interactive_uitest.cc
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <memory>
+
#include "base/memory/memory_pressure_monitor.h"
#include "base/path_service.h"
#include "base/run_loop.h"
@@ -14,6 +16,7 @@
#include "chrome/browser/glic/glic_view.h"
#include "chrome/browser/glic/glic_window_controller.h"
#include "chrome/browser/glic/interactive_glic_test.h"
+#include "chrome/browser/glic/launcher/glic_controller.h"
#include "chrome/browser/lifetime/application_lifetime_desktop.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
#include "chrome/browser/ui/browser_element_identifiers.h"
@@ -49,6 +52,11 @@
expect_widget, "CheckControllerHasWidget");
}
+ auto CheckControllerShowing(bool expect_showing) {
+ return CheckResult([this]() { return window_controller().IsShowing(); },
+ expect_showing, "CheckControllerShowing");
+ }
+
auto CheckControllerWidgetMode(GlicWindowMode mode) {
return CheckResult(
[this]() {
@@ -62,6 +70,14 @@
// TODO: Actually implement the hotkey when we know what it is.
return Do([this]() { window_controller().Toggle(nullptr); });
}
+
+ auto SimulateOpenMenuItem() {
+ return Do([this]() { glic_controller_->Show(); });
+ }
+
+ private:
+ std::unique_ptr<GlicController> glic_controller_ =
+ std::make_unique<GlicController>();
};
IN_PROC_BROWSER_TEST_F(GlicWindowControllerUiTest, ShowAndCloseAttachedWidget) {
@@ -229,6 +245,35 @@
CheckControllerHasWidget(false));
}
+IN_PROC_BROWSER_TEST_F(GlicWindowControllerUiTest, OpenMenuItemShows) {
+ RunTestSequence(SimulateOpenMenuItem(),
+ WaitForAndInstrumentGlic(kHostAndContents),
+ CheckControllerHasWidget(true),
+ CheckControllerWidgetMode(GlicWindowMode::kAttached),
+ CloseGlicWindow(), CheckControllerHasWidget(false));
+}
+
+IN_PROC_BROWSER_TEST_F(GlicWindowControllerUiTest,
+ OpenMenuItemWhenAttachedToActiveBrowserDoesNotClose) {
+ RunTestSequence(
+ OpenGlicWindow(GlicWindowMode::kAttached),
+ // Glic should close.
+ SetOnIncompatibleAction(OnIncompatibleAction::kSkipTest,
+ kActivateSurfaceIncompatibilityNotice),
+ ActivateSurface(kBrowserViewElementId), SimulateOpenMenuItem(),
+ CheckControllerShowing(true));
+}
+
+IN_PROC_BROWSER_TEST_F(GlicWindowControllerUiTest,
+ OpenMenuItemWhenDetachedActiveDoesNotClose) {
+ RunTestSequence(
+ OpenGlicWindow(GlicWindowMode::kDetached),
+ SetOnIncompatibleAction(OnIncompatibleAction::kIgnoreAndContinue,
+ kActivateSurfaceIncompatibilityNotice),
+ InAnyContext(ActivateSurface(test::kGlicHostElementId)),
+ SimulateOpenMenuItem(), CheckControllerShowing(true));
+}
+
class GlicWindowControllerWithMemoryPressureUiTest
: public GlicWindowControllerUiTest {
public:
diff --git a/chrome/browser/glic/launcher/BUILD.gn b/chrome/browser/glic/launcher/BUILD.gn
index 1bc5053..5431838f 100644
--- a/chrome/browser/glic/launcher/BUILD.gn
+++ b/chrome/browser/glic/launcher/BUILD.gn
@@ -71,10 +71,7 @@
testonly = true
defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
- sources = [
- "glic_background_mode_manager_browsertest.cc",
- "glic_controller_browsertest.cc",
- ]
+ sources = [ "glic_background_mode_manager_browsertest.cc" ]
deps = [
":launcher",
@@ -92,3 +89,16 @@
"//ui/base/accelerators/global_accelerator_listener",
]
}
+
+source_set("interactive_ui_tests") {
+ testonly = true
+ defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
+ sources = [ "glic_controller_interactive_uitest.cc" ]
+ deps = [
+ ":launcher",
+ "//chrome/browser/glic:glic",
+ "//chrome/browser/glic:impl",
+ "//chrome/browser/glic:test_support",
+ ]
+ data_deps = [ "//chrome/test/data/webui/glic:generate_test_files" ]
+}
diff --git a/chrome/browser/glic/launcher/glic_background_mode_manager.cc b/chrome/browser/glic/launcher/glic_background_mode_manager.cc
index bd834d6..14b86b7 100644
--- a/chrome/browser/glic/launcher/glic_background_mode_manager.cc
+++ b/chrome/browser/glic/launcher/glic_background_mode_manager.cc
@@ -69,7 +69,7 @@
const ui::Accelerator& accelerator) {
CHECK(accelerator == actual_registered_hotkey_);
CHECK(actual_registered_hotkey_ == expected_registered_hotkey_);
- controller_->Show();
+ controller_->Toggle();
}
void GlicBackgroundModeManager::ExecuteCommand(
diff --git a/chrome/browser/glic/launcher/glic_controller.cc b/chrome/browser/glic/launcher/glic_controller.cc
index 8d803236..c78d644 100644
--- a/chrome/browser/glic/launcher/glic_controller.cc
+++ b/chrome/browser/glic/launcher/glic_controller.cc
@@ -4,6 +4,7 @@
#include "chrome/browser/glic/launcher/glic_controller.h"
+#include "chrome/browser/glic/glic_keyed_service.h"
#include "chrome/browser/glic/glic_keyed_service_factory.h"
#include "chrome/browser/glic/glic_profile_manager.h"
@@ -12,7 +13,15 @@
GlicController::GlicController() = default;
GlicController::~GlicController() = default;
+void GlicController::Toggle() {
+ ToggleUI();
+}
+
void GlicController::Show() {
+ ToggleUI(/*prevent_close=*/true);
+}
+
+void GlicController::ToggleUI(bool prevent_close) {
Profile* profile =
glic::GlicProfileManager::GetInstance()->GetProfileForLaunch();
if (!profile) {
@@ -22,12 +31,10 @@
return;
}
- glic::GlicKeyedServiceFactory::GetGlicKeyedService(profile)->ToggleUI(
- nullptr);
-}
+ GlicKeyedService* glic_keyed_service =
+ glic::GlicKeyedServiceFactory::GetGlicKeyedService(profile);
-void GlicController::Hide() {
- glic::GlicProfileManager::GetInstance()->CloseGlicWindow();
+ glic_keyed_service->ToggleUI(nullptr, prevent_close);
}
} // namespace glic
diff --git a/chrome/browser/glic/launcher/glic_controller.h b/chrome/browser/glic/launcher/glic_controller.h
index 8f5cd76..ecc2ba7 100644
--- a/chrome/browser/glic/launcher/glic_controller.h
+++ b/chrome/browser/glic/launcher/glic_controller.h
@@ -17,11 +17,15 @@
GlicController(const GlicController&) = delete;
GlicController& operator=(const GlicController&) = delete;
+ // Toggles the glic UI.
+ virtual void Toggle();
+
// Shows the glic UI.
virtual void Show();
- // Hides the glic UI.
- void Hide();
+ private:
+ // Helper that implements both Toggle and Show.
+ void ToggleUI(bool prevent_close = false);
};
} // namespace glic
diff --git a/chrome/browser/glic/launcher/glic_controller_browsertest.cc b/chrome/browser/glic/launcher/glic_controller_browsertest.cc
deleted file mode 100644
index e5a349d..0000000
--- a/chrome/browser/glic/launcher/glic_controller_browsertest.cc
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright 2025 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/glic/launcher/glic_controller.h"
-
-#include "base/test/scoped_feature_list.h"
-#include "chrome/browser/glic/glic_profile_manager.h"
-#include "chrome/common/chrome_features.h"
-#include "chrome/test/base/in_process_browser_test.h"
-#include "content/public/test/browser_test.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace glic {
-
-class GlicControllerBrowserTest : public InProcessBrowserTest {
- public:
- GlicControllerBrowserTest() {
- scoped_feature_list_.InitWithFeatures(
- /*enabled_features=*/{features::kGlic, features::kTabstripComboButton},
- /*disabled_features=*/{});
- }
-
- ~GlicControllerBrowserTest() override = default;
-
- protected:
- GlicController& glic_controller() { return glic_controller_; }
-
- GlicController glic_controller_;
- base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-IN_PROC_BROWSER_TEST_F(GlicControllerBrowserTest, ShowAndHide) {
- EXPECT_FALSE(GlicProfileManager::GetInstance()->HasActiveGlicService());
-
- glic_controller().Show();
- EXPECT_TRUE(GlicProfileManager::GetInstance()->HasActiveGlicService());
-
- glic_controller().Hide();
- EXPECT_FALSE(GlicProfileManager::GetInstance()->HasActiveGlicService());
-}
-
-} // namespace glic
diff --git a/chrome/browser/glic/launcher/glic_controller_interactive_uitest.cc b/chrome/browser/glic/launcher/glic_controller_interactive_uitest.cc
new file mode 100644
index 0000000..9f57494
--- /dev/null
+++ b/chrome/browser/glic/launcher/glic_controller_interactive_uitest.cc
@@ -0,0 +1,62 @@
+// Copyright 2025 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/glic/glic_keyed_service_factory.h"
+#include "chrome/browser/glic/glic_profile_manager.h"
+#include "chrome/browser/glic/glic_window_controller.h"
+#include "chrome/browser/glic/interactive_glic_test.h"
+#include "chrome/browser/glic/launcher/glic_controller.h"
+#include "chrome/common/chrome_features.h"
+#include "content/public/test/browser_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace glic {
+
+class GlicControllerUiTest : public test::InteractiveGlicTest {
+ public:
+ GlicControllerUiTest() = default;
+ ~GlicControllerUiTest() override = default;
+
+ protected:
+ GlicController& glic_controller() { return glic_controller_; }
+
+ GlicController glic_controller_;
+};
+
+IN_PROC_BROWSER_TEST_F(GlicControllerUiTest, Toggle) {
+ Profile* profile =
+ glic::GlicProfileManager::GetInstance()->GetProfileForLaunch();
+ GlicKeyedService* glic_keyed_service =
+ glic::GlicKeyedServiceFactory::GetGlicKeyedService(profile);
+ ASSERT_FALSE(glic_keyed_service->window_controller().IsShowing());
+
+ RunTestSequence(ObserveState(test::internal::kGlicWindowControllerState,
+ std::ref(window_controller())),
+ Do([this]() { glic_controller().Toggle(); }),
+ WaitForState(test::internal::kGlicWindowControllerState,
+ GlicWindowController::State::kOpen),
+ Do([this]() { glic_controller().Toggle(); }),
+ WaitForState(test::internal::kGlicWindowControllerState,
+ GlicWindowController::State::kClosed), );
+}
+
+IN_PROC_BROWSER_TEST_F(GlicControllerUiTest, Show) {
+ Profile* profile =
+ glic::GlicProfileManager::GetInstance()->GetProfileForLaunch();
+ GlicKeyedService* glic_keyed_service =
+ glic::GlicKeyedServiceFactory::GetGlicKeyedService(profile);
+ ASSERT_FALSE(glic_keyed_service->window_controller().IsShowing());
+
+ RunTestSequence(ObserveState(test::internal::kGlicWindowControllerState,
+ std::ref(window_controller())),
+ Do([this]() { glic_controller().Show(); }),
+ WaitForState(test::internal::kGlicWindowControllerState,
+ GlicWindowController::State::kOpen),
+ Do([this]() { glic_controller().Show(); }),
+ WaitForState(test::internal::kGlicWindowControllerState,
+ GlicWindowController::State::kOpen));
+}
+
+} // namespace glic
diff --git a/chrome/browser/glic/launcher/glic_status_icon.cc b/chrome/browser/glic/launcher/glic_status_icon.cc
index b670529..a973c2c 100644
--- a/chrome/browser/glic/launcher/glic_status_icon.cc
+++ b/chrome/browser/glic/launcher/glic_status_icon.cc
@@ -90,7 +90,7 @@
}
void GlicStatusIcon::OnStatusIconClicked() {
- controller_->Show();
+ controller_->Toggle();
}
void GlicStatusIcon::ExecuteCommand(int command_id, int event_flags) {
diff --git a/chrome/browser/glic/launcher/glic_status_icon_unittest.cc b/chrome/browser/glic/launcher/glic_status_icon_unittest.cc
index 10b62ed..54b5a65 100644
--- a/chrome/browser/glic/launcher/glic_status_icon_unittest.cc
+++ b/chrome/browser/glic/launcher/glic_status_icon_unittest.cc
@@ -63,6 +63,7 @@
class MockGlicController : public GlicController {
public:
+ MOCK_METHOD0(Toggle, void());
MOCK_METHOD0(Show, void());
};
@@ -125,7 +126,7 @@
#if !BUILDFLAG(IS_LINUX)
TEST_F(GlicStatusIconTest, OnStatusIconClicked) {
- EXPECT_CALL(*glic_controller(), Show).Times(1);
+ EXPECT_CALL(*glic_controller(), Toggle).Times(1);
status_icon()->DispatchClickEvent();
}
#endif
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index 4f5f398..1e0d070c 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -11512,8 +11512,14 @@
}
if (enable_glic) {
- deps += [ "//chrome/browser/glic:interactive_ui_tests" ]
- data_deps += [ "//chrome/browser/glic:interactive_ui_tests" ]
+ deps += [
+ "//chrome/browser/glic:interactive_ui_tests",
+ "//chrome/browser/glic/launcher:interactive_ui_tests",
+ ]
+ data_deps += [
+ "//chrome/browser/glic:interactive_ui_tests",
+ "//chrome/browser/glic/launcher:interactive_ui_tests",
+ ]
}
sources += [
