Create MediaRouterActionController and MediaRouterUIService

chrome/browser/ui/toolbar/media_router_action_controller.h

Index: chrome/browser/ui/toolbar/media_router_action_controller.h
diff --git a/chrome/browser/ui/toolbar/media_router_action_controller.h b/chrome/browser/ui/toolbar/media_router_action_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..8e3ecf64a062f993d8684d48e0b4206a30219f11
--- /dev/null
+++ b/chrome/browser/ui/toolbar/media_router_action_controller.h
@@ -0,0 +1,85 @@
+// 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_

[mark a. foltz, 2016/09/02 22:19:09]

I wonder if we should put this in its own folder,
chrome/browser/ui/media_router.  It doesn't seem related to the implementation
of the toolbar, it's specific to the media router UX.

+#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"
+#include "chrome/browser/media/router/issues_observer.h"
+#include "chrome/browser/media/router/media_router_factory.h"
+#include "chrome/browser/media/router/media_routes_observer.h"
+#include "chrome/browser/profiles/profile.h"
+
+class MediaRouterActionControllerUnitTest;
+
+// Controller for MediaRouterAction that determines when to show and hide the
+// action icon on the toolbar.
+// There should be one instance of this class per profile.

[imcheng, 2016/08/31 05:39:48]

If it were really the case, would it be better to make this a KeyedService?

1) Define a MediaRouterActionControllerFactory class that extends
BrowserContextKeyedServiceFactory.
2) The factory class would DependOn MediaRouterFactory

That way, you wouldn't need to declare a new method on MediaRouter and add a
dependency to ui/toolbar. Instead we would treat MediaRouterActionController as
just another observer of MediaRouter.

In terms of instantiation, I don't think it makes sense for it to be done during
MediaRouterBase::Initialize(). Instead it should be done by whoever uses
MediaRouterActionController (which I would guess is something in ui/toolbar?)

[mark a. foltz, 2016/09/02 22:19:09]

Takumi and I discussed this.  This code didn't seem worth the boilerplate and
overhead of instantiating another KeyedService.
It would make most sense for something in chrome/browser/ui to own this.  The
toolbar owners have pushed back on adding a dependency from the toolbar on this
code.  At this point the likely path long term is to add a
chrome/browser/ui/media_router folder and an ownership path from something else
in browser/ui.  If we continue to get push back we can go the keyed service
route, I suppose.

+class MediaRouterActionController
+    : public media_router::IssuesObserver,
+      public media_router::MediaRoutesObserver {
+ public:
+  class Observer {

[mark a. foltz, 2016/09/02 22:19:09]

I don't see any observers implemented in this change.  Who is observing this
class?

Can you document this if it's required?

[takumif, 2016/09/07 21:48:33]

It's for MRAction to observe MRActionController. Added a comment.

+   public:
+    virtual void OnIssueUpdated(const media_router::Issue* issue);

[imcheng, 2016/08/31 05:39:49]

Should these be pure virtual methods, i.e. Observer should be an interface?

Actually, will we have observers other than |delegate_| (which seemed to have
been special cased in the .cc)?

[mark a. foltz, 2016/09/02 22:19:10]

Pass by const reference

[takumif, 2016/09/07 21:48:33]

MediaRouterActions will be observing |this|, rather than separately observing
issues and routes.

[takumif, 2016/09/07 21:48:33]

I'd like to be able to pass a nullptr to indicate the lack of issues.

+    virtual void OnLocalRouteUpdated(bool has_local_route);

[mark a. foltz, 2016/09/02 22:19:10]

Bikeshed: slightly prefer an API

OnNumLocalRoutesChanged(size_t num_routes)

This makes it clear the meaning of what is being passed in the argument.

[takumif, 2016/09/07 21:48:33]

Do we expect to ever have more than one local route?
Also, MRAction only needs to know whether there exists a local route. Would it
be better to name it something like OnLocalRoutePresenceChanged()?

+  };
+
+  MediaRouterActionController(
+      Profile* profile, media_router::MediaRouter* router);

[mark a. foltz, 2016/09/02 22:19:09]

I'm not a huge fan of the circular dependency between MediaRouter and this
class.  Derek, is there a different way we can construct observers that are
owned by the MediaRouter?

[imcheng, 2016/09/08 20:49:31]

I would suggest not having MediaRouter depend on MediaRouterActionController. To
me it doesn't really make sense for MediaRouter to own it, since in general
MediaRouter is not aware of UI specific logic.

+  ~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;
+
+  // Toggles the preferences to always show the Media Router action icon on the
+  // toolbar.
+  void ToggleActionVisibilityPreference();

[mark a. foltz, 2016/09/02 22:19:09]

This API is error prone:
- You have to check the state of the pref before calling it to know what it is
going to do.
- If you accidentally call it twice it will create hard to find bugs.

Better to have SetAlwaysShowActionPref() and RemoveAlwaysShowActionPref().

[takumif, 2016/09/07 21:48:33]

I've changed it to SetAlwaysShowActionPref(bool always_show). Is it still
preferable to have two separate methods instead?

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

[mark a. foltz, 2016/09/02 22:19:09]

If a caller wants to find the current issue should they instead register an
observer/delegate for OnIssueUpdated?

[takumif, 2016/09/07 21:48:33]

When a MRAction is instantiated, it needs to know whether there already is an
issue / local route, so it calls HasLocalRoute() and GetIssue().

+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ private:
+  friend class MediaRouterActionControllerUnitTest;

[mark a. foltz, 2016/09/02 22:19:09]

The pattern I've seen is to use FRIEND_TEST_ALL_PREFIXES to friend individual
test cases instead of friending the base class.  But I could go either way on
it.

[takumif, 2016/09/07 21:48:33]

I wanted to give the base class access to private attributes, but changed the
test so that's no longer necessary and removed this line.

+
+  // Adds or removes the Media Router action icon to/from the |delegate_|,
+  // depending on whether we have issues or local routes.
+  void UpdateActionVisibility();

[mark a. foltz, 2016/09/02 22:19:09]

Bikeshed:
1. This may be a no-op if the action is already in the toolbar.
2. The action may not be visible if it's in the overflow.

Suggest updating documentation and tweaking the name.

[takumif, 2016/09/07 21:48:33]

Done.

+
+  // Whether the preference to always show the Media Router action icon is
+  // turned on for the current profile.
+  bool ShouldAlwaysShowIcon();

[mark a. foltz, 2016/09/02 22:19:09]

Nit:
Please use 'Action' consistently to refer to the widget in the toolbar.
The icon is a property of the action :)

Here, maybe GetAlwaysShowActionPref()

[takumif, 2016/09/07 21:48:33]

Done.

+
+  // The profile |this| is associated with. There should be one instance of this
+  // class per profile.
+  Profile* profile_;

[imcheng, 2016/08/31 05:39:49]

Profile* const profile_;

[takumif, 2016/09/07 21:48:33]

Done.

+
+  // The delegate that is responsible for showing and hiding the icon on the
+  // toolbar.

[mark a. foltz, 2016/09/02 22:19:09]

Please document ownership & why delegate_ going to outlive this object.

[takumif, 2016/09/07 21:48:33]

Done.

+  extensions::ComponentMigrationHelper::ComponentActionDelegate* delegate_;

[imcheng, 2016/08/31 05:39:49]

...ComponentActionDelegate* const delegate_;

[mark a. foltz, 2016/09/02 22:19:09]

component_action_delegate_

[takumif, 2016/09/07 21:48:33]

Done.

[takumif, 2016/09/07 21:48:33]

Done.

+
+  std::unique_ptr<media_router::Issue> issue_;
+  bool has_local_display_route_;
+
+  base::ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterActionController);
+};
+
+#endif  // CHROME_BROWSER_UI_TOOLBAR_MEDIA_ROUTER_ACTION_CONTROLLER_H_