Make a ShowExtensionActionPopup function

Make a ShowExtensionActionPopup function

More-or-less consolidate calls to show an ExtensionAction's
popup into ExtensionActionAPI. This has the happy effect of
reducing the amount of piping we have to do in the UI code
(cutting a few steps out of the Browser -> BrowserWindow ->
Toolbar -> BrowserActionsContainer -> BrowserActionView chain),
and, more importantly, lets page action command executions
happen in the BrowserActionsContainer when the redesign is
enabled.

Hopefully, this means that we can stop showing page actions
in the location bar with the switch enabled - but that's
another patch.

BUG=397259
TBR=sky@chromium.org (minor changes to test_browser_window)

Committed: https://crrev.com/9a1800e4df575c7f7035cfbfddb84dcf879d217c
Cr-Commit-Position: refs/heads/master@{#292462}

[Devlin, 2014-08-26 23:07:27]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-08-26 23:07:34]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-08-26 23:07:40]

Patchset #1 (id:40001) has been deleted

[Devlin, 2014-08-26 23:57:54]

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

[Devlin, 2014-08-26 23:57:54]

Hey Finnur - sorry to spam you with reviews.  And I promise this one's not as
scary as it looks!

[Finnur, 2014-08-27 11:52:50]

> Hey Finnur - sorry to spam you with reviews.  

No worries, happy to see the progress.

> And I promise this one's not as scary as it looks!

Jeez, you nearly gave me a heart attack there! :)

[Finnur, 2014-08-27 11:53:02]

And now, with comments...

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:855: //
We only allow the a popup in the active window.
s/the a/the/

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:856:
Browser* browser = chrome::FindLastActiveWithProfile(
Hmmm... is there no way around looking up the browser object?

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.h
(right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.h:92: //
|grant_active_tab_permissions|.
Hmm... I wonder if we should just duplicate the comment for it here, since
|grant_active_tab_permissions| is important to get right (to avoid security
flaws).

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:122: // popup for the given
|extension|.
Definitely comment |grant_active_tab|.

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:95: bool
PageActionDecoration::ActivatePageAction(bool grant_active_tab) {
Does anyone check the return value from this?

[Devlin, 2014-08-27 15:43:22]

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:855: //
We only allow the a popup in the active window.
On 2014/08/27 11:53:02, Finnur wrote:
> s/the a/the/

Done.

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:856:
Browser* browser = chrome::FindLastActiveWithProfile(
On 2014/08/27 11:53:02, Finnur wrote:
> Hmmm... is there no way around looking up the browser object?

Sadly, I don't think so, no :(  I think there are a few flaws of the
chrome.browserAction.showPopup() function, one of which is that it doesn't take
a tab id and simply tries to open in the active browser, whichever that happens
to be.  Since it can be called from any context, I think the only way to find
the active browser is to look it up directly.

This is pretty similar to the old behavior, in which we'd ask every single
browser (indirectly, via a BrowserActionsContainer) to display the popup, and it
would iff it was the active browser.

Of course, I'm constantly discovering new functions here.  Is there an easier
way to find the active Browser?

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.h
(right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.h:92: //
|grant_active_tab_permissions|.
On 2014/08/27 11:53:02, Finnur wrote:
> Hmm... I wonder if we should just duplicate the comment for it here, since
> |grant_active_tab_permissions| is important to get right (to avoid security
> flaws).

Done.

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:122: // popup for the given
|extension|.
On 2014/08/27 11:53:02, Finnur wrote:
> Definitely comment |grant_active_tab|.

Done.

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/489183005/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:95: bool
PageActionDecoration::ActivatePageAction(bool grant_active_tab) {
On 2014/08/27 11:53:02, Finnur wrote:
> Does anyone check the return value from this?

Yep - this is called from LocationBarViewMac::ShowPageActionPopup(), which, in
turn, is checked by ExtensionActionAPI::ShowExtensionActionPopup().

[Finnur, 2014-08-27 15:46:42]

Fair enough. LGTM

[Devlin, 2014-08-27 15:49:59]

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

[Devlin, 2014-08-27 15:49:59]

+Peter for c/b/ui/[^cocoa]
+Avi for c/b/ui/cocoa

[Peter Kasting, 2014-08-27 19:41:19]

LGTM

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:907: const
Extension* extension, bool grant_active_tab) {
Nit: One arg per line

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:914:
ExtensionPopup::SHOW, grant_active_tab) : false;
Nit: This seems a little briefer:

  return view && view->view_controller()->ExecuteAction(ExtensionPopup::SHOW,
                                                        grant_active_tab);

[Devlin, 2014-08-27 20:47:16]

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:907: const
Extension* extension, bool grant_active_tab) {
On 2014/08/27 19:41:19, Peter Kasting wrote:
> Nit: One arg per line

Done.

https://codereview.chromium.org/489183005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:914:
ExtensionPopup::SHOW, grant_active_tab) : false;
On 2014/08/27 19:41:19, Peter Kasting wrote:
> Nit: This seems a little briefer:
> 
>   return view && view->view_controller()->ExecuteAction(ExtensionPopup::SHOW,
>                                                         grant_active_tab);

Done.

[Avi (use Gerrit), 2014-08-28 17:49:28]

Cocoa stuff LGTM

[Devlin, 2014-08-28 19:07:26]

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

[Devlin, 2014-08-28 19:07:30]

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

[commit-bot: I haz the power, 2014-08-28 19:09:22]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/489183005/...

[commit-bot: I haz the power, 2014-08-28 20:10:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-28 21:07:12]

Committed patchset #3 (id:100001) as 176d5cfaa8edbc502c146771fc393597a073dfd5

[commit-bot: I haz the power, 2014-09-10 03:02:14]

Patchset 3 (id:??) landed as
https://crrev.com/9a1800e4df575c7f7035cfbfddb84dcf879d217c
Cr-Commit-Position: refs/heads/master@{#292462}