[Media Router] Check MediaRouterDialogControllerImpl only sets toolbar MediaRouterAction.

chrome/browser/ui/toolbar/media_router_action.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/toolbar/media_router_action.h"
 
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/router/issue.h"
 #include "chrome/browser/media/router/media_route.h"
@@ skipping 18 matching lines @@
 
 namespace {
 
 media_router::MediaRouter* GetMediaRouter(Browser* browser) {
   return media_router::MediaRouterFactory::GetApiForBrowserContext(
       browser->profile());
 }
 
 }  // namespace
 
-MediaRouterAction::MediaRouterAction(Browser* browser)
+MediaRouterAction::MediaRouterAction(Browser* browser,
+                                     ToolbarActionsBar* toolbar_actions_bar)
     : media_router::IssuesObserver(GetMediaRouter(browser)),
       media_router::LocalMediaRoutesObserver(GetMediaRouter(browser)),
       media_router_active_icon_(
           ui::ResourceBundle::GetSharedInstance()
               .GetImageNamed(IDR_MEDIA_ROUTER_ACTIVE_ICON)),
       media_router_error_icon_(ui::ResourceBundle::GetSharedInstance()
                                    .GetImageNamed(IDR_MEDIA_ROUTER_ERROR_ICON)),
       media_router_idle_icon_(ui::ResourceBundle::GetSharedInstance()
                                   .GetImageNamed(IDR_MEDIA_ROUTER_IDLE_ICON)),
       media_router_warning_icon_(
           ui::ResourceBundle::GetSharedInstance()
               .GetImageNamed(IDR_MEDIA_ROUTER_WARNING_ICON)),
       current_icon_(&media_router_idle_icon_),
       has_local_display_route_(false),
       delegate_(nullptr),
       browser_(browser),
+      toolbar_actions_bar_(toolbar_actions_bar),
       platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)),
       contextual_menu_(browser),
       tab_strip_model_observer_(this),
       weak_ptr_factory_(this) {
   DCHECK(browser_);
+  DCHECK(toolbar_actions_bar_);
   tab_strip_model_observer_.Add(browser_->tab_strip_model());
 
   RegisterObserver();
   OnHasLocalDisplayRouteUpdated(
       GetMediaRouter(browser)->HasLocalDisplayRoute());
 }
 
 MediaRouterAction::~MediaRouterAction() {
   UnregisterObserver();
 }
@@ skipping 114 matching lines @@
     delegate_->OnPopupShown(true);
 }
 
 void MediaRouterAction::UpdatePopupState() {
   MediaRouterDialogControllerImpl* controller =
       GetMediaRouterDialogController();
 
   if (!controller)
     return;
 
-  // Immediately keep track of MediaRouterAction in the controller. If it was
-  // already set, this should be a no-op.
-  controller->SetMediaRouterAction(weak_ptr_factory_.GetWeakPtr());
+  // 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.

[Peter Kasting, 2015/12/03 22:00:19]

Now the problem is that there's lots of useful background info here but I'm not
sure precisely how it relates to what this code is doing.  This is primarily
because I don't really understand what UpdatePopupState() is doing and when it's
run relative to when the other events you describe here (e.g. the app menu
dropdown with the overflow area in it being closed) are happening.  I note that
the header file here includes no comments on what this function does...

[apacible, 2015/12/07 21:52:43]

Added comments in the header file for UpdatePopupState().

+  // 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();
   else
     OnPopupHidden();
 }
 
 MediaRouterDialogControllerImpl*
@@ skipping 28 matching lines @@
   if (issue_) {
     if (issue_->severity() == media_router::Issue::FATAL)
       return &media_router_error_icon_;
     if (issue_->severity() == media_router::Issue::WARNING)
       return &media_router_warning_icon_;
   }
 
   return has_local_display_route_ ? &media_router_active_icon_
                                   : &media_router_idle_icon_;
 }