Link MediaSessionTabHelper with native MediaSession [CL is going to be split]

content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java

+// Copyright 2016 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.
+
+package org.chromium.content_public.browser;
+
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.content_public.common.MediaMetadata;
+
+/**
+ * The class for listening to native {@link MediaSession} and operating on it. It is the Java
+ * counterpart of {@link MediaSessionDelegateAndroid}. The Java object and its linked native object
+ * must notify each other when anyone of them is destroyed. Clients of this class should create a
+ * {@link MediaSessionDelegate} object and explicitly call
+ * {@link WebContents#addMediaSessionDelegate} with the object as an argument. Clients of this class
+ * should also call {@link MediaSessionDelegate#disconnectMediaSession} when dereferencing the
+ * object.
+ */
+@JNINamespace("content")
+public class MediaSessionDelegate {

[whywhat, 2016/10/20 22:56:53]

Do you expect other derived classes from this one?
Perhaps merge subclass from MSTH here (and move it to this separate file)?

[Zhiqiang Zhang (Slow), 2016/10/21 13:43:53]

I prefer not to merge it, since the subclass in MSTH is an inner class and need
to access MSTH methods. Besides, having a base class like this is easier to mock
in testing.

+    // The native MediaSessionDelegateAndroid object
+    private long mNativeMediaSessionDelegateAndroid;
+
+    /**
+     * Disconnects this delegate from the associated MediaSession. Clients of this class must call
+     * this method when dereferencing this object. The clean up is done in
+     * {@link MediaSessionDelegate#mediaSessionDisconnected}.
+     */
+    public final void disconnectMediaSession() {
+        if (hasNativeMediaSession()) {
+            nativeDisconnectMediaSession(mNativeMediaSessionDelegateAndroid);
+        }
+    }
+
+    /**
+     * Called when this {@link MediaSessionDelegate} has been disconnected from a native
+     * {@link MediaSession}.
+     */
+    public void mediaSessionDisconnected() {}
+
+    /**
+     * Called when the native {@link MediaSession} state has changed.
+     * @param isControllable Whether the native {@link MediaSession} is controllable.
+     * @param isSuspended Whether the native {@link MediaSession} is suspended.
+     */
+    @CalledByNative
+    public void mediaSessionStateChanged(boolean isControllable, boolean isSuspended) {}
+
+    /**
+     * Called when the native {@link MediaSession} has changed metadata.
+     * @param metadata The new metadata of the native {@link MediaSession}. "null" is for unsetting
+     * metadata.
+     */
+    @CalledByNative
+    public void mediaSessionMetadataChanged(MediaMetadata metadata) {}
+
+    /**
+     * Resume the native {@link MediaSession}.
+     */
+    public void resumeMediaSession() {
+        if (hasNativeMediaSession()) nativeResumeMediaSession(mNativeMediaSessionDelegateAndroid);

[whywhat, 2016/10/20 22:56:53]

I think if you set mMediaSessionDelegate to null in mediaSessionDisconnected()
you don't need to have these checks for hasNativeMediaSession.

[Zhiqiang Zhang (Slow), 2016/10/21 13:43:53]

Yes. Just in case the client still somehow calls these methods after
disconnected.

[whywhat, 2016/10/21 15:51:36]

How? If you set the only reference to it to null, you should be fine.

+    }
+
+    /**
+     * Suspend the native {@link MediaSession}.
+     */
+    public void suspendMediaSession() {
+        if (hasNativeMediaSession()) nativeSuspendMediaSession(mNativeMediaSessionDelegateAndroid);
+    }
+
+    /**
+     * Stop the native {@link MediaSession}.
+     */
+    public void stopMediaSession() {
+        if (hasNativeMediaSession()) nativeStopMediaSession(mNativeMediaSessionDelegateAndroid);
+    }
+
+    @CalledByNative
+    private final void mediaSessionConnected(long nativeMediaSessionDelegateAndroid) {
+        mNativeMediaSessionDelegateAndroid = nativeMediaSessionDelegateAndroid;
+    }
+
+    @CalledByNative
+    private void mediaSessionDisconnectedWrapper() {

[whywhat, 2016/10/20 22:56:53]

why wrapper?

Perhaps you could've:

@CalledByNative
private void onNativeDelegateCreated(long nativeMediaSessionDelegateAndroid); //
this'd better be a ctor, I think

@CalledByNative
private void onNativeDelegateDestroyed();

[Zhiqiang Zhang (Slow), 2016/10/21 13:43:53]

Yes, sorry I'm bad at names. Using "on-" now. However I still prefer
connected/disconnected, since it might be removing the delegate from the
MediaSession while the session is still alive.

[whywhat, 2016/10/21 15:51:36]

Does it matter that MediaSession is still alive if the native delegate was
actually destroyed? Do you mean this Java object can be attached/reattached to
different instances of the native counterpart?

+        mNativeMediaSessionDelegateAndroid = 0;
+        mediaSessionDisconnected();
+    }
+
+    /**
+     * @return Whether this {@link MediaSessionDelegate} object is connected to a native
+     * MediaSession.
+     */
+    private boolean hasNativeMediaSession() {
+        return mNativeMediaSessionDelegateAndroid != 0;
+    }
+
+    private native void nativeResumeMediaSession(long nativeMediaSessionDelegateAndroid);
+    private native void nativeSuspendMediaSession(long nativeMediaSessionDelegateAndroid);
+    private native void nativeStopMediaSession(long nativeMediaSessionDelegateAndroid);
+    private native void nativeDisconnectMediaSession(long nativeMediaSessionDelegateAndroid);
+}