MediaRouterAction: Only observe Media Routes when there is a local route.

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/strings/utf_string_conversions.h"
 #include "chrome/browser/media/router/issue.h"
 #include "chrome/browser/media/router/media_route.h"
 #include "chrome/browser/media/router/media_router.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/media_router_mojo_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.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_platform_delegate.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_delegate.h"
 #include "chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.h"
 #include "chrome/grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image_skia.h"
 
 using media_router::MediaRouterDialogControllerImpl;
 
+MediaRouterAction::ActionMediaRoutesObserver::ActionMediaRoutesObserver(
+    media_router::MediaRouter* router,
+    MediaRouterAction* action)
+    : MediaRoutesObserver(router),
+      action_(action) {
+  DCHECK(action);
+}
+
+MediaRouterAction::ActionMediaRoutesObserver::~ActionMediaRoutesObserver() {
+}
+
+void MediaRouterAction::ActionMediaRoutesObserver::OnRoutesUpdated(
+    const std::vector<media_router::MediaRoute>& routes) {
+  bool has_local_route =
+      std::find_if(routes.begin(), routes.end(),
+                   [](const media_router::MediaRoute& route) {
+                      return route.is_local(); }) !=
+      routes.end();
+
+  action_->SetHasLocalRoute(has_local_route);

[imcheng, 2015/10/01 18:46:50]

there should be a comment that |this| will be deleted beyond this point if
has_local_route is false.

[apacible, 2015/10/01 23:21:47]

Done (after moving this to MediaRouterMojoImpl).

+}
+
 MediaRouterAction::MediaRouterAction(Browser* browser)
     : media_router::IssuesObserver(GetMediaRouter(browser)),
-      media_router::MediaRoutesObserver(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_route_(false),
       delegate_(nullptr),
       browser_(browser),
+      routes_observer_(new ActionMediaRoutesObserver(

[imcheng, 2015/10/01 18:46:50]

So we are still sending a routes query on construction. I want to see if we can
delay that until we know there is a local route. In fact, we can probably only
depend on LocalMediaRoutesObserver in this class. We may have to add an
HasLocalRoutes() to MediaRouter to determine the initial state.

[apacible, 2015/10/01 23:21:47]

Talked offline. Done.

+                           GetMediaRouter(browser), this)),
       platform_delegate_(MediaRouterActionPlatformDelegate::Create(browser)),
       contextual_menu_(browser),
       tab_strip_model_observer_(this),
       weak_ptr_factory_(this) {
   DCHECK(browser_);
   tab_strip_model_observer_.Add(browser_->tab_strip_model());
 }
 
 MediaRouterAction::~MediaRouterAction() {
+  routes_observer_.reset();

[imcheng, 2015/10/01 18:46:50]

not needed?

[apacible, 2015/10/01 23:21:47]

Done.

 }
 
 std::string MediaRouterAction::GetId() const {
   return ComponentToolbarActionsFactory::kMediaRouterActionId;
 }
 
 void MediaRouterAction::SetDelegate(ToolbarActionViewDelegate* delegate) {
   delegate_ = delegate;
 
   // Updates the current popup state if |delegate_| is non-null and has
@@ skipping 67 matching lines @@
 bool MediaRouterAction::DisabledClickOpensMenu() const {
   return false;
 }
 
 void MediaRouterAction::OnIssueUpdated(const media_router::Issue* issue) {
   issue_.reset(issue ? new media_router::Issue(*issue) : nullptr);
 
   MaybeUpdateIcon();
 }
 
-void MediaRouterAction::OnRoutesUpdated(
-    const std::vector<media_router::MediaRoute>& routes) {
-  has_local_route_ =
-      std::find_if(routes.begin(), routes.end(),
-                   [](const media_router::MediaRoute& route) {
-                      return route.is_local(); }) !=
-      routes.end();
-  MaybeUpdateIcon();
+void MediaRouterAction::OnLocalRouteCreated() {
+  if (!routes_observer_)
+    routes_observer_.reset(new ActionMediaRoutesObserver(
+        GetMediaRouter(browser_), this));
 }
 
 void MediaRouterAction::ActiveTabChanged(content::WebContents* old_contents,
                                          content::WebContents* new_contents,
                                          int index,
                                          int reason) {
   UpdatePopupState();
 }
 
 void MediaRouterAction::OnPopupHidden() {
   if (delegate_)
     delegate_->OnPopupClosed();
 }
 
 void MediaRouterAction::OnPopupShown() {
   // We depress the action regardless of whether ExecuteAction() was user
   // initiated.
   if (delegate_)
     delegate_->OnPopupShown(true);
 }
 
+void MediaRouterAction::SetHasLocalRoute(bool has_local_route) {
+  has_local_route_ = has_local_route;
+
+  if (!has_local_route_)
+    routes_observer_.reset();
+
+  MaybeUpdateIcon();
+}
+
 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());
 
   // Update the button in case the pressed state is out of sync with dialog
   // visibility.
   if (controller->IsShowingMediaRouterDialog())
     OnPopupShown();
   else
     OnPopupHidden();
 }
 
 MediaRouterDialogControllerImpl*
 MediaRouterAction::GetMediaRouterDialogController() {
   DCHECK(delegate_);
   content::WebContents* web_contents = delegate_->GetCurrentWebContents();
   DCHECK(web_contents);
   return MediaRouterDialogControllerImpl::GetOrCreateForWebContents(
       web_contents);
 }
 
 media_router::MediaRouter* MediaRouterAction::GetMediaRouter(Browser* browser) {

[imcheng, 2015/10/01 18:46:50]

Can this be a non-member function in anonymous namespace?

[apacible, 2015/10/01 23:21:47]

Done.

   return media_router::MediaRouterFactory::GetApiForBrowserContext(
       static_cast<content::BrowserContext*>(browser->profile()));

[imcheng, 2015/10/01 18:46:50]

Is the static_cast necessary?

[apacible, 2015/10/01 23:21:47]

Removed.

 }
 
 MediaRouterActionPlatformDelegate* MediaRouterAction::GetPlatformDelegate() {
   return platform_delegate_.get();
 }
 
 void MediaRouterAction::MaybeUpdateIcon() {
   const gfx::Image* new_icon = GetCurrentIcon();
 
   // Update the current state if it has changed.
@@ skipping 12 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_route_ ?
       &media_router_active_icon_ : &media_router_idle_icon_;
 }