Fix MRDialogDelegate and tests

[MR] Fix MediaRouterDialogDelegate and MediaRouterActionUnitTest.

- MediaRouterDialogDelegate has a WeakPtr to MediaRouterAction, but it
it never set in the ctor. The SetAction() method appears to be never
called. As a result, the WeakPtr is always null. This patch sets action_
in the ctor. Changed expectations in test.

- MediaRouterActionUnitTest setup is incorrect. It should call
MediaRouterTest to have the kEnableMediaRouter switch activated.

- MediaRouterActionUnitTest should only be inlcuded if we are building
Media Router.

BUG=548456
Committed: https://crrev.com/7b1b702ddf43a7fb214a460ff43d976b73e1a973
Cr-Commit-Position: refs/heads/master@{#356978}

[imcheng, 2015-10-27 22:57:49]

Description was changed from

==========
[MR] Fix MRDialogDelegate and tests

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

to

==========
[MR] Fix MRDialogDelegate and tests

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

[imcheng, 2015-10-27 23:11:40]

imcheng@chromium.org changed reviewers:
+ apacible@chromium.org, msw@chromium.org

[imcheng, 2015-10-27 23:11:40]

msw: PTAL at chrome/browser/ui/toolbar/media_router_action_unittest.cc
apacible: PTAL at everything

Thanks!

[msw, 2015-10-27 23:26:02]

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

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:22: using
testing::AtLeast;
nit: inline testing:: below instead of adding this using statement.

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:29:
MOCK_CONST_METHOD0(GetCurrentWebContents, content::WebContents*());
nit: drop content:: or inline elsewhere and remove |using content::WebContents;|

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:108: //
BrowserWithTestWindowTest:
nit: // MediaRouterTest:

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:294:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(AtLeast(1));
Why AtLeast once here and not Times(1)?

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:297:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
Weird that resetting the closed dialog calls OnPopupClosed()...

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:303:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(AtLeast(1));
Why AtLeast once here and not Times(1)?

[imcheng, 2015-10-28 00:19:23]

PTAL

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

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:22: using
testing::AtLeast;
On 2015/10/27 23:26:02, msw wrote:
> nit: inline testing:: below instead of adding this using statement.

Done.

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:29:
MOCK_CONST_METHOD0(GetCurrentWebContents, content::WebContents*());
On 2015/10/27 23:26:02, msw wrote:
> nit: drop content:: or inline elsewhere and remove |using
content::WebContents;|

Done.

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:108: //
BrowserWithTestWindowTest:
On 2015/10/27 23:26:02, msw wrote:
> nit: // MediaRouterTest:

Done.

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:294:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(AtLeast(1));
On 2015/10/27 23:26:02, msw wrote:
> Why AtLeast once here and not Times(1)?

Now that we are actually setting MediaRouterAction on MediaRouterDialogDelegate.
OnPopupClosed() can now also be called from there. I *think* we can remove the
call made from MediaRouterDialogController (L192-195), which I have done in the
new patchset. I will let Jennifer review the logic there.

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:297:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
On 2015/10/27 23:26:02, msw wrote:
> Weird that resetting the closed dialog calls OnPopupClosed()...

Agreed... Also Reset and CloseMediaRouterDialog are not public methods. It's
probably better to call the public HideMediaRouterDialog which also does a
Reset().

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:303:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(AtLeast(1));
On 2015/10/27 23:26:02, msw wrote:
> Why AtLeast once here and not Times(1)?

See above.

[msw, 2015-10-28 17:45:47]

rubber stamp lgtm, but the close codepaths aren't entirely obvious to me.

[apacible, 2015-10-29 02:06:59]

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

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:297:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
On 2015/10/28 00:19:23, imcheng1 wrote:
> On 2015/10/27 23:26:02, msw wrote:
> > Weird that resetting the closed dialog calls OnPopupClosed()...
> 
> Agreed... Also Reset and CloseMediaRouterDialog are not public methods. It's
> probably better to call the public HideMediaRouterDialog which also does a
> Reset().

IIUC we added OnPopupClosed() when resetting the closed dialog since
HideMediaRouterDialog isn't explicitly called if the dialog is closed via [esc]
key or when the tab is closed from the tab bar. How does removing OnPopupHidden
from Reset() and CloseMediaRouterDialog() affect the behavior?

[apacible, 2015-10-29 02:07:32]

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

https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_action_unittest.cc:297:
EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
On 2015/10/29 02:06:58, apacible wrote:
> On 2015/10/28 00:19:23, imcheng1 wrote:
> > On 2015/10/27 23:26:02, msw wrote:
> > > Weird that resetting the closed dialog calls OnPopupClosed()...
> > 
> > Agreed... Also Reset and CloseMediaRouterDialog are not public methods. It's
> > probably better to call the public HideMediaRouterDialog which also does a
> > Reset().
> 
> IIUC we added OnPopupClosed() when resetting the closed dialog since
> HideMediaRouterDialog isn't explicitly called if the dialog is closed via
[esc]
> key or when the tab is closed from the tab bar. How does removing
OnPopupHidden
> from Reset() and CloseMediaRouterDialog() affect the behavior?

s/IIUC/IIRC

[imcheng, 2015-10-29 03:27:39]

On 2015/10/29 02:07:32, apacible wrote:
>
https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
> File chrome/browser/ui/toolbar/media_router_action_unittest.cc (right):
> 
>
https://codereview.chromium.org/1417893005/diff/40001/chrome/browser/ui/toolb...
> chrome/browser/ui/toolbar/media_router_action_unittest.cc:297:
> EXPECT_CALL(*mock_delegate, OnPopupClosed()).Times(1);
> On 2015/10/29 02:06:58, apacible wrote:
> > On 2015/10/28 00:19:23, imcheng1 wrote:
> > > On 2015/10/27 23:26:02, msw wrote:
> > > > Weird that resetting the closed dialog calls OnPopupClosed()...
> > > 
> > > Agreed... Also Reset and CloseMediaRouterDialog are not public methods.
It's
> > > probably better to call the public HideMediaRouterDialog which also does a
> > > Reset().
> > 
> > IIUC we added OnPopupClosed() when resetting the closed dialog since
> > HideMediaRouterDialog isn't explicitly called if the dialog is closed via
> [esc]
> > key or when the tab is closed from the tab bar. How does removing
> OnPopupHidden
> > from Reset() and CloseMediaRouterDialog() affect the behavior?
> 
> s/IIUC/IIRC

The cases you mentioned should already be covered by the call made in
MediaRouterDialogDelegate::OnDialogClosed(). We can confirm quickly tomorrow.

[imcheng, 2015-10-29 20:33:14]

I have just tested closing the dialog via tab closing and the esc key and they
both behave as intended.

[apacible, 2015-10-29 22:08:15]

Thanks! lgtm

[imcheng, 2015-10-29 22:13:25]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2015-10-29 22:13:51]

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

[commit-bot: I haz the power, 2015-10-29 23:17:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-29 23:18:38]

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