[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar MediaRouterAction.

chrome/browser/ui/toolbar/media_router_action_unittest.cc

Index: chrome/browser/ui/toolbar/media_router_action_unittest.cc
diff --git a/chrome/browser/ui/toolbar/media_router_action_unittest.cc b/chrome/browser/ui/toolbar/media_router_action_unittest.cc
index 579e98370d5c1b839189302c94e0955bc5d32e96..f5338b82b4e690acc2581341bef767a97180946f 100644
--- a/chrome/browser/ui/toolbar/media_router_action_unittest.cc
+++ b/chrome/browser/ui/toolbar/media_router_action_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/media_router_action.h"
@@ -34,8 +35,9 @@ class MockToolbarActionViewDelegate : public ToolbarActionViewDelegate {
 
 class TestMediaRouterAction : public MediaRouterAction {
  public:
-  explicit TestMediaRouterAction(Browser* browser)
-      : MediaRouterAction(browser),
+  explicit TestMediaRouterAction(Browser* browser,

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

Nit: No explicit

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

Done.

+                                 ToolbarActionsBar* toolbar_actions_bar)
+      : MediaRouterAction(browser, toolbar_actions_bar),
         controller_(nullptr),
         platform_delegate_(nullptr) {}
   ~TestMediaRouterAction() override {}
@@ -107,11 +109,16 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
   // MediaRouterTest:
   void SetUp() override {
     MediaRouterTest::SetUp();
-    action_.reset(new TestMediaRouterAction(browser()));
+    browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),

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

Can we instantiate these in the constructor rather than in SetUp(), or does
MediaRouterTest::SetUp() have to run first for some reason?

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

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...

[Peter Kasting, 2015/12/03 22:00:19]

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

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

Done.

+                                                              false));

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

Tiny nit: Personally I'd find wrapping after "reset(" more readable

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

Done.

+    action_.reset(new TestMediaRouterAction(
+                      browser(),
+                      browser_action_test_util_->GetToolbarActionsBar()));
   }
 
   void TearDown() override {
     action_.reset();
+    browser_action_test_util_.reset();

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

Why do we need to explicitly reset these here?

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

Removed.

[Peter Kasting, 2015/12/03 22:00:19]

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

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

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.

     MediaRouterTest::TearDown();
   }
 
@@ -133,6 +140,10 @@ class MediaRouterActionUnitTest : public MediaRouterTest {
  private:
   scoped_ptr<TestMediaRouterAction> action_;
 
+  // A BrowserActionTestUtil object constructed with the associated
+  // ToolbarActionsBar.
+  scoped_ptr<BrowserActionTestUtil> browser_action_test_util_;
+
   // Fake Issues.
   const media_router::Issue fake_issue_notification_;
   const media_router::Issue fake_issue_warning_;