[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar MediaRouterAction.

[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the overflow would require the explicit opening of the overflow menu. However, in case there is an initialization change, this added check ensures only the toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A new MediaRouterDialogControllerImpl is created for every new browser window that is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
Committed: https://crrev.com/c0b006adb6231ef65d9e99bd16d527addfa28f4c
Cr-Commit-Position: refs/heads/master@{#363693}

[apacible, 2015-12-02 22:08:15]

Description was changed from

==========
check mra is toolbar not overflow

BUG=
==========

to

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl. Currently,
the toolbar action is created before the overflow action since the overflow
would require the explicit opening of the overflow menu. However, in case there
is an initialization change, this added check ensures only the toolbar
MediaRouterAction instance is set in MediaRouterDialogControllerImpl.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

[apacible, 2015-12-02 22:11:44]

Description was changed from

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl. Currently,
the toolbar action is created before the overflow action since the overflow
would require the explicit opening of the overflow menu. However, in case there
is an initialization change, this added check ensures only the toolbar
MediaRouterAction instance is set in MediaRouterDialogControllerImpl.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

to

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the
overflow would require the explicit opening of the overflow menu. However, in
case there is an initialization change, this added check ensures only the
toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A
new MediaRouterDialogControllerImpl is created for every new browser window that
is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

[apacible, 2015-12-02 22:11:44]

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

[apacible, 2015-12-02 22:12:29]

PTAL, thanks!

[Peter Kasting, 2015-12-03 04:48:53]

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action.cc:199: // than the overflow menu.
If it was already set, this should be a no-op.
This first sentence restates what the code does.  If you need to leave comments
on the code, they should describe _why_ more than _what_.

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:38: explicit
TestMediaRouterAction(Browser* browser,
Nit: No explicit

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:112:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),
Can we instantiate these in the constructor rather than in SetUp(), or does
MediaRouterTest::SetUp() have to run first for some reason?

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:113: false));
Tiny nit: Personally I'd find wrapping after "reset(" more readable

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:121:
browser_action_test_util_.reset();
Why do we need to explicitly reset these here?

[apacible, 2015-12-03 21:53:10]

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action.cc:199: // than the overflow menu.
If it was already set, this should be a no-op.
On 2015/12/03 04:48:53, Peter Kasting wrote:
> This first sentence restates what the code does.  If you need to leave
comments
> on the code, they should describe _why_ more than _what_.

Done.

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:38: explicit
TestMediaRouterAction(Browser* browser,
On 2015/12/03 04:48:53, Peter Kasting wrote:
> Nit: No explicit

Done.

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:112:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),
On 2015/12/03 04:48:53, Peter Kasting wrote:
> Can we instantiate these in the constructor rather than in SetUp(), or does
> MediaRouterTest::SetUp() have to run first for some reason?

To use browser(), we have to wait for BrowserWithTestWindowTest's SetUp to be
called:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/b...

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:113: false));
On 2015/12/03 04:48:53, Peter Kasting wrote:
> Tiny nit: Personally I'd find wrapping after "reset(" more readable

Done.

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:121:
browser_action_test_util_.reset();
On 2015/12/03 04:48:53, Peter Kasting wrote:
> Why do we need to explicitly reset these here?

Removed.

[Peter Kasting, 2015-12-03 22:00:20]

LGTM

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:112:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),
On 2015/12/03 21:53:10, apacible wrote:
> On 2015/12/03 04:48:53, Peter Kasting wrote:
> > Can we instantiate these in the constructor rather than in SetUp(), or does
> > MediaRouterTest::SetUp() have to run first for some reason?
> 
> To use browser(), we have to wait for BrowserWithTestWindowTest's SetUp to be
> called:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/b...

Ah.  Might be nice to just comment "browser() will only be valid once
BrowserWithTestWindowTest::SetUp() has run."

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:121:
browser_action_test_util_.reset();
On 2015/12/03 21:53:10, apacible wrote:
> On 2015/12/03 04:48:53, Peter Kasting wrote:
> > Why do we need to explicitly reset these here?
> 
> Removed.

We're still resetting |action_|.  Is that necessary?

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

https://codereview.chromium.org/1495653003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:201: // in overflow mode, it
will be destroyed when the overflow menu is closed.
Now the problem is that there's lots of useful background info here but I'm not
sure precisely how it relates to what this code is doing.  This is primarily
because I don't really understand what UpdatePopupState() is doing and when it's
run relative to when the other events you describe here (e.g. the app menu
dropdown with the overflow area in it being closed) are happening.  I note that
the header file here includes no comments on what this function does...

[apacible, 2015-12-07 18:24:55]

Patchset #4 (id:60001) has been deleted

[apacible, 2015-12-07 21:47:26]

Patchset #6 (id:120001) has been deleted

[apacible, 2015-12-07 21:47:34]

Patchset #5 (id:100001) has been deleted

[apacible, 2015-12-07 21:52:43]

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:112:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),
On 2015/12/03 22:00:19, Peter Kasting wrote:
> On 2015/12/03 21:53:10, apacible wrote:
> > On 2015/12/03 04:48:53, Peter Kasting wrote:
> > > Can we instantiate these in the constructor rather than in SetUp(), or
does
> > > MediaRouterTest::SetUp() have to run first for some reason?
> > 
> > To use browser(), we have to wait for BrowserWithTestWindowTest's SetUp to
be
> > called:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/b...
> 
> Ah.  Might be nice to just comment "browser() will only be valid once
> BrowserWithTestWindowTest::SetUp() has run."

Done.

https://codereview.chromium.org/1495653003/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:121:
browser_action_test_util_.reset();
On 2015/12/03 22:00:19, Peter Kasting wrote:
> On 2015/12/03 21:53:10, apacible wrote:
> > On 2015/12/03 04:48:53, Peter Kasting wrote:
> > > Why do we need to explicitly reset these here?
> > 
> > Removed.
> 
> We're still resetting |action_|.  Is that necessary?

Yes. As it turns out, we still need to reset |browser_action_test_util_| or the
teardown on mac crashes. I've added the line back.

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

https://codereview.chromium.org/1495653003/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action.cc:201: // in overflow mode, it
will be destroyed when the overflow menu is closed.
On 2015/12/03 22:00:19, Peter Kasting wrote:
> Now the problem is that there's lots of useful background info here but I'm
not
> sure precisely how it relates to what this code is doing.  This is primarily
> because I don't really understand what UpdatePopupState() is doing and when
it's
> run relative to when the other events you describe here (e.g. the app menu
> dropdown with the overflow area in it being closed) are happening.  I note
that
> the header file here includes no comments on what this function does...

Added comments in the header file for UpdatePopupState().

[apacible, 2015-12-08 00:42:14]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-12-08 00:42:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1495653003/#ps140001
(title: "Changes per pkasting@'s comments. Fixed OSX segfaults.")

[commit-bot: I haz the power, 2015-12-08 00:47:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495653003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495653003/140001

[commit-bot: I haz the power, 2015-12-08 01:36:44]

Description was changed from

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the
overflow would require the explicit opening of the overflow menu. However, in
case there is an initialization change, this added check ensures only the
toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A
new MediaRouterDialogControllerImpl is created for every new browser window that
is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

to

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the
overflow would require the explicit opening of the overflow menu. However, in
case there is an initialization change, this added check ensures only the
toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A
new MediaRouterDialogControllerImpl is created for every new browser window that
is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

[commit-bot: I haz the power, 2015-12-08 01:36:45]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-12-08 01:37:41]

Description was changed from

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the
overflow would require the explicit opening of the overflow menu. However, in
case there is an initialization change, this added check ensures only the
toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A
new MediaRouterDialogControllerImpl is created for every new browser window that
is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578
==========

to

==========
[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar
MediaRouterAction.

This is a follow up to an earlier patch[1] to only keep track of the first
MediaRouterAction that is created in MediaRouterDialogControllerImpl.

Currently, the toolbar action is created before the overflow action since the
overflow would require the explicit opening of the overflow menu. However, in
case there is an initialization change, this added check ensures only the
toolbar MediaRouterAction instance is set in MediaRouterDialogControllerImpl. A
new MediaRouterDialogControllerImpl is created for every new browser window that
is opened.

[1] https://codereview.chromium.org/1476053002/

BUG=531578

Committed: https://crrev.com/c0b006adb6231ef65d9e99bd16d527addfa28f4c
Cr-Commit-Position: refs/heads/master@{#363693}
==========

[commit-bot: I haz the power, 2015-12-08 01:37:41]

Patchset 5 (id:??) landed as
https://crrev.com/c0b006adb6231ef65d9e99bd16d527addfa28f4c
Cr-Commit-Position: refs/heads/master@{#363693}