Decouple MediaSession messages from WebContents (full patch)

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..72121257beaa2312409b8704cbeb917f8a143266
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
@@ -0,0 +1,44 @@
+// 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;
+
+/**
+ * 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 {
+    /**
+     * Construct a {@link MediaSessionObserver} and start observing |mediaSession|.
+     * @param mediaSession The {@link MediaSession} to observe.
+     */
+    public MediaSessionObserver(MediaSession mediaSession) {

[boliu, 2016/10/29 01:11:28]

protected?

[Zhiqiang Zhang (Slow), 2016/10/31 11:57:34]

Dones.

+        mediaSession.addObserver(this);

[boliu, 2016/10/29 01:11:28]

if you have this, then client should never have to call addObserver directly so
it shouldn't be part of the public interface

then it's odd to have removeObserver but not add, so maybe makes more sense to
remove this constructor instead?

[Zhiqiang Zhang (Slow), 2016/10/31 11:57:34]

I know we have two solutions for the observers:
A. Let observers start observing on construction, and stop observing on
destruction. For A, the observer need to store a weak reference to
MediaSessionImpl
B. Let users create observers and add/remove observers.

Seems you and jam@ like B more, since there are not too many users.
After going with B, I found it's so painful on the user side.
Also we actually have some more users, since the tests should be counted :)

So probably I have at least 5 reasons why we should go with A. Here I just list
the major reasons.

1) So for B, we have to expose AddObserver/RemoveObserver to content/public. And
I found exposing these methods are not very useful since they are called every
time on constructing and cleaning up an observer. While for A, add/remove is
implied in construction/destruction, if the user no longer needs it, it just
need to destroy it.
2) When letting users handle add/remove, they have to know which MediaSession
they are observing, it's natural to bind MediaSession to the observer.
3) Also it's really easy to make mistakes (already seen this issue when updating
the tests), such as adding observers in the base class and add it again in a
derived class or in the embedder. Also, they need to remember to remove the
observer before it destructs, otherwise MediaSession will have a invalid
reference to a deleted observer.
4) If there are reference cycle problem involving MediaSessionImpl.java, users
are more likely to make mistakes since the user may hold a strong reference to
MediaSessionImpl instead of a WeakReference.

Do you have a strong opinion why B is better? I know we argued this back and
forth several times, but when I did changes on the user side, I really found B
is annoying and making things complicated.

I made two patchs (#6 and #7) implementing either, we can land any of them when
we reach an agreement.

[boliu, 2016/10/31 18:58:55]

I didn't say anything about removing observers. Java doesn't have a destructor,
and don't implement one. So you most definitely need a removeObserver in java.
Not really.
1) is not a reason
2) that's the case with both A or B
3) huh? c++ or java? base class should be pure virtual or pure interface, no?
4) yeah don't implement destructor in java, but also, I don't see the problem of
observer holding strong ref to MediaSessionImpl.java?

Overall, it's not a big deal either way. But the points you made don't really
work. I didn't read the code, but if you are using WeakReference to implement a
pseudo destructor in java, then that's definitely way *worse* than the previous
state.

[Zhiqiang Zhang (Slow), 2016/10/31 19:50:03]

Are you OK if I do A in native and B in Java? It's the same as WebContents. I
think A really makes the native users easier.

[boliu, 2016/10/31 20:13:19]

Do what jam@ says for native. I didn't even look at the native code..

For java, we've already established A would not work since there is no
destructor in java. But B is not the only option. If you really want to register
the observer in its constructor, then you can have a destroy method on the
observer (to replace MediaSession.removeObserver). That would be more consistent
than removing addObserver but keeping removeObserver.

[Zhiqiang Zhang (Slow), 2016/10/31 22:22:26]

OK, so finally I realized what you mean. I was confused before so I was thinking
you don't like pattern A. So you were actually talking about GC-related issues.
The "destructor in Java" I mentioned is actually "stopObserving", not
"destroy()".

So now I'm implementing A in native and Java, and having a strong reference to
MediaSessionImpl here. jam@ is not very strong against A so I think we can go
this way.

+    }
+
+    /**
+     * 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) {}
+}