[Media Router] Implement MediaRouterAction + MediaRouterUI tests.

[Media Router] Implement MediaRouterAction + MediaRouterUI tests.

Implement MediaRouterAction to use MediaRouterDialogController to show
and hide the Media Router Dialog.

Added a views browser test for
MediaRouterUI, that uses the ToolbarActionView as the delegate that
provides the initiator WebContents to the MediaRouterAction.

Re-ordered media router browser tests definitions in gyp to exclude
the views specific test in non toolkit_views builds.

BUG=464216
Committed: https://crrev.com/12fcff4be186bffb2e382ddf2ac4052864a674fb
Cr-Commit-Position: refs/heads/master@{#332860}

[imcheng (use chromium acct), 2015-06-02 16:19:18]

imcheng@google.com changed reviewers:
+ jcivelli@chromium.org, pkasting@chromium.org

[imcheng (use chromium acct), 2015-06-02 16:19:19]

PTAL, thanks!

jcivelli: chrome/test/BUILD.gn
pkasting: everything else

[imcheng (use chromium acct), 2015-06-02 18:05:00]

Patchset #1 (id:1) has been deleted

[imcheng (use chromium acct), 2015-06-02 18:05:05]

Patchset #1 (id:20001) has been deleted

[imcheng (use chromium acct), 2015-06-03 16:18:37]

Ping for both reviewers.

[imcheng (use chromium acct), 2015-06-03 23:37:32]

imcheng@google.com changed reviewers:
+ msw@chromium.org

[imcheng (use chromium acct), 2015-06-03 23:37:33]

+msw, could you take a look at the patch please?

[Peter Kasting, 2015-06-03 23:49:54]

On 2015/06/03 23:37:33, imcheng wrote:
> +msw, could you take a look at the patch please?

If you're going to add a reviewer, please say what you want them to review. 
Also say if this affects your existing reviewers.  Are you trying to ask for
msw's review because you haven't gotten one from me yet?

[imcheng (use chromium acct), 2015-06-03 23:58:00]

On 2015/06/03 23:49:54, Peter Kasting wrote:
> On 2015/06/03 23:37:33, imcheng wrote:
> > +msw, could you take a look at the patch please?
> 
> If you're going to add a reviewer, please say what you want them to review. 
> Also say if this affects your existing reviewers.

Okay. I will remember to do that.

> Are you trying to ask for msw's review because you haven't gotten one from me
yet?

Yes. I've tried to reach you but haven't heard back. So I added msw@ to review
everything except for chrome/test/BUILD.gn. Will you be able to review it?

[Peter Kasting, 2015-06-04 00:02:07]

On 2015/06/03 23:58:00, imcheng wrote:
> On 2015/06/03 23:49:54, Peter Kasting wrote:
> > On 2015/06/03 23:37:33, imcheng wrote:
> > > +msw, could you take a look at the patch please?
> > 
> > If you're going to add a reviewer, please say what you want them to review. 
> > Also say if this affects your existing reviewers.
> 
> Okay. I will remember to do that.
> 
> > Are you trying to ask for msw's review because you haven't gotten one from
me
> yet?
> 
> Yes. I've tried to reach you but haven't heard back. So I added msw@ to review
> everything except for chrome/test/BUILD.gn. Will you be able to review it?

Yes; please be patient.  You pinged after less than a day had elapsed and went
looking for a new reviewer on the same day as your first ping.  I simply haven't
had time to get to this until this afternoon.

[Peter Kasting, 2015-06-04 00:17:53]

I don't feel particularly confident in reviewing the .gypi/.gn changes.

Most of the rest is nits.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (left):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:66: NOTIMPLEMENTED();
Nit: It looks like most of these functions have trivial implementations that are
likely shared with most implementers.  Consider placing these default
implementations in the base class instead to reduce boilerplate in the
subclasses.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:78: content::WebContents*
web_contents = delegate_->GetCurrentWebContents();
Nit: Why do you not DCHECK(delegate_) here, but you do in the two functions
below?  Seems like these functions should agree on the right way.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:81:
MediaRouterDialogController::GetOrCreateForWebContents(web_contents);
Nit: You have a bunch of code duplicated in these functions.  Consider adding a
helper like "GetMediaController()" which encapsulates everything except the
final call on the controller.  Then you can just do
"GetMediaController()->CloseMediaRouterDialog();" in this function (and
similarly below).

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:38: content::WebContents*
GetMediaRouterDialog() const;
Const functions may not return non-const pointers.  Return a const pointer, or
if that's impossible make the function non-const.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:30:
BrowserActionsContainer* main_bar =
Nit: I think |browser_actions| might be a better name for this, since it matches
the helper function used to get it, and "main bar" is multiply confusing ("main"
implies there's a "secondary" somewhere, and "bar" implies this is the whole
toolbar).

You could also use |browser_actions_container| for more explicit clarity at the
cost of no longer matching the helper name.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:53:
scoped_ptr<MediaRouterAction> mr_action_;
Nit: |media_router_action_| (or at least |router_action_|).  Avoid
abbreviations, especially ones like "mr" which look like "Mr." and thus are just
confusing.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:59:
IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, MediaRouterCommands) {
What is this actually testing?  The comments say what the test is doing but not
what we expect to happen and what the test overall accomplishes.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:109:
media_router::MediaRouterDialogController::FromWebContents(web_contents);
Nit: media_router:: namespace qualifier not necessary (and you don't use it
below)

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:116:
return controller;
Nit: This function is more verbose than it needs to be, because
CreateForWebContents() will automatically no-op if the object exists.  So just
something like:

{
  // This call does nothing if the controller already exists.
  MediaRouterDialogController::CreateForWebContents(web_contents);
  return MediaRouterDialogController::FromWebContents(web_contents);
}

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:25: //
|web_contents|, creating one it does not exist. Returned value guaranteed
Nit: it -> if it

[imcheng (use chromium acct), 2015-06-04 01:04:20]

Thanks for the review.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (left):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:66: NOTIMPLEMENTED();
On 2015/06/04 00:17:52, Peter Kasting wrote:
> Nit: It looks like most of these functions have trivial implementations that
are
> likely shared with most implementers.  Consider placing these default
> implementations in the base class instead to reduce boilerplate in the
> subclasses.

Acknowledged. Most other implementations seem to be different. Given the limited
number of implementations currently (3, 1 of which is test), I would defer this
until it becomes more obvious that it's better to have a default implementation
in the base class.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:78: content::WebContents*
web_contents = delegate_->GetCurrentWebContents();
On 2015/06/04 00:17:52, Peter Kasting wrote:
> Nit: Why do you not DCHECK(delegate_) here, but you do in the two functions
> below?  Seems like these functions should agree on the right way.

Done.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:81:
MediaRouterDialogController::GetOrCreateForWebContents(web_contents);
On 2015/06/04 00:17:52, Peter Kasting wrote:
> Nit: You have a bunch of code duplicated in these functions.  Consider adding
a
> helper like "GetMediaController()" which encapsulates everything except the
> final call on the controller.  Then you can just do
> "GetMediaController()->CloseMediaRouterDialog();" in this function (and
> similarly below).

Done.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_action.h (right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.h:38: content::WebContents*
GetMediaRouterDialog() const;
On 2015/06/04 00:17:52, Peter Kasting wrote:
> Const functions may not return non-const pointers.  Return a const pointer, or
> if that's impossible make the function non-const.

Removed this function since it is not used.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:30:
BrowserActionsContainer* main_bar =
On 2015/06/04 00:17:53, Peter Kasting wrote:
> Nit: I think |browser_actions| might be a better name for this, since it
matches
> the helper function used to get it, and "main bar" is multiply confusing
("main"
> implies there's a "secondary" somewhere, and "bar" implies this is the whole
> toolbar).
> 
> You could also use |browser_actions_container| for more explicit clarity at
the
> cost of no longer matching the helper name.

Done.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:53:
scoped_ptr<MediaRouterAction> mr_action_;
On 2015/06/04 00:17:53, Peter Kasting wrote:
> Nit: |media_router_action_| (or at least |router_action_|).  Avoid
> abbreviations, especially ones like "mr" which look like "Mr." and thus are
just
> confusing.

Done.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:59:
IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, MediaRouterCommands) {
On 2015/06/04 00:17:53, Peter Kasting wrote:
> What is this actually testing?  The comments say what the test is doing but
not
> what we expect to happen and what the test overall accomplishes.

Renamed the function to be more explicit about expectations and added
supplementary comments to the test.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:109:
media_router::MediaRouterDialogController::FromWebContents(web_contents);
On 2015/06/04 00:17:53, Peter Kasting wrote:
> Nit: media_router:: namespace qualifier not necessary (and you don't use it
> below)

Done.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.cc:116:
return controller;
On 2015/06/04 00:17:53, Peter Kasting wrote:
> Nit: This function is more verbose than it needs to be, because
> CreateForWebContents() will automatically no-op if the object exists.  So just
> something like:
> 
> {
>   // This call does nothing if the controller already exists.
>   MediaRouterDialogController::CreateForWebContents(web_contents);
>   return MediaRouterDialogController::FromWebContents(web_contents);
> }

My reasoning was that although CreateForWebContents is no-op when called a
second time, it still has to check for the existence for the
MediaRouterDialogController in order for it to no-op. It's also an extra
function call. But I guess it does not make a big difference in terms of
performance, so I took your suggestion.

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1149893020/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:25: //
|web_contents|, creating one it does not exist. Returned value guaranteed
On 2015/06/04 00:17:53, Peter Kasting wrote:
> Nit: it -> if it

Done.

[Peter Kasting, 2015-06-04 07:13:03]

LGTM

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:60:
IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
This test is better now, but it still wouldn't go amiss to add a comment above
the test summarizing it, e.g. talking about that this tests various different
types of navigations that should trigger creation of a media router dialog and
ensures they also remove any previously-visible dialogs, or whatever would
actually be accurate.

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:75: // We
expect a new dialog WebContents to be created by calling this.
If the previous dialog wasn't removed or a new WebContents isn't created, will
the test hang indefinitely, or eventually figure this out and time out or
something?  It would be nice to fail fast, if doing so is feasible.  (Maybe it's
not.)

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:25: //
|web_contents|, creating one if it does not exist. Returned value
Tiny nit: "Returned value" -> "The returned pointer is"

[Jay Civelli, 2015-06-04 15:45:19]

https://codereview.chromium.org/1149893020/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2443: ['enable_media_router==1', {
Any specific reason why this was moved here?

[imcheng (use chromium acct), 2015-06-04 16:30:44]

https://codereview.chromium.org/1149893020/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:2443: ['enable_media_router==1', {
On 2015/06/04 15:45:19, Jay Civelli wrote:
> Any specific reason why this was moved here?

This was moved here so that it is parsed after the tookit_views conditions in
L2390-L2402. The media router test included in
chrome_browser_tests_views_non_mac_sources there will be removed here if
enable_media_router is not defined.

[Jay Civelli, 2015-06-04 16:34:29]

lgtm

[imcheng (use chromium acct), 2015-06-04 16:45:44]

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
(right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:60:
IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
On 2015/06/04 07:13:03, Peter Kasting wrote:
> This test is better now, but it still wouldn't go amiss to add a comment above
> the test summarizing it, e.g. talking about that this tests various different
> types of navigations that should trigger creation of a media router dialog and
> ensures they also remove any previously-visible dialogs, or whatever would
> actually be accurate.

Done.

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:75: // We
expect a new dialog WebContents to be created by calling this.
On 2015/06/04 07:13:03, Peter Kasting wrote:
> If the previous dialog wasn't removed or a new WebContents isn't created, will
> the test hang indefinitely, or eventually figure this out and time out or
> something?  It would be nice to fail fast, if doing so is feasible.  (Maybe
it's
> not.)

I believe browser tests have a builtin timeout (something like 60s but is
configurable) so this will not hang indefinitely. We could add a timeout to say
the TestNavigationObserver if we wanted a stricter timeout, but it is not needed
right now.

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_dialog_controller.h
(right):

https://codereview.chromium.org/1149893020/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_dialog_controller.h:25: //
|web_contents|, creating one if it does not exist. Returned value
On 2015/06/04 07:13:03, Peter Kasting wrote:
> Tiny nit: "Returned value" -> "The returned pointer is"

Done.

[imcheng (use chromium acct), 2015-06-04 18:28:17]

The CQ bit was checked by imcheng@google.com

[imcheng (use chromium acct), 2015-06-04 18:28:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jcivelli@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1149893020/#ps80001
(title: "Addressed pkasting's 2nd comments")

[commit-bot: I haz the power, 2015-06-04 18:29:25]

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

[commit-bot: I haz the power, 2015-06-04 18:37:22]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-06-04 18:39:13]

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