[Media Router] Custom Controls 2 - add MediaRouter::GetRouteController()

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
index 81812e9918c592b7fd6cb81bf106366b92ac9d04..8819dcc914e770b27b2af179b8035da2c25affd4 100644
--- a/chrome/browser/media/router/mojo/media_route_controller.h
+++ b/chrome/browser/media/router/mojo/media_route_controller.h
@@ -15,6 +15,8 @@
 
 namespace media_router {
 
+class MediaRouter;
+
 // 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.
@@ -26,9 +28,9 @@ namespace media_router {
 //
 // 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.
+// controller is terminated or has an error, Invalidate() will be called by the
+// MediaRouter or OnMojoConnectionError() to make observers dispose their
+// refptrs.
 class MediaRouteController : public mojom::MediaStatusObserver,
                              public base::RefCounted<MediaRouteController> {
  public:
@@ -66,12 +68,15 @@ class MediaRouteController : public mojom::MediaStatusObserver,
   };
 
   // Constructs a MediaRouteController that forwards media commands to
-  // |media_controller|. |media_controller| must be bound to a message pipe.
+  // |mojo_media_controller|. |mojo_media_controller| must be bound to a message

[mark a. foltz, 2017/04/11 19:43:19]

When does this binding actually happen?  It looks like it's done in the callback
in MediaRouterMojoImpl, after the ctor is called.

[takumif, 2017/04/12 23:11:36]

Right, I forgot to update the comment. Updated.

+  // pipe. |media_router| will be notified when the MediaRouteController is
+  // destroyed.

[mark a. foltz, 2017/04/11 19:43:19]

Can you be specific: "notified via DetachRouteController"

[takumif, 2017/04/12 23:11:36]

Done.

   MediaRouteController(const MediaRoute::Id& route_id,
-                       mojom::MediaControllerPtr media_controller);
+                       mojom::MediaControllerPtr mojo_media_controller,
+                       MediaRouter* media_router);
 
   // Media controller methods for forwarding commands to a
-  // mojom::MediaControllerPtr held in |media_controller_|.
+  // mojom::MediaControllerPtr held in |mojo_media_controller_|.
   void Play();
   void Pause();
   void Seek(base::TimeDelta time);
@@ -82,11 +87,14 @@ class MediaRouteController : public 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.
+  // Notifies |observers_| to dispose their references to |this|. |this| gets

[mark a. foltz, 2017/04/11 19:43:19]

I don't mind calling out |this| when it's ambiguous which object is being
referred to.  However, maybe you can drop some of these references by clarifying
the surrounding text or just saying "the controller".

[takumif, 2017/04/12 23:11:36]

Done.

+  // destroyed when all the references are disposed.
   void Invalidate();
 
+  // Returns a mojo pointer bound to |this| by |binding_|. If |binding_| is

[mark a. foltz, 2017/04/11 19:43:19]

binding_ is an implementation detail of this class - this can be written not to
mention it:

Returns a MediaStatusObserverPtr bound to |this|.  If a previous observer was
bound, its binding is invalidated.

[takumif, 2017/04/12 23:11:36]

Done.

+  // already set, then the previous binding gets invalidated.
+  mojom::MediaStatusObserverPtr BindObserverPtr();
+
   MediaRoute::Id route_id() const { return route_id_; }
 
  private:
@@ -97,16 +105,30 @@ class MediaRouteController : public mojom::MediaStatusObserver,
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Called when the connection between |this| and the mojo MediaController or
+  // the mojo MediaStatus provider is no longer valid. Notifies |media_router_|

[mark a. foltz, 2017/04/11 19:43:19]

s/mojo MediaController/MediaControllerPtr/
s/mojo Media Status provider/MediaStatusObserver binding/

[takumif, 2017/04/12 23:11:36]

Done.

+  // and |observers_| to dispose their references to |this|.
+  void OnMojoConnectionError();
+
   // The ID of the Media Route that |this| controls.
   const MediaRoute::Id route_id_;
 
   // Handle to the mojom::MediaController that receives media commands.
-  mojom::MediaControllerPtr media_controller_;
+  mojom::MediaControllerPtr mojo_media_controller_;
+
+  // |media_router_| will be notified when |this| is destroyed.
+  MediaRouter* const media_router_;
+
+  // 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

[mark a. foltz, 2017/04/11 19:43:19]

s/|this| notifies/are notified/

[takumif, 2017/04/12 23:11:36]

Done.

   // ownership of |this| through scoped_refptr.
   base::ObserverList<Observer> observers_;
 
+  // This becomes false when |this| is invalidated.
+  bool is_valid_ = true;
+
   DISALLOW_COPY_AND_ASSIGN(MediaRouteController);
 };