[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"
+#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

[imcheng, 2017/03/16 20:00:43]

I think it would be nice to document the life cycle of this class and add
example usage here. Specific points:

- How to construct and/or initialize this class
- Invalidation
- Destruction when all observers are removed. 

[takumif, 2017/03/17 21:48:01]

Adding comments/usage.

+// which holds a scoped_refptr.
+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);

[imcheng, 2017/03/16 20:00:43]

const scoped_refptr<MediaRouteController>&

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

Note that it is considered OK to pass by value (and std::move instead)

+
+    // 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.
+    virtual void OnControllerInvalidated();

[imcheng, 2017/03/16 20:00:43]

Why make this virtual? Seems like you should enforce this behavior always.

[takumif, 2017/03/17 21:48:02]

Yes, the behavior should be enforced, but I also want to extend this method to
notify MRUI that the controller is no longer valid, and the observer should be
disposed. Commenting that this should be called by overriding methods.

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

A pattern I've seen is to make this non-virtual, and have it call into a virtual
method to perform implementation-specific cleanups.

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

Done.

+
+    MediaRouteController* controller() { return controller_.get(); }
+
+   private:
+    scoped_refptr<MediaRouteController> controller_;
+
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+
+  // |this| will forward media commands to |media_controller|.
+  // |destructor_callback| will be called in the dtor of |this|.
+  MediaRouteController(MediaRoute::Id route_id,

[imcheng, 2017/03/16 20:00:43]

const ref

[takumif, 2017/03/17 21:48:02]

Done.

+                       mojom::MediaControllerPtr media_controller,
+                       base::Closure destructor_callback);

[imcheng, 2017/03/16 20:00:43]

const ref

[takumif, 2017/03/17 21:48:02]

Done.

+
+  // Media controller methods for forwarding commands to |media_controller_|.
+  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|.
+  void OnControllerInvalidated();
+
+  // Called when |media_controller_| is bound.
+  void OnControllerBound();
+
+  // Returns a mojo pointer bound to |this| by |binding_|. If |binding_| is
+  // already set, then the previous binding gets invalidated.
+  mojom::MediaStatusObserverPtr GetObserverPtr();

[imcheng, 2017/03/16 20:00:43]

Suggest naming this BindObserverPtr() or similar so it is more clear. See:
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...

[takumif, 2017/03/17 21:48:01]

Got it. Also moving this method to CL #2.

+
+  const MediaRoute::Id& route_id() const { return route_id_; }

[imcheng, 2017/03/16 20:00:43]

Return by value

[takumif, 2017/03/17 21:48:02]

Done.

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