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

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

 // 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.
 
 #include "chrome/browser/media/router/mojo/media_route_controller.h"
 
 #include <utility>
 
+#include "chrome/browser/media/router/media_router.h"
+
 namespace media_router {
 
 MediaRouteController::Observer::Observer(
     scoped_refptr<MediaRouteController> controller)
     : controller_(std::move(controller)) {
   controller_->AddObserver(this);
 }
 
 MediaRouteController::Observer::~Observer() {
   if (controller_)
     controller_->RemoveObserver(this);
 }
 
 void MediaRouteController::Observer::InvalidateController() {
   controller_->RemoveObserver(this);

[takumif, 2017/03/29 02:34:49]

No longer necessary because the controller won't use the observer list after
|is_valid_| becomes false.

   controller_ = nullptr;
   OnControllerInvalidated();
 }
 
 void MediaRouteController::Observer::OnControllerInvalidated() {}
 
 MediaRouteController::MediaRouteController(
     const MediaRoute::Id& route_id,
-    mojom::MediaControllerPtr media_controller)
-    : route_id_(route_id), media_controller_(std::move(media_controller)) {
+    mojom::MediaControllerPtr media_controller,
+    MediaRouter* media_router)
+    : route_id_(route_id),
+      media_controller_(std::move(media_controller)),
+      media_router_(media_router),

[dcheng, 2017/03/27 22:15:26]

Shall we DCHECK(media_router)?

[takumif, 2017/03/29 02:34:49]

Done.

+      binding_(this) {
   DCHECK(media_controller_.is_bound());
   media_controller_.set_connection_error_handler(
       base::Bind(&MediaRouteController::Invalidate, base::Unretained(this)));

[imcheng, 2017/03/28 01:18:37]

Suggest adding a OnMojoConnectionError method that would call into MR to detach
itself and call Invalidate().

[takumif, 2017/03/29 02:34:49]

I'm not sure if we need OnMojoConnectionError() and Invalidate() separately, if
the point is to keep DetachRouteController() out of Invalidate(). We can just
call DetachRouteController() from Invalidate(), since Invalidate() can call the
dtor and the dtor has to call DetachRouteController() anyways (to cover the case
in which the controller is deleted by deleting all the scoped_refptrs to it).

[imcheng, 2017/03/31 01:30:24]

Right now we have MR::RemoveInvalidRouteControllers -> MRC::Invalidate ->
MR::DetachRouteController, which is redundant. By separating the Mojo error case
from the route terminated case, it's clearer who is supposed to perform the map
entry cleanup.

 }
 
 void MediaRouteController::Play() {
   media_controller_->Play();
 }
 
 void MediaRouteController::Pause() {
   media_controller_->Pause();
 }
 
 void MediaRouteController::Seek(base::TimeDelta time) {
   media_controller_->Seek(time);
 }
 
 void MediaRouteController::SetMute(bool mute) {
   media_controller_->SetMute(mute);
 }
 
 void MediaRouteController::SetVolume(float volume) {
   media_controller_->SetVolume(volume);
 }
 
 void MediaRouteController::OnMediaStatusUpdated(const MediaStatus& status) {
   for (Observer& observer : observers_)
     observer.OnMediaStatusUpdated(status);
 }
 
 void MediaRouteController::Invalidate() {

[imcheng, 2017/03/28 01:18:37]

To make this API a bit more safe from implementation errors (since it does
require a very specific contract to be followed):

- Add a boolean "is_valid" that tracks whether this controller instance is no
longer valid and detached from MediaRouter. You can use the boolean to perform
DCHECKs and other checks, such as in AddObserver().
- In On MojoConnectionError(), call into MediaRouter to detach the controller
proactively rather than relying on dtor cleanup behavior.

[takumif, 2017/03/29 02:34:49]

Added is_valid_ that gets set to false in Invalidate(). Since Invalidate()
notifies the observers, the observers shouldn't/can't be using the controller
after the controller becomes invalid.

   for (Observer& observer : observers_)
     observer.InvalidateController();
   // |this| is deleted here!
 }
 
-MediaRouteController::~MediaRouteController() {}
+mojom::MediaStatusObserverPtr MediaRouteController::BindObserverPtr() {
+  return binding_.CreateInterfacePtrAndBind();

[imcheng, 2017/03/28 01:18:37]

Did you mean to store it in a variable and return it last?

[takumif, 2017/03/29 02:34:49]

Yes. I'm surprised that this even compiled.

+  binding_.set_connection_error_handler(
+      base::Bind(&MediaRouteController::Invalidate, base::Unretained(this)));
+}
+
+MediaRouteController::~MediaRouteController() {
+  media_router_->OnRouteControllerDestroyed(route_id_);
+}
 
 void MediaRouteController::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void MediaRouteController::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 }  // namespace media_router