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

Link MediaSessionTabHelper with native MediaSession

This CL links MediaSessionTabHelper with the native MediaSession to
avoid plumbing MediaSession messages through WebContents and
WebContentsObserver. This will greatly reduce the effort of making
changes on the interface that MediaSession and MediaSessionTabHelper
communicate.

Explaining doc:
https://docs.google.com/a/google.com/document/d/1duLlKQ6Y8ogC9VUbDvdTz8-Rn45Mrove0uD5j089Le8/edit?usp=sharing

BUG=654891

[Zhiqiang Zhang (Slow), 2016-10-19 19:07:16]

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

[Zhiqiang Zhang (Slow), 2016-10-19 19:07:17]

PTAL

This is the initial CL, which is partially working.
The CL is still rough, but I need some general feedback.
So please don't go too much into details.

boliu@, please look at the changes on the Java side and see if it looks good in
general.

[boliu, 2016-10-19 20:39:40]

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:250:
std::map<MediaSessionObserver*, std::unique_ptr<MediaSessionObserver>>;
would scoped_vector be better? is there going to be that many observers in
practice that a hashed data structure is needed?

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.cc:40:
session->PassObserverOwnership(
do this in CreateAndRegister, there is no bare pointer needed

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.h:42:
base::android::ScopedJavaGlobalRef<jobject> j_delegate_;
this is content, so need to be safe for android webview as well, where strong
references from native to java *may* lead to memory leaks because webview relies
on java gc to destroy native side. I know this doesn't run on webview same
argument goes in case someone does start using this in webview in the future.

so in this case, native "owns" the java object here. is there a guarantee when
exactly the java side is released, from inside content code? if not, maybe we
should use a weak ref and add null checks instead

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:16:
public boolean hasNativeMediaSession() {
private?

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:21:
if (hasNativeMediaSession())
nit: need braces

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:41:
public void resumeMediaSession() {}
are these supposed to call into native?

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

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:309:
void addMediaSessionDelegate(MediaSessionDelegate delegate);
nit: s/add/set/ ?

[whywhat, 2016-10-20 15:38:33]

I haven't wrapped my head around the ownership scheme here completely yet, might
come up with some suggestions on how to simplify it.

https://codereview.chromium.org/2439483003/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/2439483003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:45:
private WebContents mWebContents;
this seems to be unused, I guess destroy() is handled by MediaSessionDelegate
now?

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:79: void
PassObserverOwnership(std::unique_ptr<MediaSessionObserver> owned_observer);
this seems like an antipattern, along with the map below...

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_observer.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.cc:13: Observe(session);
don't do that in the ctor - if we ever call any virtual method of MSO from the
same callstack, it won't get handled.
google style guide forbids doing any non-trivial work in ctors for this reason

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.cc:13: Observe(session);
don't do that in the ctor - if we ever call any virtual method of MSO from the
same callstack, it won't get handled.
google style guide forbids doing any non-trivial work in ctors for this reason
you can make the ctor private and have a create function

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.h:19: virtual
~MediaSessionObserver() = default;
if you "unobserve" in dtor, you can probably remove the ownership of observers
in MediaSession.

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_android.cc:622:
MediaSessionDelegateAndroid::CreateAndRegister(
perhaps the delegates could be owned by WebContents?

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

Patchset #2 (id:20001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-20 16:22:32]

PTAL

Addressed & replied comments from Anton and boliu@.
Cleaned up the ownership and inter-reference issue. Added comments in the code.

Now the patch is working and passed my manual test :)
Hope it don't break too many tests on the bots.

https://codereview.chromium.org/2439483003/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/2439483003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:45:
private WebContents mWebContents;
On 2016/10/20 15:38:33, whywhat wrote:
> this seems to be unused, I guess destroy() is handled by MediaSessionDelegate
> now?

destroy() is now MediaSessionDelegate.mediaSessionDisconnected().

It's not fully unused now. We still need mWebContents in setWebContents().
Especially for checking `mWebContents == webContents`. Otherwise, we might end
up destroy the current delegate and create another for the same WebContents.

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:79: void
PassObserverOwnership(std::unique_ptr<MediaSessionObserver> owned_observer);
On 2016/10/20 15:38:33, whywhat wrote:
> this seems like an antipattern, along with the map below...

It's a problem that I've been struggling with. I discussed with Mounir and he's
fine to have MSO that are owned and not owned by MediaSession.

As you said in the other comment, maybe we could make MSO be owned by
WebContents, but actually it's the same as owned by MediaSession, since MS and
WC have the same lifetime. However there might be some other cases we don't want
it be owned by WC or MS, such as in tests, or the observer (CastMediaBlocker) in
chromecast/

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:250:
std::map<MediaSessionObserver*, std::unique_ptr<MediaSessionObserver>>;
On 2016/10/19 20:39:40, boliu wrote:
> would scoped_vector be better? is there going to be that many observers in
> practice that a hashed data structure is needed?

The map is used when removing observers from the session. We don't expect many
observers, so I think using std::map is not very expensive.

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.cc:40:
session->PassObserverOwnership(
On 2016/10/19 20:39:40, boliu wrote:
> do this in CreateAndRegister, there is no bare pointer needed

Done.

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.h:42:
base::android::ScopedJavaGlobalRef<jobject> j_delegate_;
On 2016/10/19 20:39:40, boliu wrote:
> this is content, so need to be safe for android webview as well, where strong
> references from native to java *may* lead to memory leaks because webview
relies
> on java gc to destroy native side. I know this doesn't run on webview same
> argument goes in case someone does start using this in webview in the future.
> 
> so in this case, native "owns" the java object here. is there a guarantee when
> exactly the java side is released, from inside content code? if not, maybe we
> should use a weak ref and add null checks instead

You are right. For our case, we don't need strong reference from native to Java.
If the the reference count in Java is 0, it should be GCed.

Using JavaObjectWeakGlobalRef now.

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_observer.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.cc:13: Observe(session);
On 2016/10/20 15:38:33, whywhat wrote:
> don't do that in the ctor - if we ever call any virtual method of MSO from the
> same callstack, it won't get handled.
> google style guide forbids doing any non-trivial work in ctors for this reason
> you can make the ctor private and have a create function

This pattern was used in WebContentsObserver. Actually Observe() is a private
non-virtual method so it should be fine to call it in the ctor. Since the ctor
has an argument, it must be called by subclass ctors, so all MSO will add itself
to MS in the ctor. Do you have strong opinions on this pattern?

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.h:19: virtual
~MediaSessionObserver() = default;
On 2016/10/20 15:38:33, whywhat wrote:
> if you "unobserve" in dtor, you can probably remove the ownership of observers
> in MediaSession.

Yes. It is now in the new patch.

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_android.cc:622:
MediaSessionDelegateAndroid::CreateAndRegister(
On 2016/10/20 15:38:33, whywhat wrote:
> perhaps the delegates could be owned by WebContents?

I prefer not to pollute WebContents. Actually MediaSession has the same lifetime
as WebContents.

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:16:
public boolean hasNativeMediaSession() {
On 2016/10/19 20:39:40, boliu wrote:
> private?

Done.

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:21:
if (hasNativeMediaSession())
On 2016/10/19 20:39:40, boliu wrote:
> nit: need braces

Done.

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:41:
public void resumeMediaSession() {}
On 2016/10/19 20:39:40, boliu wrote:
> are these supposed to call into native?

Yes. Added javadoc.

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

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:309:
void addMediaSessionDelegate(MediaSessionDelegate delegate);
On 2016/10/19 20:39:40, boliu wrote:
> nit: s/add/set/ ?

Not really, we allow adding multiple MediaSessionDelegates.

[boliu, 2016-10-20 16:28:33]

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:250:
std::map<MediaSessionObserver*, std::unique_ptr<MediaSessionObserver>>;
On 2016/10/20 16:22:32, Zhiqiang Zhang wrote:
> On 2016/10/19 20:39:40, boliu wrote:
> > would scoped_vector be better? is there going to be that many observers in
> > practice that a hashed data structure is needed?
> 
> The map is used when removing observers from the session. We don't expect many
> observers, so I think using std::map is not very expensive.

Hmm? Usually the argument goes that vectors are slower for random access, but is
ok if N is tiny, where vector's memory efficiency wins out. I've never seen
anyone argue hash map is slow before :p

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

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:309:
void addMediaSessionDelegate(MediaSessionDelegate delegate);
On 2016/10/20 16:22:32, Zhiqiang Zhang wrote:
> On 2016/10/19 20:39:40, boliu wrote:
> > nit: s/add/set/ ?
> 
> Not really, we allow adding multiple MediaSessionDelegates.

Usually you can have only one "delegate", but multiple "observers". Are these
supposed to be observers?

[Zhiqiang Zhang (Slow), 2016-10-20 17:07:03]

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session.h:250:
std::map<MediaSessionObserver*, std::unique_ptr<MediaSessionObserver>>;
On 2016/10/20 16:28:33, boliu wrote:
> On 2016/10/20 16:22:32, Zhiqiang Zhang wrote:
> > On 2016/10/19 20:39:40, boliu wrote:
> > > would scoped_vector be better? is there going to be that many observers in
> > > practice that a hashed data structure is needed?
> > 
> > The map is used when removing observers from the session. We don't expect
many
> > observers, so I think using std::map is not very expensive.
> 
> Hmm? Usually the argument goes that vectors are slower for random access, but
is
> ok if N is tiny, where vector's memory efficiency wins out. I've never seen
> anyone argue hash map is slow before :p

You are right, actually base::ObserverList uses std::vector<>.
Done.

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

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:309:
void addMediaSessionDelegate(MediaSessionDelegate delegate);
On 2016/10/20 16:28:33, boliu wrote:
> On 2016/10/20 16:22:32, Zhiqiang Zhang wrote:
> > On 2016/10/19 20:39:40, boliu wrote:
> > > nit: s/add/set/ ?
> > 
> > Not really, we allow adding multiple MediaSessionDelegates.
> 
> Usually you can have only one "delegate", but multiple "observers". Are these
> supposed to be observers?

Actually "delegate" inherits from "observer", a delegate can both observe
MediaSession changes and perform actions on MediaSession. Ideally this could be
solved by adding a Java counterpart of MediaSession, but we don't have plans.

We haven't come up with a better name yet. Do you have any suggestion? Or Anton
has some ideas?

[boliu, 2016-10-20 17:10:19]

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

https://codereview.chromium.org/2439483003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:309:
void addMediaSessionDelegate(MediaSessionDelegate delegate);
On 2016/10/20 17:07:03, Zhiqiang Zhang wrote:
> On 2016/10/20 16:28:33, boliu wrote:
> > On 2016/10/20 16:22:32, Zhiqiang Zhang wrote:
> > > On 2016/10/19 20:39:40, boliu wrote:
> > > > nit: s/add/set/ ?
> > > 
> > > Not really, we allow adding multiple MediaSessionDelegates.
> > 
> > Usually you can have only one "delegate", but multiple "observers". Are
these
> > supposed to be observers?
> 
> Actually "delegate" inherits from "observer", a delegate can both observe
> MediaSession changes and perform actions on MediaSession.

Feels like that fact should live in the embedder not content.

Like WebContents has a Delegate and a bunch of Observers. It doesn't care if the
embedder's Delegate also implements the Observer. (Although embedder have to be
careful of ownership.(

On the other hand, does this really need to be that complicated? I'd suggest
trying to simplify if possible, without actually knowing any of this code

> Ideally this could be
> solved by adding a Java counterpart of MediaSession, but we don't have plans.
> 
> We haven't come up with a better name yet. Do you have any suggestion? Or
Anton
> has some ideas?

[Zhiqiang Zhang (Slow), 2016-10-20 17:55:06]

On 2016/10/20 17:10:19, boliu wrote:
> Feels like that fact should live in the embedder not content.
> 
> Like WebContents has a Delegate and a bunch of Observers. It doesn't care if
the
> embedder's Delegate also implements the Observer. (Although embedder have to
be
> careful of ownership.(
> 
> On the other hand, does this really need to be that complicated? I'd suggest
> trying to simplify if possible, without actually knowing any of this code

I understand. So actually the embedder of MediaSessionObserver is Java
MediaSessionTabHelper, which does not has a native counterpart.
So I'm thinking of having a "manager" for observers that have been piped through
JNI, but it seems that the manager can be merged into MediaSession, so I'm
having "MediaSession-owned observers".
Do you think I need to split it out? or Do you have better ideas for the
ownership?

[whywhat, 2016-10-20 18:18:50]

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_observer.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_observer.cc:13: Observe(session);
On 2016/10/20 at 16:22:32, Zhiqiang Zhang wrote:
> On 2016/10/20 15:38:33, whywhat wrote:
> > don't do that in the ctor - if we ever call any virtual method of MSO from
the
> > same callstack, it won't get handled.
> > google style guide forbids doing any non-trivial work in ctors for this
reason
> > you can make the ctor private and have a create function
> 
> This pattern was used in WebContentsObserver. Actually Observe() is a private
non-virtual method so it should be fine to call it in the ctor. Since the ctor
has an argument, it must be called by subclass ctors, so all MSO will add itself
to MS in the ctor. Do you have strong opinions on this pattern?

The problem is if Observe results in an event being sent back to the observer
immediately (on the same stack). That will call one of the MSO virtual methods
but the derived class won't be initialized yet.

It's probably hard to fix WebContentsObserver but with MediaRouter & Cast I had
a couple of issues where the event wouldn't fire because of that. Debugging such
kind of things is painful so I'd rather prevent it.

[whywhat, 2016-10-20 18:21:16]

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_android.cc:622:
MediaSessionDelegateAndroid::CreateAndRegister(
On 2016/10/20 at 16:22:32, Zhiqiang Zhang wrote:
> On 2016/10/20 15:38:33, whywhat wrote:
> > perhaps the delegates could be owned by WebContents?
> 
> I prefer not to pollute WebContents. Actually MediaSession has the same
lifetime as WebContents.

Can we just own MSO in MediaSession and avoid using the observer pattern at all
then? You just register MSO with MS, it owns it and calls its methods. We don't
have cases of MSOs that are not owned by MS or that are owned but don't want to
observe the events right? If we do, I'll have to think more, perhaps :)

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

On 2016/10/20 18:21:16, whywhat wrote:
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> File content/browser/web_contents/web_contents_android.cc (right):
> 
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> content/browser/web_contents/web_contents_android.cc:622:
> MediaSessionDelegateAndroid::CreateAndRegister(
> On 2016/10/20 at 16:22:32, Zhiqiang Zhang wrote:
> > On 2016/10/20 15:38:33, whywhat wrote:
> > > perhaps the delegates could be owned by WebContents?
> > 
> > I prefer not to pollute WebContents. Actually MediaSession has the same
> lifetime as WebContents.
> 
> Can we just own MSO in MediaSession and avoid using the observer pattern at
all
> then? You just register MSO with MS, it owns it and calls its methods. We
don't
> have cases of MSOs that are not owned by MS or that are owned but don't want
to
> observe the events right? If we do, I'll have to think more, perhaps :)

Seems we have :/ See CastMediaBlocker in chromecast/. I only see they own the
observer in a test class, but didn't see the production code (I assume it's in a
separate codebase).

[Zhiqiang Zhang (Slow), 2016-10-20 19:50:41]

On 2016/10/20 18:28:57, Zhiqiang Zhang wrote:
> On 2016/10/20 18:21:16, whywhat wrote:
> >
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> > File content/browser/web_contents/web_contents_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> > content/browser/web_contents/web_contents_android.cc:622:
> > MediaSessionDelegateAndroid::CreateAndRegister(
> > On 2016/10/20 at 16:22:32, Zhiqiang Zhang wrote:
> > > On 2016/10/20 15:38:33, whywhat wrote:
> > > > perhaps the delegates could be owned by WebContents?
> > > 
> > > I prefer not to pollute WebContents. Actually MediaSession has the same
> > lifetime as WebContents.
> > 
> > Can we just own MSO in MediaSession and avoid using the observer pattern at
> all
> > then? You just register MSO with MS, it owns it and calls its methods. We
> don't
> > have cases of MSOs that are not owned by MS or that are owned but don't want
> to
> > observe the events right? If we do, I'll have to think more, perhaps :)
> 
> Seems we have :/ See CastMediaBlocker in chromecast/. I only see they own the
> observer in a test class, but didn't see the production code (I assume it's in
a
> separate codebase).

BTW, I agree it does not make much sense to keep an observer that does not
observe MediaSession.
So maybe we could let observers be owned by MediaSession. Then the chromecast/
guys need to fix on their side. WDYT?

[whywhat, 2016-10-20 21:00:05]

On 2016/10/20 at 19:50:41, zqzhang wrote:
> On 2016/10/20 18:28:57, Zhiqiang Zhang wrote:
> > On 2016/10/20 18:21:16, whywhat wrote:
> > >
> >
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> > > File content/browser/web_contents/web_contents_android.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2439483003/diff/1/content/browser/web_content...
> > > content/browser/web_contents/web_contents_android.cc:622:
> > > MediaSessionDelegateAndroid::CreateAndRegister(
> > > On 2016/10/20 at 16:22:32, Zhiqiang Zhang wrote:
> > > > On 2016/10/20 15:38:33, whywhat wrote:
> > > > > perhaps the delegates could be owned by WebContents?
> > > > 
> > > > I prefer not to pollute WebContents. Actually MediaSession has the same
> > > lifetime as WebContents.
> > > 
> > > Can we just own MSO in MediaSession and avoid using the observer pattern
at
> > all
> > > then? You just register MSO with MS, it owns it and calls its methods. We
> > don't
> > > have cases of MSOs that are not owned by MS or that are owned but don't
want
> > to
> > > observe the events right? If we do, I'll have to think more, perhaps :)
> > 
> > Seems we have :/ See CastMediaBlocker in chromecast/. I only see they own
the
> > observer in a test class, but didn't see the production code (I assume it's
in a
> > separate codebase).
> 
> BTW, I agree it does not make much sense to keep an observer that does not
observe MediaSession.
> So maybe we could let observers be owned by MediaSession. Then the chromecast/
guys need to fix on their side. WDYT?

+derekjchow for that question

[whywhat, 2016-10-20 22:56:54]

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

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:61:
private static HashMap<Tab, MediaSessionTabHelper> sTabToHelperMap =
having an extra method used just for testing is fine-ish, having a static map
that presumably keeps Tabs alive because it holds references to it, is a whole
different thing.

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:168:
webContents.addMediaSessionDelegate(delegate);
nit: why "add" can WC have more than one delegate?

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:173:
if (mWebContents == webContents) return;
seems like we don't need mWebContents here, we could just have setWebContents()
on MediaSessionDelegate/MediaImageManager check that.

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:178:
if (mMediaSessionDelegate != null)
mMediaSessionDelegate.disconnectMediaSession();
this will always be null after cleanupWebContents.

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.cc:43:
j_delegate_.reset();
Don't think you need to do this in dtor.

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.h (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.h:16: //
MediaSession. This method is the native counterpart of Java
s/method/class

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.h:17: //
MediaSessionDelegate. A native object its linked Java object must notify each
s/A native object its linked/The native and the/

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:21:
public class MediaSessionDelegate {
Do you expect other derived classes from this one?
Perhaps merge subclass from MSTH here (and move it to this separate file)?

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:62:
if (hasNativeMediaSession())
nativeResumeMediaSession(mNativeMediaSessionDelegateAndroid);
I think if you set mMediaSessionDelegate to null in mediaSessionDisconnected()
you don't need to have these checks for hasNativeMediaSession.

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:85:
private void mediaSessionDisconnectedWrapper() {
why wrapper?

Perhaps you could've:

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

@CalledByNative
private void onNativeDelegateDestroyed();

[whywhat, 2016-10-20 23:05:51]

I wonder if we could've:

MediaSessionObserver + MediaSessionObserverProxy/MediaSessionObserverAndroid
like WebContentsObserver - MSTH becomes MediaSessionObserver instead of
WebContentsObserver.

MediaSession.java + a way to get to it from WebContents.java - MSTH keeps
MediaSession it gets from WC in setWebContents and uses that to call
play/pause/etc.

I think we're fine for WebView since MSTH is only created for tabs and that's
what starts the whole machinery of Java and native classes being created.

+tedchoc on the architectural thoughts - we're trying extract
WebContentsObserver/WebContents methods we plumbed for MediaSession in
https://codereview.chromium.org/1159113006 since we'll need more plumbing and
don't want to expand WC more. WDYT is the best and safest approach here? Any
examples?

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

PTAL

Addressed Anton's comment.

I see Anton's point is "MediaSessionDelegate" is not great. It's a hybrid of
MediaSessionObserver and a MediaSession "controller".

I think this layout is enough for our needs and is not that bad. Here's what I
thought for the points we've been arguing about:

#1 MediaSessionDelegate or MediaSessionObserver+MediaSession Android (as Anton
suggested)?

I think we don't have strong needs for accessing the native MediaSession from
Android now. We just need some simple controls on the MediaSession so I think
it's fine to put them into the delegate. Also, if we have a MediaSession
counterpart in Android, the name will clash with the MediaSession in Android OS
(then we need another name).


#2 MediaSessionObserver ownership

I think it's fine to allow MediaSessionObserver either be owned by MediaSession
or by other embedders. The reason is that for the case of JNI, there's no better
embedders than MediaSession.

I've seem WebContentsObserver being owned by WebContentsImpl or other embedders,
so it should be a strong anti-pattern.


#3 One or many MediaSessionDelegate(s)

I think it doesn't matter much. I think the design should be simpler if we allow
multiple MediaSessionDelegates. Otherwise we need to manage the one-to-many, or
resetting logic somewhere. I think for WebContentsObserver, there's a WCOProxy
for piping through JNI, but I suspose that's for reducing the cost of passing
structures through JNI for all observers. For our case, since there's only one
delegate on the Java side at the moment, we don't need to care about avoid the
JNI cost too much.

I think we should land this CL if the general layout sounds good. There's no
right-or-wrong on some issues so we have a plenty of space for improvement, but
I prefer to move the discussion later so we don't block the progress for media
controls for too long.

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

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:61:
private static HashMap<Tab, MediaSessionTabHelper> sTabToHelperMap =
On 2016/10/20 22:56:53, whywhat wrote:
> having an extra method used just for testing is fine-ish, having a static map
> that presumably keeps Tabs alive because it holds references to it, is a whole
> different thing.

Ahh, using SparseArray<> now, with TabId as key.

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:168:
webContents.addMediaSessionDelegate(delegate);
On 2016/10/20 22:56:53, whywhat wrote:
> nit: why "add" can WC have more than one delegate?

Currently not. Using "set" might make content::MediaSessionDelegateAndroid
complex since it must handle the "reset" case then.

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:173:
if (mWebContents == webContents) return;
On 2016/10/20 22:56:53, whywhat wrote:
> seems like we don't need mWebContents here, we could just have
setWebContents()
> on MediaSessionDelegate/MediaImageManager check that.

Maybe we can do it in a separate CL?

https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:178:
if (mMediaSessionDelegate != null)
mMediaSessionDelegate.disconnectMediaSession();
On 2016/10/20 22:56:53, whywhat wrote:
> this will always be null after cleanupWebContents.

Done.

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.cc:43:
j_delegate_.reset();
On 2016/10/20 22:56:53, whywhat wrote:
> Don't think you need to do this in dtor.

Done.

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.h (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.h:16: //
MediaSession. This method is the native counterpart of Java
On 2016/10/20 22:56:53, whywhat wrote:
> s/method/class

Done.

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.h:17: //
MediaSessionDelegate. A native object its linked Java object must notify each
On 2016/10/20 22:56:53, whywhat wrote:
> s/A native object its linked/The native and the/

Done.

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:21:
public class MediaSessionDelegate {
On 2016/10/20 22:56:53, whywhat wrote:
> Do you expect other derived classes from this one?
> Perhaps merge subclass from MSTH here (and move it to this separate file)?

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

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:62:
if (hasNativeMediaSession())
nativeResumeMediaSession(mNativeMediaSessionDelegateAndroid);
On 2016/10/20 22:56:53, whywhat wrote:
> I think if you set mMediaSessionDelegate to null in mediaSessionDisconnected()
> you don't need to have these checks for hasNativeMediaSession.

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

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:85:
private void mediaSessionDisconnectedWrapper() {
On 2016/10/20 22:56:53, whywhat wrote:
> why wrapper?
> 
> Perhaps you could've:
> 
> @CalledByNative
> private void onNativeDelegateCreated(long nativeMediaSessionDelegateAndroid);
//
> this'd better be a ctor, I think
> 
> @CalledByNative
> private void onNativeDelegateDestroyed();

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

[boliu, 2016-10-21 14:37:15]

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.cc:50: if
(!j_delegate_.is_empty()) {
this in theory is not safe because gc could run between this check and when you
dereference again in the line below (in practice, dunno what dalvik/art does)

safe way is to dereference first into ScopedJavaLocalRef, then null check the
local ref

[Zhiqiang Zhang (Slow), 2016-10-21 15:05:23]

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.cc:50: if
(!j_delegate_.is_empty()) {
On 2016/10/21 14:37:15, boliu wrote:
> this in theory is not safe because gc could run between this check and when
you
> dereference again in the line below (in practice, dunno what dalvik/art does)
> 
> safe way is to dereference first into ScopedJavaLocalRef, then null check the
> local ref

Done.

[whywhat, 2016-10-21 15:46:05]

On 2016/10/21 at 13:43:53, zqzhang wrote:
> PTAL
> 
> Addressed Anton's comment.
> 
> I see Anton's point is "MediaSessionDelegate" is not great. It's a hybrid of
MediaSessionObserver and a MediaSession "controller".
> 
> I think this layout is enough for our needs and is not that bad. Here's what I
thought for the points we've been arguing about:
> 
> #1 MediaSessionDelegate or MediaSessionObserver+MediaSession Android (as Anton
suggested)?
> 
> I think we don't have strong needs for accessing the native MediaSession from
Android now. We just need some simple controls on the MediaSession so I think
it's fine to put them into the delegate. Also, if we have a MediaSession
counterpart in Android, the name will clash with the MediaSession in Android OS
(then we need another name).

We have packages in Java for that. Worst case we could call it
ChromeMediaSession to avoid confusion.
The only purpose of MediaSessionDelegate is to control MediaSession - it passes
the calls from Java via JNI to MediaSession. Having MediaSession.java seems to
remove a layer of the extra class. You don't have to expose the whole native
MediaSession class in Java, just the methods that will allow to control it.

> 
> 
> #2 MediaSessionObserver ownership
> 
> I think it's fine to allow MediaSessionObserver either be owned by
MediaSession or by other embedders. The reason is that for the case of JNI,
there's no better embedders than MediaSession.

This adds lots of complexity of managing ownership in a CL that is supposed to
reduce complexity by avoiding reviews for WebContentsObserver plumbing. I'm not
sure the trade-off is worth it.

> 
> I've seem WebContentsObserver being owned by WebContentsImpl or other
embedders, so it should be a strong anti-pattern.

Sorry, what do you mean by strong anti-pattern? Anti-pattern is something we
should avoid, strong anti-pattern we should avoid even more :)

WebContentsImpl keeps observers in the ObserverList which holds weak references.
It only owns MediaWebContentsObserver as a single instance (could we reuse that?
would we have more than one MediaSessionObserver here?)

> 
> 
> #3 One or many MediaSessionDelegate(s)
> 
> I think it doesn't matter much. I think the design should be simpler if we
allow multiple MediaSessionDelegates. Otherwise we need to manage the
one-to-many, or resetting logic somewhere.

Don't you still manage that somewhere else since we only have one
MediaSessionDelegate? If you have addMediaSessionDelegate, do you have
removeMediaSessionDelegate or delegates accumulate until MediaSession is
destroyed?

> I think for WebContentsObserver, there's a WCOProxy for piping through JNI,
but I suspose that's for reducing the cost of passing structures through JNI for
all observers. For our case, since there's only one delegate on the Java side at
the moment, we don't need to care about avoid the JNI cost too much.
> 
> I think we should land this CL if the general layout sounds good. There's no
right-or-wrong on some issues so we have a plenty of space for improvement, but
I prefer to move the discussion later so we don't block the progress for media
controls for too long.

Progress on the feature is not blocked on this CL, you can just plumb what you
need via WebContents/WebContentsObserver, right?

> 
>
https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
> File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):
> 
>
https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:61:
private static HashMap<Tab, MediaSessionTabHelper> sTabToHelperMap =
> On 2016/10/20 22:56:53, whywhat wrote:
> > having an extra method used just for testing is fine-ish, having a static
map
> > that presumably keeps Tabs alive because it holds references to it, is a
whole
> > different thing.
> 
> Ahh, using SparseArray<> now, with TabId as key.

Is it really that needed for tests? I'd be great if we keep test-specific code
to the tests.

> 
>
https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:168:
webContents.addMediaSessionDelegate(delegate);
> On 2016/10/20 22:56:53, whywhat wrote:
> > nit: why "add" can WC have more than one delegate?
> 
> Currently not. Using "set" might make content::MediaSessionDelegateAndroid
complex since it must handle the "reset" case then.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:173:
if (mWebContents == webContents) return;
> On 2016/10/20 22:56:53, whywhat wrote:
> > seems like we don't need mWebContents here, we could just have
setWebContents()
> > on MediaSessionDelegate/MediaImageManager check that.
> 
> Maybe we can do it in a separate CL?
> 
>
https://codereview.chromium.org/2439483003/diff/60001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:178:
if (mMediaSessionDelegate != null)
mMediaSessionDelegate.disconnectMediaSession();
> On 2016/10/20 22:56:53, whywhat wrote:
> > this will always be null after cleanupWebContents.
> 
> Done.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
> File content/browser/media/session/media_session_delegate_android.cc (right):
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
> content/browser/media/session/media_session_delegate_android.cc:43:
j_delegate_.reset();
> On 2016/10/20 22:56:53, whywhat wrote:
> > Don't think you need to do this in dtor.
> 
> Done.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
> File content/browser/media/session/media_session_delegate_android.h (right):
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
> content/browser/media/session/media_session_delegate_android.h:16: //
MediaSession. This method is the native counterpart of Java
> On 2016/10/20 22:56:53, whywhat wrote:
> > s/method/class
> 
> Done.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/browser/media/s...
> content/browser/media/session/media_session_delegate_android.h:17: //
MediaSessionDelegate. A native object its linked Java object must notify each
> On 2016/10/20 22:56:53, whywhat wrote:
> > s/A native object its linked/The native and the/
> 
> Done.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
> File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:21:
public class MediaSessionDelegate {
> On 2016/10/20 22:56:53, whywhat wrote:
> > Do you expect other derived classes from this one?
> > Perhaps merge subclass from MSTH here (and move it to this separate file)?
> 
> I prefer not to merge it, since the subclass in MSTH is an inner class and
need to access MSTH methods. Besides, having a base class like this is easier to
mock in testing.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:62:
if (hasNativeMediaSession())
nativeResumeMediaSession(mNativeMediaSessionDelegateAndroid);
> On 2016/10/20 22:56:53, whywhat wrote:
> > I think if you set mMediaSessionDelegate to null in
mediaSessionDisconnected()
> > you don't need to have these checks for hasNativeMediaSession.
> 
> Yes. Just in case the client still somehow calls these methods after
disconnected.
> 
>
https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
>
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:85:
private void mediaSessionDisconnectedWrapper() {
> On 2016/10/20 22:56:53, whywhat wrote:
> > why wrapper?
> > 
> > Perhaps you could've:
> > 
> > @CalledByNative
> > private void onNativeDelegateCreated(long
nativeMediaSessionDelegateAndroid); //
> > this'd better be a ctor, I think
> > 
> > @CalledByNative
> > private void onNativeDelegateDestroyed();
> 
> Yes, sorry I'm bad at names. Using "on-" now. However I still prefer
connected/disconnected, since it might be removing the delegate from the
MediaSession while the session is still alive.

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

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:62:
if (hasNativeMediaSession())
nativeResumeMediaSession(mNativeMediaSessionDelegateAndroid);
On 2016/10/21 at 13:43:53, Zhiqiang Zhang wrote:
> On 2016/10/20 22:56:53, whywhat wrote:
> > I think if you set mMediaSessionDelegate to null in
mediaSessionDisconnected()
> > you don't need to have these checks for hasNativeMediaSession.
> 
> Yes. Just in case the client still somehow calls these methods after
disconnected.

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

https://codereview.chromium.org/2439483003/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:85:
private void mediaSessionDisconnectedWrapper() {
On 2016/10/21 at 13:43:53, Zhiqiang Zhang wrote:
> On 2016/10/20 22:56:53, whywhat wrote:
> > why wrapper?
> > 
> > Perhaps you could've:
> > 
> > @CalledByNative
> > private void onNativeDelegateCreated(long
nativeMediaSessionDelegateAndroid); //
> > this'd better be a ctor, I think
> > 
> > @CalledByNative
> > private void onNativeDelegateDestroyed();
> 
> Yes, sorry I'm bad at names. Using "on-" now. However I still prefer
connected/disconnected, since it might be removing the delegate from the
MediaSession while the session is still alive.

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

[whywhat, 2016-10-21 18:28:15]

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session.cc (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session.cc:61: owned_observers_.clear();
I think MediaSession should notify MediaSessionObservers it's about to be
destroyed so they could set their session_ to nullptr.
It's not MediaSessionDelegate's responsibility.

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session.cc:74: return;
this return belongs to the if above

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session.h (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session.h:85: void
PassObserverOwnership(std::unique_ptr<MediaSessionObserver> observer);
what I don't like about this is that it's only used for the MediaSessionDelegate
to be owned by MediaSession. So why does it have to do with observers? What if
we keep the delegate in WebContentsAndroid?

WebContentsAndroid would create the native delegate for the WebContents's
MediaSession in its ctor (which would create the Java object in its ctor, a
common pattern) and pass its GetJavaObject to the WebContentsImpl.java like
NavigationController. 

Then in setWebContents in MediaSessionTabHelper you would just set mDelegate =
webContents.getMediaSessionDelegate() and mDelegate.setObserver(this); // or new
MediaSessionObserver() { ... };

I think that would make the codepath for creating MediaSessionDelegate and its
ownership much clearer (maybe it should be called MediaSessionProxy or
something).

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_delegate_android.cc (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_delegate_android.cc:32:
Java_MediaSessionDelegate_onMediaSessionConnected(
I think "connect/disconnect" terms are misused and overloaded here. IIUC, this
is about notifying the Java and native parts about being created/destroyed so
they clean up the corresponding referenced.

These might go away with my comment above.

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session_observer.cc (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session_observer.cc:13: Observe(session);
I don't feel strongly about removing this, TBH. Just be aware that it may result
in extra code like this:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

(MediaSinkObserver in C++ calls Observe in ctor which calls
CastMediaRouteProvider.startObservingMediaSinks which may cause onSinksReceived
if they're known already - on desktop the sinks are returned via Mojo/IPC so the
MediaSinkObserver derived class gets fully constructed; on Android we have to
post a task...).

Perhaps when Observe doesn't have any side effects, it's fine and RAII
convenient.

https://codereview.chromium.org/2439483003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_android.cc (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_android.cc:618: void
WebContentsAndroid::AddMediaSessionDelegate(
When is the delegate going to be removed?
Do you think we could create the delegate in advance in WCA? Does it have to be
on demand?

https://codereview.chromium.org/2439483003/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java
(right):

https://codereview.chromium.org/2439483003/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/MediaSessionDelegate.java:95:
return mNativeMediaSessionDelegateAndroid != 0;
Why do you think any methods of this class can be called when this is 0? Could
we prevent that to remove the checks?

[whywhat, 2016-10-21 18:31:02]

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.h:42:
base::android::ScopedJavaGlobalRef<jobject> j_delegate_;
On 2016/10/19 at 20:39:40, boliu wrote:
> this is content, so need to be safe for android webview as well, where strong
references from native to java *may* lead to memory leaks because webview relies
on java gc to destroy native side. I know this doesn't run on webview same
argument goes in case someone does start using this in webview in the future.
> 
> so in this case, native "owns" the java object here. is there a guarantee when
exactly the java side is released, from inside content code? if not, maybe we
should use a weak ref and add null checks instead

Would a guarantee like "when WebContents is destroyed, the GlobalRef and all
Java references to the Java object will be removed so the object will be GCed"
work?

[boliu, 2016-10-21 19:56:02]

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

https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_delegate_android.h:42:
base::android::ScopedJavaGlobalRef<jobject> j_delegate_;
On 2016/10/21 18:31:02, whywhat wrote:
> On 2016/10/19 at 20:39:40, boliu wrote:
> > this is content, so need to be safe for android webview as well, where
strong
> references from native to java *may* lead to memory leaks because webview
relies
> on java gc to destroy native side. I know this doesn't run on webview same
> argument goes in case someone does start using this in webview in the future.
> > 
> > so in this case, native "owns" the java object here. is there a guarantee
when
> exactly the java side is released, from inside content code? if not, maybe we
> should use a weak ref and add null checks instead
> 
> Would a guarantee like "when WebContents is destroyed, the GlobalRef and all
> Java references to the Java object will be removed so the object will be GCed"
> work?

No, because WebContents is destroyed by gc, so if any of these java references
accidentally prevents WebContents (or that whole chain of objects) from gc-ed,
then things will leak.

The argument that works for content is that the references has to be dropped
when onDetachedFromWindow is called.

[derekjchow1, 2016-10-21 20:47:29]

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

[derekjchow1, 2016-10-21 20:47:30]

The Cast team uses the C++ content/public APIs to stop media playback.

https://cs.chromium.org/chromium/src/chromecast/browser/cast_media_blocker.h

Removing the media session functions in WebContents and WebContentsObserver
would break us. Looking at the Java implementation, ISTM a MediaSessionDelegate
class was added to replace the deleted interfaces. I think we exposed a similar
MediaSessionDelegate delegate interface to the C++ content/public WebContents,
we could easily rework CastMediaBlocker.

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
File content/browser/media/session/media_session.cc (right):

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session.cc:61: owned_observers_.clear();
On 2016/10/21 18:28:14, whywhat wrote:
> I think MediaSession should notify MediaSessionObservers it's about to be
> destroyed so they could set their session_ to nullptr.
> It's not MediaSessionDelegate's responsibility.

+1. This is the pattern used in WebContentsObserver.

https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse...

You could add a MediaSessionDestroyed function to MediaSessionObserver. This
notification would be sent out in the destructor of MediaSession.
MediaSessionDelegate can then "own itself" and delete itself when it gets that
notification. This way a PassObserverOwnership function is no longer needed.

https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
content/browser/media/session/media_session.cc:70: for (auto iter =
owned_observers_.begin(); iter != owned_observers_.end();
You could use std::find here.

[whywhat, 2016-10-21 21:05:24]

On 2016/10/21 at 19:56:02, boliu wrote:
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
> File content/browser/media/session/media_session_delegate_android.h (right):
> 
>
https://codereview.chromium.org/2439483003/diff/1/content/browser/media/sessi...
> content/browser/media/session/media_session_delegate_android.h:42:
base::android::ScopedJavaGlobalRef<jobject> j_delegate_;
> On 2016/10/21 18:31:02, whywhat wrote:
> > On 2016/10/19 at 20:39:40, boliu wrote:
> > > this is content, so need to be safe for android webview as well, where
strong
> > references from native to java *may* lead to memory leaks because webview
relies
> > on java gc to destroy native side. I know this doesn't run on webview same
> > argument goes in case someone does start using this in webview in the
future.
> > > 
> > > so in this case, native "owns" the java object here. is there a guarantee
when
> > exactly the java side is released, from inside content code? if not, maybe
we
> > should use a weak ref and add null checks instead
> > 
> > Would a guarantee like "when WebContents is destroyed, the GlobalRef and all
> > Java references to the Java object will be removed so the object will be
GCed"
> > work?
> 
> No, because WebContents is destroyed by gc, so if any of these java references
accidentally prevents WebContents (or that whole chain of objects) from gc-ed,
then things will leak.
> 
> The argument that works for content is that the references has to be dropped
when onDetachedFromWindow is called.

Ah, so the Java objects under content/ must not reference WebContents in any way
if they're kept via strong global ref by the native side.

[whywhat, 2016-10-21 21:08:19]

On 2016/10/21 at 20:47:30, derekjchow wrote:
> The Cast team uses the C++ content/public APIs to stop media playback.
> 
> https://cs.chromium.org/chromium/src/chromecast/browser/cast_media_blocker.h
> 
> Removing the media session functions in WebContents and WebContentsObserver
> would break us. Looking at the Java implementation, ISTM a
MediaSessionDelegate
> class was added to replace the deleted interfaces. I think we exposed a
similar
> MediaSessionDelegate delegate interface to the C++ content/public WebContents,
> we could easily rework CastMediaBlocker.

MediaSessionDelegate is what suspends/resumes/stops media session from the
browser controls. MediaSessionObserver is what receives the events like
MediaSessionStateChanged et al. Sounds like you'll need to make CastMediaBlocker
a MediaSessionObserver.

Zhiqiang is going to add more methods to the observer and delegate later on,
FYI.

> 
>
https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
> File content/browser/media/session/media_session.cc (right):
> 
>
https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
> content/browser/media/session/media_session.cc:61: owned_observers_.clear();
> On 2016/10/21 18:28:14, whywhat wrote:
> > I think MediaSession should notify MediaSessionObservers it's about to be
> > destroyed so they could set their session_ to nullptr.
> > It's not MediaSessionDelegate's responsibility.
> 
> +1. This is the pattern used in WebContentsObserver.
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse...
> 
> You could add a MediaSessionDestroyed function to MediaSessionObserver. This
notification would be sent out in the destructor of MediaSession.
MediaSessionDelegate can then "own itself" and delete itself when it gets that
notification. This way a PassObserverOwnership function is no longer needed.

I'd prefer the object not owning itself if it can be avoided.

> 
>
https://codereview.chromium.org/2439483003/diff/80001/content/browser/media/s...
> content/browser/media/session/media_session.cc:70: for (auto iter =
owned_observers_.begin(); iter != owned_observers_.end();
> You could use std::find here.

[Zhiqiang Zhang (Slow), 2016-10-21 21:41:41]

Patchset #5 (id:100001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-21 21:45:13]

PTAL

I finally managed to split MediaSessionDelgate into MediaSessionObserver and
ChromeMediaSession. Should be solving many existing problems now. Will update
the layout doc (as in the CL description)

Please be gentle... This patch only compiles and I haven't tested yet. Need some
general feedback.

[Zhiqiang Zhang (Slow), 2016-10-22 23:08:05]

Patchset #8 (id:180001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-22 23:08:18]

Patchset #8 (id:200001) has been deleted

[Zhiqiang Zhang (Slow), 2016-10-24 10:43:34]

Thanks for all reviewers.
I finally come with a better plan (updated the doc in the CL description).

However, this CL is getting huge and we better split it into several small CLs.
So please don't review this one at the moment.

The bug tracking this issue is https://crbug.com/658678. Go there for detailed
plans.

[Zhiqiang Zhang (Slow), 2016-10-24 12:19:50]

Description was changed from

==========
Link MediaSessionTabHelper with native MediaSession

This CL links MediaSessionTabHelper with the native MediaSession to
avoid plumbing MediaSession messages through WebContents and
WebContentsObserver. This will greatly reduce the effort of making
changes on the interface that MediaSession and MediaSessionTabHelper
communicate.

Explaining doc:
https://docs.google.com/a/google.com/document/d/1duLlKQ6Y8ogC9VUbDvdTz8-Rn45M...

BUG=654891
==========

to

==========
Link MediaSessionTabHelper with native MediaSession

This CL links MediaSessionTabHelper with the native MediaSession to
avoid plumbing MediaSession messages through WebContents and
WebContentsObserver. This will greatly reduce the effort of making
changes on the interface that MediaSession and MediaSessionTabHelper
communicate.

Explaining doc:
https://docs.google.com/a/google.com/document/d/1duLlKQ6Y8ogC9VUbDvdTz8-Rn45M...

BUG=654891
==========