Add media controls to MediaSession in Blink

Add media controls to MediaSession in Blink

This CL add the media control interfaces to Blink MediaSession.

Explainer draft:
https://github.com/WICG/mediasession/blob/master/explainer.md#media-controls

BUG=656563
Committed: https://crrev.com/a61be6ec45bcd6e53525509fe4907e62b6bb5ef1
Cr-Commit-Position: refs/heads/master@{#426220}

[Zhiqiang Zhang (Slow), 2016-10-17 10:46:05]

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

[commit-bot: I haz the power, 2016-10-17 10:46:24]

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-17 12:45:57]

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

[commit-bot: I haz the power, 2016-10-17 12:45:58]

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

[Zhiqiang Zhang (Slow), 2016-10-17 13:14:39]

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

[whywhat, 2016-10-17 15:32:35]

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:63: //
https://crbug.com/xxxxxx
The link gives me 404.

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:64: #define
DEFINE_MEDIA_SESSION_ATTRIBUTE_EVENT_LISTENER(attribute)    \
Ditto in the header.

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:33: // Event
getters and setters. Not using DEFINE_ATTRIBUTE_EVENT_LISTENER macro
I thought the common way to do it was to override
attachEventListener/removeEventListener from EventTarget and handle custom
actions there.

Unless there're other precedents or reasons why doing a custom thing is better,
I'd prefer you use the usual macro (imagine if the macro is modified to support
some shiny new thing - how will the change owner know to update this class?)

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:11:
[CallWith=ScriptState] readonly attribute MediaSession mediaSession;
This is something I'll borrow for the RemotePlayback CL :)

[whywhat, 2016-10-17 15:41:46]

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:33: // Event
getters and setters. Not using DEFINE_ATTRIBUTE_EVENT_LISTENER macro
On 2016/10/17 at 15:32:35, whywhat wrote:
> I thought the common way to do it was to override
attachEventListener/removeEventListener from EventTarget and handle custom
actions there.

s/attachEventListener/addEventListener

> 
> Unless there're other precedents or reasons why doing a custom thing is
better, I'd prefer you use the usual macro (imagine if the macro is modified to
support some shiny new thing - how will the change owner know to update this
class?)

Another downside is that Web authors would rather use addEventListener instead
of assigning one directly and this would not go via setOnplaypause at all (while
the opposite is true if you use the standard macro.).

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

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

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

PTAL. Addressed Anton's comments
+foolip for RS EventTypeNames.in

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:63: //
https://crbug.com/xxxxxx
On 2016/10/17 15:32:34, whywhat wrote:
> The link gives me 404.

Fixed.

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:64: #define
DEFINE_MEDIA_SESSION_ATTRIBUTE_EVENT_LISTENER(attribute)    \
On 2016/10/17 15:32:35, whywhat wrote:
> Ditto in the header.

Done.

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:33: // Event
getters and setters. Not using DEFINE_ATTRIBUTE_EVENT_LISTENER macro
On 2016/10/17 15:32:35, whywhat wrote:
> I thought the common way to do it was to override
> attachEventListener/removeEventListener from EventTarget and handle custom
> actions there.
> 
> Unless there're other precedents or reasons why doing a custom thing is
better,
> I'd prefer you use the usual macro (imagine if the macro is modified to
support
> some shiny new thing - how will the change owner know to update this class?)
Thanks. Done.
Though there might be a drawback when the page sets the listener to a new value,
since MediaSessionService will receive an unset and a set signal. Then our media
buttons might flicker, but I think it's probably fine.

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:11:
[CallWith=ScriptState] readonly attribute MediaSession mediaSession;
On 2016/10/17 15:32:35, whywhat wrote:
> This is something I'll borrow for the RemotePlayback CL :)

Actually I borrowed something from RemotePlayback too :)

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

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

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

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-17 17:32:06]

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:33: // Event
getters and setters. Not using DEFINE_ATTRIBUTE_EVENT_LISTENER macro
On 2016/10/17 at 17:19:08, Zhiqiang Zhang wrote:
> On 2016/10/17 15:32:35, whywhat wrote:
> > I thought the common way to do it was to override
> > attachEventListener/removeEventListener from EventTarget and handle custom
> > actions there.
> > 
> > Unless there're other precedents or reasons why doing a custom thing is
better,
> > I'd prefer you use the usual macro (imagine if the macro is modified to
support
> > some shiny new thing - how will the change owner know to update this class?)
> Thanks. Done.
> Though there might be a drawback when the page sets the listener to a new
value, since MediaSessionService will receive an unset and a set signal. Then
our media buttons might flicker, but I think it's probably fine.

That's already the case with add/removeEventListener the page will more likely
use anyway if it's not careful with the order (setting event handler as an
attribute is a deprecated practice AFAIK) :/ So it's a limitation of the spec
really. Perhaps we could bundle requests to the Mojo service to avoid the issue.

Or we could discuss it at the spec level again (whether we need something extra
to the event handlers for the page to define the supported events) or just wait
to see if it becomes a problem - I'm not sure the pages would change the event
handlers often rather than from null to something and back.

[whywhat, 2016-10-17 17:37:24]

lgtm btw

[commit-bot: I haz the power, 2016-10-17 18:49:18]

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

[commit-bot: I haz the power, 2016-10-17 18:49:19]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-10-18 17:06:30]

lgtm

[whywhat, 2016-10-18 17:25:45]

On 2016/10/17 at 17:32:06, whywhat wrote:
>
https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):
> 
>
https://codereview.chromium.org/2428523002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/mediasession/MediaSession.h:33: // Event
getters and setters. Not using DEFINE_ATTRIBUTE_EVENT_LISTENER macro
> On 2016/10/17 at 17:19:08, Zhiqiang Zhang wrote:
> > On 2016/10/17 15:32:35, whywhat wrote:
> > > I thought the common way to do it was to override
> > > attachEventListener/removeEventListener from EventTarget and handle custom
> > > actions there.
> > > 
> > > Unless there're other precedents or reasons why doing a custom thing is
better,
> > > I'd prefer you use the usual macro (imagine if the macro is modified to
support
> > > some shiny new thing - how will the change owner know to update this
class?)
> > Thanks. Done.
> > Though there might be a drawback when the page sets the listener to a new
value, since MediaSessionService will receive an unset and a set signal. Then
our media buttons might flicker, but I think it's probably fine.
> 
> That's already the case with add/removeEventListener the page will more likely
use anyway if it's not careful with the order (setting event handler as an
attribute is a deprecated practice AFAIK) :/ So it's a limitation of the spec
really. Perhaps we could bundle requests to the Mojo service to avoid the issue.
> 
> Or we could discuss it at the spec level again (whether we need something
extra to the event handlers for the page to define the supported events) or just
wait to see if it becomes a problem - I'm not sure the pages would change the
event handlers often rather than from null to something and back.

@mlamouri, wdyt? how do we avoid blinking of the ui in the future if pages
remove/add listeners dynamically? I can imagine the notification blinking for
each listener being added (the page can't add all of them at the same time).

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

Ping! foolip@, can you approve the EventTypeNames.in changes?

[foolip, 2016-10-19 13:35:45]

EventTypeNames.in LGTM, left some thoughts.

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTypeNames.in:201: seekbackward
If this is "skip back x seconds", I guess a new event interface will be needed
for this to hold the "x".

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

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: //
TODO(zqzhang): Notify MediaSessionService the handler has been set. See
Not sure what MediaSessionService will do, but guessing that it'll affect what
bits of UI are visible.

Since there's no hasEventListener(), this is something that web developers can't
do, and there's currently no spec language one can use to do the same thing.
This is intentional, as it would mean that adding event listeners could change
behavior. (There are already cases where this happens, like when prefixed events
fire only if there were no event handlers for the unprefixed one, but none of
this has been enshrined in a spec yet.)

Some mechanism like .showTheseThings(['play', 'rewind', 'star']) might be
necessary. Since these overrides don't do anything yet, I'd suggest dropping
them, and if I'm just wrong you can add them when you need them.

[Zhiqiang Zhang (Slow), 2016-10-19 14:54:05]

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTypeNames.in:201: seekbackward
On 2016/10/19 13:35:44, foolip wrote:
> If this is "skip back x seconds", I guess a new event interface will be needed
> for this to hold the "x".

You're right. Let's just send an event for now and iterate if we need the
duration.

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

https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: //
TODO(zqzhang): Notify MediaSessionService the handler has been set. See
On 2016/10/19 13:35:45, foolip wrote:
> Not sure what MediaSessionService will do, but guessing that it'll affect what
> bits of UI are visible.
> 
> Since there's no hasEventListener(), this is something that web developers
can't
> do, and there's currently no spec language one can use to do the same thing.
> This is intentional, as it would mean that adding event listeners could change
> behavior. (There are already cases where this happens, like when prefixed
events
> fire only if there were no event handlers for the unprefixed one, but none of
> this has been enshrined in a spec yet.)
> 
> Some mechanism like .showTheseThings(['play', 'rewind', 'star']) might be
> necessary. Since these overrides don't do anything yet, I'd suggest dropping
> them, and if I'm just wrong you can add them when you need them.

Yes, there are some arguments on the API about this, but we haven't decided yet.

[Zhiqiang Zhang (Slow), 2016-10-19 14:57:23]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-19 14:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, foolip@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2428523002/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2016-10-19 14:57:38]

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

[mlamouri (slow - plz ping), 2016-10-19 14:57:45]

On 2016/10/19 at 14:54:05, zqzhang wrote:
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/events/EventTypeNames.in (right):
> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/events/EventTypeNames.in:201: seekbackward
> On 2016/10/19 13:35:44, foolip wrote:
> > If this is "skip back x seconds", I guess a new event interface will be
needed
> > for this to hold the "x".
> 
> You're right. Let's just send an event for now and iterate if we need the
duration.
> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):
> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: //
TODO(zqzhang): Notify MediaSessionService the handler has been set. See
> On 2016/10/19 13:35:45, foolip wrote:
> > Not sure what MediaSessionService will do, but guessing that it'll affect
what
> > bits of UI are visible.
> > 
> > Since there's no hasEventListener(), this is something that web developers
can't
> > do, and there's currently no spec language one can use to do the same thing.
> > This is intentional, as it would mean that adding event listeners could
change
> > behavior. (There are already cases where this happens, like when prefixed
events
> > fire only if there were no event handlers for the unprefixed one, but none
of
> > this has been enshrined in a spec yet.)
> > 
> > Some mechanism like .showTheseThings(['play', 'rewind', 'star']) might be
> > necessary. Since these overrides don't do anything yet, I'd suggest dropping
> > them, and if I'm just wrong you can add them when you need them.
> 
> Yes, there are some arguments on the API about this, but we haven't decided
yet.

I think it's fine to iterate on this on the API side. foolip@, do you want us to
include you in the discussions?

@avayvod, I agree that we might want to avoid the icon flickering somehow. I
think we should discuss that in a GitHub issue.

[whywhat, 2016-10-19 15:00:21]

On 2016/10/19 at 14:54:05, zqzhang wrote:
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/events/EventTypeNames.in (right):
> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/events/EventTypeNames.in:201: seekbackward
> On 2016/10/19 13:35:44, foolip wrote:
> > If this is "skip back x seconds", I guess a new event interface will be
needed
> > for this to hold the "x".
> 
> You're right. Let's just send an event for now and iterate if we need the
duration.

IIUC, the page may skip x seconds when handling the event and prevent the
default action which would be to skip by some default x.

> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):
> 
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: //
TODO(zqzhang): Notify MediaSessionService the handler has been set. See
> On 2016/10/19 13:35:45, foolip wrote:
> > Not sure what MediaSessionService will do, but guessing that it'll affect
what
> > bits of UI are visible.
> > 
> > Since there's no hasEventListener(), this is something that web developers
can't
> > do, and there's currently no spec language one can use to do the same thing.
> > This is intentional, as it would mean that adding event listeners could
change
> > behavior. (There are already cases where this happens, like when prefixed
events
> > fire only if there were no event handlers for the unprefixed one, but none
of
> > this has been enshrined in a spec yet.)
> > 
> > Some mechanism like .showTheseThings(['play', 'rewind', 'star']) might be
> > necessary. Since these overrides don't do anything yet, I'd suggest dropping
> > them, and if I'm just wrong you can add them when you need them.
> 
> Yes, there are some arguments on the API about this, but we haven't decided
yet.

We should have a github issue maybe? Currently the arguments were only in the
explainer PR, IIRC and the decision was to use the fact the event handlers are
present to detect what to show.

[commit-bot: I haz the power, 2016-10-19 16:33:45]

Committed patchset #3 (id:40001)

[foolip, 2016-10-20 08:11:48]

On 2016/10/19 14:57:45, mlamouri wrote:
> On 2016/10/19 at 14:54:05, zqzhang wrote:
> >
>
https://codereview.chromium.org/2428523002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:66: //
> TODO(zqzhang): Notify MediaSessionService the handler has been set. See
> > On 2016/10/19 13:35:45, foolip wrote:
> > > Not sure what MediaSessionService will do, but guessing that it'll affect
> what
> > > bits of UI are visible.
> > > 
> > > Since there's no hasEventListener(), this is something that web developers
> can't
> > > do, and there's currently no spec language one can use to do the same
thing.
> > > This is intentional, as it would mean that adding event listeners could
> change
> > > behavior. (There are already cases where this happens, like when prefixed
> events
> > > fire only if there were no event handlers for the unprefixed one, but none
> of
> > > this has been enshrined in a spec yet.)
> > > 
> > > Some mechanism like .showTheseThings(['play', 'rewind', 'star']) might be
> > > necessary. Since these overrides don't do anything yet, I'd suggest
dropping
> > > them, and if I'm just wrong you can add them when you need them.
> > 
> > Yes, there are some arguments on the API about this, but we haven't decided
> yet.
> 
> I think it's fine to iterate on this on the API side. foolip@, do you want us
to
> include you in the discussions?

No need, just flagging that this particular solution is problematic. I found out
in https://github.com/whatwg/html/issues/1900 that I was wrong about the
prefixed/unprefixed events, that was added in
https://github.com/whatwg/dom/pull/168.

That's still not the primitive you need though, if you want to go down this path
I suggest a DOM issue early on, and probably also highlighting it when you ask
for TAG review.

[commit-bot: I haz the power, 2016-10-21 13:09:20]

Description was changed from

==========
Add media controls to MediaSession in Blink

This CL add the media control interfaces to Blink MediaSession.

Explainer draft:
https://github.com/WICG/mediasession/blob/master/explainer.md#media-controls

BUG=656563
==========

to

==========
Add media controls to MediaSession in Blink

This CL add the media control interfaces to Blink MediaSession.

Explainer draft:
https://github.com/WICG/mediasession/blob/master/explainer.md#media-controls

BUG=656563

Committed: https://crrev.com/a61be6ec45bcd6e53525509fe4907e62b6bb5ef1
Cr-Commit-Position: refs/heads/master@{#426220}
==========

[commit-bot: I haz the power, 2016-10-21 13:09:21]

Patchset 3 (id:??) landed as
https://crrev.com/a61be6ec45bcd6e53525509fe4907e62b6bb5ef1
Cr-Commit-Position: refs/heads/master@{#426220}