Add stub for assigning media session to media elements

Add stub for assigning media session to media elements

Introduces a partial IDL interface for HTMLMediaElement containing an
optional media session attribute implemented as a supplement to media
elements.

BUG=497735
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198272

[davve, 2015-06-30 10:06:27]

davve@opera.com changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org, philipj@opera.com

[davve, 2015-06-30 10:06:27]

I will try out the oilpan side of things but otherwise I think the general basic
structure of the supplement is complete.

[davve, 2015-07-01 14:51:52]

davve@opera.com changed reviewers:
+ sigbjornf@opera.com

[davve, 2015-07-01 14:51:53]

Adding sof as reviewer for oilpan related stuff on HTMLMediaElementMediaSession.

[sof, 2015-07-01 15:18:06]

oilpan lgtm

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt
(right):

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt:2: FAIL
HTMLMediaElement set session assert_true: Run either manually by passing the
MediaSession flag or as part of the virtual mediasession testsuite. expected
true got false
is the expected file here intentionally?

[davve, 2015-07-01 15:27:55]

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt
(right):

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt:2: FAIL
HTMLMediaElement set session assert_true: Run either manually by passing the
MediaSession flag or as part of the virtual mediasession testsuite. expected
true got false
On 2015/07/01 15:18:05, sof wrote:
> is the expected file here intentionally?

Yes, but that's not saying the way I've done it is optimal.

The MediaSession flag is not part of the experimental set so not enabled by
default in when running LayoutTests/. (The idea is to test what we ship.) In
addition to this, a virtual testsuite enables the MediaSession flag and run the
media session tests with other (virtual) expected files. Thus -expected.txt
files in media/mediasession will contain fails and -expected.txt files in
virtual/mediasession generally contain passes.

Does this make sense?

[sof, 2015-07-01 15:31:40]

On 2015/07/01 15:27:55, David Vest wrote:
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> File LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt:2:
FAIL
> HTMLMediaElement set session assert_true: Run either manually by passing the
> MediaSession flag or as part of the virtual mediasession testsuite. expected
> true got false
> On 2015/07/01 15:18:05, sof wrote:
> > is the expected file here intentionally?
> 
> Yes, but that's not saying the way I've done it is optimal.
> 
> The MediaSession flag is not part of the experimental set so not enabled by
> default in when running LayoutTests/. (The idea is to test what we ship.) In
> addition to this, a virtual testsuite enables the MediaSession flag and run
the
> media session tests with other (virtual) expected files. Thus -expected.txt
> files in media/mediasession will contain fails and -expected.txt files in
> virtual/mediasession generally contain passes.
> 
> Does this make sense?

I see. That works, but have you considered making the feature status=test, or is
that too invasive to other tests for the time being?

[davve, 2015-07-01 15:57:12]

On 2015/07/01 15:31:40, sof wrote:
> On 2015/07/01 15:27:55, David Vest wrote:
> >
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> > File
LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt
> > (right):
> > 
> >
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> > LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt:2:
> FAIL
> > HTMLMediaElement set session assert_true: Run either manually by passing the
> > MediaSession flag or as part of the virtual mediasession testsuite. expected
> > true got false
> > On 2015/07/01 15:18:05, sof wrote:
> > > is the expected file here intentionally?
> > 
> > Yes, but that's not saying the way I've done it is optimal.
> > 
> > The MediaSession flag is not part of the experimental set so not enabled by
> > default in when running LayoutTests/. (The idea is to test what we ship.) In
> > addition to this, a virtual testsuite enables the MediaSession flag and run
> the
> > media session tests with other (virtual) expected files. Thus -expected.txt
> > files in media/mediasession will contain fails and -expected.txt files in
> > virtual/mediasession generally contain passes.
> > 
> > Does this make sense?
> 
> I see. That works, but have you considered making the feature status=test, or
is
> that too invasive to other tests for the time being?

The plan was to adhere to

https://groups.google.com/a/chromium.org/d/msg/blink-dev/gHgdMtlnYOo/DNBOEvk1...

That said, the issue seems to be a bit controversial. Using status=test would
avoid the extra expected files but not test for adverse behavior with the flag
off. I don't feel very strongly on the issue. Even if we use status=test we
could use the stable virtual testsuite to test that we don't leak any web
exposed features prematurely. WDYT?

[sof, 2015-07-02 06:45:04]

On 2015/07/01 15:57:12, David Vest wrote:
> On 2015/07/01 15:31:40, sof wrote:
> > On 2015/07/01 15:27:55, David Vest wrote:
> > >
> >
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> > > File
> LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
> > >
LayoutTests/media/mediasession/htmlmediaelement-set-session-expected.txt:2:
> > FAIL
> > > HTMLMediaElement set session assert_true: Run either manually by passing
the
> > > MediaSession flag or as part of the virtual mediasession testsuite.
expected
> > > true got false
> > > On 2015/07/01 15:18:05, sof wrote:
> > > > is the expected file here intentionally?
> > > 
> > > Yes, but that's not saying the way I've done it is optimal.
> > > 
> > > The MediaSession flag is not part of the experimental set so not enabled
by
> > > default in when running LayoutTests/. (The idea is to test what we ship.)
In
> > > addition to this, a virtual testsuite enables the MediaSession flag and
run
> > the
> > > media session tests with other (virtual) expected files. Thus
-expected.txt
> > > files in media/mediasession will contain fails and -expected.txt files in
> > > virtual/mediasession generally contain passes.
> > > 
> > > Does this make sense?
> > 
> > I see. That works, but have you considered making the feature status=test,
or
> is
> > that too invasive to other tests for the time being?
> 
> The plan was to adhere to
> 
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gHgdMtlnYOo/DNBOEvk1...
> 
> That said, the issue seems to be a bit controversial. Using status=test would
> avoid the extra expected files but not test for adverse behavior with the flag
> off. I don't feel very strongly on the issue. Even if we use status=test we
> could use the stable virtual testsuite to test that we don't leak any web
> exposed features prematurely. WDYT?

It sounds very reasonable, leaving the feature "status="-free for now.

[philipj_slow, 2015-07-02 08:53:15]

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.cpp (right):

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:12:
HTMLMediaElementMediaSession& thisElement =
HTMLMediaElementMediaSession::from(mediaElement);
This is as in modules/encryptedmedia, but thisElement isnt' actually an element.
Maybe "supplement"? Not perfect, but at least short and accurate.

For the getter I think it would be worthwhile to avoid creating the supplement
if it's not already there and instead just return null.

The Fullscreen supplement has a fromIfExists that is used in some similar
situations.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:18:
HTMLMediaElementMediaSession& thisElement =
HTMLMediaElementMediaSession::from(mediaElement);
from() doesn't need the prefix, so in the cases a one-liner would do:
from(mediaElement).m_session = session;

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:29:
HTMLMediaElementMediaSession* supplement =
static_cast<HTMLMediaElementMediaSession*>(WillBeHeapSupplement<HTMLMediaElement>::from(element,
supplementName()));
As in the Fullscreen code, this will be able to use fromIfExists.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.h (right):

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.h:22: static
HTMLMediaElementMediaSession& from(HTMLMediaElement&);
These two could possibly be private?

[mlamouri (slow - plz ping), 2015-07-02 09:59:04]

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:5: <div
id="log"></div>
nit: do you need that <div>?

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.idl:5: partial
interface HTMLMediaElement {
nit: add a link to the spec (ie.
https://mediasession.spec.whatwg.org/#extensions-to-the-htmlmediaelement-inte...)

[davve, 2015-07-02 12:15:44]

PTAL

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/100001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:5: <div
id="log"></div>
On 2015/07/02 09:59:04, Mounir Lamouri wrote:
> nit: do you need that <div>?

No, not really. Old habit or something. Removed here and from the other test.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.cpp (right):

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:12:
HTMLMediaElementMediaSession& thisElement =
HTMLMediaElementMediaSession::from(mediaElement);
On 2015/07/02 08:53:15, philipj wrote:
> This is as in modules/encryptedmedia, but thisElement isnt' actually an
element.
> Maybe "supplement"? Not perfect, but at least short and accurate.
> 
> For the getter I think it would be worthwhile to avoid creating the supplement
> if it's not already there and instead just return null.
> 
> The Fullscreen supplement has a fromIfExists that is used in some similar
> situations.

fromIfExists seems like a good addition. Done.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:18:
HTMLMediaElementMediaSession& thisElement =
HTMLMediaElementMediaSession::from(mediaElement);
On 2015/07/02 08:53:15, philipj wrote:
> from() doesn't need the prefix, so in the cases a one-liner would do:
> from(mediaElement).m_session = session; 

Done.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.cpp:29:
HTMLMediaElementMediaSession* supplement =
static_cast<HTMLMediaElementMediaSession*>(WillBeHeapSupplement<HTMLMediaElement>::from(element,
supplementName()));
On 2015/07/02 08:53:15, philipj wrote:
> As in the Fullscreen code, this will be able to use fromIfExists.

Done.

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/100001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.idl:5: partial
interface HTMLMediaElement {
On 2015/07/02 09:59:04, Mounir Lamouri wrote:
> nit: add a link to the spec (ie.
>
https://mediasession.spec.whatwg.org/#extensions-to-the-htmlmediaelement-inte...)

Good catch. (Funny that I almost managed to sneak that past philipj. ;)

[philipj_slow, 2015-07-02 12:50:48]

https://codereview.chromium.org/1217743003/diff/140001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/140001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.idl:5: //
https://mediasession.spec.whatwg.org/#extensions-to-the-htmlmediaelement-inte...
I like a blank line after the URL, or have become used to it at least.

And [TypeChecking=Interface] because the default are wrong :)

[philipj_slow, 2015-07-02 12:51:38]

https://codereview.chromium.org/1217743003/diff/160001/Source/modules/mediase...
File Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/160001/Source/modules/mediase...
Source/modules/mediasession/MediaSession.idl:5: //
https://mediasession.spec.whatwg.org/#mediasession
Another pet peeve is to link to
https://mediasession.spec.whatwg.org/#the-mediasession-interface so that you
don't have to scroll up to see the constructor.

[davve, 2015-07-02 14:40:28]

https://codereview.chromium.org/1217743003/diff/140001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/140001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.idl:5: //
https://mediasession.spec.whatwg.org/#extensions-to-the-htmlmediaelement-inte...
On 2015/07/02 12:50:48, philipj wrote:
> I like a blank line after the URL, or have become used to it at least.
> 
> And [TypeChecking=Interface] because the default are wrong :)

Done.

https://codereview.chromium.org/1217743003/diff/160001/Source/modules/mediase...
File Source/modules/mediasession/MediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/160001/Source/modules/mediase...
Source/modules/mediasession/MediaSession.idl:5: //
https://mediasession.spec.whatwg.org/#mediasession
On 2015/07/02 12:51:38, philipj wrote:
> Another pet peeve is to link to
> https://mediasession.spec.whatwg.org/#the-mediasession-interface so that you
> don't have to scroll up to see the constructor.

Done.

[philipj_slow, 2015-07-02 20:22:49]

LGTM % nits

Mounir, Anton?

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:12:
assert_true(mediaSession instanceof MediaSession);
I don't think this and the following assert is needed in this test, they should
be covered by other tests and look kind of paranoid in this context.

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.idl (right):

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.idl:7:
[TypeChecking=Interface] partial interface HTMLMediaElement {
I tested this and verified no change to the generated code:

[
    RuntimeEnabled=MediaSession,
    TypeChecking=Interface,
] partial interface HTMLMediaElement {
    attribute MediaSession? session;
};


Turns out I can't upload changes to this review because "You (philipj@opera.com)
don't own this issue (1217743003)"

[whywhat, 2015-07-02 22:04:54]

lgtm

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:8: "Run either
manually by passing the MediaSession flag or as part of the virtual mediasession
testsuite.");
btw, why isn't the feature enabled as 'test' in RuntimeFeaturesEnabled.in
instead of having a virtual testsuite?

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.h (right):

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.h:16: class
HTMLMediaElementMediaSession final : public
NoBaseWillBeGarbageCollected<HTMLMediaElementMediaSession>, public
WillBeHeapSupplement<HTMLMediaElement> {
nit: I think a comment for this class could be welcome by someone who doesn't
work on the API but stumbles upon the code in the future.

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/modules...
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/modules...
Source/modules/modules.gypi:942: 'mediasession/HTMLMediaElementMediaSession.h',
nit: alphabetical order (H before M)?

[davve, 2015-07-03 08:25:11]

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:8: "Run either
manually by passing the MediaSession flag or as part of the virtual mediasession
testsuite.");
On 2015/07/02 22:04:54, whywhat wrote:
> btw, why isn't the feature enabled as 'test' in RuntimeFeaturesEnabled.in
> instead of having a virtual testsuite?

The idea was to try out Jochen's suggestion in:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/gHgdMtlnYOo/DNBOEvk1...

The main advantage being that all other layout tests will test the behaviour
with the flag _off_, which is what we'll release during the implementation
period. Thus, we test what we ship.

The main issues I've stumbled into so far is:

 * The presubmit script doesn't currently allow "empty" (as in all PASS, no
warnings) -expected.txt files for testharness.js, tests not even in virtual. We
do need those to not fallback to the non-virtual expected files, which will fail
during the implementation period.

 * Extra churn. Ties into the above but we'll get two -expected.txt (one PASS
and one FAIL) files where we with status=test would have none.

So in the end, it might not be worth it for such a relatively separated feature
such as MediaSession. WDYT?

[davve, 2015-07-03 09:29:20]

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
File LayoutTests/media/mediasession/htmlmediaelement-set-session.html (right):

https://codereview.chromium.org/1217743003/diff/200001/LayoutTests/media/medi...
LayoutTests/media/mediasession/htmlmediaelement-set-session.html:12:
assert_true(mediaSession instanceof MediaSession);
On 2015/07/02 20:22:49, philipj wrote:
> I don't think this and the following assert is needed in this test, they
should
> be covered by other tests and look kind of paranoid in this context.

Done.

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
File Source/modules/mediasession/HTMLMediaElementMediaSession.h (right):

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/mediase...
Source/modules/mediasession/HTMLMediaElementMediaSession.h:16: class
HTMLMediaElementMediaSession final : public
NoBaseWillBeGarbageCollected<HTMLMediaElementMediaSession>, public
WillBeHeapSupplement<HTMLMediaElement> {
On 2015/07/02 22:04:54, whywhat wrote:
> nit: I think a comment for this class could be welcome by someone who doesn't
> work on the API but stumbles upon the code in the future.

I added _something_. Not sure that's what you had in mind though.

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/modules...
File Source/modules/modules.gypi (right):

https://codereview.chromium.org/1217743003/diff/200001/Source/modules/modules...
Source/modules/modules.gypi:942: 'mediasession/HTMLMediaElementMediaSession.h',
On 2015/07/02 22:04:54, whywhat wrote:
> nit: alphabetical order (H before M)?

Done.

[mlamouri (slow - plz ping), 2015-07-03 09:30:53]

lgtm

[davve, 2015-07-03 10:54:34]

The CQ bit was checked by davve@opera.com

[davve, 2015-07-03 10:54:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
avayvod@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1217743003/#ps220001
(title: "Review comments round #3 - idl issues, gypi issues, doc issues")

[davve, 2015-07-03 10:54:44]

The CQ bit was unchecked by davve@opera.com

[davve, 2015-07-03 11:30:16]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-07-03 11:30:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217743003/220001

[commit-bot: I haz the power, 2015-07-03 11:33:43]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198272