Decouple MediaSession messages from WebContents (full patch)

Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_IkeHb5SJ73d5nwzxZYV8/edit?usp=sharing

BUG=658678
Committed: https://crrev.com/1adf3cb5c81b84968f4e541edc97f6cd6f2a96b4
Cr-Commit-Position: refs/heads/master@{#428976}

[Zhiqiang Zhang (Slow), 2016-10-26 12:39:38]

zqzhang@chromium.org changed reviewers:
+ jam@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-26 12:40:57]

Description was changed from

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678
==========

to

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678
==========

[Zhiqiang Zhang (Slow), 2016-10-26 12:50:37]

Some annotations to help you review.

https://codereview.chromium.org/2453623003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2453623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
This is the Java user.

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
chromecast/browser/cast_media_blocker.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
This is the native user.

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/audio_focus_delegate.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/audio_focus_delegate.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Changes to audio_focus_delegate* are just a result of the MediaSession
abstraction, so they need MediaSessionImpl instead of MediaSession.

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_android.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
This is a hybrid of "WebContentsAndroid & WebContentsObserverProxy" for
MediaSession. Used for communicating between native and Java.

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_controller.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_controller.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
Changes to media_session_controller* are just a result of the MediaSession
abstraction, so they need MediaSessionImpl instead of MediaSession.

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_impl.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
The interface has been extracted into content/public/

https://codereview.chromium.org/2453623003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2453623003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
This is a hybrid of "WebContentsImpl & WebContentsObserverProxy" (Java) for
MediaSession (Java). Used to communicate between native and Java.

[jam, 2016-10-26 17:30:59]

lgtm with small nits

I read through the whole change, but it'd be good to also get more specific
owners for
content/browser/media/*
in case I missed anything.

and definitely for 
chrome/android/*
content/public/android/*
since I'm not familiar with the java side.

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
chromecast/browser/cast_media_blocker.h:34: // content::WebContentsObserver
implementation:
nit: update

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_android.h:27:
MediaSessionAndroid(MediaSession* session);
nit: explicit

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
content/public/browser/media_session_observer.h:22: MediaSession*
media_session() const;
this seems unnecessary, i.e. the two place that uses this can store the
WebContents or MediaSession directly.

I realize WebContentsObserver does that, but that was to avoid a few hundred
derived classes doing the same thing. but here there's only two users. ditto for
the StopObserving method and the destructor

[jam, 2016-10-26 17:45:45]

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/OWNERS
File content/public/browser/OWNERS (right):

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/OWNE...
content/public/browser/OWNERS:6: # MediaSession specific changes
also, please remove this part. we only want content/owners reviewing
content/public changes to ensure consistency with the rest of the public API.

I realize the above section goes against that. I wasn't aware of that and I'm
reaching out to these owners to undo that.

[Zhiqiang Zhang (Slow), 2016-10-26 18:44:25]

Great thanks! I'll find reviewers for the split CLs and come back to you for
rubberstamping after they approved.

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2453623003/diff/1/chromecast/browser/cast_med...
chromecast/browser/cast_media_blocker.h:34: // content::WebContentsObserver
implementation:
On 2016/10/26 17:30:59, jam wrote:
> nit: update

Done in the splitted CL.

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_android.h:27:
MediaSessionAndroid(MediaSession* session);
On 2016/10/26 17:30:59, jam wrote:
> nit: explicit

Done in the splitted CL.

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/OWNERS
File content/public/browser/OWNERS (right):

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/OWNE...
content/public/browser/OWNERS:6: # MediaSession specific changes
On 2016/10/26 17:45:45, jam wrote:
> also, please remove this part. we only want content/owners reviewing
> content/public changes to ensure consistency with the rest of the public API.
> 
> I realize the above section goes against that. I wasn't aware of that and I'm
> reaching out to these owners to undo that.

Done in the splitted CL.

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
content/public/browser/media_session_observer.h:22: MediaSession*
media_session() const;
On 2016/10/26 17:30:59, jam wrote:
> this seems unnecessary, i.e. the two place that uses this can store the
> WebContents or MediaSession directly.
> 
> I realize WebContentsObserver does that, but that was to avoid a few hundred
> derived classes doing the same thing. but here there's only two users. ditto
for
> the StopObserving method and the destructor

I slightly disagree here. We actually have three places using the observer:
CastMediaBlocker, MediaSessionAndroid (for JNI) and MediaSessionTabHelper
(Java). These functions are what every user should do, so putting them here
should avoid misuses.

It's a bit confusing to remove both media_session() and StopObserving() at the
same time. If we remove StopObserving(), then all users should explicitly
call MediaSession::RemoveObserver() to avoid dangling reference, then every user
have to keep a copy of MediaSession. Then users may think: why not have a
media_session() helper method?

[Zhiqiang Zhang (Slow), 2016-10-28 22:40:06]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, boliu@chromium.org, halliwell@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-28 22:40:07]

PTAL.
Sorry I have to bring all you guys into this one combined CL and land them
together.

boliu@: please look at content/public/android
avayvod@: please look at *media*
halliwell@: just need your rubberstamp, it was cherry-picked from
https://codereview.chromium.org/2450433002/ (which you already RS'ed, just have
some slight changes due to the content API shape)

cc-ing derekjchow@.

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2453623003/diff/1/content/public/browser/medi...
content/public/browser/media_session_observer.h:22: MediaSession*
media_session() const;
On 2016/10/26 18:44:25, Zhiqiang Zhang wrote:
> On 2016/10/26 17:30:59, jam wrote:
> > this seems unnecessary, i.e. the two place that uses this can store the
> > WebContents or MediaSession directly.
> > 
> > I realize WebContentsObserver does that, but that was to avoid a few hundred
> > derived classes doing the same thing. but here there's only two users. ditto
> for
> > the StopObserving method and the destructor
> 
> I slightly disagree here. We actually have three places using the observer:
> CastMediaBlocker, MediaSessionAndroid (for JNI) and MediaSessionTabHelper
> (Java). These functions are what every user should do, so putting them here
> should avoid misuses.
> 
> It's a bit confusing to remove both media_session() and StopObserving() at the
> same time. If we remove StopObserving(), then all users should explicitly
> call MediaSession::RemoveObserver() to avoid dangling reference, then every
user
> have to keep a copy of MediaSession. Then users may think: why not have a
> media_session() helper method?

Removed all the MediaSession getter and StopObserving()

[Zhiqiang Zhang (Slow), 2016-10-28 22:40:58]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 22:41:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 23:02:41]

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

[commit-bot: I haz the power, 2016-10-28 23:02:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[halliwell, 2016-10-28 23:54:45]

chromecast/ lgtm % nit

https://codereview.chromium.org/2453623003/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2453623003/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:24: content::WebContents* web_contents);
remove explicit

[Zhiqiang Zhang (Slow), 2016-10-29 00:19:26]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 00:20:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 00:38:30]

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

[commit-bot: I haz the power, 2016-10-29 00:38:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

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

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:19: bool
RegisterMediaSessionNatives(JNIEnv* env);
remove

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
Leaking the java object outside of class is generally a bad pattern
(unfortunately we do it all over the place)

in this case, it can be avoided by friending the jni function

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
I raised the point about strong reference from native earlier. Don't remember
getting an answer though, if onDetachedFromWindow guarantees this object will be
deleted?

fwiw WebContentsImpl.java/etc is already wrong here, and should be fixed. don't
make this wrong too :)

https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:16:
* @return The MediaSession associated with |contents|.
should note here that it may be null

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:19:
public MediaSessionObserver(MediaSession mediaSession) {
protected?

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
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?

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:326:
ObserverList.RewindableIterator<WebContentsObserver> getObserversForTesting();
looks like this can be removed too

[whywhat, 2016-10-29 05:21:57]

lgtm

[Zhiqiang Zhang (Slow), 2016-10-29 17:47:56]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 17:48:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 17:58:53]

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

[commit-bot: I haz the power, 2016-10-29 17:58:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Zhiqiang Zhang (Slow), 2016-10-29 21:58:39]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 21:58:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 22:17:03]

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

[commit-bot: I haz the power, 2016-10-29 22:17:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Zhiqiang Zhang (Slow), 2016-10-29 22:51:23]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 22:51:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 23:21:37]

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

[commit-bot: I haz the power, 2016-10-29 23:21:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Zhiqiang Zhang (Slow), 2016-10-30 00:15:01]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-30 00:15:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-30 01:22:43]

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

[commit-bot: I haz the power, 2016-10-30 01:22:44]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Zhiqiang Zhang (Slow), 2016-10-30 14:58:41]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-30 14:58:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-30 16:28:28]

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

[commit-bot: I haz the power, 2016-10-30 16:28:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-10-31 11:57:35]

PTAL for boliu@

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:19: bool
RegisterMediaSessionNatives(JNIEnv* env);
On 2016/10/29 01:11:28, boliu wrote:
> remove

Done.

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
On 2016/10/29 01:11:28, boliu wrote:
> Leaking the java object outside of class is generally a bad pattern
> (unfortunately we do it all over the place)
> 
> in this case, it can be avoided by friending the jni function

Ugh, I tried but failed. The problem is that the jni function is a static
function (declared in the generated jni header), so it has to be declared before
MediaSessionAndroid as a static function. Then all other files including
media_session_impl.h will get compilation failure saying "static ...(...)" is
declared but not defined (-Werror=unused-function).

I added a comment above this method to alert users to take care.

Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may prevent
leaking the reference.

Or maybe I think there are some limitations in the JNI generator?  If Java can
call a static member method of a class, we can eliminate some need for
non-class-member "GetJavaObject()".

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
I don't know about how GC on WebContentsImpl.java works, but I can only explain
the link chain for MediaSession and let you decide. The link chain is:
WebContentsImpl(native) -> MediaSession(native) -> MediaSessionAndroid(native)
-> MediaSessionImpl.Java
When WebContentsImpl(native) is destroyed, all the native objects on the chain
will be destroyed.

The Java user (non-WebView) will has the following chain:
Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
Also I think the link MediaSessionTabHelper->Tab can be changed to
WeakReference.

Is there a possible leak on this? Or maybe we should make
MediaSessionTabHelper->MediaSessionImpl weak if yes.

I think "a Java user referencing MediaSessionImpl.java" is safer than "a Java
user referencing WebContentsImpl.java". If the user does't reference too high on
the reference chain, it should be OK.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:16:
* @return The MediaSession associated with |contents|.
On 2016/10/29 01:11:28, boliu wrote:
> should note here that it may be null

Done.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:19:
public MediaSessionObserver(MediaSession mediaSession) {
On 2016/10/29 01:11:28, boliu wrote:
> protected?

Dones.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
On 2016/10/29 01:11:28, boliu wrote:
> 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?

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.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:326:
ObserverList.RewindableIterator<WebContentsObserver> getObserversForTesting();
On 2016/10/29 01:11:28, boliu wrote:
> looks like this can be removed too

Done.

[Zhiqiang Zhang (Slow), 2016-10-31 16:28:25]

Patchset #8 (id:140001) has been deleted

[boliu, 2016-10-31 18:58:56]

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> On 2016/10/29 01:11:28, boliu wrote:
> > Leaking the java object outside of class is generally a bad pattern
> > (unfortunately we do it all over the place)
> > 
> > in this case, it can be avoided by friending the jni function
> 
> Ugh, I tried but failed. The problem is that the jni function is a static
> function (declared in the generated jni header), so it has to be declared
before
> MediaSessionAndroid as a static function. Then all other files including
> media_session_impl.h will get compilation failure saying "static ...(...)" is
> declared but not defined (-Werror=unused-function).
> 
> I added a comment above this method to alert users to take care.
> 
> Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may prevent
> leaking the reference.
> 
> Or maybe I think there are some limitations in the JNI generator?  If Java can
> call a static member method of a class, we can eliminate some need for
> non-class-member "GetJavaObject()".

Oh, static :/

You could declare another function, so static function calls this function calls
GetJavaObject, but that's like back to the original problem of exposing that
other function.

One step further would be to forward declare an inner class/struct, which can
hide more details...

Or alternative is to implement it java, but java WebContents doesn't have a
UserData equivalent afaik, so maybe not.

Inner class/struct it is then?

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> I don't know about how GC on WebContentsImpl.java works, but I can only
explain
> the link chain for MediaSession and let you decide. The link chain is:
> WebContentsImpl(native) -> MediaSession(native) -> MediaSessionAndroid(native)
> -> MediaSessionImpl.Java
> When WebContentsImpl(native) is destroyed, all the native objects on the chain
> will be destroyed.
> 
> The Java user (non-WebView) will has the following chain:
> Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
> Also I think the link MediaSessionTabHelper->Tab can be changed to
> WeakReference.
> 
> Is there a possible leak on this?

Yes in webview, where native WebContentsImpl destruction relies on gc, so a
strong reference from native (ie a gc root) can't transitively reference
WebContents and prevent it from garbage collected.

> Or maybe we should make
> MediaSessionTabHelper->MediaSessionImpl weak if yes.

Be very careful that *something* is keeping the MediaSessionImpl alive on the
java side (java references are generally not gc roots, other than static and
local references) while the WebContents is alive. Is there something like that?

> 
> I think "a Java user referencing MediaSessionImpl.java" is safer than "a Java
> user referencing WebContentsImpl.java". If the user does't reference too high
on
> the reference chain, it should be OK.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> On 2016/10/29 01:11:28, boliu wrote:
> > 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?
> 
> 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

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.

> 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 :)

Not really.

> 
> 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.

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]

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 18:58:55, boliu wrote:
> On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > I don't know about how GC on WebContentsImpl.java works, but I can only
> explain
> > the link chain for MediaSession and let you decide. The link chain is:
> > WebContentsImpl(native) -> MediaSession(native) ->
MediaSessionAndroid(native)
> > -> MediaSessionImpl.Java
> > When WebContentsImpl(native) is destroyed, all the native objects on the
chain
> > will be destroyed.
> > 
> > The Java user (non-WebView) will has the following chain:
> > Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
> > Also I think the link MediaSessionTabHelper->Tab can be changed to
> > WeakReference.
> > 
> > Is there a possible leak on this?
> 
> Yes in webview, where native WebContentsImpl destruction relies on gc, so a
> strong reference from native (ie a gc root) can't transitively reference
> WebContents and prevent it from garbage collected.
> 
> > Or maybe we should make
> > MediaSessionTabHelper->MediaSessionImpl weak if yes.
> 
> Be very careful that *something* is keeping the MediaSessionImpl alive on the
> java side (java references are generally not gc roots, other than static and
> local references) while the WebContents is alive. Is there something like
that?
> 
> > 
> > I think "a Java user referencing MediaSessionImpl.java" is safer than "a
Java
> > user referencing WebContentsImpl.java". If the user does't reference too
high
> on
> > the reference chain, it should be OK.
> 

I think the problem lies in the client side.

If we use weak reference here, then we still need to find some object to hold a
reference of the MediaSessionImpl.java, otherwise it will be garbage collected
when we still want to use it. WebContentsImpl.java could possibly hold the
MediaSesssionImpl.java reference, but that still cannot prevent having a
reference cycle if the observer prevents WebContents being gc'ed.

So I think if the client is causing a leak, it doesn't matter we use a
strong/weak reference here.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
On 2016/10/31 18:58:55, boliu wrote:
> On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > On 2016/10/29 01:11:28, boliu wrote:
> > > 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?
> > 
> > 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
> 
> 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.
> 
> > 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 :)
> 
> Not really.
> 
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> On 2016/10/31 18:58:55, boliu wrote:
> > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > I don't know about how GC on WebContentsImpl.java works, but I can only
> > explain
> > > the link chain for MediaSession and let you decide. The link chain is:
> > > WebContentsImpl(native) -> MediaSession(native) ->
> MediaSessionAndroid(native)
> > > -> MediaSessionImpl.Java
> > > When WebContentsImpl(native) is destroyed, all the native objects on the
> chain
> > > will be destroyed.
> > > 
> > > The Java user (non-WebView) will has the following chain:
> > > Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
> > > Also I think the link MediaSessionTabHelper->Tab can be changed to
> > > WeakReference.
> > > 
> > > Is there a possible leak on this?
> > 
> > Yes in webview, where native WebContentsImpl destruction relies on gc, so a
> > strong reference from native (ie a gc root) can't transitively reference
> > WebContents and prevent it from garbage collected.
> > 
> > > Or maybe we should make
> > > MediaSessionTabHelper->MediaSessionImpl weak if yes.
> > 
> > Be very careful that *something* is keeping the MediaSessionImpl alive on
the
> > java side (java references are generally not gc roots, other than static and
> > local references) while the WebContents is alive. Is there something like
> that?
> > 
> > > 
> > > I think "a Java user referencing MediaSessionImpl.java" is safer than "a
> Java
> > > user referencing WebContentsImpl.java". If the user does't reference too
> high
> > on
> > > the reference chain, it should be OK.
> > 
> 
> I think the problem lies in the client side.

Meaning outside of content?

> 
> If we use weak reference here, then we still need to find some object to hold
a
> reference of the MediaSessionImpl.java, otherwise it will be garbage collected
> when we still want to use it.

Yes exactly.

> WebContentsImpl.java could possibly hold the
> MediaSesssionImpl.java reference, but that still cannot prevent having a
> reference cycle if the observer prevents WebContents being gc'ed.

I think you might be misunderstanding how garbage collection works.

Reference cycles are fine. gc is able to figure out the whole cycle is no longer
reachable from a gc root, and then collect them all in one go (this of course is
different from ref counting)

It's also ok for java WebContentsImpl/MediaSesssionImpl/MediaSessionObserver to
have strong references between each other without causing gc issues, since none
of those references are gc roots.

If you are worried client introducing *another* gc root in client code, then
that's always a possibility. But at least the problem is not with this code.

> 
> So I think if the client is causing a leak, it doesn't matter we use a
> strong/weak reference here.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> On 2016/10/31 18:58:55, boliu wrote:
> > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > On 2016/10/29 01:11:28, boliu wrote:
> > > > 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?
> > > 
> > > 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
> > 
> > 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.
> > 
> > > 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
:)
> > 
> > Not really.
> > 
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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]

PTAL
Hope we are getting close to the final solution

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
On 2016/10/31 18:58:55, boliu wrote:
> On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > On 2016/10/29 01:11:28, boliu wrote:
> > > Leaking the java object outside of class is generally a bad pattern
> > > (unfortunately we do it all over the place)
> > > 
> > > in this case, it can be avoided by friending the jni function
> > 
> > Ugh, I tried but failed. The problem is that the jni function is a static
> > function (declared in the generated jni header), so it has to be declared
> before
> > MediaSessionAndroid as a static function. Then all other files including
> > media_session_impl.h will get compilation failure saying "static ...(...)"
is
> > declared but not defined (-Werror=unused-function).
> > 
> > I added a comment above this method to alert users to take care.
> > 
> > Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may
prevent
> > leaking the reference.
> > 
> > Or maybe I think there are some limitations in the JNI generator?  If Java
can
> > call a static member method of a class, we can eliminate some need for
> > non-class-member "GetJavaObject()".
> 
> Oh, static :/
> 
> You could declare another function, so static function calls this function
calls
> GetJavaObject, but that's like back to the original problem of exposing that
> other function.
> 
> One step further would be to forward declare an inner class/struct, which can
> hide more details...
> 
> Or alternative is to implement it java, but java WebContents doesn't have a
> UserData equivalent afaik, so maybe not.
> 
> Inner class/struct it is then?

Done. But actually I found other files including the header can define the inner
struct themselves while not triggering a redefinition error. So basically this
solution is making leaking the object harder but still possible. 
Not sure if it can be caught by more strict compiler/linker checks :/

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 20:13:19, boliu wrote:
> On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> > On 2016/10/31 18:58:55, boliu wrote:
> > > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > > I don't know about how GC on WebContentsImpl.java works, but I can only
> > > explain
> > > > the link chain for MediaSession and let you decide. The link chain is:
> > > > WebContentsImpl(native) -> MediaSession(native) ->
> > MediaSessionAndroid(native)
> > > > -> MediaSessionImpl.Java
> > > > When WebContentsImpl(native) is destroyed, all the native objects on the
> > chain
> > > > will be destroyed.
> > > > 
> > > > The Java user (non-WebView) will has the following chain:
> > > > Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
> > > > Also I think the link MediaSessionTabHelper->Tab can be changed to
> > > > WeakReference.
> > > > 
> > > > Is there a possible leak on this?
> > > 
> > > Yes in webview, where native WebContentsImpl destruction relies on gc, so
a
> > > strong reference from native (ie a gc root) can't transitively reference
> > > WebContents and prevent it from garbage collected.
> > > 
> > > > Or maybe we should make
> > > > MediaSessionTabHelper->MediaSessionImpl weak if yes.
> > > 
> > > Be very careful that *something* is keeping the MediaSessionImpl alive on
> the
> > > java side (java references are generally not gc roots, other than static
and
> > > local references) while the WebContents is alive. Is there something like
> > that?
> > > 
> > > > 
> > > > I think "a Java user referencing MediaSessionImpl.java" is safer than "a
> > Java
> > > > user referencing WebContentsImpl.java". If the user does't reference too
> > high
> > > on
> > > > the reference chain, it should be OK.
> > > 
> > 
> > I think the problem lies in the client side.
> 
> Meaning outside of content?

Yes.

> > 
> > If we use weak reference here, then we still need to find some object to
hold
> a
> > reference of the MediaSessionImpl.java, otherwise it will be garbage
collected
> > when we still want to use it.
> 
> Yes exactly.
> 
> > WebContentsImpl.java could possibly hold the
> > MediaSesssionImpl.java reference, but that still cannot prevent having a
> > reference cycle if the observer prevents WebContents being gc'ed.
> 
> I think you might be misunderstanding how garbage collection works.
> 
> Reference cycles are fine. gc is able to figure out the whole cycle is no
longer
> reachable from a gc root, and then collect them all in one go (this of course
is
> different from ref counting)
> 
> It's also ok for java WebContentsImpl/MediaSesssionImpl/MediaSessionObserver
to
> have strong references between each other without causing gc issues, since
none
> of those references are gc roots.
> 
> If you are worried client introducing *another* gc root in client code, then
> that's always a possibility. But at least the problem is not with this code.
> 
> > 
> > So I think if the client is causing a leak, it doesn't matter we use a
> > strong/weak reference here.
> 
So IIUC WebContentsAndroid is holding a gc root of WebContentsImpl.java which it
should not, right?

Now I should go with having weak reference here and let WebContentsImpl.java
hold the reference.

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):

https://codereview.chromium.org/2453623003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:20:
mediaSession.addObserver(this);
On 2016/10/31 20:13:19, boliu wrote:
> On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> > On 2016/10/31 18:58:55, boliu wrote:
> > > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > > On 2016/10/29 01:11:28, boliu wrote:
> > > > > 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?
> > > > 
> > > > 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
> > > 
> > > 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.
> > > 
> > > > 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
> :)
> > > 
> > > Not really.
> > > 
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[Zhiqiang Zhang (Slow), 2016-10-31 22:22:48]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 22:23:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-10-31 22:44:03]

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
On 2016/10/31 22:22:26, Zhiqiang Zhang wrote:
> On 2016/10/31 18:58:55, boliu wrote:
> > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > On 2016/10/29 01:11:28, boliu wrote:
> > > > Leaking the java object outside of class is generally a bad pattern
> > > > (unfortunately we do it all over the place)
> > > > 
> > > > in this case, it can be avoided by friending the jni function
> > > 
> > > Ugh, I tried but failed. The problem is that the jni function is a static
> > > function (declared in the generated jni header), so it has to be declared
> > before
> > > MediaSessionAndroid as a static function. Then all other files including
> > > media_session_impl.h will get compilation failure saying "static ...(...)"
> is
> > > declared but not defined (-Werror=unused-function).
> > > 
> > > I added a comment above this method to alert users to take care.
> > > 
> > > Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may
> prevent
> > > leaking the reference.
> > > 
> > > Or maybe I think there are some limitations in the JNI generator?  If Java
> can
> > > call a static member method of a class, we can eliminate some need for
> > > non-class-member "GetJavaObject()".
> > 
> > Oh, static :/
> > 
> > You could declare another function, so static function calls this function
> calls
> > GetJavaObject, but that's like back to the original problem of exposing that
> > other function.
> > 
> > One step further would be to forward declare an inner class/struct, which
can
> > hide more details...
> > 
> > Or alternative is to implement it java, but java WebContents doesn't have a
> > UserData equivalent afaik, so maybe not.
> > 
> > Inner class/struct it is then?
> 
> Done. But actually I found other files including the header can define the
inner
> struct themselves while not triggering a redefinition error.

Really? I'd expect a link error. Perhaps a runtime error if you are building
component build and your definitions are in different shared libraries..

> So basically this
> solution is making leaking the object harder but still possible. 
> Not sure if it can be caught by more strict compiler/linker checks :/

You said for the other comment that java WebContentsImpl is holding the java
side, maybe you should just implement this on the java side as well?

But this is ok too

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 22:22:26, Zhiqiang Zhang wrote:
> On 2016/10/31 20:13:19, boliu wrote:
> > On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> > > On 2016/10/31 18:58:55, boliu wrote:
> > > > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > > > I don't know about how GC on WebContentsImpl.java works, but I can
only
> > > > explain
> > > > > the link chain for MediaSession and let you decide. The link chain is:
> > > > > WebContentsImpl(native) -> MediaSession(native) ->
> > > MediaSessionAndroid(native)
> > > > > -> MediaSessionImpl.Java
> > > > > When WebContentsImpl(native) is destroyed, all the native objects on
the
> > > chain
> > > > > will be destroyed.
> > > > > 
> > > > > The Java user (non-WebView) will has the following chain:
> > > > > Tab.java<->MediaSessionTabHelper.java
(observer)<->MediaSessionImpl.java
> > > > > Also I think the link MediaSessionTabHelper->Tab can be changed to
> > > > > WeakReference.
> > > > > 
> > > > > Is there a possible leak on this?
> > > > 
> > > > Yes in webview, where native WebContentsImpl destruction relies on gc,
so
> a
> > > > strong reference from native (ie a gc root) can't transitively reference
> > > > WebContents and prevent it from garbage collected.
> > > > 
> > > > > Or maybe we should make
> > > > > MediaSessionTabHelper->MediaSessionImpl weak if yes.
> > > > 
> > > > Be very careful that *something* is keeping the MediaSessionImpl alive
on
> > the
> > > > java side (java references are generally not gc roots, other than static
> and
> > > > local references) while the WebContents is alive. Is there something
like
> > > that?
> > > > 
> > > > > 
> > > > > I think "a Java user referencing MediaSessionImpl.java" is safer than
"a
> > > Java
> > > > > user referencing WebContentsImpl.java". If the user does't reference
too
> > > high
> > > > on
> > > > > the reference chain, it should be OK.
> > > > 
> > > 
> > > I think the problem lies in the client side.
> > 
> > Meaning outside of content?
> 
> Yes.
> 
> > > 
> > > If we use weak reference here, then we still need to find some object to
> hold
> > a
> > > reference of the MediaSessionImpl.java, otherwise it will be garbage
> collected
> > > when we still want to use it.
> > 
> > Yes exactly.
> > 
> > > WebContentsImpl.java could possibly hold the
> > > MediaSesssionImpl.java reference, but that still cannot prevent having a
> > > reference cycle if the observer prevents WebContents being gc'ed.
> > 
> > I think you might be misunderstanding how garbage collection works.
> > 
> > Reference cycles are fine. gc is able to figure out the whole cycle is no
> longer
> > reachable from a gc root, and then collect them all in one go (this of
course
> is
> > different from ref counting)
> > 
> > It's also ok for java WebContentsImpl/MediaSesssionImpl/MediaSessionObserver
> to
> > have strong references between each other without causing gc issues, since
> none
> > of those references are gc roots.
> > 
> > If you are worried client introducing *another* gc root in client code, then
> > that's always a possibility. But at least the problem is not with this code.
> > 
> > > 
> > > So I think if the client is causing a leak, it doesn't matter we use a
> > > strong/weak reference here.
> > 
> So IIUC WebContentsAndroid is holding a gc root of WebContentsImpl.java which
it
> should not, right?

That's a good point. Yes it will still be referenced by WebContentsImpl's
existing gc root. But overall, the point here is don't introduce *another* gc
root.

> 
> Now I should go with having weak reference here and let WebContentsImpl.java
> hold the reference.

https://codereview.chromium.org/2453623003/diff/180001/content/browser/media/...
File content/browser/media/session/media_session_android.cc (right):

https://codereview.chromium.org/2453623003/diff/180001/content/browser/media/...
content/browser/media/session/media_session_android.cc:80: if
(j_media_session_.is_empty())
in theory, this check is not safe, there is no guarantee gc is single threaded,
and it should be possible for gc to run after this check, in which case you'll
get a null pointer. (in practice, I'm not entirely sure if art actually does
that, but better be safe than sorry..)

you need to make it into a ScopedJavaLocalRef, then check for null on the local
ref

[Zhiqiang Zhang (Slow), 2016-10-31 23:30:29]

PTAL for boliu@

Fixed the issue when getting weak ref.

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.h (right):

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
On 2016/10/31 22:44:02, boliu wrote:
> On 2016/10/31 22:22:26, Zhiqiang Zhang wrote:
> > On 2016/10/31 18:58:55, boliu wrote:
> > > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > > On 2016/10/29 01:11:28, boliu wrote:
> > > > > Leaking the java object outside of class is generally a bad pattern
> > > > > (unfortunately we do it all over the place)
> > > > > 
> > > > > in this case, it can be avoided by friending the jni function
> > > > 
> > > > Ugh, I tried but failed. The problem is that the jni function is a
static
> > > > function (declared in the generated jni header), so it has to be
declared
> > > before
> > > > MediaSessionAndroid as a static function. Then all other files including
> > > > media_session_impl.h will get compilation failure saying "static
...(...)"
> > is
> > > > declared but not defined (-Werror=unused-function).
> > > > 
> > > > I added a comment above this method to alert users to take care.
> > > > 
> > > > Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may
> > prevent
> > > > leaking the reference.
> > > > 
> > > > Or maybe I think there are some limitations in the JNI generator?  If
Java
> > can
> > > > call a static member method of a class, we can eliminate some need for
> > > > non-class-member "GetJavaObject()".
> > > 
> > > Oh, static :/
> > > 
> > > You could declare another function, so static function calls this function
> > calls
> > > GetJavaObject, but that's like back to the original problem of exposing
that
> > > other function.
> > > 
> > > One step further would be to forward declare an inner class/struct, which
> can
> > > hide more details...
> > > 
> > > Or alternative is to implement it java, but java WebContents doesn't have
a
> > > UserData equivalent afaik, so maybe not.
> > > 
> > > Inner class/struct it is then?
> > 
> > Done. But actually I found other files including the header can define the
> inner
> > struct themselves while not triggering a redefinition error.
> 
> Really? I'd expect a link error. Perhaps a runtime error if you are building
> component build and your definitions are in different shared libraries..

Maybe... 

> > So basically this
> > solution is making leaking the object harder but still possible. 
> > Not sure if it can be caught by more strict compiler/linker checks :/
> 
> You said for the other comment that java WebContentsImpl is holding the java
> side, maybe you should just implement this on the java side as well?
> 
> But this is ok too

WebContentsImpl is currently package-scoped (see its declaration) so I cannot do
it for now.
I left a comment in MediaSession.fromWebContents()

https://codereview.chromium.org/2453623003/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:48:
base::android::ScopedJavaGlobalRef<jobject> j_media_session_;
On 2016/10/31 22:44:02, boliu wrote:
> On 2016/10/31 22:22:26, Zhiqiang Zhang wrote:
> > On 2016/10/31 20:13:19, boliu wrote:
> > > On 2016/10/31 19:50:03, Zhiqiang Zhang wrote:
> > > > On 2016/10/31 18:58:55, boliu wrote:
> > > > > On 2016/10/31 11:57:34, Zhiqiang Zhang wrote:
> > > > > > I don't know about how GC on WebContentsImpl.java works, but I can
> only
> > > > > explain
> > > > > > the link chain for MediaSession and let you decide. The link chain
is:
> > > > > > WebContentsImpl(native) -> MediaSession(native) ->
> > > > MediaSessionAndroid(native)
> > > > > > -> MediaSessionImpl.Java
> > > > > > When WebContentsImpl(native) is destroyed, all the native objects on
> the
> > > > chain
> > > > > > will be destroyed.
> > > > > > 
> > > > > > The Java user (non-WebView) will has the following chain:
> > > > > > Tab.java<->MediaSessionTabHelper.java
> (observer)<->MediaSessionImpl.java
> > > > > > Also I think the link MediaSessionTabHelper->Tab can be changed to
> > > > > > WeakReference.
> > > > > > 
> > > > > > Is there a possible leak on this?
> > > > > 
> > > > > Yes in webview, where native WebContentsImpl destruction relies on gc,
> so
> > a
> > > > > strong reference from native (ie a gc root) can't transitively
reference
> > > > > WebContents and prevent it from garbage collected.
> > > > > 
> > > > > > Or maybe we should make
> > > > > > MediaSessionTabHelper->MediaSessionImpl weak if yes.
> > > > > 
> > > > > Be very careful that *something* is keeping the MediaSessionImpl alive
> on
> > > the
> > > > > java side (java references are generally not gc roots, other than
static
> > and
> > > > > local references) while the WebContents is alive. Is there something
> like
> > > > that?
> > > > > 
> > > > > > 
> > > > > > I think "a Java user referencing MediaSessionImpl.java" is safer
than
> "a
> > > > Java
> > > > > > user referencing WebContentsImpl.java". If the user does't reference
> too
> > > > high
> > > > > on
> > > > > > the reference chain, it should be OK.
> > > > > 
> > > > 
> > > > I think the problem lies in the client side.
> > > 
> > > Meaning outside of content?
> > 
> > Yes.
> > 
> > > > 
> > > > If we use weak reference here, then we still need to find some object to
> > hold
> > > a
> > > > reference of the MediaSessionImpl.java, otherwise it will be garbage
> > collected
> > > > when we still want to use it.
> > > 
> > > Yes exactly.
> > > 
> > > > WebContentsImpl.java could possibly hold the
> > > > MediaSesssionImpl.java reference, but that still cannot prevent having a
> > > > reference cycle if the observer prevents WebContents being gc'ed.
> > > 
> > > I think you might be misunderstanding how garbage collection works.
> > > 
> > > Reference cycles are fine. gc is able to figure out the whole cycle is no
> > longer
> > > reachable from a gc root, and then collect them all in one go (this of
> course
> > is
> > > different from ref counting)
> > > 
> > > It's also ok for java
WebContentsImpl/MediaSesssionImpl/MediaSessionObserver
> > to
> > > have strong references between each other without causing gc issues, since
> > none
> > > of those references are gc roots.
> > > 
> > > If you are worried client introducing *another* gc root in client code,
then
> > > that's always a possibility. But at least the problem is not with this
code.
> > > 
> > > > 
> > > > So I think if the client is causing a leak, it doesn't matter we use a
> > > > strong/weak reference here.
> > > 
> > So IIUC WebContentsAndroid is holding a gc root of WebContentsImpl.java
which
> it
> > should not, right?
> 
> That's a good point. Yes it will still be referenced by WebContentsImpl's
> existing gc root. But overall, the point here is don't introduce *another* gc
> root.
> 
> > 
> > Now I should go with having weak reference here and let WebContentsImpl.java
> > hold the reference.
> 

Acknowledged.

https://codereview.chromium.org/2453623003/diff/180001/content/browser/media/...
File content/browser/media/session/media_session_android.cc (right):

https://codereview.chromium.org/2453623003/diff/180001/content/browser/media/...
content/browser/media/session/media_session_android.cc:80: if
(j_media_session_.is_empty())
On 2016/10/31 22:44:02, boliu wrote:
> in theory, this check is not safe, there is no guarantee gc is single
threaded,
> and it should be possible for gc to run after this check, in which case you'll
> get a null pointer. (in practice, I'm not entirely sure if art actually does
> that, but better be safe than sorry..)
> 
> you need to make it into a ScopedJavaLocalRef, then check for null on the
local
> ref

Yes, you mentioned this before and I lost it after several refactoring. They are
fixed now.

[Zhiqiang Zhang (Slow), 2016-10-31 23:30:33]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 23:31:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 00:57:37]

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

[commit-bot: I haz the power, 2016-11-01 00:57:39]

Dry run: This issue passed the CQ dry run.

[boliu, 2016-11-01 03:29:27]

lgtm

[Zhiqiang Zhang (Slow), 2016-11-01 10:37:32]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-01 10:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
halliwell@chromium.org, avayvod@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2453623003/#ps220001
(title: "nit")

[commit-bot: I haz the power, 2016-11-01 10:37:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 11:27:15]

Description was changed from

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678
==========

to

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678
==========

[commit-bot: I haz the power, 2016-11-01 11:27:17]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-11-01 11:29:37]

Description was changed from

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678
==========

to

==========
Decouple MediaSession messages from WebContents

This CL separates MediaSession messages from WebContents. It includes
the following changes:

* Abstract MediaSession interface and put it into
  content/public/browser.
  * Move WebContents.[Resume|Suspend|Stop]MediaSession to the
    MediaSession interface.
* Add MediaSessionObserver to content/public/browser.
  * Move WebContentsObserver.MediaSession* to the MediaSessionObserver
    interface.
* Add MediaSession content/public API in Java.
  * Move MediaSession* in the Java WebContent API to MediaSession.
* Let clients use the new MediaSession content API.

The overall structral change is described in:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

BUG=658678

Committed: https://crrev.com/1adf3cb5c81b84968f4e541edc97f6cd6f2a96b4
Cr-Commit-Position: refs/heads/master@{#428976}
==========

[commit-bot: I haz the power, 2016-11-01 11:29:39]

Patchset 11 (id:??) landed as
https://crrev.com/1adf3cb5c81b84968f4e541edc97f6cd6f2a96b4
Cr-Commit-Position: refs/heads/master@{#428976}