[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());
+  // Only call SetMediaRouterAction() if |this| lives on the toolbar rather
+  // than the overflow menu. If it was already set, this should be a no-op.

[Peter Kasting, 2015/12/03 04:48:53]

This first sentence restates what the code does.  If you need to leave comments
on the code, they should describe _why_ more than _what_.

[apacible, 2015/12/03 21:53:10]

Done.

+  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_;
 }