Show Media Router toolbar icon ephemerally for MR dialogs

chrome/browser/ui/toolbar/media_router_action_controller.h

Index: chrome/browser/ui/toolbar/media_router_action_controller.h
diff --git a/chrome/browser/ui/toolbar/media_router_action_controller.h b/chrome/browser/ui/toolbar/media_router_action_controller.h
index 9965f6b3c2e04380e4c762a919d5934457aad0dd..938f8fd948bc01df93a1169e34db646808e4ce3c 100644
--- a/chrome/browser/ui/toolbar/media_router_action_controller.h
+++ b/chrome/browser/ui/toolbar/media_router_action_controller.h
@@ -15,6 +15,10 @@
 
 using extensions::ComponentMigrationHelper;
 
+namespace media_router {
+class MockMediaRouterActionController;
+}  // namespace media_router
+
 // Controller for MediaRouterAction that determines when to show and hide the
 // action icon on the toolbar. There should be one instance of this class per
 // profile, and it should only be used on the UI thread.
@@ -32,9 +36,17 @@ class MediaRouterActionController : public media_router::IssuesObserver,
                        const std::vector<media_router::MediaRoute::Id>&
                            joinable_route_ids) override;
 
+  // Overridden in tests.

[mark a. foltz, 2016/10/17 18:57:40]

While that's true, can you also document what these methods are for in the real
implementation?

[takumif, 2016/10/27 23:06:38]

Done.

+  virtual void OnDialogShown();
+  virtual void OnDialogHidden();
+
  private:
   friend class MediaRouterActionControllerUnitTest;
-  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionControllerUnitTest, EphemeralIcon);
+  friend class media_router::MockMediaRouterActionController;
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionControllerUnitTest,
+                           EphemeralIconForRoutesAndIssues);
+  FRIEND_TEST_ALL_PREFIXES(MediaRouterActionControllerUnitTest,
+                           EphemeralIconForDialog);
 
   // Constructor for injecting dependencies in tests.

[mark a. foltz, 2016/10/17 18:57:40]

Would prefer making this public versus having to friend and forward declare
mocks in the  implementation to use it.  I think it's good to have a constructor
in the API that explicitly declares dependencies.

[takumif, 2016/10/27 23:06:38]

Done.

   MediaRouterActionController(
@@ -46,8 +58,8 @@ class MediaRouterActionController : public media_router::IssuesObserver,
 
   // Adds or removes the Media Router action icon to/from
   // |component_action_delegate_| if necessary, depending on whether or not
-  // we have issues or local routes.
-  void MaybeAddOrRemoveAction();
+  // we have issues, local routes or a dialog.
+  virtual void MaybeAddOrRemoveAction();
 
   // Returns |true| if the Media Router action should be present on the toolbar
   // or the overflow menu.
@@ -69,6 +81,9 @@ class MediaRouterActionController : public media_router::IssuesObserver,
   bool has_issue_ = false;
   bool has_local_display_route_ = false;
 
+  // The number of dialogs that are currently open.
+  size_t dialog_count_ = 0;
+
   PrefChangeRegistrar pref_change_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaRouterActionController);