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

Decouple MediaSession messages from WebContents in Java

This CL decouples MediaSession messages from WebContents in Java.
It includes the following changes:

* Add MediaSession and MediaSessionObserver in Java content_public.
* Remove all MediaSession messages from WebContents and add them to
  MediaSession/MediaSessionObserver.
* Add MediaSessionAndroid and MediaSessionImpl (Java) as a proxy between Java
  and native.
* Let MediaSessionTabHelper use the new content API.

BUG=658678

[Zhiqiang Zhang (Slow), 2016-10-26 18:56:21]

Description was changed from

==========
MediaSession/MediaSessionObserver in Java

BUG=
==========

to

==========
Decouple MediaSession messages from WebContents in Java

This CL decouples MediaSession messages from WebContents in Java.
It includes the following changes:

* Add MediaSession and MediaSessionObserver in Java content_public.
* Remove all MediaSession messages from WebContents and add them to
  MediaSession/MediaSessionObserver.
* Add MediaSessionAndroid and MediaSessionImpl (Java) as a proxy between Java
  and native.
* Let MediaSessionTabHelper use the new content API.

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

[Zhiqiang Zhang (Slow), 2016-10-26 18:59:34]

zqzhang@chromium.org changed reviewers:
+ boliu@chromium.org, mlamouri@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-26 18:59:35]

PTAL

+boliu for reviewing JNI & Java content_public
+mlamouri for reviewing media

I merged all these work into one huge CL
(https://codereview.chromium.org/2453623003/) and sent to jam@ and got approval
on the content API \o/

So the next steps is to review the smaller ones, and get rubber stamping from
jam@

[Zhiqiang Zhang (Slow), 2016-10-26 19:02:08]

+CC avayvod (to avoid spamming you too many reviews)

[mlamouri (slow - plz ping), 2016-10-28 10:24:51]

Looks great! Thanks for moving MediaSession away from WebContents :)

lgtm with comments applied.

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

https://codereview.chromium.org/2444833004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:173:
mMediaSessionObserver = createMediaSessionObserver(mediaSession);
Why not checking for `mediaSession != null`?

https://codereview.chromium.org/2444833004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:182:
mMediaSessionObserver = null;
style:
```
if (mMediaSessionObserver == null) return;
mMediaSessionObserver.getMediaSession().removeObserver(mMediaSessionObserver);
```

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

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:22: DCHECK(env);
Here and below, why do you DCHECK(env)? It's not commonly done.

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:43: DCHECK(contents);
What about early return? Reading WebContents::FromJavaWebContents, `contents`
might be null.

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:84: }
style: no need for { }

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

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:30: static bool
Register(JNIEnv* env);
style: move `static` above ctor and dtor

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaSession();
Maybe name it GetJavaObject() to match current patterns?

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:78: DCHECK(contents);
Early return?

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

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:88:
mNativeMediaSessionAndroid = 0;
Maybe do this after calling the observers?

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

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:28:
/**
style: leave empty line after `addObserver`

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:44:
/**
style: leave empty line after `resume`

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:48:
/**
style: leave empty line after `suspend`

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

Patchset #4 (id:60001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-28 11:18:49]

PTAL for boliu@

This CL depends on the native MediaSession/MediaSession observer patch. We got
approval on the overall refactoring from jam@, so we are good to go.

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

https://codereview.chromium.org/2444833004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:173:
mMediaSessionObserver = createMediaSessionObserver(mediaSession);
On 2016/10/28 10:24:50, mlamouri wrote:
> Why not checking for `mediaSession != null`?

Done.

https://codereview.chromium.org/2444833004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:182:
mMediaSessionObserver = null;
On 2016/10/28 10:24:50, mlamouri wrote:
> style:
> ```
> if (mMediaSessionObserver == null) return;
> mMediaSessionObserver.getMediaSession().removeObserver(mMediaSessionObserver);
> ```

Done.

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

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:22: DCHECK(env);
On 2016/10/28 10:24:50, mlamouri wrote:
> Here and below, why do you DCHECK(env)? It's not commonly done.

Yes, removed. I saw you used it in AudioFocusDelegate so I thought it was a
common pattern...

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:43: DCHECK(contents);
On 2016/10/28 10:24:50, mlamouri wrote:
> What about early return? Reading WebContents::FromJavaWebContents, `contents`
> might be null.

Done.

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:84: }
On 2016/10/28 10:24:50, mlamouri wrote:
> style: no need for { } 

Done.

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

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:30: static bool
Register(JNIEnv* env);
On 2016/10/28 10:24:50, mlamouri wrote:
> style: move `static` above ctor and dtor

Done.

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.h:32:
base::android::ScopedJavaLocalRef<jobject> GetJavaSession();
On 2016/10/28 10:24:50, mlamouri wrote:
> Maybe name it GetJavaObject() to match current patterns?

Done.

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2444833004/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_impl.cc:78: DCHECK(contents);
On 2016/10/28 10:24:50, mlamouri wrote:
> Early return?

Done.

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

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:88:
mNativeMediaSessionAndroid = 0;
On 2016/10/28 10:24:50, mlamouri wrote:
> Maybe do this after calling the observers?

Done.

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

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:28:
/**
On 2016/10/28 10:24:50, mlamouri wrote:
> style: leave empty line after `addObserver`

Done.

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:44:
/**
On 2016/10/28 10:24:50, mlamouri wrote:
> style: leave empty line after `resume`

Done.

https://codereview.chromium.org/2444833004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:48:
/**
On 2016/10/28 10:24:50, mlamouri wrote:
> style: leave empty line after `suspend` 

Done.

[Zhiqiang Zhang (Slow), 2016-10-28 12:57:18]

BTW, boliu@, about the question you asked before (about having only one set of
MediaSesssion content/public API in native or Java).

We are going to have more MediaSession clients for other platforms, so it should
be fine to have the API both in native and Java.
Also, MediaSessionTabHelper cannot be implemented in native, so Java plumbing
can't be avoided.

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

only looked at the public interface for now

On 2016/10/28 12:57:18, Zhiqiang Zhang wrote:
> Also, MediaSessionTabHelper cannot be implemented in native, so Java plumbing
> can't be avoided.

Sure. That doesn't mean the plumbing needs to be in content though?

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:20:
return nativeGetMediaSessionFromWebContents(contents);
I think this can call a static method on MediaSessionImpl, then this class
doesn't need to generate jni header or have native methods

anothe possibility, what if WebContents has a getMediaSession method? then this
can just be a pure interface

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSession.java:39:
public abstract ObserverList.RewindableIterator<MediaSessionObserver>
getObserversForTesting();
where will this be used?

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:32:
public final MediaSession getMediaSession() {
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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:63:
public final void stopObserving() {
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..)

[boliu, 2016-10-28 18:00:17]

On 2016/10/28 17:39:13, boliu wrote:
> only looked at the public interface for now
> 
> On 2016/10/28 12:57:18, Zhiqiang Zhang wrote:
> > Also, MediaSessionTabHelper cannot be implemented in native, so Java
plumbing
> > can't be avoided.
> 
> Sure. That doesn't mean the plumbing needs to be in content though?

perhaps that should be a question the media session owners. chrome is going to
be the only content embedder using this code right?

[whywhat, 2016-10-28 19:14:37]

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

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

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:64:
}
Seems like you could call tearDown from here (or even merge it with this
method).
You could also use mediaSessionDestroyed() to stop observing the MS.
Unless there's use for having these as separate methods I'd merge them.

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:32:
public final MediaSession getMediaSession() {
On 2016/10/28 at 17:39:13, boliu wrote:
> 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

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:63:
public final void stopObserving() {
On 2016/10/28 at 17:39:13, boliu wrote:
> 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?

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.

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

[boliu, 2016-10-28 20:29:15]

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:32:
public final MediaSession getMediaSession() {
On 2016/10/28 19:14:38, whywhat wrote:
> On 2016/10/28 at 17:39:13, boliu wrote:
> > 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
> 
> 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).

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?

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:63:
public final void stopObserving() {
On 2016/10/28 19:14:38, whywhat wrote:
> On 2016/10/28 at 17:39:13, boliu wrote:
> > 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?
> 
> 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.
> 
> > 
> > also nit: can you group the final methods together (which are really not
part
> of the public interface except for quirks in java..)
> 

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]

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:32:
public final MediaSession getMediaSession() {
On 2016/10/28 20:29:14, boliu wrote:
> On 2016/10/28 19:14:38, whywhat wrote:
> > On 2016/10/28 at 17:39:13, boliu wrote:
> > > 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
> > 
> > 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).
> 
> 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?

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

https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:63:
public final void stopObserving() {
On 2016/10/28 20:29:14, boliu wrote:
> On 2016/10/28 19:14:38, whywhat wrote:
> > On 2016/10/28 at 17:39:13, boliu wrote:
> > > 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?
> > 
> > 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.
> > 
> > > 
> > > also nit: can you group the final methods together (which are really not
> part
> > of the public interface except for quirks in java..)
> > 
> 
> Never mind about the observing multiple sessions. But regardless, why does
> stopObserving need to exist at all?

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

[boliu, 2016-10-28 20:38:36]

We don't have hard and fast rules for what java stuff should be in content api,
and what should be in an embedder (like chrome) or a component (like
web_contents_delegate_android). And looks like no one has super strong feelings
(speak up if you do!) So I'm more or less making an arbitrary decision here:
let's keep it in content for now.

[whywhat, 2016-10-29 05:19:59]

On 2016/10/28 at 20:29:15, boliu wrote:
>
https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
> File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java
(right):
> 
>
https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:32:
public final MediaSession getMediaSession() {
> On 2016/10/28 19:14:38, whywhat wrote:
> > On 2016/10/28 at 17:39:13, boliu wrote:
> > > 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
> > 
> > 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).
> 
> 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?
> 
>
https://codereview.chromium.org/2444833004/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionObserver.java:63:
public final void stopObserving() {
> On 2016/10/28 19:14:38, whywhat wrote:
> > On 2016/10/28 at 17:39:13, boliu wrote:
> > > 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?
> > 
> > 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.
> > 
> > > 
> > > also nit: can you group the final methods together (which are really not
part
> > of the public interface except for quirks in java..)
> > 
> 
> Never mind about the observing multiple sessions. But regardless, why does
stopObserving need to exist at all?

If we don't keep session in the base class, I agree one method like
MediaSessionDestroyed should suffice.

[whywhat, 2016-10-29 05:20:32]

lgtm