Show bubbles for ExtensionsToolbarContainer
Fixes loading of extensions that require activation and page reload to
work. Showing a confirmation bubble is necessary for a user to be able
to accept reloading the page so that the extension can inject on page
load.
Bug: chromium:959930
Change-Id: I7a0563eb5cb5fe46496b8ea12ce30d68d1467641
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1641550
Commit-Queue: Peter Boström <[email protected]>
Reviewed-by: Devlin <[email protected]>
Reviewed-by: Elly Fong-Jones <[email protected]>
Cr-Commit-Position: refs/heads/master@{#669367}
diff --git a/chrome/browser/extensions/extension_action_runner.cc b/chrome/browser/extensions/extension_action_runner.cc
index eb0d4128..ced1a58b 100644
--- a/chrome/browser/extensions/extension_action_runner.cc
+++ b/chrome/browser/extensions/extension_action_runner.cc
@@ -373,18 +373,18 @@
const base::Callback<void(ToolbarActionsBarBubbleDelegate::CloseAction)>&
callback) {
Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
- ToolbarActionsBar* toolbar_actions_bar =
- browser ? browser->window()->GetToolbarActionsBar() : nullptr;
- if (toolbar_actions_bar) {
- if (default_bubble_close_action_for_testing_) {
- base::ThreadTaskRunnerHandle::Get()->PostTask(
- FROM_HERE,
- base::BindOnce(callback, *default_bubble_close_action_for_testing_));
- } else {
- toolbar_actions_bar->ShowToolbarActionBubble(
- std::make_unique<BlockedActionBubbleDelegate>(callback,
- extension->id()));
- }
+ ExtensionsContainer* const extensions_container =
+ browser ? browser->window()->GetExtensionsContainer() : nullptr;
+ if (!extensions_container)
+ return;
+ if (default_bubble_close_action_for_testing_) {
+ base::ThreadTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE,
+ base::BindOnce(callback, *default_bubble_close_action_for_testing_));
+ } else {
+ extensions_container->ShowToolbarActionBubble(
+ std::make_unique<BlockedActionBubbleDelegate>(callback,
+ extension->id()));
}
}
diff --git a/chrome/browser/ui/browser_window.h b/chrome/browser/ui/browser_window.h
index 3acdf53..f0c296f 100644
--- a/chrome/browser/ui/browser_window.h
+++ b/chrome/browser/ui/browser_window.h
@@ -35,6 +35,7 @@
class Browser;
class DownloadShelf;
class ExclusiveAccessContext;
+class ExtensionsContainer;
class FindBar;
class GURL;
class LocationBar;
@@ -276,8 +277,13 @@
virtual void FocusToolbar() = 0;
// Returns the ToolbarActionsBar associated with the window, if any.
+ // TODO(pbos): Replace usages of |GetToolbarActionsBar| with
+ // |GetExtensionsContainer|.
virtual ToolbarActionsBar* GetToolbarActionsBar() = 0;
+ // Returns the ExtensionsContainer associated with the window, if any.
+ virtual ExtensionsContainer* GetExtensionsContainer() = 0;
+
// Called from toolbar subviews during their show/hide animations.
virtual void ToolbarSizeChanged(bool is_animating) = 0;
diff --git a/chrome/browser/ui/extensions/extensions_container.h b/chrome/browser/ui/extensions/extensions_container.h
index 1f54184e0..46d643e 100644
--- a/chrome/browser/ui/extensions/extensions_container.h
+++ b/chrome/browser/ui/extensions/extensions_container.h
@@ -10,6 +10,7 @@
#include "base/callback_forward.h"
class ToolbarActionViewController;
+class ToolbarActionsBarBubbleDelegate;
// An interface for containers in the toolbar that host extensions.
class ExtensionsContainer {
@@ -48,6 +49,14 @@
virtual void PopOutAction(ToolbarActionViewController* action,
bool is_sticky,
const base::Closure& closure) = 0;
+
+ // Displays the given |bubble| once the toolbar is no longer animating.
+ virtual void ShowToolbarActionBubble(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) = 0;
+
+ // Same as above, but uses PostTask() in all cases.
+ virtual void ShowToolbarActionBubbleAsync(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) = 0;
};
#endif // CHROME_BROWSER_UI_EXTENSIONS_EXTENSIONS_CONTAINER_H_
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.h b/chrome/browser/ui/toolbar/toolbar_actions_bar.h
index e46bf85..8ceb350c 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.h
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.h
@@ -193,13 +193,6 @@
void AddObserver(ToolbarActionsBarObserver* observer);
void RemoveObserver(ToolbarActionsBarObserver* observer);
- // Displays the given |bubble| once the toolbar is no longer animating.
- void ShowToolbarActionBubble(
- std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble);
- // Same as above, but uses PostTask() in all cases.
- void ShowToolbarActionBubbleAsync(
- std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble);
-
// Returns the underlying toolbar actions, but does not order them. Primarily
// for use in testing.
const ToolbarActions& toolbar_actions_unordered() const {
@@ -248,6 +241,10 @@
void PopOutAction(ToolbarActionViewController* action,
bool is_sticky,
const base::Closure& closure) override;
+ void ShowToolbarActionBubble(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) override;
+ void ShowToolbarActionBubbleAsync(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) override;
private:
// Returns the insets by which the icon area bounds (See GetIconAreaRect())
diff --git a/chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc b/chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc
index 64368f8..50c92f8 100644
--- a/chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc
+++ b/chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc
@@ -9,17 +9,24 @@
#include "base/path_service.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/extensions/chrome_test_extension_loader.h"
+#include "chrome/browser/extensions/extension_action_runner.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/extensions/extensions_menu_button.h"
#include "chrome/browser/ui/views/extensions/extensions_toolbar_button.h"
#include "chrome/browser/ui/views/extensions/extensions_toolbar_container.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/webui_url_constants.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
+#include "net/dns/mock_host_resolver.h"
#include "ui/views/test/button_test_api.h"
#include "ui/views/test/widget_test.h"
+#include "ui/views/window/dialog_client_view.h"
class ExtensionsMenuViewBrowserTest : public DialogBrowserTest {
protected:
@@ -36,6 +43,11 @@
DialogBrowserTest::SetUp();
}
+ void SetUpOnMainThread() override {
+ DialogBrowserTest::SetUpOnMainThread();
+ host_resolver()->AddRule("*", "127.0.0.1");
+ }
+
void ShowUi(const std::string& name) override {
ui::MouseEvent click_event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
base::TimeTicks(), ui::EF_LEFT_MOUSE_BUTTON, 0);
@@ -130,7 +142,7 @@
}
IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
- TriggeringExtensionClosesMenu) {
+ ActivationWithReloadNeeded_Accept) {
LoadTestExtension("extensions/trigger_actions/browser_action");
ShowUi("");
VerifyUi();
@@ -186,5 +198,68 @@
browser()->tab_strip_model()->GetActiveWebContents()->GetVisibleURL());
}
+class ActivateWithReloadExtensionsMenuBrowserTest
+ : public ExtensionsMenuViewBrowserTest,
+ public ::testing::WithParamInterface<bool> {};
+
+IN_PROC_BROWSER_TEST_P(ActivateWithReloadExtensionsMenuBrowserTest,
+ ActivateWithReload) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ LoadTestExtension("extensions/blocked_actions/content_scripts");
+ auto extension = extensions_.back();
+ extensions::ScriptingPermissionsModifier modifier(browser()->profile(),
+ extension);
+ modifier.SetWithholdHostPermissions(true);
+
+ ui_test_utils::NavigateToURL(
+ browser(), embedded_test_server()->GetURL("example.com", "/empty.html"));
+
+ ShowUi("");
+ VerifyUi();
+
+ content::WebContents* web_contents =
+ browser()->tab_strip_model()->GetActiveWebContents();
+
+ extensions::ExtensionActionRunner* action_runner =
+ extensions::ExtensionActionRunner::GetForWebContents(web_contents);
+
+ EXPECT_TRUE(action_runner->WantsToRun(extension.get()));
+
+ TriggerSingleExtensionButton();
+
+ auto* const action_bubble = BrowserView::GetBrowserViewForBrowser(browser())
+ ->toolbar()
+ ->extensions_container()
+ ->action_bubble_public_for_testing();
+ ASSERT_TRUE(action_bubble);
+
+ views::DialogClientView* const dialog_client_view =
+ action_bubble->GetDialogClientView();
+
+ const bool accept_reload_dialog = GetParam();
+ if (accept_reload_dialog) {
+ content::TestNavigationObserver observer(web_contents);
+ dialog_client_view->AcceptWindow();
+ EXPECT_TRUE(web_contents->IsLoading());
+ // Wait for reload to finish.
+ observer.WaitForNavigationFinished();
+ EXPECT_TRUE(observer.last_navigation_succeeded());
+ // After reload the extension should be allowed to run.
+ EXPECT_FALSE(action_runner->WantsToRun(extension.get()));
+ } else {
+ dialog_client_view->CancelWindow();
+ EXPECT_FALSE(web_contents->IsLoading());
+ EXPECT_TRUE(action_runner->WantsToRun(extension.get()));
+ }
+}
+
+INSTANTIATE_TEST_SUITE_P(AcceptDialog,
+ ActivateWithReloadExtensionsMenuBrowserTest,
+ testing::Values(true));
+
+INSTANTIATE_TEST_SUITE_P(CancelDialog,
+ ActivateWithReloadExtensionsMenuBrowserTest,
+ testing::Values(false));
+
// TODO(pbos): Add test coverage that makes sure removing popped-out extensions
// properly disposes of the popup.
diff --git a/chrome/browser/ui/views/extensions/extensions_toolbar_container.cc b/chrome/browser/ui/views/extensions/extensions_toolbar_container.cc
index b8676d1..d963686 100644
--- a/chrome/browser/ui/views/extensions/extensions_toolbar_container.cc
+++ b/chrome/browser/ui/views/extensions/extensions_toolbar_container.cc
@@ -9,6 +9,8 @@
#include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
#include "chrome/browser/ui/views/extensions/extensions_menu_view.h"
#include "chrome/browser/ui/views/extensions/extensions_toolbar_button.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
+#include "ui/views/widget/widget_observer.h"
ExtensionsToolbarContainer::ExtensionsToolbarContainer(Browser* browser)
: ToolbarIconContainerView(/*uses_highlight=*/true),
@@ -21,7 +23,13 @@
CreateActions();
}
-ExtensionsToolbarContainer::~ExtensionsToolbarContainer() = default;
+ExtensionsToolbarContainer::~ExtensionsToolbarContainer() {
+ if (active_bubble_)
+ active_bubble_->GetWidget()->Close();
+ // We should synchronously receive the OnWidgetClosing() event, so we should
+ // always have cleared the active bubble by now.
+ DCHECK(!active_bubble_);
+}
void ExtensionsToolbarContainer::UpdateAllIcons() {
extensions_button_->UpdateIcon();
@@ -44,7 +52,9 @@
bool ExtensionsToolbarContainer::IsActionVisibleOnToolbar(
const ToolbarActionViewController* action) const {
return model_->IsActionPinned(action->GetId()) ||
- action == popped_out_action_;
+ action == popped_out_action_ ||
+ (active_bubble_ &&
+ action->GetId() == active_bubble_->GetAnchorActionId());
}
void ExtensionsToolbarContainer::UndoPopOut() {
@@ -92,6 +102,34 @@
closure.Run();
}
+void ExtensionsToolbarContainer::ShowToolbarActionBubble(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> controller) {
+ // TODO(pbos): Make sure we finish animations before showing the bubble.
+
+ auto iter = icons_.find(controller->GetAnchorActionId());
+
+ views::View* const anchor_view = iter != icons_.end()
+ ? static_cast<View*>(iter->second.get())
+ : extensions_button_;
+
+ anchor_view->SetVisible(true);
+
+ active_bubble_ = new ToolbarActionsBarBubbleViews(
+ anchor_view, gfx::Point(), anchor_view != extensions_button_,
+ std::move(controller));
+ views::BubbleDialogDelegateView::CreateBubble(active_bubble_)
+ ->AddObserver(this);
+ active_bubble_->Show();
+}
+
+void ExtensionsToolbarContainer::ShowToolbarActionBubbleAsync(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) {
+ base::ThreadTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE,
+ base::BindOnce(&ExtensionsToolbarContainer::ShowToolbarActionBubble,
+ weak_ptr_factory_.GetWeakPtr(), std::move(bubble)));
+}
+
void ExtensionsToolbarContainer::OnToolbarActionAdded(
const ToolbarActionsModel::ActionId& action_id,
int index) {
@@ -219,3 +257,27 @@
// TODO(pbos): Implement
return false;
}
+
+void ExtensionsToolbarContainer::OnWidgetClosing(views::Widget* widget) {
+ ClearActiveBubble(widget);
+}
+
+void ExtensionsToolbarContainer::OnWidgetDestroying(views::Widget* widget) {
+ ClearActiveBubble(widget);
+}
+
+void ExtensionsToolbarContainer::ClearActiveBubble(views::Widget* widget) {
+ DCHECK(active_bubble_);
+ DCHECK_EQ(active_bubble_->GetWidget(), widget);
+ ToolbarActionViewController* const action =
+ GetActionForId(active_bubble_->GetAnchorActionId());
+ // TODO(pbos): Note that this crashes if a bubble anchors to the menu and not
+ // to an extension that gets popped out. This should be fixed, but a test
+ // should first be added to make sure that it's covered.
+ CHECK(action);
+ active_bubble_ = nullptr;
+ widget->RemoveObserver(this);
+ // Note that we only hide this view if it's not visible for other reasons
+ // than displaying the bubble.
+ icons_[action->GetId()]->SetVisible(IsActionVisibleOnToolbar(action));
+}
diff --git a/chrome/browser/ui/views/extensions/extensions_toolbar_container.h b/chrome/browser/ui/views/extensions/extensions_toolbar_container.h
index 919e980..3998230 100644
--- a/chrome/browser/ui/views/extensions/extensions_toolbar_container.h
+++ b/chrome/browser/ui/views/extensions/extensions_toolbar_container.h
@@ -9,6 +9,7 @@
#include <memory>
#include <vector>
+#include "base/optional.h"
#include "chrome/browser/ui/extensions/extensions_container.h"
#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
#include "chrome/browser/ui/views/toolbar/toolbar_action_view.h"
@@ -17,6 +18,7 @@
class Browser;
class ExtensionsToolbarButton;
class ToolbarActionViewController;
+class ToolbarActionsBarBubbleViews;
// Container for extensions shown in the toolbar. These include pinned
// extensions and extensions that are 'popped out' transitively to show dialogs
@@ -29,7 +31,8 @@
class ExtensionsToolbarContainer : public ToolbarIconContainerView,
public ExtensionsContainer,
public ToolbarActionsModel::Observer,
- public ToolbarActionView::Delegate {
+ public ToolbarActionView::Delegate,
+ public views::WidgetObserver {
public:
explicit ExtensionsToolbarContainer(Browser* browser);
~ExtensionsToolbarContainer() override;
@@ -38,6 +41,10 @@
return extensions_button_;
}
+ ToolbarActionsBarBubbleViews* action_bubble_public_for_testing() {
+ return active_bubble_;
+ }
+
// ToolbarIconContainerView:
void UpdateAllIcons() override;
@@ -54,6 +61,9 @@
// popped out actions, extensions button).
void ReorderViews();
+ // Clears the |active_bubble_|, and unregisters the container as an observer.
+ void ClearActiveBubble(views::Widget* widget);
+
// ExtensionsContainer:
ToolbarActionViewController* GetActionForId(
const std::string& action_id) override;
@@ -67,6 +77,10 @@
void PopOutAction(ToolbarActionViewController* action,
bool is_sticky,
const base::Closure& closure) override;
+ void ShowToolbarActionBubble(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) override;
+ void ShowToolbarActionBubbleAsync(
+ std::unique_ptr<ToolbarActionsBarBubbleDelegate> bubble) override;
// ToolbarActionsModel::Observer:
void OnToolbarActionAdded(const ToolbarActionsModel::ActionId& action_id,
@@ -97,6 +111,10 @@
const gfx::Point& press_pt,
const gfx::Point& p) override;
+ // views::WidgetObserver:
+ void OnWidgetClosing(views::Widget* widget) override;
+ void OnWidgetDestroying(views::Widget* widget) override;
+
Browser* const browser_;
ToolbarActionsModel* const model_;
ScopedObserver<ToolbarActionsModel, ToolbarActionsModel::Observer>
@@ -116,6 +134,11 @@
// The action that triggered the current popup, if any.
ToolbarActionViewController* popup_owner_ = nullptr;
+ // The extension bubble that is actively showing, if any.
+ ToolbarActionsBarBubbleViews* active_bubble_ = nullptr;
+
+ base::WeakPtrFactory<ExtensionsToolbarContainer> weak_ptr_factory_{this};
+
DISALLOW_COPY_AND_ASSIGN(ExtensionsToolbarContainer);
};
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index c90d55c..77939df 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -83,6 +83,7 @@
#include "chrome/browser/ui/views/download/download_shelf_view.h"
#include "chrome/browser/ui/views/exclusive_access_bubble_views.h"
#include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
+#include "chrome/browser/ui/views/extensions/extensions_toolbar_container.h"
#include "chrome/browser/ui/views/find_bar_host.h"
#include "chrome/browser/ui/views/frame/app_menu_button.h"
#include "chrome/browser/ui/views/frame/browser_view_layout.h"
@@ -1167,6 +1168,12 @@
return container ? container->toolbar_actions_bar() : nullptr;
}
+ExtensionsContainer* BrowserView::GetExtensionsContainer() {
+ if (toolbar_ && toolbar_->extensions_container())
+ return toolbar_->extensions_container();
+ return GetToolbarActionsBar();
+}
+
void BrowserView::ToolbarSizeChanged(bool is_animating) {
if (is_animating)
contents_web_view_->SetFastResize(true);
diff --git a/chrome/browser/ui/views/frame/browser_view.h b/chrome/browser/ui/views/frame/browser_view.h
index fcfaa4fa..b2ddd63 100644
--- a/chrome/browser/ui/views/frame/browser_view.h
+++ b/chrome/browser/ui/views/frame/browser_view.h
@@ -347,6 +347,7 @@
void ResetToolbarTabState(content::WebContents* contents) override;
void FocusToolbar() override;
ToolbarActionsBar* GetToolbarActionsBar() override;
+ ExtensionsContainer* GetExtensionsContainer() override;
void ToolbarSizeChanged(bool is_animating) override;
void TabDraggingStatusChanged(bool is_dragging) override;
void FocusAppMenu() override;
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
index f6e7ff6..741dc25 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
@@ -54,6 +54,10 @@
GetWidget()->Show();
}
+std::string ToolbarActionsBarBubbleViews::GetAnchorActionId() {
+ return delegate_->GetAnchorActionId();
+}
+
std::unique_ptr<views::View> ToolbarActionsBarBubbleViews::CreateExtraView() {
std::unique_ptr<ToolbarActionsBarBubbleDelegate::ExtraViewInfo>
extra_view_info = delegate_->GetExtraViewInfo();
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h
index 89cb1c8..1e105f7 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h
@@ -32,6 +32,7 @@
~ToolbarActionsBarBubbleViews() override;
void Show();
+ std::string GetAnchorActionId();
const views::Label* body_text() const { return body_text_; }
const views::Label* item_list() const { return item_list_; }
