Share popout code for ShowToolbarActionBubble

Removes ExtensionsToolbarContainer::ShowActiveBubble by using
::ShowWidgetForExtension in ::ShowToolbarActionBubble. This removes one
implementation of popping out an extension and showing a bubble anchored
to it.

Bug: chromium:1039847
Change-Id: I8f33d72082a833fbb3f427b84e3b8e247ff34b0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999445
Commit-Queue: Caroline Rising <corising@chromium.org>
Reviewed-by: Caroline Rising <corising@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731628}

chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc

@@ -110,7 +110,8 @@ class ExtensionsMenuViewBrowserTest : public DialogBrowserTest {
           GetExtensionsToolbarContainer();
       // Clicking the extension should close the extensions menu, pop out the
       // extension, and display the "reload this page" bubble.
-      EXPECT_TRUE(container->action_bubble_public_for_testing());
+      EXPECT_TRUE(container->GetAnchoredWidgetForExtensionForTesting(
+          extensions_[0]->id()));
       EXPECT_FALSE(container->GetPoppedOutAction());
       EXPECT_FALSE(ExtensionsMenuView::IsShowing());
     } else if (ui_test_name_ == "UninstallDialog_Accept" ||
@@ -280,12 +281,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
   test_dir.WriteFile(FILE_PATH_LITERAL("script.js"),
                      "console.log('injected!');");
 
-  scoped_refptr<const extensions::Extension> extension =
+  extensions_.push_back(
       extensions::ChromeTestExtensionLoader(profile()).LoadExtension(
-          test_dir.UnpackedPath());
-  ASSERT_TRUE(extension);
+          test_dir.UnpackedPath()));
+  ASSERT_EQ(1u, extensions_.size());
+  ASSERT_TRUE(extensions_.front());
 
-  extensions::ScriptingPermissionsModifier(profile(), extension)
+  extensions::ScriptingPermissionsModifier(profile(), extensions_.front())
       .SetWithholdHostPermissions(true);
 
   // Navigate to a page the extension wants to run on.
@@ -432,10 +434,13 @@ IN_PROC_BROWSER_TEST_P(ActivateWithReloadExtensionsMenuBrowserTest,
 
   TriggerSingleExtensionButton();
 
-  auto* const action_bubble = BrowserView::GetBrowserViewForBrowser(browser())
-                                  ->toolbar()
-                                  ->extensions_container()
-                                  ->action_bubble_public_for_testing();
+  auto* const action_bubble =
+      BrowserView::GetBrowserViewForBrowser(browser())
+          ->toolbar()
+          ->extensions_container()
+          ->GetAnchoredWidgetForExtensionForTesting(extensions_[0]->id())
+          ->widget_delegate()
+          ->AsDialogDelegate();
   ASSERT_TRUE(action_bubble);
 
   const bool accept_reload_dialog = GetParam();

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -74,12 +74,6 @@ ExtensionsToolbarContainer::~ExtensionsToolbarContainer() {
   // The widgets should close synchronously (resulting in OnWidgetClosing()),
   // so |anchored_widgets_| should now be empty.
   CHECK(anchored_widgets_.empty());
-
-  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() {
@@ -110,6 +104,16 @@ void ExtensionsToolbarContainer::ShowWidgetForExtension(
           weak_ptr_factory_.GetWeakPtr(), widget));
 }
 
+views::Widget*
+ExtensionsToolbarContainer::GetAnchoredWidgetForExtensionForTesting(
+    const std::string& extension_id) {
+  auto iter = std::find_if(anchored_widgets_.begin(), anchored_widgets_.end(),
+                           [extension_id](const auto& info) {
+                             return info.extension_id == extension_id;
+                           });
+  return iter->widget;
+}
+
 void ExtensionsToolbarContainer::UpdateIconVisibility(
     const std::string& extension_id) {
   auto it = icons_.find(extension_id);
@@ -169,9 +173,7 @@ bool ExtensionsToolbarContainer::IsActionVisibleOnToolbar(
   }
 
   return model_->IsActionPinned(action->GetId()) ||
-         action == popped_out_action_ ||
-         (active_bubble_ &&
-          action->GetId() == active_bubble_->GetAnchorActionId());
+         action == popped_out_action_;
 }
 
 void ExtensionsToolbarContainer::UndoPopOut() {
@@ -233,27 +235,21 @@ bool ExtensionsToolbarContainer::ShowToolbarActionPopup(
 
 void ExtensionsToolbarContainer::ShowToolbarActionBubble(
     std::unique_ptr<ToolbarActionsBarBubbleDelegate> controller) {
-  auto iter = icons_.find(controller->GetAnchorActionId());
+  const std::string extension_id = controller->GetAnchorActionId();
 
-  views::View* const anchor_view = iter != icons_.end()
-                                       ? static_cast<View*>(iter->second.get())
-                                       : extensions_button_;
+  views::View* const anchor_view = GetViewForId(extension_id);
 
-  anchor_view->SetVisible(true);
+  views::Widget* const widget = views::BubbleDialogDelegateView::CreateBubble(
+      new ToolbarActionsBarBubbleViews(
+          anchor_view ? anchor_view : extensions_button_,
+          anchor_view != nullptr, std::move(controller)));
 
-  static_cast<views::AnimatingLayoutManager*>(GetLayoutManager())
-      ->PostOrQueueAction(
-          base::BindOnce(&ExtensionsToolbarContainer::ShowActiveBubble,
-                         weak_ptr_factory_.GetWeakPtr(), anchor_view,
-                         base::Passed(std::move(controller))));
+  ShowWidgetForExtension(widget, extension_id);
 }
 
 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)));
+  ShowToolbarActionBubble(std::move(bubble));
 }
 
 void ExtensionsToolbarContainer::OnTabStripModelChanged(
@@ -489,37 +485,17 @@ void ExtensionsToolbarContainer::OnWidgetClosing(views::Widget* widget) {
   auto iter = std::find_if(
       anchored_widgets_.begin(), anchored_widgets_.end(),
       [widget](const auto& info) { return info.widget == widget; });
-  if (iter != anchored_widgets_.end()) {
-    iter->widget->RemoveObserver(this);
-    const std::string extension_id = std::move(iter->extension_id);
-    anchored_widgets_.erase(iter);
-    UpdateIconVisibility(extension_id);
-  }
-
-  if (active_bubble_ && active_bubble_->GetWidget() == widget)
-    ClearActiveBubble(widget);
+  DCHECK(iter != anchored_widgets_.end());
+  iter->widget->RemoveObserver(this);
+  const std::string extension_id = std::move(iter->extension_id);
+  anchored_widgets_.erase(iter);
+  UpdateIconVisibility(extension_id);
 }
 
 void ExtensionsToolbarContainer::OnWidgetDestroying(views::Widget* widget) {
   OnWidgetClosing(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));
-}
-
 size_t ExtensionsToolbarContainer::WidthToIconCount(int x_offset) {
   const int element_padding = GetLayoutConstant(TOOLBAR_ELEMENT_PADDING);
   size_t unclamped_count =
@@ -549,13 +525,3 @@ void ExtensionsToolbarContainer::SetExtensionIconVisibility(
       views::Button::STATE_NORMAL,
       visible ? GetExtensionIcon(extension_view) : gfx::ImageSkia());
 }
-
-void ExtensionsToolbarContainer::ShowActiveBubble(
-    views::View* anchor_view,
-    std::unique_ptr<ToolbarActionsBarBubbleDelegate> controller) {
-  active_bubble_ = new ToolbarActionsBarBubbleViews(
-      anchor_view, anchor_view != extensions_button_, std::move(controller));
-  views::BubbleDialogDelegateView::CreateBubble(active_bubble_)
-      ->AddObserver(this);
-  active_bubble_->Show();
-}

chrome/browser/ui/views/extensions/extensions_toolbar_container.h

@@ -19,7 +19,6 @@
 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
@@ -45,9 +44,6 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   ExtensionsToolbarButton* extensions_button() const {
     return extensions_button_;
   }
-  ToolbarActionsBarBubbleViews* action_bubble_public_for_testing() {
-    return active_bubble_;
-  }
   const ToolbarIconMap& icons_for_testing() const { return icons_; }
   ToolbarActionViewController* popup_owner_for_testing() {
     return popup_owner_;
@@ -62,6 +58,11 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   void ShowWidgetForExtension(views::Widget* widget,
                               const std::string& extension_id);
 
+  // Gets the widget that anchors to the extension (or is about to anchor to the
+  // extension, pending pop-out).
+  views::Widget* GetAnchoredWidgetForExtensionForTesting(
+      const std::string& extension_id);
+
   // ToolbarIconContainerView:
   void UpdateAllIcons() override;
   bool GetDropFormats(int* formats,
@@ -128,11 +129,6 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   // Posted from |ShowWidgetForExtension|.
   void AnchorAndShowWidgetImmediately(views::Widget* widget);
 
-  void ShowActiveBubble(
-      views::View* anchor_view,
-      std::unique_ptr<ToolbarActionsBarBubbleDelegate> controller);
-
-
   // Creates toolbar actions and icons corresponding to the model. This is only
   // called in the constructor or when the model initializes and should not be
   // called for subsequent changes to the model.
@@ -145,9 +141,6 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   // popped out actions, extensions button).
   void ReorderViews();
 
-  // Clears the |active_bubble_|, and unregisters the container as an observer.
-  void ClearActiveBubble(views::Widget* widget);
-
   // Utility function for going from width to icon counts.
   size_t WidthToIconCount(int x_offset);
 
@@ -200,9 +193,6 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   // 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;
-
   // The widgets currently popped out and, for each, the extension it is
   // associated with. See AnchoredWidget.
   std::vector<AnchoredWidget> anchored_widgets_;