Show Media Router toolbar icon ephemerally for MR dialogs

chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc

Index: chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
diff --git a/chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc b/chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
index 11a18b7bd704a50ea35c013b4b249c10862ff716..d1be8c6d1ef72d6bcbe59c6eb8d857924bfc6ab2 100644
--- a/chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
+++ b/chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc
@@ -18,6 +18,7 @@
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_action_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -41,25 +42,10 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
             ->toolbar()
             ->browser_actions();
     ASSERT_TRUE(browser_actions_container);
-
-    browser_action_test_util_.reset(new BrowserActionTestUtil(browser(),
-                                                              false));
-    media_router_action_.reset(new MediaRouterAction(browser(),
-        browser_action_test_util_->GetToolbarActionsBar()));
-
-    toolbar_action_view_widget_ = new views::Widget();
-    views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
-    params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-    toolbar_action_view_widget_->Init(params);
-    toolbar_action_view_widget_->Show();
-
-    // Sets delegate on |media_router_action_|.
-    toolbar_action_view_ = new ToolbarActionView(media_router_action_.get(),
-                                                 browser_actions_container);
-    toolbar_action_view_widget_->SetContentsView(toolbar_action_view_);
+    toolbar_actions_bar_ = browser_actions_container->toolbar_actions_bar();
 
     action_controller_ =
-        &MediaRouterUIService::Get(browser()->profile())->action_controller_;
+        MediaRouterUIService::Get(browser()->profile())->action_controller();
 
     issue_.reset(new Issue(
         "title notification", "message notification",
@@ -71,13 +57,6 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
                           "description", true, std::string(), true)};
   }
 
-  void TearDownOnMainThread() override {
-    toolbar_action_view_widget_->Close();
-    media_router_action_.reset();
-    browser_action_test_util_.reset();
-    InProcessBrowserTest::TearDownOnMainThread();
-  }
-
   void OpenMediaRouterDialogAndWaitForNewWebContents() {
     content::TestNavigationObserver nav_observer(NULL);
     nav_observer.StartWatchingNewWebContents();
@@ -102,9 +81,20 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
     nav_observer.StopWatchingNewWebContents();
   }
 
+  MediaRouterAction* GetMediaRouterAction() {

[msw, 2016/11/04 00:07:16]

nit: expose MediaRouterDialogControllerImpl::action_ with a [test-only] getter?

[takumif, 2016/11/09 04:37:27]

Done.

+    for (auto* action : toolbar_actions_bar_->GetActions()) {
+      if (action->GetId() ==
+          ComponentToolbarActionsFactory::kMediaRouterActionId) {
+        return static_cast<MediaRouterAction*>(action);
+      }
+    }
+    NOTREACHED();
+    return nullptr;
+  }
+
   void ExecuteMediaRouterAction(AppMenuButton* app_menu_button) {
     EXPECT_TRUE(app_menu_button->IsMenuShowing());
-    media_router_action_->ExecuteAction(true);
+    GetMediaRouterAction()->ExecuteAction(true);
   }
 
   bool ActionExists() {
@@ -121,16 +111,7 @@ class MediaRouterUIBrowserTest : public InProcessBrowserTest {
   }
 
  protected:
-  // Must be initialized after |InProcessBrowserTest::SetUpOnMainThread|.
-  std::unique_ptr<BrowserActionTestUtil> browser_action_test_util_;
-  std::unique_ptr<MediaRouterAction> media_router_action_;
-
-  // ToolbarActionView constructed to set the delegate on
-  // |media_router_action_|.
-  ToolbarActionView* toolbar_action_view_ = nullptr;
-
-  // Hosts the |toolbar_action_view_|.
-  views::Widget* toolbar_action_view_widget_ = nullptr;
+  ToolbarActionsBar* toolbar_actions_bar_ = nullptr;
 
   std::unique_ptr<Issue> issue_;
 
@@ -147,6 +128,9 @@ IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
   // Make sure there is 1 tab and media router is enabled.
   ASSERT_EQ(1, browser()->tab_strip_model()->count());
 
+  SetAlwaysShowActionPref(true);

[msw, 2016/11/04 00:07:16]

Why is this important (for a disabled test)?

[takumif, 2016/11/09 04:37:27]

This test was disabled recently.

Without this line we'd be executing the action without having the action icon on
the toolbar, which doesn't reflect actual usage.

+  EXPECT_TRUE(ActionExists());
+
   OpenMediaRouterDialogAndWaitForNewWebContents();
 
   // Reload the browser and wait.
@@ -165,9 +149,12 @@ IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
   // The navigation should have removed the previously created dialog.
   // We expect a new dialog WebContents to be created by calling this.
   OpenMediaRouterDialogAndWaitForNewWebContents();
+
+  SetAlwaysShowActionPref(false);
 }
 
-IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, EphemeralToolbarIcon) {
+IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
+                       EphemeralToolbarIconForRoutesAndIssues) {
   action_controller_->OnIssueUpdated(issue_.get());
   EXPECT_TRUE(ActionExists());
   action_controller_->OnIssueUpdated(nullptr);
@@ -186,6 +173,38 @@ IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest, EphemeralToolbarIcon) {
 }
 
 IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
+                       EphemeralToolbarIconForDialog) {
+  MediaRouterDialogControllerImpl* dialog_controller =

[msw, 2016/11/04 00:07:16]

Why is the impl subclass used here? It seems like all the functions needed are
available through the MediaRouterDialogController interface.

[takumif, 2016/11/09 04:37:27]

It'd be okay to use either, but only the Impl class (the desktop implementation
of MRDC, as opposed to the Android implementation) has the toolbar action
related code. I'm going to call the action() getter on the Impl class above, so
I'll keep using the Impl class here for the sake of consistency.

[msw, 2016/11/09 20:32:33]

I think it's generally beneficial to use the most abstract interface possible,
in both production and testing, that way clients are less likely to rely upon or
fall victim to implementation-specific details.

[takumif, 2016/11/10 01:53:59]

Got it. Using MediaRouterDialogController. Thanks for explaining.

+      MediaRouterDialogControllerImpl::GetOrCreateForWebContents(
+          browser()->tab_strip_model()->GetActiveWebContents());
+
+  dialog_controller->ShowMediaRouterDialog();

[msw, 2016/11/04 00:07:16]

nit: EXPECT_FALSE(ActionExists()); above this

[takumif, 2016/11/09 04:37:27]

Done.

+  EXPECT_TRUE(ActionExists());
+  dialog_controller->HideMediaRouterDialog();
+  EXPECT_FALSE(ActionExists());
+
+  dialog_controller->ShowMediaRouterDialog();
+  EXPECT_TRUE(ActionExists());
+  // Clicking on the toolbar icon should hide both the dialog and the icon.
+  GetMediaRouterAction()->ExecuteAction(true);
+  EXPECT_FALSE(dialog_controller->IsShowingMediaRouterDialog());

[msw, 2016/11/04 00:07:16]

To be clear, calling ExecuteAction above calls through to HideMediaRouterDialog
and reduces the dialog_count_, right?

[takumif, 2016/11/09 04:37:27]

Right.

+  EXPECT_FALSE(ActionExists());
+
+  dialog_controller->ShowMediaRouterDialog();
+  SetAlwaysShowActionPref(true);
+  // When the pref is set to true, hiding the dialog shouldn't hide the icon.
+  dialog_controller->HideMediaRouterDialog();
+  EXPECT_TRUE(ActionExists());
+  dialog_controller->ShowMediaRouterDialog();
+  // While the dialog is showing, setting the pref to false shouldn't hide the
+  // icon.
+  SetAlwaysShowActionPref(false);
+  EXPECT_TRUE(ActionExists());
+  dialog_controller->HideMediaRouterDialog();
+  EXPECT_FALSE(ActionExists());
+}
+
+IN_PROC_BROWSER_TEST_F(MediaRouterUIBrowserTest,
                        EphemeralToolbarIconWithMultipleWindows) {
   action_controller_->OnRoutesUpdated(routes_, std::vector<MediaRoute::Id>());
   EXPECT_TRUE(ActionExists());