Move ShowPopup logic from BrowserActionsContainer to BrowserActionView

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/extensions/accelerator_priority.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "extensions/common/extension.h"
@@ skipping 107 matching lines @@
   // to GTK requires that we regrab our browser action images.
   registrar_.Add(
       this,
       chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
       content::Source<ThemeService>(
           ThemeServiceFactory::GetForProfile(browser->profile())));
 }
 
 void BrowserActionButton::Destroy() {
   MaybeUnregisterExtensionCommand(false);
-
+  HidePopup();
   if (menu_runner_) {
     menu_runner_->Cancel();
     base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   } else {
     delete this;
   }
 }
 
 void BrowserActionButton::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
@@ skipping 17 matching lines @@
   return true;
 }
 
 void BrowserActionButton::GetAccessibleState(ui::AXViewState* state) {
   views::MenuButton::GetAccessibleState(state);
   state->role = ui::AX_ROLE_BUTTON;
 }
 
 void BrowserActionButton::ButtonPressed(views::Button* sender,
                                         const ui::Event& event) {
-  delegate_->OnBrowserActionExecuted(this);
+  ExecuteBrowserAction();
 }
 
 void BrowserActionButton::ShowContextMenuForView(
     View* source,
     const gfx::Point& point,
     ui::MenuSourceType source_type) {
   if (!extension()->ShowConfigureContextMenus())
     return;
 
   SetButtonPushed();
 
   // 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);
 
   views::Widget* parent = NULL;
   int run_types = views::MenuRunner::HAS_MNEMONICS |
                   views::MenuRunner::CONTEXT_MENU;
   if (delegate_->ShownInsideMenu()) {
     run_types |= views::MenuRunner::IS_NESTED;
     // RunMenuAt expects a nested menu to be parented by the same widget as the
     // already visible menu, in this case the Chrome menu.
@@ skipping 60 matching lines @@
 bool BrowserActionButton::IsPopup() {
   int tab_id = delegate_->GetCurrentTabId();
   return (tab_id < 0) ? false : browser_action_->HasPopup(tab_id);
 }
 
 GURL BrowserActionButton::GetPopupUrl() {
   int tab_id = delegate_->GetCurrentTabId();
   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) {
   switch (type) {
     case chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_UPDATED:
       UpdateState();
       // The browser action may have become visible/hidden so we need to make
       // sure the state gets updated.
       delegate_->OnBrowserActionVisibilityChanged();
       break;
@@ skipping 24 matching lines @@
 void BrowserActionButton::OnIconUpdated() {
   UpdateState();
   if (icon_observer_)
     icon_observer_->OnIconUpdated(GetIconWithBadge());
 }
 
 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.
 
   // The return value of this method is returned via OnMousePressed.
   // We need to return false here since we're handing off focus to another
   // widget/view, and true will grab it right back and try to send events
   // to us.
   return false;
 }
@@ skipping 50 matching lines @@
 }
 
 bool BrowserActionButton::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   // Normal priority shortcuts must be handled via standard browser commands to
   // be processed at the proper time.
   if (GetAcceleratorPriority(accelerator, extension_) ==
       ui::AcceleratorManager::kNormalPriority)
     return false;
 
-  delegate_->OnBrowserActionExecuted(this);
+  ExecuteBrowserAction();
   return true;
 }
 
 void BrowserActionButton::SetButtonPushed() {
   SetState(views::CustomButton::STATE_PRESSED);
   menu_visible_ = true;
 }
 
 void BrowserActionButton::SetButtonNotPushed() {
   SetState(views::CustomButton::STATE_NORMAL);
@@ skipping 13 matching lines @@
   return browser_action_->GetIconWithBadge(icon, tab_id, spacing);
 }
 
 gfx::ImageSkia BrowserActionButton::GetIconForTest() {
   return GetImage(views::Button::STATE_NORMAL);
 }
 
 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());
   extensions::Command browser_action_command;
   if (command_service->GetBrowserActionCommand(
           extension_->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
     keybinding_.reset(new ui::Accelerator(
@@ skipping 16 matching lines @@
   extensions::Command browser_action_command;
   if (!only_if_active || !command_service->GetBrowserActionCommand(
           extension_->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
     GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
     keybinding_.reset(NULL);
   }
 }