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

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

Instead of opening a popup in the wrench menu, extension actions that are
overflowed should close the wrench menu and then use the wrench menu button as
a reference view for the popup.

BUG=413003
Committed: https://crrev.com/634906f9eca3cb6ba5ace8061e15d2525a5696f8
Cr-Commit-Position: refs/heads/master@{#294587}

[Devlin, 2014-09-10 23:15:10]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2014-09-10 23:15:11]

Sorry for the review bombardment. :/

[Finnur, 2014-09-11 09:17:43]

LGTM

[Devlin, 2014-09-11 17:51:26]

rdevlin.cronin@chromium.org changed reviewers:
+ pkasting@chromium.org

[Devlin, 2014-09-11 17:51:27]

Peter, mind taking a look?

[Peter Kasting, 2014-09-11 20:23:13]

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_action_view_controller.cc
(right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_action_view_controller.cc:249: if
(preferred_controller != this)
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?

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:290:
BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu();
Nit: I'd probably wrap like this to ensure the arms are aligned:

  return chevron ?
      chevron_ :
      BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu();

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:307:
GetViewForExtension(extension);
Tiny nit: Personally, I think this indentation is unusual and I'd do the
following, but up to you.

  return in_overflow_mode() ?
      main_container_->GetViewForExtension(extension) :
      GetViewForExtension(extension);

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

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_action_view_controller.cc
(right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_action_view_controller.cc:249: if
(preferred_controller != this)
On 2014/09/11 20:23:12, Peter Kasting wrote:
> 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?

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.

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:290:
BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu();
On 2014/09/11 20:23:13, Peter Kasting wrote:
> Nit: I'd probably wrap like this to ensure the arms are aligned:
> 
>   return chevron ?
>       chevron_ :
>       BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu();

Sure, done.

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:307:
GetViewForExtension(extension);
On 2014/09/11 20:23:13, Peter Kasting wrote:
> Tiny nit: Personally, I think this indentation is unusual and I'd do the
> following, but up to you.
> 
>   return in_overflow_mode() ?
>       main_container_->GetViewForExtension(extension) :
>       GetViewForExtension(extension);

I don't feel strongly, so happy to oblige. :)

[Peter Kasting, 2014-09-11 20:57:38]

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_action_view_controller.cc
(right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_action_view_controller.cc:249: if
(preferred_controller != this)
On 2014/09/11 20:53:42, Devlin wrote:
> On 2014/09/11 20:23:12, Peter Kasting wrote:
> > 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?
> 
> I think that's actually more complicated overall.  This is called from
> ExecuteAction, which is, in turn, called from several different call sites

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]

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_action_view_controller.cc
(right):

https://codereview.chromium.org/561023002/diff/1/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_action_view_controller.cc:249: if
(preferred_controller != this)
On 2014/09/11 20:57:37, Peter Kasting wrote:
> On 2014/09/11 20:53:42, Devlin wrote:
> > On 2014/09/11 20:23:12, Peter Kasting wrote:
> > > 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?
> > 
> > I think that's actually more complicated overall.  This is called from
> > ExecuteAction, which is, in turn, called from several different call sites
> 
> 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?

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

[Peter Kasting, 2014-09-11 21:07:21]

LGTM

[Devlin, 2014-09-11 22:31:00]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-11 22:33:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561023002/20002

[commit-bot: I haz the power, 2014-09-12 01:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 01:52:39]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2014-09-12 14:02:01]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-12 14:02:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561023002/20002

[commit-bot: I haz the power, 2014-09-12 14:36:36]

Committed patchset #4 (id:20002) as fcb400180390ba8182c72e7d642e24c2e3711a06

[commit-bot: I haz the power, 2014-09-12 14:44:20]

Patchset 4 (id:??) landed as
https://crrev.com/634906f9eca3cb6ba5ace8061e15d2525a5696f8
Cr-Commit-Position: refs/heads/master@{#294587}