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

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

Index: chrome/browser/media/router/mojo/media_route_controller.h
diff --git a/chrome/browser/media/router/mojo/media_route_controller.h b/chrome/browser/media/router/mojo/media_route_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..13169a33132e1a3030222b8a8737f1dbf622a41e
--- /dev/null
+++ b/chrome/browser/media/router/mojo/media_route_controller.h
@@ -0,0 +1,115 @@
+// 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/memory/ref_counted.h"
+#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 which holds a scoped_refptr to it. All
+// the classes that host a scoped_refptr must inherit from the Observer class.

[mark a. foltz, 2017/03/20 23:47:03]

Did you mean hold instead of host?

[takumif, 2017/03/21 04:00:07]

Yes, changed.

+// An observer should be instantiated with a scoped_refptr obtained through
+// MediaRouter::GetRouteController().
+//
+// A MediaRouteController instance is destroyed when all its observers dispose
+// their references to it. When the Mojo connection with the out-of-process
+// controller is terminated or has an error, OnControllerInvalidated() will be
+// called by the MediaRouter or a Mojo error handler to make observers dispose
+// their refptrs.
+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;
+
+    // The pointer returned by this getter should not be stored, as it can be

[mark a. foltz, 2017/03/20 23:47:03]

Returns a reference to the observed MediaRouteController.

[takumif, 2017/03/21 04:00:07]

Done.

+    // invalidated at any time.
+    MediaRouteController* controller() { return controller_.get(); }

[mark a. foltz, 2017/03/20 23:47:03]

Adding a comment here is not sufficient.  There's no reason to return a raw
pointer, as constructing a copy of a scoped pointer is cheap and makes the
object lifetime simpler to reason about.

[takumif, 2017/03/21 04:00:07]

Done.

+
+   private:
+    friend class MediaRouteController;
+
+    // Disposes the reference to the controller. Must be called by methods
+    // overriding it.

[mark a. foltz, 2017/03/20 23:47:03]

Didn't follow the second sentence - this method isn't virtual so it can't be
overridden.

[takumif, 2017/03/21 04:00:07]

Sorry, it's out of date. Removed.

+    void OnControllerInvalidated();
+
+    // Called by OnControllerInvalidated() after the reference to the controller
+    // is disposed. Overridden by subclasses to do custom cleanup.

[mark a. foltz, 2017/03/20 23:47:03]

Perhaps note that this is called immediately before |controller_| is destroyed,
so that it should not retain any references to it.

[takumif, 2017/03/21 04:00:07]

This is called after |controller_| is reset, so there shouldn't be any
references left anyways, unless I'm misunderstanding what you're saying.

+    virtual void OnControllerDisposed();
+
+    scoped_refptr<MediaRouteController> controller_;
+
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+
+  // |this| will forward media commands to |media_controller|.

[mark a. foltz, 2017/03/20 23:47:03]

Constructs a MediaRouteController that forwards...

[takumif, 2017/03/21 04:00:07]

Done.

+  // |media_controller| must be bound to a message pipe. |destructor_callback|

[mark a. foltz, 2017/03/20 23:47:03]

Remove |destructor_callback| comment

[takumif, 2017/03/21 04:00:07]

Done.

+  // will be called in the dtor of |this|.
+  MediaRouteController(const MediaRoute::Id& route_id,
+                       mojom::MediaControllerPtr media_controller);
+
+  // Media controller methods for forwarding commands to a
+  // mojom::MediaControllerPtr held in |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|.
+  // |this| gets destroyed when all the references are disposed.
+  void OnControllerInvalidated();
+
+  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.

[mark a. foltz, 2017/03/20 23:47:03]

Technically, mojo services can be bound in-process.  Maybe "Handle to the
mojom::MediaController that receives media commands."

[takumif, 2017/03/21 04:00:07]

Done.

+  mojom::MediaControllerPtr media_controller_;
+
+  // 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_