Create MediaRouterActionController and MediaRouterUIService

chrome/browser/ui/toolbar/media_router_action_controller.h

+// 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.
+
+#ifndef CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_CONTROLLER_H_
+#define CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_CONTROLLER_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/observer_list.h"
+#include "chrome/browser/extensions/component_migration_helper.h"

[mark a. foltz, 2016/09/09 17:48:29]

Can you forward declare ComponentMigrationHelper?

[takumif, 2016/09/12 19:01:32]

No, because it includes declaration for ComponentActionDelegate. Perhaps
ComponentActionDelegate should be in its own file?

+#include "chrome/browser/media/router/issues_observer.h"
+#include "chrome/browser/media/router/media_router_factory.h"

[mark a. foltz, 2016/09/09 17:48:29]

Is this used? Or can you forward declare MediaRouter?

[takumif, 2016/09/12 19:01:32]

It's not used here. Removed.

+#include "chrome/browser/media/router/media_routes_observer.h"
+#include "chrome/browser/profiles/profile.h"

[mark a. foltz, 2016/09/09 17:48:29]

Can you forward declare Profile?

[takumif, 2016/09/12 19:01:31]

Done.

+#include "components/prefs/pref_change_registrar.h"
+
+class MediaRouterActionControllerUnitTest;

[mark a. foltz, 2016/09/09 17:48:29]

This seems to be unused

[takumif, 2016/09/12 19:01:32]

Removed.

+
+// Controller for MediaRouterAction that determines when to show and hide the

[mark a. foltz, 2016/09/09 17:48:29]

Please document threading assumptions, i.e. this class should only be used on
the UI thread.

[takumif, 2016/09/12 19:01:31]

Done.

+// action icon on the toolbar.
+// There should be one instance of this class per profile.

[mark a. foltz, 2016/09/09 17:48:28]

Sorry for the churn, but my current thinking is we should bite the bullet and
make a ProfileKeyed service that owns the MRAC.  That way we can keep a cleaner
layering of UI and MR code.

e.g.

MediaRouterUIService ---(owns)---> MRAC
MediaRouterUIService ---(depends)---> ToolbarActionsModel 
MediaRouterUIService ---(depends)---> MediaRouter

For now, it will just have the MRAC, but if there are other aspects to the MR UI
model that are profile keyed, they can be owned by that service as well.

What do you think Derek?  Are there other profile keyed objects that would
belong there?

[imcheng, 2016/09/09 18:12:25]

I think that makes sense given that we face opposition in introducing dependency
in ToolbarActionsModel. It is another way to express dependency / lifetime
guarantee of TAM and MR, but it does isolate the ToolbarActionsModel keyed
service from dependency to MR, which I suppose is beneficial in some ways.

+class MediaRouterActionController : public media_router::IssuesObserver,
+                                    public media_router::MediaRoutesObserver {
+ public:
+  // MediaRouterAction will observe MediaRouterActionController to update its
+  // icon.
+  class Observer {

[mark a. foltz, 2016/09/09 17:48:29]

This does not seem to be used (yet).  Is there a separate patch to
MediaRouterAction coming?

Design: 
Without seeing the code that implements this observer, it's hard to evaluate the
API.  I might have an API that more closely mirrors the intended states of the
MRA; that puts the logic that maps the state of the MR to the state of the MRA
into the controller, and means the MRA doesn't have to depend directly on MR
objects.

E.g.

OnWarningIssue()
OnSevereIssue()
OnIssueResolved()

// Some route is connected
OnRouteConnected()
// All routes are terminated
OnRoutesTerminated()

+   public:
+    virtual void OnIssueUpdated(const media_router::Issue* issue) = 0;
+    virtual void OnLocalRouteUpdated(bool has_local_route) = 0;
+  };
+
+  MediaRouterActionController(Profile* profile,
+                              media_router::MediaRouter* router);
+  MediaRouterActionController(

[mark a. foltz, 2016/09/09 17:48:28]

Normally |router| and |component_action_delegate| would be looked up from
|profile|.
If this ctor is for tests-only to inject dependencies, please comment it as
such.

[takumif, 2016/09/12 19:01:32]

Done.

+      Profile* profile,
+      media_router::MediaRouter* router,
+      extensions::ComponentMigrationHelper::ComponentActionDelegate*
+          component_action_delegate);
+  ~MediaRouterActionController() override;
+
+  // media_router::IssuesObserver:
+  void OnIssueUpdated(const media_router::Issue* issue) override;
+
+  // media_router::MediaRoutesObserver:
+  void OnRoutesUpdated(const std::vector<media_router::MediaRoute>& routes,
+                       const std::vector<media_router::MediaRoute::Id>&
+                           joinable_route_ids) override;
+
+  // Sets the preferences to always show the Media Router action icon on the
+  // toolbar.
+  // Also updates the presence of the action if necessary.

[mark a. foltz, 2016/09/09 17:48:29]

Nit: wrap comment to previous line

[takumif, 2016/09/12 19:01:31]

Done.

+  void SetAlwaysShowActionPref(bool always_show);
+
+  bool HasLocalRoute() const;

[mark a. foltz, 2016/09/09 17:48:28]

Should this be part of the Observer API?

If this is to initialize the state of the action, you could have this call
Observer::OnLocalRouteUpdated (or the equivalent) when an observer is first
added.

+
+  // Returns the current issue, or a nullptr if there is none.
+  media_router::Issue* GetIssue() const;

[mark a. foltz, 2016/09/09 17:48:29]

Similar comments to |HasLocalRoute()|.

+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ private:
+  // Adds or removes the Media Router action icon to/from
+  // |component_action_delegate_| if necessary, depending on whether or not
+  // we have issues or local routes.
+  void MaybeAddOrRemoveAction();
+
+  // Whether the preference to always show the Media Router action icon is
+  // turned on for the current profile.
+  bool GetAlwaysShowActionPref();
+
+  // Returns ture if the Media Router action should be present on the toolbar
+  // or the overflow menu.
+  bool IsActionEnabled();
+
+  // The profile |this| is associated with. There should be one instance of this
+  // class per profile.
+  Profile* const profile_;
+
+  // The delegate that is responsible for showing and hiding the icon on the
+  // toolbar. It is a KeyedService and outlives |this|.
+  extensions::ComponentMigrationHelper::ComponentActionDelegate* const
+      component_action_delegate_;
+
+  std::unique_ptr<media_router::Issue> issue_;
+  bool has_local_display_route_;
+
+  base::ObserverList<Observer> observers_;
+
+  PrefChangeRegistrar pref_change_registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterActionController);
+};
+
+#endif  // CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_CONTROLLER_H_