Create MediaRouterActionController and MediaRouterUIService

chrome/browser/ui/toolbar/media_router_action_controller.cc

+// Copyright 2016 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_controller.h"
+
+#include "chrome/browser/extensions/component_migration_helper.h"
+#include "chrome/browser/media/router/media_router_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
+#include "chrome/common/pref_names.h"
+
+MediaRouterActionController::MediaRouterActionController(Profile* profile)
+    : MediaRouterActionController(
+          profile,
+          media_router::MediaRouterFactory::GetApiForBrowserContext(profile),
+          ToolbarActionsModel::Get(profile),
+          ToolbarActionsModel::Get(profile)->component_migration_helper()) {}
+
+MediaRouterActionController::MediaRouterActionController(
+    Profile* profile,
+    media_router::MediaRouter* router,
+    extensions::ComponentMigrationHelper::ComponentActionDelegate*

[imcheng, 2016/09/13 23:13:10]

nit: I think we should extract ComponentActionDelegate out of
ComponentMigrationHelper, now that it is no longer specific to CMH.

[takumif, 2016/09/14 04:00:52]

I'm not sure where that'd go. ComponentMigrationHelper is in c/b/extensions, but
ComponentActionDelegate doesn't really have to do with extensions. Maybe
c/b/ui/toolbar is better?

The extraction would also touch ToolbarActionsModel, etc. Maybe it can happen in
its own CL?

+        component_action_delegate,
+    extensions::ComponentMigrationHelper* component_migration_helper)
+    : media_router::IssuesObserver(router),
+      media_router::MediaRoutesObserver(router),
+      profile_(profile),
+      component_action_delegate_(component_action_delegate),
+      component_migration_helper_(component_migration_helper),
+      has_issue_(false),
+      has_local_display_route_(false) {
+  DCHECK(profile_);
+  DCHECK(component_action_delegate_);
+  RegisterObserver();  // media_router::IssuesObserver.
+  pref_change_registrar_.Init(profile->GetPrefs());

[mark a. foltz, 2016/09/13 18:23:05]

I wonder if changing the pref in incognito affects the pref in the parent
profile.
If not, do we hide the checkbox?  Hmmm.

[takumif, 2016/09/14 04:00:52]

Pref changes made while in incognito do persist. Is that what we want? If not,
we'd either need a separate pref dictionary for incognito, or not let the icon
persist in incognito, either of which would require changes to
ComponentMigrationHelper. Or let it share the pref but not show the context menu
option, as you mentioned.

Showing/hiding of the ephemeral icon is synced in real-time between incognito
and normal tabs as well. My understanding is that this happens because the MR
instance is shared between incognito and normal profiles.

+  pref_change_registrar_.Add(
+      prefs::kToolbarMigratedComponentActionStatus,
+      base::Bind(&MediaRouterActionController::MaybeAddOrRemoveAction,
+                 base::Unretained(this)));
+}
+
+MediaRouterActionController::~MediaRouterActionController() {
+  UnregisterObserver();  // media_router::IssuesObserver.
+}
+
+void MediaRouterActionController::OnIssueUpdated(
+    const media_router::Issue* issue) {
+  has_issue_ = issue != nullptr;
+  MaybeAddOrRemoveAction();
+}
+
+void MediaRouterActionController::OnRoutesUpdated(
+    const std::vector<media_router::MediaRoute>& routes,
+    const std::vector<media_router::MediaRoute::Id>& joinable_route_ids) {
+  has_local_display_route_ =
+      std::find_if(routes.begin(), routes.end(),
+                   [](const media_router::MediaRoute& route) {
+                     return route.is_local() && route.for_display();
+                   }) != routes.end();
+
+  MaybeAddOrRemoveAction();
+}
+
+void MediaRouterActionController::MaybeAddOrRemoveAction() {
+  const std::string& action_id =
+      ComponentToolbarActionsFactory::kMediaRouterActionId;

[mark a. foltz, 2016/09/13 18:23:05]

Can you write 

using ComponentToolbarActionsFactory::kMediaRouterActionId;

and 

kMediaRouterActionId below.

(Maybe the compiler will optimize out the pointer you're allocating; unsure.)

[takumif, 2016/09/14 04:00:52]

using ComponentToolbarActionsFactory::kMediaRouterActionId;

gives a compilation error (can't use "using" for a class member). Making it a
file-level const reference in an unnamed namespace.

+
+  if (IsActionEnabled()) {
+    if (!component_action_delegate_->HasComponentAction(action_id))
+      component_action_delegate_->AddComponentAction(action_id);
+  } else {
+    if (component_action_delegate_->HasComponentAction(action_id))
+      component_action_delegate_->RemoveComponentAction(action_id);
+  }
+}
+
+bool MediaRouterActionController::IsActionEnabled() {

[imcheng, 2016/09/13 23:13:10]

I think this method makes more sense if it is named ShouldEnableAction()

[takumif, 2016/09/14 04:00:52]

Renamed.

+  return component_migration_helper_->GetComponentActionPref(

[imcheng, 2016/09/13 23:13:10]

nit: as a small optimization, we can reorder the statement to check
has_local_display_route_ and has_issue_ first.

[takumif, 2016/09/14 04:00:52]

Done.

+             ComponentToolbarActionsFactory::kMediaRouterActionId) ||
+         has_local_display_route_ || has_issue_;
+}