[Blink>MediaSession] Use setActionCallback() instead of event listeners for media control actions

[Blink>MediaSession] Use setActionCallback() instead of event listeners for media control actions

As the MediaSession spec changed from using event-listener-based
to callback-based fashion for media controls, we need to keep the
implementation up to date.

Spec PR:
https://github.com/WICG/mediasession/pull/159

BUG=675963
Review-Url: https://codereview.chromium.org/2589893002
Cr-Commit-Position: refs/heads/master@{#441787}
Committed: https://chromium.googlesource.com/chromium/src/+/3194c19094e8d713487876ebfd15821752a31739

[Zhiqiang Zhang (Slow), 2016-12-19 21:18:08]

Description was changed from

==========
[Blink>MediaSession] Use setActionHandler() instead of event listeners for media
control actions

BUG=WIP
==========

to

==========
[Blink>MediaSession] Use setActionCallback() instead of event listeners for
media control actions

BUG=WIP
==========

[Zhiqiang Zhang (Slow), 2016-12-19 21:21:07]

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

[Zhiqiang Zhang (Slow), 2016-12-19 21:21:08]

bashi@, when preparing this CL, I came up with an v8 bindings issue for IDL
callbacks. Can you take a look at MediaSession.idl?

Thanks :)

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:34: // Why
`MediaSessionActionCallback?` will generate bindings with
`Nullable<MediaSessionActionCallback*>` instead of
`MediaSessionActionCallback*`?
bashi@, do you have answer to this question? If I use VoidCallback, the
generated binding will use `VoidCallback*`.

[bashi, 2016-12-20 06:02:10]

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:34: // Why
`MediaSessionActionCallback?` will generate bindings with
`Nullable<MediaSessionActionCallback*>` instead of
`MediaSessionActionCallback*`?
On 2016/12/19 21:21:08, Zhiqiang Zhang wrote:
> bashi@, do you have answer to this question? If I use VoidCallback, the
> generated binding will use `VoidCallback*`.

That seems a bug in the code generator. Thank you for reaching out to me. I'll
upload a fix.

[Zhiqiang Zhang (Slow), 2016-12-20 11:30:20]

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

[Zhiqiang Zhang (Slow), 2016-12-20 11:30:20]

+mlamouri to take a initial look

I used "Callback" instead of "Handler" because it seems like a convention

[mlamouri (slow - plz ping), 2016-12-20 13:25:58]

Left a few comments. sgtm. Let me know when it's ready for a full review.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8MediaSessionCustom.cpp
(right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8MediaSessionCustom.cpp:17:
mediaSession->setV8ReferencesForCallbacks(isolate, wrapper);
nit: you can probably do this in one line.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:56: const
String& eventName) {
s/eventName/actionName/?

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:165:
service->DisableAction(mojomAction.value());
Would it make sense to have timer in order to bundle multiple action
removed/added in the same event loop?

It's okay to do in a follow-up.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:221: }
style: you can drop the { }

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:18: "playpause",
playpause is no longer there, right?

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:35: void
setActionCallback(MediaSessionAction action, MediaSessionActionCallback
callback);
I would prefer to keep Handler to match the discussion on the spec issue. We can
always change this later if needed.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:8:
#include "bindings/core/v8/ScriptState.h"
why?

[Zhiqiang Zhang (Slow), 2016-12-20 13:53:07]

mlamouri@, PTAL :)

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8MediaSessionCustom.cpp
(right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/custom/V8MediaSessionCustom.cpp:17:
mediaSession->setV8ReferencesForCallbacks(isolate, wrapper);
On 2016/12/20 13:25:57, mlamouri wrote:
> nit: you can probably do this in one line.

Done.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:56: const
String& eventName) {
On 2016/12/20 13:25:57, mlamouri wrote:
> s/eventName/actionName/?

Done.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:165:
service->DisableAction(mojomAction.value());
On 2016/12/20 13:25:57, mlamouri wrote:
> Would it make sense to have timer in order to bundle multiple action
> removed/added in the same event loop?
> 
> It's okay to do in a follow-up.

We are still passing individual actions through mojo. If we do a timer, we might
need to pass the whole set of actions instead. WDYT?

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:221: }
On 2016/12/20 13:25:57, mlamouri wrote:
> style: you can drop the { }

Done.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:18: "playpause",
On 2016/12/20 13:25:57, mlamouri wrote:
> playpause is no longer there, right?

Thanks for the catch. Done :)

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:35: void
setActionCallback(MediaSessionAction action, MediaSessionActionCallback
callback);
On 2016/12/20 13:25:58, mlamouri wrote:
> I would prefer to keep Handler to match the discussion on the spec issue. We
can
> always change this later if needed.

Done.

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2589893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:8:
#include "bindings/core/v8/ScriptState.h"
On 2016/12/20 13:25:58, mlamouri wrote:
> why?

This is because I removed some #include from MediaSession.h and ScriptState is
not here anymore.

Anyway, we need to declare ScriptState regardless of whether we include
MediaSession.h, so I now forward-declare it and include the header in the .cpp
file.

[Zhiqiang Zhang (Slow), 2016-12-20 13:55:08]

On 2016/12/20 13:53:07, Zhiqiang Zhang wrote:
> mlamouri@, PTAL :)
> 
Maybe after I rebase my CL on bashi@'s fix for binding generator :)

[bashi, 2016-12-20 13:59:20]

On 2016/12/20 13:55:08, Zhiqiang Zhang wrote:
> On 2016/12/20 13:53:07, Zhiqiang Zhang wrote:
> > mlamouri@, PTAL :)
> > 
> Maybe after I rebase my CL on bashi@'s fix for binding generator :)

trying to submit :)

[Zhiqiang Zhang (Slow), 2016-12-20 19:22:20]

mlamouri & bashi, PTAL

bashi, I got a follow-up issue with bindings that needs to be fixed before
landing this CL (https://crbug.com/676027)

[mlamouri (slow - plz ping), 2016-12-23 12:13:59]

lgtm

Looks great! :)

Can you update the CL description and apply the comments below?

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html
(right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html:14:
}, "test that setActionHandler() can be executed for supported actions");
nit: s/test/Test/

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html:22:
}, "test that setActionHandler() throws exception for unsupported actions");
ditto

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html
(right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html:44:
console.log(action + " " + isEnabled);
Should you remove this?

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:154: LOG(INFO)
<< action << reinterpret_cast<size_t>(handler);
You might want to remove this :)

[Zhiqiang Zhang (Slow), 2017-01-05 09:29:16]

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html
(right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html:14:
}, "test that setActionHandler() can be executed for supported actions");
On 2016/12/23 12:13:58, mlamouri wrote:
> nit: s/test/Test/

Done.

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-setactionhandler.html:22:
}, "test that setActionHandler() throws exception for unsupported actions");
On 2016/12/23 12:13:58, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html
(right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/mediasession/mojo/media-control-set-handler-notifies-service.html:44:
console.log(action + " " + isEnabled);
On 2016/12/23 12:13:58, mlamouri wrote:
> Should you remove this?

Done.

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2589893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:154: LOG(INFO)
<< action << reinterpret_cast<size_t>(handler);
On 2016/12/23 12:13:58, mlamouri wrote:
> You might want to remove this :)

Done.

[Zhiqiang Zhang (Slow), 2017-01-05 09:32:31]

Description was changed from

==========
[Blink>MediaSession] Use setActionCallback() instead of event listeners for
media control actions

BUG=WIP
==========

to

==========
[Blink>MediaSession] Use setActionCallback() instead of event listeners for
media control actions

As the MediaSession spec changed from using event-listener-based
to callback-based fashion for media controls, we need to keep the
implementation up to date.

Spec PR:
https://github.com/WICG/mediasession/pull/159

BUG=675963
==========

[Zhiqiang Zhang (Slow), 2017-01-05 09:35:14]

bashi@, PTAL third_party/WebKit/Source/bindings/*
This CL is ready :)

[Zhiqiang Zhang (Slow), 2017-01-05 09:35:30]

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

[commit-bot: I haz the power, 2017-01-05 09:35:50]

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

[commit-bot: I haz the power, 2017-01-05 11:38:25]

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

[commit-bot: I haz the power, 2017-01-05 11:38:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Zhiqiang Zhang (Slow), 2017-01-05 13:30:39]

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

[commit-bot: I haz the power, 2017-01-05 13:30:50]

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

[commit-bot: I haz the power, 2017-01-05 16:23:07]

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

[commit-bot: I haz the power, 2017-01-05 16:23:08]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-01-05 22:56:57]

bindings/ lgtm

[Zhiqiang Zhang (Slow), 2017-01-05 23:06:22]

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

[Zhiqiang Zhang (Slow), 2017-01-05 23:06:23]

+haraken: EventTypeNames.in

[haraken, 2017-01-05 23:10:01]

LGTM

[Zhiqiang Zhang (Slow), 2017-01-05 23:38:38]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-01-05 23:38:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2589893002/#ps160001
(title: "fixed tests")

[commit-bot: I haz the power, 2017-01-05 23:39:09]

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, 2017-01-06 01:25:23]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483659518276290,
"parent_rev": "ddded05fefc02ea8728712c829adba8b48b39f3a", "commit_rev":
"3194c19094e8d713487876ebfd15821752a31739"}

[commit-bot: I haz the power, 2017-01-06 01:25:49]

Description was changed from

==========
[Blink>MediaSession] Use setActionCallback() instead of event listeners for
media control actions

As the MediaSession spec changed from using event-listener-based
to callback-based fashion for media controls, we need to keep the
implementation up to date.

Spec PR:
https://github.com/WICG/mediasession/pull/159

BUG=675963
==========

to

==========
[Blink>MediaSession] Use setActionCallback() instead of event listeners for
media control actions

As the MediaSession spec changed from using event-listener-based
to callback-based fashion for media controls, we need to keep the
implementation up to date.

Spec PR:
https://github.com/WICG/mediasession/pull/159

BUG=675963

Review-Url: https://codereview.chromium.org/2589893002
Cr-Commit-Position: refs/heads/master@{#441787}
Committed:
https://chromium.googlesource.com/chromium/src/+/3194c19094e8d713487876ebfd15...
==========

[commit-bot: I haz the power, 2017-01-06 01:25:50]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3194c19094e8d713487876ebfd15...