Use the wrench menu as a reference view for overflowed extension actions' popups

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

 // Copyright 2014 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/extensions/extension_action_view_controller.h"
 
 #include "base/logging.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action.h"
@@ skipping 226 matching lines @@
   // If another extension action wants to show its context menu, allow it to.
   if (!followup_context_menu_task_.is_null()) {
     base::Closure task = followup_context_menu_task_;
     followup_context_menu_task_ = base::Closure();
     task.Run();
   }
 }
 
 bool ExtensionActionViewController::ShowPopupWithUrl(
     ExtensionPopup::ShowAction show_action, const GURL& popup_url) {
+  ExtensionActionViewController* preferred_controller =
+      delegate_->GetPreferredPopupViewController();
+  if (preferred_controller != this)

[Peter Kasting, 2014/09/11 20:23:12]

Can we just make callers call this method on the correct controller instead of
allowing them to call it on the wrong one and then fixing that up here?

[Devlin, 2014/09/11 20:53:42]

I think that's actually more complicated overall.  This is called from
ExecuteAction, which is, in turn, called from several different call sites
(including mouse clicks, api calls, keyboard commands, etc).  For most of those,
there's no reason to know to direct the call to a "non-overflowed" vs.
"overflowed" extension action, and for some (like mouse clicks), it's extremely
painful (we'd have to forward left-clicks to one controller, right clicks stay
in here, etc).  I'd prefer to only forward the call when we need to - which is
in this case.

[Peter Kasting, 2014/09/11 20:57:37]

I'm only suggesting hoisting this to ExecuteAction().  Basically, that function
would call "delegate_->GetPreferredPopupViewController()->ShowPopupWithUrl()"
instead of "ShowPopupWithUrl()".  Isn't that all you need?

[Devlin, 2014/09/11 21:04:07]

Ah, misunderstood.  Yes, that's fine - done.  (Only had it this way to keep all
the popup-related logic confined to one method.)

+    return preferred_controller->ShowPopupWithUrl(show_action, popup_url);
+
   // If we're already showing the popup for this browser action, just hide it
   // and return.
   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();
 
   // Similarly, don't allow a context menu and a popup to be showing
   // simultaneously.
@@ skipping 48 matching lines @@
 }
 
 void ExtensionActionViewController::CleanupPopup(bool close_widget) {
   DCHECK(popup_);
   delegate_->CleanupPopup();
   popup_->GetWidget()->RemoveObserver(this);
   if (close_widget)
     popup_->GetWidget()->Close();
   popup_ = NULL;
 }