Show the Cast toolbar icon ephemerally when Cast is in use

chrome/browser/ui/toolbar/media_router_action.cc

Index: chrome/browser/ui/toolbar/media_router_action.cc
diff --git a/chrome/browser/ui/toolbar/media_router_action.cc b/chrome/browser/ui/toolbar/media_router_action.cc
index 97c60b2a4d4204916f94fb4734162b036ff85d49..aed2f7d81a235d67557860f4a05a63a8e37c8c87 100644
--- a/chrome/browser/ui/toolbar/media_router_action.cc
+++ b/chrome/browser/ui/toolbar/media_router_action.cc
@@ -17,8 +17,13 @@
 #include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
 #include "chrome/browser/ui/toolbar/media_router_action_platform_delegate.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_delegate.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
 #include "chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/pref_service_factory.h"
+#include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image_skia.h"
@@ -47,11 +52,13 @@ MediaRouterAction::MediaRouterAction(Browser* browser,
       platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)),
       contextual_menu_(browser),
       tab_strip_model_observer_(this),
+      toolbar_actions_bar_observer_(this),
       weak_ptr_factory_(this) {
   DCHECK(browser_);
   DCHECK(toolbar_actions_bar_);
   tab_strip_model_observer_.Add(browser_->tab_strip_model());
-
+  toolbar_actions_bar_observer_.Add(toolbar_actions_bar_);
+  contextual_menu_.SetMediaRouterAction(this);
   RegisterObserver();
 }
 
@@ -113,7 +120,6 @@ bool MediaRouterAction::HasPopup(
 
 void MediaRouterAction::HidePopup() {
   GetMediaRouterDialogController()->HideMediaRouterDialog();
-  OnPopupHidden();
 }
 
 gfx::NativeView MediaRouterAction::GetPopupNativeView() {
@@ -160,12 +166,15 @@ void MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
 void MediaRouterAction::OnRoutesUpdated(
     const std::vector<media_router::MediaRoute>& routes,
     const std::vector<media_router::MediaRoute::Id>& joinable_route_ids) {
+  bool had_local_display_route = has_local_display_route_;
   has_local_display_route_ =
       std::find_if(routes.begin(), routes.end(),
                    [](const media_router::MediaRoute& route) {
                      return route.is_local() && route.for_display();
                    }) != routes.end();
   MaybeUpdateIcon();
+  if (has_local_display_route_ != had_local_display_route)
+    UpdateVisibility();
 }
 
 void MediaRouterAction::ActiveTabChanged(content::WebContents* old_contents,
@@ -173,6 +182,41 @@ void MediaRouterAction::ActiveTabChanged(content::WebContents* old_contents,
                                          int index,
                                          int reason) {
   UpdatePopupState();
+  UpdateVisibility();
+}
+
+void MediaRouterAction::ToggleVisibilityPreference() {
+  browser_->profile()->GetPrefs()->SetBoolean(
+      prefs::kMediaRouterAlwaysShowActionIcon, !ShouldAlwaysShowIcon());
+  UpdateVisibility();
+}
+
+void MediaRouterAction::UpdateVisibility() {
+  if (!delegate_)

[Devlin, 2016/07/29 19:10:22]

Document when this can happen.

[takumif, 2016/08/02 04:58:41]

This should only happen in tests. Would it make more sense to override these
methods in the TestMediaRouterAction class and check for |delegate_| there (and
replace the ones here with DCHECKs)? Something like:

void TestMediaRouterAction::UpdateVisibility() override {
  if (!delegate_)
    return;
  MediaRouterAction::UpdateVisibility();
}

[Devlin, 2016/08/02 20:18:51]

Ideally, I think my preference would be to create a
TestToolbarActionViewDelegate() with stub implementations for most of the
methods.  Is that feasible?

[takumif, 2016/08/03 22:03:50]

We do have a mock delegate, and since it wasn't always being set, I changed
that. Now the dialog controller is null, and this one can't really be mocked
without the web content present, so I overrode this method (now renamed to
MaybeRemoveAction) in the TestMediaRouterAction to check for the presence of
dialog controller there.

+    return;
+
+  MediaRouterDialogControllerImpl* controller =
+      GetMediaRouterDialogController();
+  bool show_icon = ShouldAlwaysShowIcon() ||
+                   controller->IsShowingMediaRouterDialog() ||
+                   has_local_display_route_;
+  ToolbarActionsModel* model = ToolbarActionsModel::Get(browser_->profile());
+  if (show_icon) {
+    if (!model->HasComponentAction(GetId()))

[Devlin, 2016/07/29 19:10:22]

Thinking about this.  In theory, the ToolbarActionsBar should represent the
state of the ToolbarActionsModel % a few ordering concerns (e.g. popped out
actions).  When could this action exist, but not be present in the model?

[takumif, 2016/08/02 04:58:41]

Right, now that I reverted the changes to the ToolbarActionsBar, the action
should exist iff it's present in the model (except for in tests). That means
this wouldn't get called. Changing the method and its name to take care of just
removing the action.

+      model->AddComponentAction(GetId());
+  } else if (model->HasComponentAction(GetId())) {
+    model->RemoveComponentAction(GetId());
+  }
+}
+
+void MediaRouterAction::OnToolbarActionsBarAnimationEnded() {
+  // Depress the action if the dialog is shown, release it otherwise.
+  if (delegate_) {

[Devlin, 2016/07/29 19:10:22]

again, document when this can be false.

[takumif, 2016/08/02 04:58:41]

Please see the comment at line 195

+    if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog())
+      delegate_->OnPopupShown(true);
+    else
+      delegate_->OnPopupClosed();
+  }
 }
 
 void MediaRouterAction::OnPopupHidden() {
@@ -203,11 +247,14 @@ void MediaRouterAction::UpdatePopupState() {
     controller->SetMediaRouterAction(weak_ptr_factory_.GetWeakPtr());
 
   // Update the button in case the pressed state is out of sync with dialog
-  // visibility.
-  if (controller->IsShowingMediaRouterDialog())
-    OnPopupShown();
-  else
-    OnPopupHidden();
+  // visibility. If we just added the ephemeral icon to the toolbar and need to
+  // depress it, we do that in |OnToolbarActionsBarAnimationEnded|.
+  if (delegate_) {

[Devlin, 2016/07/29 19:10:22]

ditto

[takumif, 2016/08/02 04:58:41]

Please see line 195

+    if (ShouldAlwaysShowIcon() && controller->IsShowingMediaRouterDialog())
+      delegate_->OnPopupShown(true);
+    else if (!controller->IsShowingMediaRouterDialog())
+      delegate_->OnPopupClosed();
+  }
 }
 
 MediaRouterDialogControllerImpl*
@@ -249,3 +296,12 @@ gfx::VectorIconId MediaRouterAction::GetCurrentIcon() const {
   return has_local_display_route_ ? gfx::VectorIconId::MEDIA_ROUTER_ACTIVE
                                   : gfx::VectorIconId::MEDIA_ROUTER_IDLE;
 }
+
+base::WeakPtr<MediaRouterAction> MediaRouterAction::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+bool MediaRouterAction::ShouldAlwaysShowIcon() {
+  PrefService* pref_service = browser_->profile()->GetPrefs();
+  return pref_service->GetBoolean(prefs::kMediaRouterAlwaysShowActionIcon);
+}