Show the Cast toolbar icon ephemerally when Cast is in use

chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc

Index: chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc b/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
index ad69808749ce7dd72bfd8aedcdc801c2c8f82e9b..f479ba86f80d45317ed7b722c5948d9ebdb6511d 100644
--- a/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
+++ b/chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc
@@ -15,6 +15,8 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
 #include "chrome/browser/ui/toolbar/media_router_action.h"
 #include "chrome/browser/ui/webui/constrained_web_dialog_ui.h"
 #include "chrome/browser/ui/webui/media_router/media_router_ui.h"
@@ -52,10 +54,9 @@ constexpr const int kWidth = 340;
 // will look like.
 class MediaRouterDialogDelegate : public WebDialogDelegate {
  public:
-  MediaRouterDialogDelegate(base::WeakPtr<MediaRouterAction> action,
+  explicit MediaRouterDialogDelegate(
       const base::WeakPtr<MediaRouterDialogControllerImpl>& controller)
-      : action_(action),
-        controller_(controller) {}
+      : controller_(controller) {}
   ~MediaRouterDialogDelegate() override {}
 
   // WebDialogDelegate implementation.
@@ -77,7 +78,13 @@ class MediaRouterDialogDelegate : public WebDialogDelegate {
     // MediaRouterUI adds its own message handlers.
   }
 
-  void GetDialogSize(gfx::Size* size) const override;
+  void GetDialogSize(gfx::Size* size) const override {
+    DCHECK(size);
+    // GetDialogSize() is called when the browser window resizes. We may want to
+    // update the maximum height of the dialog and scale the WebUI to the new
+    // height. |size| is not set because the dialog is auto-resizeable.
+    controller_->UpdateMaxDialogSize();
+  }
 
   std::string GetDialogArgs() const override {
     return std::string();
@@ -86,8 +93,7 @@ class MediaRouterDialogDelegate : public WebDialogDelegate {
   void OnDialogClosed(const std::string& json_retval) override {
     // We don't delete |this| here because this class is owned
     // by ConstrainedWebDialogDelegate.
-    if (action_)
-      action_->OnPopupHidden();
+    controller_.get()->OnDialogClosed();

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

This .get() is unnecessary.

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

Done.

   }
 
   void OnCloseContents(WebContents* source, bool* out_close_dialog) override {
@@ -100,20 +106,11 @@ class MediaRouterDialogDelegate : public WebDialogDelegate {
   }
 
  private:
-  base::WeakPtr<MediaRouterAction> action_;
   base::WeakPtr<MediaRouterDialogControllerImpl> controller_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaRouterDialogDelegate);
 };
 
-void MediaRouterDialogDelegate::GetDialogSize(gfx::Size* size) const {
-  DCHECK(size);
-  // GetDialogSize() is called when the browser window resizes. We may want to
-  // update the maximum height of the dialog and scale the WebUI to the new
-  // height. |size| is not set because the dialog is auto-resizeable.
-  controller_->UpdateMaxDialogSize();
-}
-
 }  // namespace
 
 // static
@@ -160,7 +157,10 @@ MediaRouterDialogControllerImpl::MediaRouterDialogControllerImpl(
     WebContents* web_contents)
     : MediaRouterDialogController(web_contents),
       media_router_dialog_pending_(false),
+      tab_strip_model_observer_(this),
       weak_ptr_factory_(this) {
+  tab_strip_model_observer_.Add(
+      chrome::FindBrowserWithWebContents(initiator())->tab_strip_model());
 }
 
 MediaRouterDialogControllerImpl::~MediaRouterDialogControllerImpl() {
@@ -173,12 +173,7 @@ WebContents* MediaRouterDialogControllerImpl::GetMediaRouterDialog() const {
 
 void MediaRouterDialogControllerImpl::SetMediaRouterAction(
     const base::WeakPtr<MediaRouterAction>& action) {
-  if (!action_)
-    action_ = action;
-}
-
-bool MediaRouterDialogControllerImpl::IsShowingMediaRouterDialog() const {
-  return GetMediaRouterDialog() != nullptr;
+  action_ = action;
 }
 
 void MediaRouterDialogControllerImpl::UpdateMaxDialogSize() {
@@ -208,6 +203,30 @@ void MediaRouterDialogControllerImpl::UpdateMaxDialogSize() {
   }
 }
 
+void MediaRouterDialogControllerImpl::OnDialogClosed() {
+  if (action_)
+    action_->OnPopupHidden();

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

It seems a little messy to have so much two-way communication between
MediaRouterDialogControllerImpl and MediaRouterAction.  Can we make the action
only respond to input from the controller, rather than needing to query it so
frequently?  Or make the action only observe the controller, rather than needing
the controller to inform the action?

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

I got rid of a call to MediaRouterAction::UpdateVisibility.

Now the action methods we call from the controller are OnPopupHidden and
OnPopupShown.

The controller methods we call from the action are SetMediaRouterAction,
IsShowingMediaRouterDialog, ShowMediaRouterDialog, and HideMediaRouterDialog.

We might be able to get rid of the calls to IsShowingMediaRouterDialog by
keeping a boolean for whether the popup is shown, and updating it from
OnPopupShown/Hidden and when there are tab switches etc. Would that be better
than calling IsShowingMediaRouterDialog?

I'm not sure what you mean by make the action observe the controller. Do you
mean making an observer interface for the controller, and making the controller
keep a list of observers that it calls on events?

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

At a high level, it's good to avoid two-way communication like this in code,
because it tightly couples everything.  It makes it more difficult to test, and
generally leads to code that is also more difficult to understand.  If there's a
way to have at least one of these classes be unaware of the other, it's
preferable.  An Observer or Delegate implementation is often used towards that
end - the observer posts events without knowledge of its consumers, and a
delegate is unaware of the owner's logic; they are each self-contained
implementations.  Those aren't the only solutions; they just happen to be used
commonly.

All that said, I'm not an OWNER of media router-specific code (just toolbar and
extension stuff), and this looks better than it was, so I'll leave it up to
Jennifer whether this should be changed further. :)

+}
+
+bool MediaRouterDialogControllerImpl::IsShowingMediaRouterDialog() const {
+  return GetMediaRouterDialog() != nullptr;
+}
+
+bool MediaRouterDialogControllerImpl::ShowMediaRouterDialog() {
+  if (!action_)
+    ShowMediaRouterAction();
+  return MediaRouterDialogController::ShowMediaRouterDialog();
+}
+
+void MediaRouterDialogControllerImpl::ActiveTabChanged(
+    content::WebContents* old_contents,
+    content::WebContents* new_contents,
+    int index,
+    int reason) {
+  if (IsShowingMediaRouterDialog() && !action_)
+    ShowMediaRouterAction();
+}
+
 void MediaRouterDialogControllerImpl::CloseMediaRouterDialog() {
   WebContents* media_router_dialog = GetMediaRouterDialog();
   if (!media_router_dialog)
@@ -235,7 +254,7 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
   // |web_dialog_delegate|'s owner is |constrained_delegate|.
   // |constrained_delegate| is owned by the parent |views::View|.
   WebDialogDelegate* web_dialog_delegate =
-      new MediaRouterDialogDelegate(action_, weak_ptr_factory_.GetWeakPtr());
+      new MediaRouterDialogDelegate(weak_ptr_factory_.GetWeakPtr());
 
   // |ShowConstrainedWebDialogWithAutoResize()| will end up creating
   // ConstrainedWebDialogDelegateViewViews containing a WebContents containing
@@ -266,14 +285,13 @@ void MediaRouterDialogControllerImpl::CreateMediaRouterDialog() {
 
   dialog_observer_.reset(new DialogWebContentsObserver(
       media_router_dialog, this));
-
-  if (action_)
-    action_->OnPopupShown();
 }
 
 void MediaRouterDialogControllerImpl::Reset() {
   MediaRouterDialogController::Reset();
   dialog_observer_.reset();
+  if (action_)
+    action_->UpdateVisibility();
 }
 
 void MediaRouterDialogControllerImpl::OnDialogNavigated(
@@ -295,6 +313,9 @@ void MediaRouterDialogControllerImpl::OnDialogNavigated(
   media_router_dialog_pending_ = false;
 
   PopulateDialog(media_router_dialog);
+
+  if (action_)
+    action_->OnPopupShown();
 }
 
 void MediaRouterDialogControllerImpl::PopulateDialog(
@@ -325,4 +346,18 @@ void MediaRouterDialogControllerImpl::PopulateDialog(
   }
 }
 
+void MediaRouterDialogControllerImpl::ShowMediaRouterAction() {
+  Profile* profile =
+      Profile::FromBrowserContext(initiator()->GetBrowserContext());
+  if (!profile)
+    return;
+
+  ToolbarActionsModel* model = ToolbarActionsModel::Get(profile);
+  if (model && !model->HasComponentAction(
+      ComponentToolbarActionsFactory::kMediaRouterActionId)) {

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

indentation seems off here.

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

Fixed.

+    model->AddComponentAction(
+        ComponentToolbarActionsFactory::kMediaRouterActionId);
+  }
+}
+
 }  // namespace media_router