Show media router toolbar icon ephemerally for active local routes and issues

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..dbb900714985e2734f1f1ac5e47fa30e62500b0e 100644
--- a/chrome/browser/ui/toolbar/media_router_action.cc
+++ b/chrome/browser/ui/toolbar/media_router_action.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/toolbar/media_router_action.h"
 
+#include <string>
+
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/router/issue.h"
@@ -17,6 +19,7 @@
 #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/grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -41,17 +44,19 @@ MediaRouterAction::MediaRouterAction(Browser* browser,
       media_router::MediaRoutesObserver(GetMediaRouter(browser)),
       current_icon_(gfx::VectorIconId::MEDIA_ROUTER_IDLE),
       has_local_display_route_(false),
+      has_dialog_(false),
       delegate_(nullptr),
       browser_(browser),
       toolbar_actions_bar_(toolbar_actions_bar),
       platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)),
-      contextual_menu_(browser),
+      contextual_menu_(browser, this),
       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_);
   RegisterObserver();
 }
 
@@ -66,13 +71,18 @@ std::string MediaRouterAction::GetId() const {
 void MediaRouterAction::SetDelegate(ToolbarActionViewDelegate* delegate) {
   delegate_ = delegate;
 
-  // Updates the current popup state if |delegate_| is non-null and has
-  // WebContents ready.
+  // Updates the current dialog state if |delegate_| has WebContents ready.
   // In cases such as opening a new browser window, SetDelegate() will be
-  // called before the WebContents is set. In those cases, we update the popup
+  // called before the WebContents is set. In those cases, we update the dialog
   // state when ActiveTabChanged() is called.
-  if (delegate_ && delegate_->GetCurrentWebContents())
-    UpdatePopupState();
+  if (delegate_ && delegate_->GetCurrentWebContents()) {
+    RegisterWithDialogController();
+    // Update the button in case the pressed state is out of sync with dialog

[imcheng, 2016/09/16 21:47:49]

nit: blank line before comments

+    // visibility. If we just added the ephemeral icon to the toolbar and need
+    // to depress it, we do that in |OnToolbarActionsBarAnimationEnded|.
+    if (GetAlwaysShowActionPref())

[imcheng, 2016/09/16 21:47:48]

question: So if I added the icon by preference (i.e., not ephemerally) then
OnToolbarActionsBarAnimationEnded won't get called, hence the need to call
UpdateDialogState here? Why would that be the case?

[takumif, 2016/09/20 14:50:46]

Actually, OnToolbarActionsBarAnimationEnded, OnActiveTabCahged, and the calls
from MRDialogControllerImpl cover all the cases, so this is not necessary.
Removing.

+      UpdateDialogState();
+  }
 }
 
 gfx::Image MediaRouterAction::GetIcon(content::WebContents* web_contents,
@@ -113,7 +123,6 @@ bool MediaRouterAction::HasPopup(
 
 void MediaRouterAction::HidePopup() {
   GetMediaRouterDialogController()->HideMediaRouterDialog();
-  OnPopupHidden();
 }
 
 gfx::NativeView MediaRouterAction::GetPopupNativeView() {
@@ -143,8 +152,8 @@ bool MediaRouterAction::ExecuteAction(bool by_user) {
 }
 
 void MediaRouterAction::UpdateState() {
-  if (delegate_)
-    delegate_->UpdateState();
+  DCHECK(delegate_);
+  delegate_->UpdateState();
 }
 
 bool MediaRouterAction::DisabledClickOpensMenu() const {
@@ -153,7 +162,6 @@ bool MediaRouterAction::DisabledClickOpensMenu() const {
 
 void MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
   issue_.reset(issue ? new media_router::Issue(*issue) : nullptr);
-
   MaybeUpdateIcon();
 }
 
@@ -172,42 +180,72 @@ void MediaRouterAction::ActiveTabChanged(content::WebContents* old_contents,
                                          content::WebContents* new_contents,
                                          int index,
                                          int reason) {
-  UpdatePopupState();
+  RegisterWithDialogController();
+  UpdateDialogState();
 }
 
-void MediaRouterAction::OnPopupHidden() {
-  if (delegate_)
+void MediaRouterAction::OnToolbarActionsBarAnimationEnded() {
+  // Depress the action if the dialog is shown.
+  DCHECK(delegate_);
+  if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog())

[imcheng, 2016/09/16 21:47:48]

Is it possible for this to return false, and do we need to call OnDialogHidden()
in that case? (Or just call UpdateDialogState() here)?

[takumif, 2016/09/20 14:50:46]

This can be false in cases such as receiving an issue while the dialog is not
open. This callback's purpose is to depress the newly created action, and we
don't have to worry about calling OnDialogHidden() because it comes
un-depressed.

+    OnDialogShown();
+}
+
+void MediaRouterAction::OnDialogHidden() {
+  if (has_dialog_) {
+    has_dialog_ = false;
+    DCHECK(delegate_);
     delegate_->OnPopupClosed();

[imcheng, 2016/09/16 21:47:49]

Will |has_dialog_| always be in sync with |delegate_|'s state?

[takumif, 2016/09/20 14:50:46]

It should be. We only call OnPopupShown/Closed() when we change |has_dialog_|.

+  }
 }
 
-void MediaRouterAction::OnPopupShown() {
-  // We depress the action regardless of whether ExecuteAction() was user
-  // initiated.
-  if (delegate_)
+void MediaRouterAction::OnDialogShown() {
+  if (!has_dialog_) {
+    has_dialog_ = true;
+    DCHECK(delegate_);
+    // We depress the action regardless of whether ExecuteAction() was user
+    // initiated.
     delegate_->OnPopupShown(true);
+  }
+}
+
+bool MediaRouterAction::GetAlwaysShowActionPref() const {
+  return ToolbarActionsModel::Get(browser_->profile())
+      ->component_migration_helper()->GetComponentActionPref(
+          ComponentToolbarActionsFactory::kMediaRouterActionId);
+}
+
+void MediaRouterAction::SetAlwaysShowActionPref(bool always_show) {
+  ToolbarActionsModel::Get(browser_->profile())->component_migration_helper()
+      ->SetComponentActionPref(
+          ComponentToolbarActionsFactory::kMediaRouterActionId, always_show);
 }
 
-void MediaRouterAction::UpdatePopupState() {
+base::WeakPtr<MediaRouterAction> MediaRouterAction::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+void MediaRouterAction::RegisterWithDialogController() {
   MediaRouterDialogControllerImpl* controller =
       GetMediaRouterDialogController();
 
   if (!controller)
     return;
 
-  // When each browser window is created, its toolbar creates a
-  // MediaRouterAction that is only destroyed when the browser window is torn
-  // down. |controller| will keep track of that instance. If |this| was created
-  // in overflow mode, it will be destroyed when the overflow menu is closed.
-  // If SetMediaRouterAction() was previously called, this is a no-op.
+  // |controller| keeps track of |this| if |this| was created with the browser
+  // window or ephemerally by activating the Cast functionality. If |this| was
+  // created in overflow mode, it will be destroyed when the overflow menu is
+  // closed. If SetMediaRouterAction() was previously called, this is a no-op.
   if (!toolbar_actions_bar_->in_overflow_mode())
     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();
+void MediaRouterAction::UpdateDialogState() {
+  DCHECK(delegate_);

[imcheng, 2016/09/16 21:47:49]

nit: this DCHECK not needed as the methods that you call already have them.

[takumif, 2016/09/20 14:50:46]

Removed.

+  if (GetMediaRouterDialogController()->IsShowingMediaRouterDialog())
+    OnDialogShown();
   else
-    OnPopupHidden();
+    OnDialogHidden();
 }
 
 MediaRouterDialogControllerImpl*
@@ -231,9 +269,10 @@ void MediaRouterAction::MaybeUpdateIcon() {
     current_icon_ = new_icon;
 
     // Tell the associated view to update its icon to reflect the change made
-    // above.
+    // above. If MaybeUpdateIcon() was called as a result of instantiating
+    // |this|, then |delegate_| may not be set yet.
     if (delegate_)
-      delegate_->UpdateState();
+      UpdateState();
   }
 }