Cast Media Router: fix a few issues to pass some tests.

Cast Media Router: fix a few issues to pass some tests.

* Properly set requestId to be used to track sequenceNumber.
* Properly re-route names that are different outside and inside MEDIA namespace.

BUG=549134, 548822

[mlamouri (slow - plz ping), 2015-10-30 15:45:10]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2015-10-30 15:57:17]

lgtm with some nits

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java
(right):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java:18:
public static CastRequestIdGenerator getInstance() {
nit: I guess we could just have a static public method getNextRequestId()
instead of getInstance().getRequestId()?

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java:28:
if (mRequestId == 0) ++mRequestId;
do we need to keep the id in the allocated range?

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
(left):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:329:
mSequenceNumber++;
nit: remove this.

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
(right):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:157:
private Map<Integer, Integer> mRequests;
nit: use SparseIntArray

[mlamouri (slow - plz ping), 2015-10-30 17:23:35]

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java
(right):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java:18:
public static CastRequestIdGenerator getInstance() {
On 2015/10/30 at 15:57:17, whywhat wrote:
> nit: I guess we could just have a static public method getNextRequestId()
instead of getInstance().getRequestId()?

Done.

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRequestIdGenerator.java:28:
if (mRequestId == 0) ++mRequestId;
On 2015/10/30 at 15:57:17, whywhat wrote:
> do we need to keep the id in the allocated range?

No.

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
(left):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:329:
mSequenceNumber++;
On 2015/10/30 at 15:57:17, whywhat wrote:
> nit: remove this.

Done.

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
(right):

https://codereview.chromium.org/1420973009/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:157:
private Map<Integer, Integer> mRequests;
On 2015/10/30 at 15:57:17, whywhat wrote:
> nit: use SparseIntArray

Done.

[mlamouri (slow - plz ping), 2015-10-30 17:27:51]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-30 17:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/1420973009/#ps40001
(title: "oups")

[commit-bot: I haz the power, 2015-10-30 17:28:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420973009/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420973009/40001

[commit-bot: I haz the power, 2015-10-30 18:44:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 18:44:11]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java:85
  Falling back to three-way merge...
  Applied patch to
'chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java'
with conflicts.
  U
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
b/chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
index
e776aebaf2a5099029dcf3ebffda21ae4b1be5c4..4b9f5ada95c85206f3b34795b715cc6f75af37b2
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastRouteController.java
@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.media.router.cast;
 
 import android.content.Context;
+import android.util.SparseIntArray;
 
 import com.google.android.gms.cast.ApplicationMetadata;
 import com.google.android.gms.cast.Cast;
@@ -46,17 +47,15 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
             "LOAD",
             "PAUSE",
             "SEEK",
-            "GET_STATUS",
             "STOP_MEDIA",
-            "SET_VOLUME",
-            "GET_STATUS",
+            "MEDIA_SET_VOLUME",
+            "MEDIA_GET_STATUS",
             "EDIT_TRACKS_INFO",
             "QUEUE_LOAD",
             "QUEUE_INSERT",
             "QUEUE_UPDATE",
             "QUEUE_REMOVE",
             "QUEUE_REORDER",
-            "MEDIA_STATUS",
     };
 
     private static final String MEDIA_SUPPORTED_COMMANDS[] = {
@@ -66,7 +65,16 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
             "stream_mute",
     };
 
-    // Map associating overloaded types with the types they represent.
+    // Sequence number used when no sequence number is required or was
initially passed.
+    private static final int INVALID_SEQUENCE_NUMBER = -1;
+    // Sequence number use instead of no sequence number when a sequence number
was expected from
+    // the client.
+    private static final int OPTIONAL_SEQUENCE_NUMBER = 0;
+
+    // Map associating types that have a different names outside of the media
namespace and inside.
+    // In other words, some types are sent as MEDIA_FOO or FOO_MEDIA by the
client by the Cast
+    // expect them to be named FOO. The reason being that FOO might exist in
multiple namespaces
+    // but the client isn't aware of namespacing.
     private static Map<String, String> sMediaOverloadedMessageTypes;
 
     // Lock used to lazy initialize sMediaOverloadedMessageTypes.
@@ -85,7 +93,18 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
                        + "\" message=\"" + message + "\"");
 
             if (MEDIA_NAMESPACE.equals(namespace) ||
RECEIVER_NAMESPACE.equals(namespace)) {
-                mSession.onMessage("v2_message", message);
+                int sequenceNumber = INVALID_SEQUENCE_NUMBER;
+                try {
+                    JSONObject jsonMessage = new JSONObject(message);
+                    int requestId = jsonMessage.getInt("requestId");
+                    if (mSession.mRequests.indexOfKey(requestId) >= 0) {
+                        sequenceNumber = mSession.mRequests.get(requestId);
+                        mSession.mRequests.delete(requestId);
+                    }
+                } catch (JSONException e) {
+                } finally {
+                    mSession.onMessage("v2_message", message, sequenceNumber);
+                }
             } else {
                 mSession.onAppMessage(namespace, message);
             }
@@ -131,11 +150,12 @@ public class CastRouteController implements
RouteController, MediaNotificationLi
     private String mSessionId;
     private String mApplicationStatus;
     private ApplicationMetadata mApplicationMetadata;
-    private int mSequenceNumber;
     private boolean mStoppingApplication;
     private boolean mDetached;
     private MediaNotificationInfo.Builder mNotificationBuilder;
 
+    private SparseIntArray mRequests;
+
     /**
      * Initializes a new {@link CastRouteController} instance.
      * @param apiClient The Google Play Services client used to create the
session.
@@ -167,6 +187,7 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
         mApplicationMetadata = metadata;
         mApplicationStatus = applicationStatus;
         mCastDevice = castDevice;
+        mRequests = new SparseIntArray();
 
         mMessageChannel = new CastMessagingChannel(this);
         addNamespace(RECEIVER_NAMESPACE);
@@ -193,7 +214,7 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
                 sMediaOverloadedMessageTypes = new HashMap<String, String>();
                 sMediaOverloadedMessageTypes.put("STOP_MEDIA", "STOP");
                 sMediaOverloadedMessageTypes.put("MEDIA_SET_VOLUME",
"SET_VOLUME");
-                sMediaOverloadedMessageTypes.put("MEDIA_GET_STATUS",
"MEDIA_STATUS");
+                sMediaOverloadedMessageTypes.put("MEDIA_GET_STATUS",
"GET_STATUS");
             }
         }
     }
@@ -217,37 +238,7 @@ public class CastRouteController implements
RouteController, MediaNotificationLi
 
     @Override
     public void close() {
-        if (mStoppingApplication) return;
-
-        if (isApiClientInvalid()) return;
-
-        mStoppingApplication = true;
-        Cast.CastApi.stopApplication(mApiClient, mSessionId)
-                .setResultCallback(new ResultCallback<Status>() {
-                    @Override
-                    public void onResult(Status status) {
-                        onMessage("remove_session", mSessionId);
-                        // TODO(avayvod): handle a failure to stop the
application.
-                        // https://crbug.com/535577
-
-                        for (String namespace : mNamespaces)
unregisterNamespace(namespace);
-                        mNamespaces.clear();
-
-                        mClients.clear();
-                        mSessionId = null;
-                        mApiClient = null;
-
-                        mRouteDelegate.onRouteClosed(CastRouteController.this);
-                        mStoppingApplication = false;
-
-                        // The detached route will be closed only if another
route joined
-                        // the same session so it will take over the
notification.
-                        if (!mDetached) {
-                            MediaNotificationManager.hide(
-                                    mTabId, R.id.presentation_notification);
-                        }
-                    }
-                });
+        stopApplication(INVALID_SEQUENCE_NUMBER);
     }
 
     @Override
@@ -312,7 +303,7 @@ public class CastRouteController implements RouteController,
MediaNotificationLi
 
     @Override
     public void onStop(int actionSource) {
-        close();
+        stopApplication(INVALID_SEQUENCE_NUMBER);
     }
 
 
@@ -321,12 +312,11 @@ public class CastRouteController implements
RouteController, MediaNotificationLi
      * @param type The type of the message (e.g. "new_session" or "v2_message")
      * @param message The message itself (encoded JSON).
      */
-    public void onMessage(String type, String message) {
+    public void onMessage(String type, String message, int sequenceNumber) {
         for (String client : mClients) {
             mRouteDelegate.onMessage(mMediaRouteId,
-                    buildInternalMessage(type, message, client,
mSequenceNumber));
+                    buildInternalMessage(type, message, client,
sequenceNumber));
         }
-        mSequenceNumber++;
     }
 
     /**
@@ -340,12 +330,47 @@ public class CastRouteController implements
RouteController, MediaNotificationLi
             jsonMessage.put("sessionId", mSessionId);
             jsonMessage.put("namespaceName", namespace);
             jsonMessage.put("message", message);
-            onMessage("app_message", jsonMessage.toString());
+            onMessage("app_message", jsonMessage.toString(),
INVALID_SEQUENCE_NUMBER);
         } catch (JSONException e) {
             Log.e(TAG, "Failed to create the message wrapper", e);
         }
     }
 
+    private void stopApplication(final int sequenceNumber) {
+        if (mStoppingApplication) return;
+
+        if (isApiClientInvalid()) return;
+
+        mStoppingApplication = true;
+        Cast.CastApi.stopApplication(mApiClient, mSessionId)
+                .setResultCallback(new ResultCallback<Status>() {
+                    @Override
+                    public void onResult(Status status) {
+                        onMessage("remove_session", mSessionId,
sequenceNumber);
+
+                        // TODO(avayvod): handle a failure to stop the
application.
+                        // https://crbug.com/535577
+
+                        for (String namespace : mNamespaces)
unregisterNamespace(namespace);
+                        mNamespaces.clear();
+
+                        mClients.clear();
+                        mSessionId = null;
+                        mApiClient = null;
+
+                        mRouteDelegate.onRouteClosed(CastRouteController.this);
+                        mStoppingApplication = false;
+
+                        // The detached route will be closed only if another
route joined
+         …
(message too large)

[commit-bot: I haz the power, 2015-10-30 18:44:43]

The CQ bit was unchecked by commit-bot@chromium.org

[whywhat, 2015-10-31 11:22:31]

Landed as https://codereview.chromium.org/1429833002