Add MediaSessionObserver to allow clients observe MediaSession directly

Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

The MediaSession is extracted as an interface in content/public/browser
to allow non-content code to access MediaSession.

A follow up of this CL is:
https://codereview.chromium.org/2450433002/

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_IkeHb5SJ73d5nwzxZYV8/edit?usp=sharing

BUG=658678

[Zhiqiang Zhang (Slow), 2016-10-24 14:20:29]

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

[commit-bot: I haz the power, 2016-10-24 14:20:40]

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

[Zhiqiang Zhang (Slow), 2016-10-24 14:33:53]

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

[commit-bot: I haz the power, 2016-10-24 14:34:07]

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

[Zhiqiang Zhang (Slow), 2016-10-24 15:03:55]

Description was changed from

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

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

to

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

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

[Zhiqiang Zhang (Slow), 2016-10-24 15:05:20]

Description was changed from

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

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

to

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

A follow up of this CL is:
https://codereview.chromium.org/2450433002/

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

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

[Zhiqiang Zhang (Slow), 2016-10-24 15:06:29]

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

[commit-bot: I haz the power, 2016-10-24 15:34:37]

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

[commit-bot: I haz the power, 2016-10-24 15:34:38]

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

[whywhat, 2016-10-24 20:29:39]

I think you should upgrade the description to mention MediaSession interface
extraction as this is the bigger part of your CL.

https://codereview.chromium.org/2441423002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:50: // non-exported template,
see htttps://crbug.com/589840. As a result, the class
nit: s/htttps/https

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/BUILD.gn (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/BUILD.gn:140: "media_session_observer.h",
nit: are MediaSession.h/cc here already?

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/media_session.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:17: // implementation will request audio
focus when players want to play audio, and
nit: Don't think sharing the implementation details for an interface is
necessary.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:22: // and allow clients to
resume/suspend/stop the managed players. MediaSession is
nit: s/allow/allows

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:23: // also the interface for
communicating with blink::MediaSession.
nit: same w/r/t sharing the implementation details

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session_observer.h:25: virtual void
MediaSessionDestroyed() {}
This should update session_.
IMHO, it'd be more convenient for subclasses if MSO set session_ to nullptr and
then called a virtual method that subclasses would override vs subclasses
remembering to call the base class method, but I'll leave it up to you.

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

Description was changed from

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

A follow up of this CL is:
https://codereview.chromium.org/2450433002/

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

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

to

==========
Add MediaSessionObserver to allow clients observe MediaSession directly

This CL is part of a larger effort to decouple MediaSession with
WebContents, which helps to avoid WebContents plumbing for MediaSession
messages.

The MediaSession is extracted as an interface in content/public/browser
to allow non-content code to access MediaSession.

A follow up of this CL is:
https://codereview.chromium.org/2450433002/

The greater effort is described in doc:
https://docs.google.com/a/chromium.org/document/d/1PfiCtOidmNrwsXcnpN7tCa_Ike...

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

[jam, 2016-10-25 00:30:30]

I'm having a hard time following how this simplifies thing. per
https://www.chromium.org/developers/content-module/content-api, we don't add new
content/public APIs without users. if this change had switching to the new API,
then it would be easier to understand how this simplifies things.

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

On 2016/10/25 00:30:30, jam wrote:
> I'm having a hard time following how this simplifies thing. per
> https://www.chromium.org/developers/content-module/content-api, we don't add
new
> content/public APIs without users. if this change had switching to the new
API,
> then it would be easier to understand how this simplifies things.

Sorry for the trouble. Here's the clarification.

The doc in the CL description shows the overall plan. There are two users for
the new API:
* CastMediaBlocker in chromecast/
* MediaSessionTabHelper (Java) in chrome/android/

Nowadays we have MediaSession messages everywhere in WebContents for notifying
users about MediaSession changes and allowing users to send commands to
MediaSession.
It's not great since MediaSession is not so strongly bound to WebContents. Also
as we are adding new messages/commands between MediaSession and its users,
WebContents will be more polluted.

For sending messages from MediaSession to its users, we need:
* WebContentsImpl
* WebContentsObserver
* WebContentsObserverProxy
* WebContentsObserverProxy (Java)
* WebContentsObserver (Java)

For sending commands from users to MediaSession, we need:
* WebContents (Java)
* WebContentsImpl (Java)
* WebContentsAndroid
* WebContents
* WebContentsImpl

So basically we want to separate the MediaSession messages/commands from
WebContents.
The plan is to add MediaSession/MediaSessionObserver API to
content/public/browser, just following the path of
WebContents/WebContentsObserver

After the change, for sending messages from MediaSession to its users, we need:
* MediaSessionObserver
* MediaSessionAndroid
* MediaSessionImpl (Java)
* MediaSessionObserver (Java)

For sending commands from users to MediaSession, we need:
* MediaSession (Java)
* MediaSessionImpl (Java)
* MediaSessionAndroid

This will simplify the layers between MediaSession and its users.

[Zhiqiang Zhang (Slow), 2016-10-25 10:22:24]

On 2016/10/25 10:18:37, Zhiqiang Zhang wrote:
> On 2016/10/25 00:30:30, jam wrote:
> > I'm having a hard time following how this simplifies thing. per
> > https://www.chromium.org/developers/content-module/content-api, we don't add
> new
> > content/public APIs without users. if this change had switching to the new
> API,
> > then it would be easier to understand how this simplifies things.
> 
> Sorry for the trouble. Here's the clarification.
> 
> The doc in the CL description shows the overall plan. There are two users for
> the new API:
> * CastMediaBlocker in chromecast/
> * MediaSessionTabHelper (Java) in chrome/android/
> 
> Nowadays we have MediaSession messages everywhere in WebContents for notifying
> users about MediaSession changes and allowing users to send commands to
> MediaSession.
> It's not great since MediaSession is not so strongly bound to WebContents.
Also
> as we are adding new messages/commands between MediaSession and its users,
> WebContents will be more polluted.
> 
> For sending messages from MediaSession to its users, we need:
> * WebContentsImpl
> * WebContentsObserver
> * WebContentsObserverProxy
> * WebContentsObserverProxy (Java)
> * WebContentsObserver (Java)
> 
> For sending commands from users to MediaSession, we need:
> * WebContents (Java)
> * WebContentsImpl (Java)
> * WebContentsAndroid
> * WebContents
> * WebContentsImpl
> 
> So basically we want to separate the MediaSession messages/commands from
> WebContents.
> The plan is to add MediaSession/MediaSessionObserver API to
> content/public/browser, just following the path of
> WebContents/WebContentsObserver
> 
> After the change, for sending messages from MediaSession to its users, we
need:
> * MediaSessionObserver
> * MediaSessionAndroid
> * MediaSessionImpl (Java)
> * MediaSessionObserver (Java)
> 
> For sending commands from users to MediaSession, we need:
> * MediaSession (Java)
> * MediaSessionImpl (Java)
> * MediaSessionAndroid
> 
> This will simplify the layers between MediaSession and its users.

Since the overall work is huge, I think it's better to do it in steps.

* This CL adds the new API in native content/public
* https://codereview.chromium.org/2450433002/ switches the chromecast/ user to
use the new API
* https://codereview.chromium.org/2444833004/ adds the new API in Java
content/public, removes the old API from WebContents, and switch the
chrome/android/ user to use the new API.

[Zhiqiang Zhang (Slow), 2016-10-25 19:08:35]

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

[commit-bot: I haz the power, 2016-10-25 19:09:24]

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

[whywhat, 2016-10-25 19:22:43]

lgtm % nits + the important comment about MSO handling MediaSessionDestroyed.

[Zhiqiang Zhang (Slow), 2016-10-25 19:51:43]

PTAL for jam@ (please note my previous replies for explaining this CL)

Addressed nits from avayvod@

https://codereview.chromium.org/2441423002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_session_impl.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/browser/media/s...
content/browser/media/session/media_session_impl.h:50: // non-exported template,
see htttps://crbug.com/589840. As a result, the class
On 2016/10/24 20:29:39, whywhat wrote:
> nit: s/htttps/https

Done.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/BUILD.gn (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/BUILD.gn:140: "media_session_observer.h",
On 2016/10/24 20:29:39, whywhat wrote:
> nit: are MediaSession.h/cc here already?

Done.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/media_session.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:17: // implementation will request audio
focus when players want to play audio, and
On 2016/10/24 20:29:39, whywhat wrote:
> nit: Don't think sharing the implementation details for an interface is
> necessary.

Removed.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:22: // and allow clients to
resume/suspend/stop the managed players. MediaSession is
On 2016/10/24 20:29:39, whywhat wrote:
> nit: s/allow/allows

Done.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session.h:23: // also the interface for
communicating with blink::MediaSession.
On 2016/10/24 20:29:39, whywhat wrote:
> nit: same w/r/t sharing the implementation details

Done.

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2441423002/diff/20001/content/public/browser/...
content/public/browser/media_session_observer.h:25: virtual void
MediaSessionDestroyed() {}
On 2016/10/24 20:29:39, whywhat wrote:
> This should update session_.
> IMHO, it'd be more convenient for subclasses if MSO set session_ to nullptr
and
> then called a virtual method that subclasses would override vs subclasses
> remembering to call the base class method, but I'll leave it up to you.

Maybe we should do reset after the method is called. WebContentsObserver also
has an empty body, but WebContents will do the cleanup after notifying all
observers about WebContentsDestroyed(). Will do it later.

[jam, 2016-10-25 21:34:28]

On 2016/10/25 10:22:24, Zhiqiang Zhang wrote:
> On 2016/10/25 10:18:37, Zhiqiang Zhang wrote:
> > On 2016/10/25 00:30:30, jam wrote:
> > > I'm having a hard time following how this simplifies thing. per
> > > https://www.chromium.org/developers/content-module/content-api, we don't
add
> > new
> > > content/public APIs without users. if this change had switching to the new
> > API,
> > > then it would be easier to understand how this simplifies things.
> > 
> > Sorry for the trouble. Here's the clarification.
> > 
> > The doc in the CL description shows the overall plan. There are two users
for
> > the new API:
> > * CastMediaBlocker in chromecast/
> > * MediaSessionTabHelper (Java) in chrome/android/
> > 
> > Nowadays we have MediaSession messages everywhere in WebContents for
notifying
> > users about MediaSession changes and allowing users to send commands to
> > MediaSession.
> > It's not great since MediaSession is not so strongly bound to WebContents.
> Also
> > as we are adding new messages/commands between MediaSession and its users,
> > WebContents will be more polluted.
> > 
> > For sending messages from MediaSession to its users, we need:
> > * WebContentsImpl
> > * WebContentsObserver
> > * WebContentsObserverProxy
> > * WebContentsObserverProxy (Java)
> > * WebContentsObserver (Java)
> > 
> > For sending commands from users to MediaSession, we need:
> > * WebContents (Java)
> > * WebContentsImpl (Java)
> > * WebContentsAndroid
> > * WebContents
> > * WebContentsImpl
> > 
> > So basically we want to separate the MediaSession messages/commands from
> > WebContents.
> > The plan is to add MediaSession/MediaSessionObserver API to
> > content/public/browser, just following the path of
> > WebContents/WebContentsObserver
> > 
> > After the change, for sending messages from MediaSession to its users, we
> need:
> > * MediaSessionObserver
> > * MediaSessionAndroid
> > * MediaSessionImpl (Java)
> > * MediaSessionObserver (Java)
> > 
> > For sending commands from users to MediaSession, we need:
> > * MediaSession (Java)
> > * MediaSessionImpl (Java)
> > * MediaSessionAndroid
> > 
> > This will simplify the layers between MediaSession and its users.
> 
> Since the overall work is huge, I think it's better to do it in steps.

For me to understand this change, I'm going to have to look at the three cls
together.

Since you already have all the changes, can you upload them as one cl, and then
I can review that? Then reviewing the split ones for landing in smaller cls
would get rubberstamp lgtm's based on the one big patch.

> 
> * This CL adds the new API in native content/public
> * https://codereview.chromium.org/2450433002/ switches the chromecast/ user to
> use the new API
> * https://codereview.chromium.org/2444833004/ adds the new API in Java
> content/public, removes the old API from WebContents, and switch the
> chrome/android/ user to use the new API.

[Zhiqiang Zhang (Slow), 2016-10-26 12:42:13]

On 2016/10/25 21:34:28, jam wrote:
> For me to understand this change, I'm going to have to look at the three cls
> together.
> 
> Since you already have all the changes, can you upload them as one cl, and
then
> I can review that? Then reviewing the split ones for landing in smaller cls
> would get rubberstamp lgtm's based on the one big patch.

Here you go:
https://codereview.chromium.org/2453623003/

Really thanks for your patience.

I think you accidentally typed keyword "l-g-t-m" by mistake :)

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

https://codereview.chromium.org/2441423002/diff/100001/content/browser/media/...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2441423002/diff/100001/content/browser/media/...
content/browser/media/session/media_session_impl.cc:66:
observer.MediaSessionDestroyed();
Would MediaSessionDestroyed and StopObserving not called one after another all
the time?
What I suggested was:

class MediaSessionObserver {
 public:
  void MediaSessionDestroyed() {
    OnMediaSessionDestroyed();
    session_->RemoveObserver(this);
    session_ = nullptr;
  }

  virtual void OnMediaSessionDestroyed() = 0; 
}

class Foo : public MediaSessionObserver {
  void OnMediaSessionDestroyed() override {
    ...
  }
}

You can make OnMediaSessionDestroyed() private in MSO to prevent it to be called
by anything but MSO.

https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
File content/public/browser/media_session_observer.cc (right):

https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
content/public/browser/media_session_observer.cc:27: if (session_)
This should never be null.

https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
File content/public/browser/media_session_observer.h (right):

https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
content/public/browser/media_session_observer.h:46: friend class
MediaSessionImpl;
This class is not part of content/public so should not be mentioned here. See my
comment above for how to solve this.

[Zhiqiang Zhang (Slow), 2016-10-28 21:15:24]

On 2016/10/28 19:19:10, whywhat wrote:
>
https://codereview.chromium.org/2441423002/diff/100001/content/browser/media/...
> File content/browser/media/session/media_session_impl.cc (right):
> 
>
https://codereview.chromium.org/2441423002/diff/100001/content/browser/media/...
> content/browser/media/session/media_session_impl.cc:66:
> observer.MediaSessionDestroyed();
> Would MediaSessionDestroyed and StopObserving not called one after another all
> the time?
> What I suggested was:
> 
> class MediaSessionObserver {
>  public:
>   void MediaSessionDestroyed() {
>     OnMediaSessionDestroyed();
>     session_->RemoveObserver(this);
>     session_ = nullptr;
>   }
> 
>   virtual void OnMediaSessionDestroyed() = 0; 
> }
> 
> class Foo : public MediaSessionObserver {
>   void OnMediaSessionDestroyed() override {
>     ...
>   }
> }
> 
> You can make OnMediaSessionDestroyed() private in MSO to prevent it to be
called
> by anything but MSO.
> 
>
https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
> File content/public/browser/media_session_observer.cc (right):
> 
>
https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
> content/public/browser/media_session_observer.cc:27: if (session_)
> This should never be null.
> 
>
https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
> File content/public/browser/media_session_observer.h (right):
> 
>
https://codereview.chromium.org/2441423002/diff/100001/content/public/browser...
> content/public/browser/media_session_observer.h:46: friend class
> MediaSessionImpl;
> This class is not part of content/public so should not be mentioned here. See
my
> comment above for how to solve this.

Seems like both content/ and content/public/android reviewers don't like storing
MediaSession* in the observer.
Now I totally removed that and we'll let the users take the responsibility to
store MediaSession* and do clean up themselves.

[Zhiqiang Zhang (Slow), 2016-10-28 21:37:06]

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

[commit-bot: I haz the power, 2016-10-28 21:37:55]

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 22:38:42]

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

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

Dry run: This issue passed the CQ dry run.