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

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

Index: chrome/browser/media/router/mojo/media_route_controller.cc
diff --git a/chrome/browser/media/router/mojo/media_route_controller.cc b/chrome/browser/media/router/mojo/media_route_controller.cc
index 372d33a7ca859377dd76c959e81767e8139fea1c..11ea2c45113066b49dc3151ef40d6e143161869b 100644
--- a/chrome/browser/media/router/mojo/media_route_controller.cc
+++ b/chrome/browser/media/router/mojo/media_route_controller.cc
@@ -6,6 +6,8 @@
 
 #include <utility>
 
+#include "chrome/browser/media/router/media_router.h"
+
 namespace media_router {
 
 MediaRouteController::Observer::Observer(
@@ -20,7 +22,6 @@ MediaRouteController::Observer::~Observer() {
 }
 
 void MediaRouteController::Observer::InvalidateController() {
-  controller_->RemoveObserver(this);
   controller_ = nullptr;
   OnControllerInvalidated();
 }
@@ -29,51 +30,81 @@ 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),
+      binding_(this) {
   DCHECK(media_controller_.is_bound());
+  DCHECK(media_router);
   media_controller_.set_connection_error_handler(
       base::Bind(&MediaRouteController::Invalidate, base::Unretained(this)));
 }
 
 void MediaRouteController::Play() {
+  DCHECK(is_valid_);
   media_controller_->Play();
 }
 
 void MediaRouteController::Pause() {
+  DCHECK(is_valid_);
   media_controller_->Pause();
 }
 
 void MediaRouteController::Seek(base::TimeDelta time) {
+  DCHECK(is_valid_);
   media_controller_->Seek(time);
 }
 
 void MediaRouteController::SetMute(bool mute) {
+  DCHECK(is_valid_);
   media_controller_->SetMute(mute);
 }
 
 void MediaRouteController::SetVolume(float volume) {
+  DCHECK(is_valid_);
   media_controller_->SetVolume(volume);
 }
 
 void MediaRouteController::OnMediaStatusUpdated(const MediaStatus& status) {
+  if (!is_valid_)
+    return;
+
   for (Observer& observer : observers_)
     observer.OnMediaStatusUpdated(status);
 }
 
 void MediaRouteController::Invalidate() {
+  is_valid_ = false;

[mark a. foltz, 2017/03/30 23:03:28]

Is the concern that the OnMediaStatusUpdated will get called after Invalidate()?
 Or that DetachMediaRouteController/observer.InvalidateController() will somehow
call back onto |this|?

[takumif, 2017/04/04 03:19:50]

Yes, and addition of new observers during or after Invalidate().

+  media_router_->DetachRouteController(route_id_, this);
   for (Observer& observer : observers_)
     observer.InvalidateController();
   // |this| is deleted here!
 }
 
-MediaRouteController::~MediaRouteController() {}
+mojom::MediaStatusObserverPtr MediaRouteController::BindObserverPtr() {
+  DCHECK(is_valid_);
+  mojom::MediaStatusObserverPtr observer_ptr =
+      binding_.CreateInterfacePtrAndBind();
+  binding_.set_connection_error_handler(
+      base::Bind(&MediaRouteController::Invalidate, base::Unretained(this)));
+
+  return observer_ptr;
+}
+
+MediaRouteController::~MediaRouteController() {
+  if (is_valid_)

[mark a. foltz, 2017/03/30 23:03:28]

DCHECK(!is_valid_)?

[takumif, 2017/04/04 03:19:50]

is_valid_ would be true here if the controller was destroyed by closing all
route details views (disposing all scoped_refptrs), in which case Invalidate()
wouldn't get called.

+    media_router_->DetachRouteController(route_id_, this);
+}
 
 void MediaRouteController::AddObserver(Observer* observer) {
+  DCHECK(is_valid_);
   observers_.AddObserver(observer);
 }
 
 void MediaRouteController::RemoveObserver(Observer* observer) {
+  DCHECK(is_valid_);
   observers_.RemoveObserver(observer);
 }