Move ShowPopup logic from BrowserActionsContainer to BrowserActionView

chrome/browser/ui/views/toolbar/browser_action_view.cc

Index: chrome/browser/ui/views/toolbar/browser_action_view.cc
diff --git a/chrome/browser/ui/views/toolbar/browser_action_view.cc b/chrome/browser/ui/views/toolbar/browser_action_view.cc
index 149148aef0d0a9d3d01b2a1d6c1c38028d6df924..f5390cbf20f248ea162eca67040158a88a67b6af 100644
--- a/chrome/browser/ui/views/toolbar/browser_action_view.cc
+++ b/chrome/browser/ui/views/toolbar/browser_action_view.cc
@@ -4,13 +4,14 @@
 
 #include "chrome/browser/ui/views/toolbar/browser_action_view.h"
 
+#include <string>
+
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_context_menu_model.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
@@ -138,7 +139,7 @@ BrowserActionButton::BrowserActionButton(const Extension* extension,
 
 void BrowserActionButton::Destroy() {
   MaybeUnregisterExtensionCommand(false);
-
+  HidePopup();
   if (menu_runner_) {
     menu_runner_->Cancel();
     base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
@@ -176,7 +177,7 @@ void BrowserActionButton::GetAccessibleState(ui::AXViewState* state) {
 
 void BrowserActionButton::ButtonPressed(views::Button* sender,
                                         const ui::Event& event) {
-  delegate_->OnBrowserActionExecuted(this);
+  ExecuteBrowserAction();
 }
 
 void BrowserActionButton::ShowContextMenuForView(
@@ -190,7 +191,7 @@ void BrowserActionButton::ShowContextMenuForView(
 
   // Reconstructs the menu every time because the menu's contents are dynamic.
   scoped_refptr<ExtensionContextMenuModel> context_menu_contents(
-      new ExtensionContextMenuModel(extension(), browser_, delegate_));
+      new ExtensionContextMenuModel(extension(), browser_, this));
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(this, &screen_loc);
 
@@ -271,6 +272,58 @@ GURL BrowserActionButton::GetPopupUrl() {
   return (tab_id < 0) ? GURL() : browser_action_->GetPopupUrl(tab_id);
 }
 
+bool BrowserActionButton::ShowPopup(
+    ExtensionPopup::ShowAction show_action,
+    bool grant_tab_permissions) {
+  GURL popup_url;
+  if (delegate_->GetModel()->ExecuteBrowserAction(
+          extension_, browser_, &popup_url, grant_tab_permissions) ==
+      extensions::ExtensionToolbarModel::ACTION_NONE) {
+    return false;
+  }
+
+  // If we're showing the same popup, just hide it and return.

[Peter Kasting, 2014/07/26 02:33:20]

Nit: The word "same" in this sentence is slightly confusing to me.  What does it
mean to have a "same" versus a "different" popup?  Here "same" just seems to
mean "any popup".  Is that because this is a specific browser action's button? 
Would this sentence read better as "already showing the popup for this browser
action" or something similar?

[Devlin, 2014/07/29 19:07:14]

Yeah - same means "the popup for this browser action", which is a much clearer
phrasing.  Changed.

+  bool already_showing = popup_ != NULL;
+
+  // Always hide the current popup, even if it's not the same.
+  // Only one popup should be visible at a time.
+  delegate_->HideActivePopup();
+  if (already_showing)
+    return false;
+
+  // Browser actions in the overflow menu can still show popups, so we may need
+  // a reference view other than this view. If so, use the overflow view.
+  views::View* reference_view =
+      parent()->visible() ? this : delegate_->GetOverflowReferenceView();
+
+  popup_ = ExtensionPopup::ShowPopup(popup_url,
+                                     browser_,
+                                     reference_view,
+                                     views::BubbleBorder::TOP_RIGHT,
+                                     show_action);
+  popup_->GetWidget()->AddObserver(this);
+  delegate_->SetPopupOwner(this);
+
+  // Only set button as pushed if it was triggered by a user click.
+  if (grant_tab_permissions)
+    SetButtonPushed();
+  return true;
+}
+
+void BrowserActionButton::HidePopup() {
+  if (popup_) {
+    popup_->GetWidget()->RemoveObserver(this);
+    popup_->GetWidget()->Close();
+    popup_ = NULL;
+    SetButtonNotPushed();
+    delegate_->SetPopupOwner(NULL);
+  }
+}
+
+void BrowserActionButton::ExecuteBrowserAction() {
+  ShowPopup(ExtensionPopup::SHOW, true);
+}
+
 void BrowserActionButton::Observe(int type,
                                   const content::NotificationSource& source,
                                   const content::NotificationDetails& details) {
@@ -315,7 +368,7 @@ bool BrowserActionButton::Activate() {
   if (!IsPopup())
     return true;
 
-  delegate_->OnBrowserActionExecuted(this);
+  ExecuteBrowserAction();
 
   // TODO(erikkay): Run a nested modal loop while the mouse is down to
   // enable menu-like drag-select behavior.
@@ -386,7 +439,7 @@ bool BrowserActionButton::AcceleratorPressed(
       ui::AcceleratorManager::kNormalPriority)
     return false;
 
-  delegate_->OnBrowserActionExecuted(this);
+  ExecuteBrowserAction();
   return true;
 }
 
@@ -420,6 +473,20 @@ gfx::ImageSkia BrowserActionButton::GetIconForTest() {
 BrowserActionButton::~BrowserActionButton() {
 }
 
+void BrowserActionButton::InspectPopup(ExtensionAction* action) {
+  DCHECK_EQ(browser_action_, action);
+  ShowPopup(ExtensionPopup::SHOW_AND_INSPECT, true);  // grant active tab
+}
+
+void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) {

[Peter Kasting, 2014/07/26 02:33:20]

This code sure looks a lot like HidePopup().  Can we make the two into one
function and/or implement one in terms of the other?

[Devlin, 2014/07/29 19:07:14]

I'm not 100% sure.  The problem being the call to widget->Close().  If it's safe
to try and close a widget even if it's destroying, we can combine easily
(tentatively done this, but not sure it's always safe to do so).  Otherwise, we
could call OnWidgetDestroying from HidePopup(), but that seems odd if we destroy
the widget as a result of the Close() call (and I think it looks very odd to
self-call OnWidgetDestroying anywhere).  The other option is to have a
Cleanup(bool close_widget) method, but that seems a bit excessive.

Thoughts/preferences?

[Peter Kasting, 2014/07/29 19:18:01]

Widget::Close()'s definition checks whether the widget is closing and returns,
but at the same time the comments suggest that's not expected to be done
intentionally.

What if HidePopup() _only_ calls Close()?  Won't that eventually trigger
OnWidgetDestroying() to be reached, so we run the rest of this code?  Or are
there problems with that flow?

[Devlin, 2014/07/29 19:51:46]

That may still be problematic.  In particular, widget_observer.h says for
OnWidgetDestroying "This typically occurs asynchronously with respect the the
close request" (side note: we should fix that typo).  Since it occurs async, we
might think there's a popup when, for all intents and purposes, there isn't.

[Peter Kasting, 2014/07/29 19:56:05]

There isn't from the user's perspective, but there is from the code's
perspective.  What are the concrete potential negative consequences?  I'd
imagine that, as long as we're not going to deref NULL somewhere or something,
we would probably be OK.

[Devlin, 2014/07/29 19:59:48]

In theory, if we have a popup showing, and then we double click the associated
browser action, the popup should close and re-open.  If we wait for the async
close, then we would only close (and then try to close again).  Certainly not a
crash, but still a negative consequence.

[Peter Kasting, 2014/07/29 20:03:02]

The async close should happen with the next message loop iteration, no?  I'd
think it would finish before the user could manage to click again, in most
cases.

I think you should bring sky into these discussions; he understands widget
closure better than I do.

[Devlin, 2014/07/29 21:25:43]

- PatchSet 2 (just calling HidePopup() from OnWidgetDestroyed) won't work,
because Scott confirms that it's a bad idea to call Close() from
OnWidgetDestroyed() (asked/answered offline - only definitely safe if it was
triggered from a Widget::Close()).
- Having HidePopup() only call widget->Close() also won't work well, because
this messes up the BrowserActionsContainer's idea of popup owner, since we can't
have delegate_->SetPopupOwner(NULL) happen after ShowPopup() finishes, which
will be the case in an async pattern. (Didn't notice this one before).

Having a few lines of duplicate code (as in PatchSet 1) will work, as would
having a third function which takes an argument "bool close_widget".

Thoughts?

[Devlin, 2014/07/29 21:45:04]

Done!

+  DCHECK(popup_);
+  DCHECK_EQ(popup_->GetWidget(), widget);
+  popup_->GetWidget()->RemoveObserver(this);
+  popup_ = NULL;
+  SetButtonNotPushed();
+  delegate_->SetPopupOwner(NULL);
+}
+
 void BrowserActionButton::MaybeRegisterExtensionCommand() {
   extensions::CommandService* command_service =
       extensions::CommandService::Get(browser_->profile());