[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 "base/observer_list.h"

[mark a. foltz, 2017/03/17 23:55:04]

#include base/memory/ref_counted.h

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

Done.

+#include "chrome/browser/media/router/media_route.h"
+#include "chrome/browser/media/router/mojo/media_controller.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace media_router {
+
+// A controller for a MediaRoute. Forwards commands for controlling the route to
+// an out-of-process controller. Notifies its observers whenever there is a
+// change in the route's MediaStatus.
+//
+// It is owned by its observers, each of which holds a scoped_refptr. All the

[mark a. foltz, 2017/03/17 23:55:04]

...scoped_refptr to it.

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

Done.

+// classes that share the ownership must inherit from the Observer class. A

[mark a. foltz, 2017/03/17 23:55:04]

that hold a scoped_refptr must

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

Done.

+// MediaRouteController instance is destroyed when all its observers dispose
+// their references to it.
+//
+// Example usage:
+//
+// Instantiate MediaRouteController and pass it into the ctor of an observer.
+// An observer will subscribe itself in its ctor, and unsubscribe in its dtor.
+//
+//   // Let ObserverImpl extend MediaRouteController::Observer.

[imcheng, 2017/03/18 00:11:32]

If we are calling this MediaRouteController and adding MR specific concepts like
MediaRoute::Id, then we might as well be more specific about its relationship
with MediaRouter:

- you can specify that a MediaRouteController is created and returned by
MediaRouter::GetRouteController instead of new'ing it out of nowhere (I realize
you moved the API definition to the 2nd patch, but comment is probably ok)
- To address dcheng's concern about destructor_callback, you can make
MediaRouteController invoke MediaRouterMojoImpl to perform cleanup on
destruction. IMO, not a big deal either way.

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

Removing the example code, and mentioning that an observer should use
MR::GetRouteController() to obtain a scoped_refptr. 
As we discussed offline, I'll be passing MediaRouterBase* into the ctor, and
will be calling its method in the dtor in the next CL. I couldn't add an unused
member to MRController, so I'll be passing MediaRouterBase* in the next CL.

+//   // Let |media_controller| be bound to a message pipe.
+//   // Let |destructor_callback| be a closure.
+//   ObserverImpl observer1(new MediaRouteController(
+//       "some route id", media_controller, destructor_callback));
+//   ObserverImpl observer2(observer1.controller());

[mark a. foltz, 2017/03/17 23:55:04]

How about:

MRC::Observer* observer;
{
scoped_refptr<> controller = MediaRouteController::Create(...)

// observer adds a scoped_refptr.
// ObserverImpl ctor can be private and friend to MRC.
observer = controller->AddObserver();
}   // controller goes out of scope, observer keeps it alive

Hiding the ctor means that the controller is exclusively refounted, and the API
for adding the first and subsequent observers is clear and consistent. 

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

I think it's better if MRController didn't know about the observer
implementation though. We are going to have an observer class that is specific
to MediaRouterUI.

ObserverImpl was a fictional implementation for Observer created for this
example. Maybe that's confusing? Removing the example usage since it'd be
different from the actual code in MRMojoImpl/MRUI.

[mark a. foltz, 2017/03/21 00:05:21]

Ok, if the Observer is implemented separately from the controller, this API
seems fine.

+//
+// Destroy it by destroying all the observers, or making the observers dispose
+// their refptrs.
+//
+//   // This call will notify the observers to dispose their refptrs.
+//   observer1.controller()->OnControllerInvalidated();

[imcheng, 2017/03/18 00:11:32]

Hmm... I don't think we actually do this in any scenario?
OnControllerInvalidated() should only be called as a response from an outside
source, such as Mojo or Media Router.

You can mention that OnControllerInvalidated() gets invoked if the route
associated with the controller is terminated, or if there is a Mojo connection
error. After that, controller() returns nullptr.

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

Right, OnControllerInvalidated() should be called only by Media Router or a Mojo
error callback. Updating the comment.

+//
+class MediaRouteController : public mojom::MediaStatusObserver,
+                             public base::RefCounted<MediaRouteController> {
+ public:
+  // Observes MediaRouteController for MediaStatus updates. The ownership of a
+  // MediaRouteController is shared by its observers.
+  class Observer {
+   public:
+    // Adds itself as an observer to |controller|.
+    explicit Observer(scoped_refptr<MediaRouteController> controller);
+
+    // Removes itself as an observer if |controller_| is still valid.
+    virtual ~Observer();
+
+    virtual void OnMediaStatusUpdated(const MediaStatus& status) = 0;
+
+    // Disposes the reference to the controller. Must be called by methods
+    // overriding it.
+    virtual void OnControllerInvalidated();

[mark a. foltz, 2017/03/17 23:55:04]

Shouldn't this be called only by the MediaController?  If so, make it private
and use a friend declaration.

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

Done.

+
+    MediaRouteController* controller() { return controller_.get(); }

[mark a. foltz, 2017/03/17 23:55:04]

Shouldn't this return controller_ as a scoped_refptr?  If you return a raw ptr
there's no guarantee of its lifetime.

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

The point of this getter is for MediaRouterUI to be able to call the
controller's methods. MediaRouterUI will not store the pointer, and will be
calling this getter on the observer it owns every time it needs the pointer.
Adding a comment that the pointer should not be stored.

+
+   private:
+    scoped_refptr<MediaRouteController> controller_;
+
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+
+  // |this| will forward media commands to |media_controller|.
+  // |media_controller| must be bound to a message pipe. |destructor_callback|
+  // will be called in the dtor of |this|.
+  MediaRouteController(const MediaRoute::Id& route_id,
+                       mojom::MediaControllerPtr media_controller,
+                       const base::Closure& destructor_callback);
+
+  // Media controller methods for forwarding commands to |media_controller_|.

[mark a. foltz, 2017/03/17 23:55:04]

...forwarding commands to a mojom::MediaControllerPtr held in
|media_controller_|.

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

Done.

+  void Play();
+  void Pause();
+  void Seek(base::TimeDelta time);
+  void SetMute(bool mute);
+  void SetVolume(float volume);
+
+  // mojom::MediaStatusObserver:
+  // Notifies |observers_| of a status update.
+  void OnMediaStatusUpdated(const MediaStatus& status) override;
+
+  // Called when the connection between |this| and |media_controller_| is no
+  // longer valid. Notifies |observers_| to dispose their references to |this|.
+  // |this| gets destroyed when all the references are disposed.
+  void OnControllerInvalidated();
+
+  // Called when |media_controller_| is bound.
+  void OnControllerBound();

[imcheng, 2017/03/18 00:11:32]

This is not needed anymore.

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

Removed.

+
+  MediaRoute::Id route_id() const { return route_id_; }
+
+ private:
+  friend class base::RefCounted<MediaRouteController>;
+
+  ~MediaRouteController() override;
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // The ID of the Media Route that |this| controls.
+  const MediaRoute::Id route_id_;
+
+  // The out-of-process controller that |this| forwards commands to.
+  mojom::MediaControllerPtr media_controller_;
+
+  // Gets called in the dtor.
+  base::Closure destructor_callback_;
+
+  // Observers that |this| notifies of status updates. The observers share the
+  // ownership of |this| through scoped_refptr.
+  base::ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouteController);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTE_CONTROLLER_H_