Close wrench menu when Media Router Action clicked in Views.

Close wrench menu when Media Router Action clicked in Views.

Currently, on |ExecuteAction|, the Media Router dialog is opened and the wrench menu stays open. The menu must be closed with a second click. With this change, the wrench menu closes automatically.

This change also adds a platform delegate to the Media Router Action for platform dependent implementations.

This CL does not include the Cocoa implementation.

BUG=508546
Committed: https://crrev.com/4b16903562a662feaecee4fc7be7092866136b67
Cr-Commit-Position: refs/heads/master@{#339457}

[apacible, 2015-07-09 23:54:12]

Patchset #1 (id:1) has been deleted

[apacible, 2015-07-10 01:56:56]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-07-13 21:11:38]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-07-13 21:42:23]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-07-13 21:49:38]

Patchset #1 (id:80001) has been deleted

[apacible, 2015-07-14 00:09:41]

Patchset #2 (id:120001) has been deleted

[apacible, 2015-07-14 00:09:47]

Patchset #2 (id:120001) has been deleted

[apacible, 2015-07-14 00:51:02]

Patchset #2 (id:50010) has been deleted

[apacible, 2015-07-14 02:17:05]

Patchset #1 (id:100001) has been deleted

[apacible, 2015-07-14 02:17:10]

Patchset #1 (id:150001) has been deleted

[apacible, 2015-07-14 02:41:06]

Patchset #1 (id:170001) has been deleted

[apacible, 2015-07-14 02:56:18]

Patchset #2 (id:210001) has been deleted

[apacible, 2015-07-14 03:31:03]

Patchset #1 (id:190001) has been deleted

[apacible, 2015-07-14 15:17:52]

Patchset #1 (id:230001) has been deleted

[apacible, 2015-07-14 15:22:49]

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

[apacible, 2015-07-14 15:23:47]

PTAL, thanks!

[Devlin, 2015-07-14 17:23:02]

Also, this seems like the type of thing that there should be a BUG= for, and
probably a test as well. :)

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:8: #include
"chrome/browser/ui/browser.h"
Doesn't actually look like you need this.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:97:
platform_delegate_.get()->CloseOverflowMenu();
no need for the .get().  Really, in either the if statement or here, but it
looks particularly odd here.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_platform_delegate.h (right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:9: #include
"chrome/browser/ui/browser.h"
Forward declaration works.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:19: // Closes
the overflow menu, if it was open.
The comment is good, but I'd rename the method to be CloseOverflowMenuIfOpen()
just to make it clear when reading the code.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:18:
: browser_(browser) {
indentation off.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:26:
if (!browser_)
When can this be null?

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.h
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.h:17:
// MediaRouterActionPlatformDelegate implementation.
nitty nit: in most of the new ui code, we simply have
// SuperClass:

so, in this case:
// MediaRouterActionPlatformDelegate:

[apacible, 2015-07-14 21:15:23]

On 2015/07/14 17:23:02, Devlin wrote:
> Also, this seems like the type of thing that there should be a BUG= for

Added 508546.

> and probably a test as well. :)
I added a check to our Media Router UI tests (views only; we don't have cocoa
tests yet).

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:8: #include
"chrome/browser/ui/browser.h"
On 2015/07/14 17:23:02, Devlin wrote:
> Doesn't actually look like you need this.

Forgot to remove this while cleaning up, done.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:97:
platform_delegate_.get()->CloseOverflowMenu();
On 2015/07/14 17:23:02, Devlin wrote:
> no need for the .get().  Really, in either the if statement or here, but it
> looks particularly odd here.

Done.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_platform_delegate.h (right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:9: #include
"chrome/browser/ui/browser.h"
On 2015/07/14 17:23:02, Devlin wrote:
> Forward declaration works.

Done.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:19: // Closes
the overflow menu, if it was open.
On 2015/07/14 17:23:02, Devlin wrote:
> The comment is good, but I'd rename the method to be CloseOverflowMenuIfOpen()
> just to make it clear when reading the code.

Done.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:18:
: browser_(browser) {
On 2015/07/14 17:23:02, Devlin wrote:
> indentation off.

Done.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:26:
if (!browser_)
On 2015/07/14 17:23:02, Devlin wrote:
> When can this be null?

Hopefully never, but I added the check just in case.

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.h
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.h:17:
// MediaRouterActionPlatformDelegate implementation.
On 2015/07/14 17:23:02, Devlin wrote:
> nitty nit: in most of the new ui code, we simply have
> // SuperClass:
> 
> so, in this case:
> // MediaRouterActionPlatformDelegate:

Good to know! Done.

[Devlin, 2015-07-14 21:31:08]

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:26:
if (!browser_)
On 2015/07/14 21:15:23, apacible wrote:
> On 2015/07/14 17:23:02, Devlin wrote:
> > When can this be null?
> 
> Hopefully never, but I added the check just in case.

When the answer is "never", it's always best to *not* guard (though a DCHECK in
the constructor could be good).  Defensively guarding like this just makes a
problem harder to find in most cases, rather than actually helping it.  If the
browser is null, other things will also almost certainly break, and it's best to
have a nice clean crash and stack trace that we can fix, rather than some
subtle, hard-to-find issue.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:10: #include
"chrome/browser/ui/browser.h"
Again, forward declaration will do.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_platform_delegate.h (right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:24: #endif  //
CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_PLATFORM_DELEGATE_VIEWS_H_
This guard comment is wrong.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:51:
ASSERT_FALSE(toolbar->IsWrenchMenuShowing());
... But the wrench menu is never opened, right?  Won't this be trivially true?

[apacible, 2015-07-14 23:08:16]

Patchset #3 (id:280001) has been deleted

[apacible, 2015-07-15 02:16:58]

Patchset #3 (id:300001) has been deleted

[apacible, 2015-07-15 16:38:07]

Patchset #3 (id:320001) has been deleted

[apacible, 2015-07-16 16:52:11]

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
File
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
(right):

https://codereview.chromium.org/1228223002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc:26:
if (!browser_)
On 2015/07/14 21:31:07, Devlin wrote:
> On 2015/07/14 21:15:23, apacible wrote:
> > On 2015/07/14 17:23:02, Devlin wrote:
> > > When can this be null?
> > 
> > Hopefully never, but I added the check just in case.
> 
> When the answer is "never", it's always best to *not* guard (though a DCHECK
in
> the constructor could be good).  Defensively guarding like this just makes a
> problem harder to find in most cases, rather than actually helping it.  If the
> browser is null, other things will also almost certainly break, and it's best
to
> have a nice clean crash and stack trace that we can fix, rather than some
> subtle, hard-to-find issue.

Thanks, done.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:10: #include
"chrome/browser/ui/browser.h"
On 2015/07/14 21:31:07, Devlin wrote:
> Again, forward declaration will do.

Done.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action_platform_delegate.h (right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action_platform_delegate.h:24: #endif  //
CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_PLATFORM_DELEGATE_VIEWS_H_
On 2015/07/14 21:31:07, Devlin wrote:
> This guard comment is wrong.

Done.

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1228223002/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:51:
ASSERT_FALSE(toolbar->IsWrenchMenuShowing());
On 2015/07/14 21:31:07, Devlin wrote:
> ... But the wrench menu is never opened, right?  Won't this be trivially true?

... good point. Opening the wrench menu prior to the ExecuteAction call now...

[Devlin, 2015-07-16 17:17:37]

just a few more small nits. :)

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:8: #include
"chrome/browser/ui/toolbar/media_router_action_platform_delegate.h"
Forward declaration. :)

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:50:
ToolbarView* toolbar =
Just by looking at this test, it's not immediately clear to me that we're
actually testing the action executed.  Can you:
- Add a comment saying something like "When the media router action executes, it
pops up a dialog with a web contents to <url>."
- Check the value of nav_observer.last_committed_url().

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:58:
ASSERT_FALSE(toolbar->IsWrenchMenuShowing());
nit: This (and the assert in ExecuteMediaRouterAction) can be EXPECTs.

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:96:
content::TestNavigationObserver nav_observer(
nit: NavigateToURL() is actually synchronous, so this is redundant.

[apacible, 2015-07-16 18:13:14]

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.h:8: #include
"chrome/browser/ui/toolbar/media_router_action_platform_delegate.h"
On 2015/07/16 17:17:37, Devlin wrote:
> Forward declaration. :)

Done.

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:50:
ToolbarView* toolbar =
On 2015/07/16 17:17:37, Devlin wrote:
> Just by looking at this test, it's not immediately clear to me that we're
> actually testing the action executed.  Can you:
> - Add a comment saying something like "When the media router action executes,
it
> pops up a dialog with a web contents to <url>."
> - Check the value of nav_observer.last_committed_url().

Done.

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:58:
ASSERT_FALSE(toolbar->IsWrenchMenuShowing());
On 2015/07/16 17:17:37, Devlin wrote:
> nit: This (and the assert in ExecuteMediaRouterAction) can be EXPECTs.

Done.

https://codereview.chromium.org/1228223002/diff/360001/chrome/browser/ui/view...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:96:
content::TestNavigationObserver nav_observer(
On 2015/07/16 17:17:37, Devlin wrote:
> nit: NavigateToURL() is actually synchronous, so this is redundant.

Done.

[Devlin, 2015-07-16 18:19:48]

lgtm

[apacible, 2015-07-16 18:25:41]

apacible@chromium.org changed reviewers:
+ kmarshall@chromium.org, pkasting@chromium.org

[apacible, 2015-07-16 18:25:42]

+pkasting for c/b/ui/ and c/b/ui/views OWNER.
+kmarshall for c/b/ui/views/media_router OWNER.

PTAL, thanks!

[apacible, 2015-07-17 17:50:27]

Friendly ping! :)

[Peter Kasting, 2015-07-17 18:05:58]

lgtm

[Kevin M, 2015-07-17 20:15:19]

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:28:
platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)) {
It seems to me that we should put the platform ifdefs (which are platform
specific details) inside PlatformDelegate instead of putting them here.
PlatformDelegate is then free to do whatever it wants to provide
platform-appropriate bindings, from just using #ifdefs to linking to different
.cc files entirely.

[apacible, 2015-07-17 20:41:50]

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:28:
platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)) {
On 2015/07/17 20:15:19, Kevin M wrote:
> It seems to me that we should put the platform ifdefs (which are platform
> specific details) inside PlatformDelegate instead of putting them here.
> PlatformDelegate is then free to do whatever it wants to provide
> platform-appropriate bindings, from just using #ifdefs to linking to different
> .cc files entirely.

There won't be any ifdefs when OSX is implemented; this is just temporary since
there isn't a stub impl for OSX yet. PlatformDelegate is only implemented for
independent platforms; none of it will be shared between both views/cocoa.

[Kevin M, 2015-07-17 20:55:50]

lgtm

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:28:
platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)) {
On 2015/07/17 20:41:50, apacible wrote:
> On 2015/07/17 20:15:19, Kevin M wrote:
> > It seems to me that we should put the platform ifdefs (which are platform
> > specific details) inside PlatformDelegate instead of putting them here.
> > PlatformDelegate is then free to do whatever it wants to provide
> > platform-appropriate bindings, from just using #ifdefs to linking to
different
> > .cc files entirely.
> 
> There won't be any ifdefs when OSX is implemented; this is just temporary
since
> there isn't a stub impl for OSX yet. PlatformDelegate is only implemented for
> independent platforms; none of it will be shared between both views/cocoa.

Ack, this is more like a code organization nit - keeping the platform switches
into a file that concerns itself with platform code makes more sense to me. You
could go about it several ways  - you could have #ifdef rules make the class
into an empty stub; you could have Create() return nullptr (but you still have
to preface your delegate->Run with an existence check); different source files;
etc.

This is just a nit so I'm LGTM'ing anyway.

[apacible, 2015-07-18 01:40:42]

Patchset #7 (id:420001) has been deleted

[apacible, 2015-07-18 02:00:11]

Patchset #7 (id:440001) has been deleted

[apacible, 2015-07-20 06:18:43]

Patchset #8 (id:480001) has been deleted

[apacible, 2015-07-20 06:18:51]

Patchset #7 (id:460001) has been deleted

[apacible, 2015-07-20 06:47:49]

Patchset #7 (id:500001) has been deleted

[apacible, 2015-07-20 07:13:26]

Patchset #7 (id:520001) has been deleted

[apacible, 2015-07-20 07:33:51]

Patchset #7 (id:540001) has been deleted

[apacible, 2015-07-20 15:40:29]

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1228223002/diff/380001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/media_router_action.cc:28:
platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)) {
On 2015/07/17 20:55:50, Kevin M wrote:
> On 2015/07/17 20:41:50, apacible wrote:
> > On 2015/07/17 20:15:19, Kevin M wrote:
> > > It seems to me that we should put the platform ifdefs (which are platform
> > > specific details) inside PlatformDelegate instead of putting them here.
> > > PlatformDelegate is then free to do whatever it wants to provide
> > > platform-appropriate bindings, from just using #ifdefs to linking to
> different
> > > .cc files entirely.
> > 
> > There won't be any ifdefs when OSX is implemented; this is just temporary
> since
> > there isn't a stub impl for OSX yet. PlatformDelegate is only implemented
for
> > independent platforms; none of it will be shared between both views/cocoa.
> 
> Ack, this is more like a code organization nit - keeping the platform switches
> into a file that concerns itself with platform code makes more sense to me.
You
> could go about it several ways  - you could have #ifdef rules make the class
> into an empty stub; you could have Create() return nullptr (but you still have
> to preface your delegate->Run with an existence check); different source
files;
> etc.
> 
> This is just a nit so I'm LGTM'ing anyway.

Done.

[apacible, 2015-07-20 15:40:36]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-07-20 15:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, pkasting@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1228223002/#ps580001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-07-20 15:40:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228223002/580001

[commit-bot: I haz the power, 2015-07-20 16:32:42]

Committed patchset #8 (id:580001)

[commit-bot: I haz the power, 2015-07-20 16:33:50]

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