Decouple MediaSession/MediaSessionObserver from WebContents in Java[OBSOLETE, go to the combined CL]

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

Index: content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
diff --git a/content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java b/content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
new file mode 100644
index 0000000000000000000000000000000000000000..5cf0542856e38be2d44d104d6c572b3f2e744172
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
@@ -0,0 +1,67 @@
+// 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.content_public.common.MediaMetadata;
+
+import javax.annotation.Nullable;
+
+/**
+ * This class is Java implementation of native MediaSessionObserver. The class receives media
+ * session messages from Java {@link MediaSession}, which acts acts as a proxy forwarding messages
+ * comming from the native MediaSession.
+ */
+public abstract class MediaSessionObserver {
+    private MediaSession mMediaSession;
+
+    /**
+     * Construct a {@link MediaSessionObserver} and start observing |mediaSession|.
+     * @param mediaSession The {@link MediaSession} to observe.
+     */
+    public MediaSessionObserver(MediaSession mediaSession) {
+        mMediaSession = mediaSession;
+        mediaSession.addObserver(this);
+    }
+
+    /**
+     * @return The observed {@link MediaSession}. Returns null when the session is destroyed.
+     */
+    @Nullable
+    public final MediaSession getMediaSession() {

[boliu, 2016/10/28 17:39:13]

this method should not be on this interface, subclass that already has to pass
the media session into the constructor doesn't need a helper method to get it

[whywhat, 2016/10/28 19:14:38]

Then each subclass will have to keep its own pointer, why is it better?
I think the justification here is for observers to handle common tasks like
keeping the pointer to what they observe up-to-date.
I think this is a common pattern in C++ at least (in Blink and content).

[boliu, 2016/10/28 20:29:14]

jam@ made the exact same comment independently for the native interface:
https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...

How many subclasses are you going to have?

[Zhiqiang Zhang (Slow), 2016/10/28 20:35:57]

OK, for now, it's just one... I'll remove it.

+        return mMediaSession;
+    }
+
+    /**
+     * Called when the observed media session is destroyed.
+     *
+     * Gives subclass a chance to do clean up. After the whole loop over
+     * {@link MediaSessionObserver}s has been finished, {@link MediaSessionObserver#getMediaSession}
+     * will return null.
+     */
+    public void mediaSessionDestroyed() {}
+
+    /**
+     * Called when the observed {@link MediaSession} state has changed.
+     * @param isControllable If the session can be resumed or suspended.
+     * @param isSuspended if the session currently suspended or not.
+     */
+    public void mediaSessionStateChanged(boolean isControllable, boolean isSuspended) {}
+
+    /**
+     * Called when the {@link MediaSession} metadata has changed.
+     * @param metadata the new MediaMetadata after change.
+     */
+    public void mediaSessionMetadataChanged(MediaMetadata metadata) {}
+
+    /**
+     * Stop observing the {@link MediaSession}.
+     *
+     * {@link MediaSessionObserver#getMediaSession} will return null after this method is called.
+     */
+    public final void stopObserving() {

[boliu, 2016/10/28 17:39:13]

keeping a reference to mMediaSession here seems wrong, why does the observer
care which media session it's observing? maybe it can observer multiple
mediasessions?

also nit: can you group the final methods together (which are really not part of
the public interface except for quirks in java..)

[whywhat, 2016/10/28 19:14:38]

Then it would have to receive MediaSession as an extra argument to each method
to be able to tell which one is calling.
I think until we need observing multiple media sessions we're fine with only
supporting one MS here.

[boliu, 2016/10/28 20:29:14]

Never mind about the observing multiple sessions. But regardless, why does
stopObserving need to exist at all?

[Zhiqiang Zhang (Slow), 2016/10/28 20:35:57]

OK, so you prefer the users take the responsibility to do clean up. I can do it.

+        if (mMediaSession != null) mMediaSession.removeObserver(this);
+        mMediaSession = null;
+    }
+}