[Media Router] Custom Controls 1 - Add MediaStatus, MediaRouteController, and mojo interfaces

chrome/browser/media/router/mojo/media_route_controller.h

+// Copyright 2017 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_MEDIA_ROUTER_MOJO_MEDIA_ROUTE_CONTROLLER_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTE_CONTROLLER_H_
+
+#include <memory>
+#include <unordered_set>
+
+#include "chrome/browser/media/router/media_route.h"
+#include "chrome/browser/media/router/media_status.h"
+#include "chrome/browser/media/router/mojo/media_controller.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace media_router {
+
+namespace mojom {
+class MediaRouteProvider;
+}
+
+class MediaRouteStatusObserver;
+
+// A controller for a MediaRoute. Forwards commands for controlling the route to

[mark a. foltz, 2017/03/06 20:06:19]

This only controls media routes that are owned out-of-process by a
MediaRouteProvider (the actual controller implementation lives in the provider).

As far as I can tell, this class applies equally whether the MRP is hosted in
the event page or in a different process type.  Suggested text:

"A controller for a MediaRoute hosted out-of-process in a Media Route Provider
(e.g., in the Media Router component extension)."

Also, I would suggest naming this e.g. ProviderMediaRouteController and making
the connection to the MRP clearer in the documentation.  imcheng@ may also have
naming suggestions.

[imcheng, 2017/03/06 23:11:41]

I think MediaRouteController is fine. The only part that is specific to the
MediaRouteProvider is the setup of the controller / observer bindings, i.e. if
we extracted the setup logic to elsewhere then this class won't need to have a
direct reference to a MediaRouteProvider. What if the initialization of this
object is done in MediaRouterMojoImpl?

[takumif, 2017/03/08 04:24:03]

We can move the initialization to MRMojoImpl, and have MediaRouteController not
have to refer to MRP. At that point there is nothing MR-specific about
MediaRouteController, so we could even call this MediaController (but I'm
hesitant to do so because of confusion with mojom::MediaController).

[mark a. foltz, 2017/03/08 22:16:12]

If the responsibility of the class is to implement mojom::MediaController
independent of the downstream binding then MediaController (or
MediaControllerImpl) is the best name for it IMO.  (I haven't looked at the most
recent patchset yet - was initialization moved out of this class?)

[takumif, 2017/03/09 19:45:30]

I'm not sure if there is a need for this class to implement
mojom::MediaController. Yes I moved initialization out in the next patch.

+// the component-extension-side controller. Notifies its observers whenever
+// there is a change in the route's MediaStatus. Gets destroyed when its
+// connection to the extension-side controller is invalidated, the route is

[mark a. foltz, 2017/03/06 20:06:19]

Will this prevent the controller from persisting after the event page suspends? 
How would the caller/UX know when this has happened?

[imcheng, 2017/03/06 23:11:41]

The event page does not suspend when there is at least one controller active.

Ideally, we can get away with not forcing the extension to stay awake, by having
this class use the same event page wakeup / queuing logic used by
MediaRouterMojoImpl. This requires some refactoring of the latter so I don't
think it's necessary for v1.

+// terminated, or the last observer of |this| is removed.

[mark a. foltz, 2017/03/06 20:06:19]

I still find the lifetime dependency between the controller and its observers
unintuitive from a design pattern point of view.  If I have a media route with
ephemeral UX for control, do I have to create and destroy the controller each
time the UX appears/disappears?  We might want the controller to persist during
the entire time the dialog is open (even if the controls are not visible).

See my earlier comments about adding a disposal API to give the browser the
ability to be explicit about when the controller is no longer needed.

[imcheng, 2017/03/06 23:11:41]

This is a bit tricky because the controller can become invalid from multiple
sources.

There are a few possible approaches. For example, the users can maintain a
scoped_refptr<MediaRouteController>. But they would also need a way to tell if
the controller is no longer valid due to route termination in order to discard
the reference.

I think it would make more sense if we defined ownership of this class and the
API the caller can use to obtain a reference to an instance.

[takumif, 2017/03/08 04:24:02]

The current design is that the event page is awake as long as there is a
MediaRouteController, so it's probably better to tie its lifetime just to route
details view, than any dialog view.

[mark a. foltz, 2017/03/08 22:16:12]

Okay, whichever design is settled upon, please update this documentation.

My opinion is that adding an observer and adding a reference count should be two
separate actions, unless there's some way to make it self-descriptive and
obvious that these are special observers that keep the thing they are observing
alive.

[takumif, 2017/03/09 19:45:30]

Would it be better if we made the step to call the destroyer callback explicit?
We could add some method like MediaRouteController::DestructIfNoObservers() that
MRUI can call after removing itself as an observer.

[mark a. foltz, 2017/03/13 18:22:31]

The API I was thinking of would always destroy the controller and notify
remaining observers that the controller had gone away (and don't expect further
status updates).

+class MediaRouteController : public mojom::MediaStatusObserver {

[mark a. foltz, 2017/03/06 20:06:19]

Shouldn't this implement mojo::MediaController?

Will an in-browser implementation of mojo::MediaController be possible to
migrate media controller implementations out of the extension in the future?

[imcheng, 2017/03/06 23:11:41]

The extension side implements mojo::MediaController.

A browser implementation of mojo::MediaController is theoretically possible.
This class would talk to it via the same MediaControllerPtr.

[takumif, 2017/03/08 04:24:02]

Given that this only receives commands from within the browser process (mostly
MediaRouterUI), I don't think there's a need to implement mojo::MediaController
for now.

+ public:
+  // Attempts to establish a mojo connection with the extension-side controller

[mark a. foltz, 2017/03/06 20:06:19]

s/extension-side controller/Media Route Provider/

[takumif, 2017/03/08 04:24:03]

Removing mentions of extension/MRP.

+  // for |route_id|. |destroyer_callback| will be called if it fails.
+  // |destroyer_callback| must destroy |this|.
+  MediaRouteController(MediaRoute::Id route_id,
+                       mojom::MediaRouteProvider* provider,

[dcheng, 2017/03/08 01:08:20]

It's a bit unusual to pass around raw pointers to Mojo interfaces. What keeps
the interface pointer alive?

[takumif, 2017/03/08 04:24:02]

Removing this in the new patch.

+                       base::Closure destroyer_callback);
+  ~MediaRouteController() override;
+
+  // Called when |media_controller_| gets bound. Calls |destroyer_callback_| if
+  // |success| is false.
+  void OnControllerSet(bool success);
+
+  // Media controller methods for forwarding commands to the extension-side

[mark a. foltz, 2017/03/06 20:06:19]

...to the Media Route Provider.

[takumif, 2017/03/08 04:24:03]

Removing mentions of extension/MRP.

+  // controller.
+  void Play();
+  void Pause();
+  void Seek(uint32_t time);
+  void SetMute(bool mute);
+  void SetVolume(float volume);
+
+  void AddObserver(MediaRouteStatusObserver* observer);

[mark a. foltz, 2017/03/06 20:06:19]

Please document which of the API override/implement mojo::MediaStatusObserver
via // comment

[takumif, 2017/03/08 04:24:02]

OnMediaStatusUpdated() is the only method.

+  // Calls |destroyer_callback_| if the last observer is removed.
+  void RemoveObserver(MediaRouteStatusObserver* observer);
+
+  // mojom::MediaStatusObserver:
+  void OnMediaStatusUpdated(const MediaStatus& status) override;
+
+ private:
+  // Gets called when |this| should no longer exist. Needs to destroy |this|.
+  base::Closure destroyer_callback_;

[dcheng, 2017/03/08 01:08:19]

This is pretty unusual. Where's the code that actually uses this interface? Why
do we need to indirect this through a callback?

[takumif, 2017/03/08 04:24:02]

This is used by MediaRouterMojoImpl which will own this (the code will be in
another CL [1]). MediaRouteController will be stored in a map, and we want to
delete it from the map whenever it has no observers. 

[1]
https://codereview.chromium.org/2728543009/diff/1/chrome/browser/media/router...

[dcheng, 2017/03/17 06:15:14]

Unless we need to use a base::Callback for flexibility/layering reasons, I would
strongly suggest that this class just talk directly to the MediaRouterMojoImpl.
This adds a lot of indirection and makes the code harder to follow.

[takumif, 2017/03/20 22:10:45]

Okay, passing MediaRouterBase* (superclass of MRMojoImpl, since MRMojoImpl is
desktop-specific) to the ctor, and will be calling its method directly (in the
next CL) instead of a callback.

+
+  // The extension-side controller that |this| forwards commands to.

[mark a. foltz, 2017/03/06 20:06:19]

Media Route Provider

[takumif, 2017/03/08 04:24:02]

Removing mentions of extension/MRP.

+  mojom::MediaControllerPtr media_controller_;
+
+  // The binding to observe the extension-side provider of status updates.

[mark a. foltz, 2017/03/06 20:06:19]

observe status updates from the Media Route Provider

[takumif, 2017/03/08 04:24:02]

Removing mentions of extension/MRP.

+  std::unique_ptr<mojo::Binding<mojom::MediaStatusObserver>> binding_;

[dcheng, 2017/03/08 01:08:20]

unique_ptr is unnecessary here. In the initializer list, you can just construct
it with |this|, and then connect it later by calling Bind() on a
InterfaceRequest.

[takumif, 2017/03/08 04:24:03]

Done.

+
+  // Observers that |this| notifies of status updates.
+  std::unordered_set<MediaRouteStatusObserver*> observers_;

[mark a. foltz, 2017/03/06 20:06:19]

base::ObserverList

[takumif, 2017/03/08 04:24:03]

Done.

+
+  mojom::MediaRouteProvider* const provider_;

[mark a. foltz, 2017/03/06 20:06:19]

Who owns this? Please document

[takumif, 2017/03/08 04:24:02]

Removed the reference.

+
+  // The ID of the Media Route that |this| controls.
+  const MediaRoute::Id route_id_;
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTE_CONTROLLER_H_