Refactor Clank cast connect logic

chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java

Index: chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java b/chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
index b770fe5246440130487035a72cf65434f9e32472..81745cd5ccbe4dc9087d5d506ddd3a648e4dcd34 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
@@ -347,7 +347,35 @@ public abstract class AbstractMediaRouteController implements MediaRouteControll
         for (UiListener listener : mUiListeners) {
             listener.onRouteSelected(route.getName(), this);
         }
-        if (mMediaStateListener != null) mMediaStateListener.onRouteSelected(route.getName());
+        // Check that somebody still cares.

[whywhat, 2015/10/14 13:28:49]

nit: this comment seems redundant, maybe say something about the case when the
listener might be null instead?

[aberent, 2015/10/15 10:31:37]

Done.

+        if (mMediaStateListener == null) return;
+
+        // Check that we actually want to cast the video

[whywhat, 2015/10/14 13:28:50]

nit: redundant comment?

[aberent, 2015/10/15 10:31:37]

Done.

+        if (!shouldCastVideo()) return;
+
+        startCastingVideo(route);
+    }
+
+    /**
+     * @param route

[whywhat, 2015/10/14 13:28:50]

nit: remove javadoc

[aberent, 2015/10/15 10:31:37]

Done.

[dgn, 2015/10/15 11:07:31]

Doesn't seem done. Same for other comment related comments here

[aberent, 2015/10/15 12:57:54]

Done.

+     */
+    private void startCastingVideo(RouteInfo route) {
+        // Pause the local player

[whywhat, 2015/10/14 13:28:50]

nit: redundant comments?

[aberent, 2015/10/15 10:31:37]

Done.

+        mMediaStateListener.pauseLocal();
+        // Swap players

[whywhat, 2015/10/14 13:28:49]

ditto

[aberent, 2015/10/15 10:31:37]

Done.

+        mMediaStateListener.switchToRemotePlayer();

[whywhat, 2015/10/14 13:28:50]

is switchToRemotePlayer always prepended with pauseLocal? should we combine the
two then?

[aberent, 2015/10/15 10:31:37]

At the moment it is, however I am thinking that Clank should probably pause the
local video as soon as the request to cast is made, which will mean moving
pauseLocal(). Still want to think about it further, so leaving it like this for
now. 

+        setDataSource(Uri.parse(mMediaStateListener.getSourceUrl()),

[whywhat, 2015/10/14 13:28:49]

nit: mMediaStateListener doesn't seem like the best name given that it's not
just state callbacks but also some methods and getters.
mDelegate of type MediaRouteControllerDelegate?

[aberent, 2015/10/15 10:31:37]

Sadly MediaStateListener is referenced in YouTubeMediaRouteController, so
changing this would require a two sided patch; which I want to avoid for this
CL.

+                mMediaStateListener.getCookies(), mMediaStateListener.getUserAgent());
+        prepareAsync(

[whywhat, 2015/10/14 13:28:50]

nit: can't prepareAsync get this from the mMediaStateListener?

[aberent, 2015/10/15 10:31:37]

It can, and there is more to do in this area; but, in this CL, I don't want to
change the interface of any methods that are used or overridden in the YouTube
code, since doing so will require a two sided patch.

+                mMediaStateListener.getFrameUrl(), mMediaStateListener.getStartPositionMillis());
+        mMediaStateListener.onPlayingRemotely(route.getName());
+    }
+
+    private boolean shouldCastVideo() {

[whywhat, 2015/10/14 13:28:50]

Seems more like canCastMedia() to me (maybe we should avoid using video since
audio casting works too with the default controls)

[aberent, 2015/10/15 10:31:37]

Done.

+        if (!isRemotePlaybackAvailable()) return false;
+        if (routeIsDefaultRoute()) return false;
+        if (!currentRouteSupportsRemotePlayback()) return false;
+        return true;

[whywhat, 2015/10/14 13:28:49]

nit: how about just

return isRemotePlaybackAvailable() && !routeIsDefaultRoute() &&
currentRouteSupportsRemotePlayback(); ?

[aberent, 2015/10/15 10:31:37]

Done.

     }
 
     @Override
@@ -445,7 +473,12 @@ public abstract class AbstractMediaRouteController implements MediaRouteControll
             for (UiListener listener : mUiListeners) {
                 listener.onPrepared(this);
             }
-            if (mMediaStateListener != null) mMediaStateListener.onPrepared();
+            if (mMediaStateListener.isPauseRequested()) pause();
+            if (mMediaStateListener.isSeekRequested()) {
+                seekTo(mMediaStateListener.getSeekLocation());
+            } else {
+                seekTo(mMediaStateListener.getLocalPosition());
+            }
             RecordCastAction.castDefaultPlayerResult(true);
             mIsPrepared = true;
         }
@@ -600,4 +633,17 @@ public abstract class AbstractMediaRouteController implements MediaRouteControll
      * @param cookies
      */
     public void setDataSource(Uri uri, String cookies) {};
+
+    @Override
+    public boolean videoTakesOverCastDevice(MediaStateListener mediaStateListener) {

[dgn, 2015/10/14 16:05:49]

nit: The name is a bit strange. I suppose reusing startCastingVideo is not
acceptable? It would have been a nice name.

[aberent, 2015/10/15 10:31:37]

Yes, but it only starts casting the new video if the RouteController is already
in use. This is the reason it is boolean, and the reason for its name (now
changed to playerTakesOverCastDevice.

See how it is used in remote_media_player_manager.cc

+        // Check if this MediaRouteControler is casting something.
+        if (!isBeingCast()) return false;
+        // Check if we want to cast the new video
+        if (!shouldCastVideo()) return false;
+        // Take over the cast device
+        if (mMediaStateListener != null) mMediaStateListener.switchToLocalPlayer();
+        mMediaStateListener = mediaStateListener;
+        startCastingVideo(mCurrentRoute);
+        return true;
+    }
 }