Non-Local Join for Media Router and Presentation API

chrome/browser/media/android/router/media_router_dialog_controller_android.cc

Index: chrome/browser/media/android/router/media_router_dialog_controller_android.cc
diff --git a/chrome/browser/media/android/router/media_router_dialog_controller_android.cc b/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
index 53ee2ea4f056dfff1cf40ec87a52db35984a60ee..315fe83c5d6982617e7399c2829e289973ca4bc7 100644
--- a/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
+++ b/chrome/browser/media/android/router/media_router_dialog_controller_android.cc
@@ -46,9 +46,7 @@ void MediaRouterDialogControllerAndroid::OnSinkSelected(
       base::Bind(&CreatePresentationSessionRequest::HandleRouteResponse,
                  base::Passed(&request)));
 
-  MediaRouter* router = MediaRouterFactory::GetApiForBrowserContext(
-      initiator()->GetBrowserContext());
-  router->CreateRoute(
+  router_->CreateRoute(
       source_id,
       ConvertJavaStringToUTF8(env, jsink_id),
       origin,
@@ -62,10 +60,7 @@ void MediaRouterDialogControllerAndroid::OnRouteClosed(
     jstring jmedia_route_id) {
   std::string media_route_id = ConvertJavaStringToUTF8(env, jmedia_route_id);
 
-  MediaRouter* router = MediaRouterFactory::GetApiForBrowserContext(
-      initiator()->GetBrowserContext());
-
-  router->CloseRoute(media_route_id);
+  router_->CloseRoute(media_route_id);
 
   CancelPresentationRequest();
 }
@@ -88,10 +83,11 @@ void MediaRouterDialogControllerAndroid::CancelPresentationRequest() {
 
 MediaRouterDialogControllerAndroid::MediaRouterDialogControllerAndroid(
     WebContents* web_contents)
-    : MediaRouterDialogController(web_contents),
-      MediaRoutesObserver(MediaRouterFactory::GetApiForBrowserContext(
-          initiator()->GetBrowserContext())) {
+    : MediaRouterDialogController(web_contents) {
   JNIEnv* env = base::android::AttachCurrentThread();
+  router_ = static_cast<MediaRouter*>(

[imcheng, 2015/11/26 00:49:45]

Can the static_cast be removed?

[matt.boetger, 2015/12/01 01:26:55]

Reverting.

+      MediaRouterFactory::GetApiForBrowserContext(
+        initiator()->GetBrowserContext()));
   java_dialog_controller_.Reset(Java_ChromeMediaRouterDialogController_create(
       env,
       reinterpret_cast<jlong>(this),
@@ -114,6 +110,12 @@ void MediaRouterDialogControllerAndroid::CreateMediaRouterDialog() {
   ScopedJavaLocalRef<jstring> jsource_urn =
       base::android::ConvertUTF8ToJavaString(env, media_source_id);
 
+  routes_observer_.reset(new UIMediaRoutesObserver(
+      router_,
+      media_source_id,

[imcheng, 2015/11/26 00:49:45]

You might want to check with Anton/Mounir if this is correct behavior. It looks
like the previous version has some MediaRoutes logic happening even before this
function is called. Either way, doesn't look like you need to pass in
|media_source_id| since android doesn't need to know joinable routes.

[matt.boetger, 2015/12/01 01:26:55]

Reverting.

+      base::Bind(&MediaRouterDialogControllerAndroid::OnRoutesUpdated,
+        base::Unretained(this))));
+
   // If it's a single route with the same source, show the controller dialog
   // instead of the device picker.
   // TODO(avayvod): maybe this logic should be in
@@ -149,7 +151,9 @@ bool MediaRouterDialogControllerAndroid::IsShowingMediaRouterDialog() const {
 }
 
 void MediaRouterDialogControllerAndroid::OnRoutesUpdated(
-    const std::vector<MediaRoute>& routes) {
+    const std::vector<MediaRoute>& routes,
+    const std::vector<MediaRoute::Id>& joinable_route_ids) {
+
   if (routes.size() != 1) {
     single_existing_route_.reset();
     return;
@@ -163,5 +167,47 @@ void MediaRouterDialogControllerAndroid::OnRoutesUpdated(
   single_existing_route_.reset(new MediaRoute(routes[0]));
 }
 
+
+MediaRouterDialogControllerAndroid::

[imcheng, 2015/11/26 00:49:45]

This looks copied from MediaRouterUI. I am not sure if android needs that logic
because I am guessing they send back only routes that can be displayed here in
the first place. But it would be good to check with Anton/Mounir.

Otherwise, it would be good to refactor this into chrome/browser/media/router.

[matt.boetger, 2015/12/01 01:26:55]

Reverting.

+  UIMediaRoutesObserver::UIMediaRoutesObserver(
+    MediaRouter* router, const MediaSource::Id source_id,
+    const RoutesUpdatedCallback& callback)
+    : MediaRoutesObserver(router, source_id), callback_(callback) {
+  DCHECK(!callback_.is_null());
+}
+
+MediaRouterDialogControllerAndroid::
+  UIMediaRoutesObserver::~UIMediaRoutesObserver() {}
+
+void MediaRouterDialogControllerAndroid::UIMediaRoutesObserver::OnRoutesUpdated(
+    const std::vector<MediaRoute>& routes,
+    const std::vector<MediaRoute::Id>& joinable_route_ids) {
+
+  std::vector<MediaRoute> routes_for_display;
+  std::vector<MediaRoute::Id> joinable_route_ids_for_display;
+  for (const MediaRoute& route : routes) {
+    if (route.for_display()) {
+#ifndef NDEBUG
+      for (const MediaRoute& existing_route : routes_for_display) {
+        if (existing_route.media_sink_id() == route.media_sink_id()) {
+          DVLOG(2) << "Received another route for display with the same sink"
+                   << " id as an existing route. "
+                   << route.media_route_id() << " has the same sink id as "
+                   << existing_route.media_sink_id() << ".";
+        }
+      }
+#endif
+      if (std::find(joinable_route_ids.begin(),

[imcheng, 2015/11/26 00:49:45]

Presumably the MRP would only consider a route as joinable only if for_display
is true?

[matt.boetger, 2015/12/01 01:26:55]

Reverting.

+          joinable_route_ids.end(), route.media_route_id()) !=
+          joinable_route_ids.end()) {
+        joinable_route_ids_for_display.push_back(route.media_route_id());
+      }
+      routes_for_display.push_back(route);
+    }
+  }
+
+  callback_.Run(routes_for_display, joinable_route_ids_for_display);
+}
+
 }  // namespace media_router