Make MediaSession per frame as an attribute of Navigator

Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190, 646490, 653894
Committed: https://crrev.com/dbbe70b8977af46a3c33d2b1c42123f7e5d5b98b
Cr-Commit-Position: refs/heads/master@{#424702}

[Zhiqiang Zhang (Slow), 2016-10-10 13:07:56]

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

BUG=
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame, which matches the spec:

https://wicg.github.io/mediasession/

BUG=643190
==========

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

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame, which matches the spec:

https://wicg.github.io/mediasession/

BUG=643190
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame, which matches the spec:

https://wicg.github.io/mediasession/

BUG=643190,646490
==========

[Zhiqiang Zhang (Slow), 2016-10-10 13:37:23]

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame, which matches the spec:

https://wicg.github.io/mediasession/

BUG=643190,646490
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490
==========

[Zhiqiang Zhang (Slow), 2016-10-10 13:37:35]

Patchset #2 (id:20001) has been deleted

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

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

[commit-bot: I haz the power, 2016-10-10 13:37:47]

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-10 13:41:01]

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

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

Please review this CL :)

Thanks to the IPC->mojo CL, we only need several small changes on the Blink
side.

mediasession-integration-test.html are a bit tricky since I need to run async
tests after a promise test, where the promise test is required for setting up
the mock MediaSessionService.

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

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490
==========

[commit-bot: I haz the power, 2016-10-10 15:17:00]

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

[commit-bot: I haz the power, 2016-10-10 15:17:01]

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), 2016-10-10 18:44:43]

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

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

+foolip for RuntimeEnabledFeatures.in change.

Is it OK to enable "MediaSession status=test"? I'm doing this since we really
want to get rid of virtual layout tests which is quite annoying.

If yes, I'll update the layout test expectations (about exposed web interfaces)
since they are now broken.

[foolip, 2016-10-10 19:02:27]

RuntimeEnabledFeatures.in lgtm, virtual tests seemed like a good idea but maybe
not :)

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

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

[commit-bot: I haz the power, 2016-10-10 19:34:47]

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-10 19:54:39]

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490,653894
==========

[mlamouri (slow - plz ping), 2016-10-10 20:30:35]

Didn't finish to review the tests but I have to run.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html (right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:9:
assert_true(image != null);
`assert_not_equals(image, null)`?

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:19: exception
= false;
style: I might misunderstand the diff but is this mis-indented?

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:24: }
ditto

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:45:
assert_metadata_equals(metadata, mojoMetadataToJS(mojoMetadata));
I don't think you should call `mojoMetadataToJS` in the test. If you add a
`setMetadataCallback` you could make sure to convert the object to a JS object
in the mock so the tests don't need to know how this work. It would also allow
you to move the "utils.js" content in the mock. WDYT?

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html:9:
assert_true(metadata != null);
as above, assert_not_equals?

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/resources/mediasessionservice-mock.js
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/mediasessionservice-mock.js:30:
if (!!this.setMetadataStub) {
That's odd. Did you copy this behaviour from another mojo service mock? It
sounds that having real methods in the mock would be better like
`setMetadataCallback`.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:12: for
(var i = 0; i < expected.artwork.length; i++) {
style: you can drop { }

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:26: if (i >
0) {
style: ditto

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:40: for
(var i = 0; i < mojoMetadata.artwork.length; i++) {
style: ditto

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp:13:
NavigatorMediaSession::NavigatorMediaSession(Navigator& navigator) {}
= default?

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

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:8:
#include "core/frame/DOMWindowProperty.h"
Do you need this include?

[whywhat, 2016-10-10 20:45:10]

overall lgtm but please find a more consistent way for the integration test :)

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html (right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:7:
test(function() {
is this change accidental? seems unrelated from the navigator.mediaSession on
the first glance.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html:2:
<title>MediaSession() constructor</title>
hm, do we need a test for that? I haven't found any precedents in the code base.
Adding a ctor would require API review and changing all the interface listed
tests, right?

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
can't we just setup the mock up for each test like in
budget-service-mock.js-using tests?
the setup to add order to tests seems very unusual :/
There's also setup(function()) in testharness.js I believe, maybe it could help.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html:59:
}, 'Test that passing unknown values to the dictionary is a no-op');
looks like an accidental formatting change to me too.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:18: return
String.fromCharCode.apply(this, mojoString16.data);
nit: seems like everywhere else null is passed as the first argument

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:27: sizes =
sizes + " ";
nit: +=

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:30: sizes =
sizes + mojoSize.width.toString() + "x" + mojoSize.height.toString();
nit: ditto

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp:13:
NavigatorMediaSession::NavigatorMediaSession(Navigator& navigator) {}
nit: = default?

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

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:17: class
NavigatorMediaSession final
nit: don't forget to write comments to new classes (although might be rather
rudimentary in this case).

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:8:
Exposed=(Window),
What does this mean?

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

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

[commit-bot: I haz the power, 2016-10-10 21:19:39]

Dry run: This issue passed the CQ dry run.

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

Some quick replies to avayvod@, haven't done any changes yet.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-constructor-hidden.html:2:
<title>MediaSession() constructor</title>
On 2016/10/10 20:45:09, whywhat wrote:
> hm, do we need a test for that? I haven't found any precedents in the code
base.
> Adding a ctor would require API review and changing all the interface listed
> tests, right?

OK, I'll remove this test. These is a test for the exposed interfaces
(global-interface-listing.html), and it should be caught by that.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
On 2016/10/10 20:45:09, whywhat wrote:
> can't we just setup the mock up for each test like in
> budget-service-mock.js-using tests?
> the setup to add order to tests seems very unusual :/
> There's also setup(function()) in testharness.js I believe, maybe it could
help.

Our test here is slightly different. It's because we are sending the metadata
through mojo and the browser end receives the metadata asynchronously. I looked
at what the budget service test do and seems doesn't fit our needs. Also,
setup() in testharness.js is synchronous :(

I think the tests in this file are very similar so I prefer not to split them
into multiple files. The reason why I run them one after another is to avoid any
chances of race. What I'm thinking is that multiple async_test could be executed
in parallel. There's an issue on testharness.js discussing executing async_tests
in sequence. https://github.com/w3c/testharness.js/issues/96
I'll try do some cleanup and see if it feels better.

[Zhiqiang Zhang (Slow), 2016-10-11 10:51:31]

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

[commit-bot: I haz the power, 2016-10-11 10:51:39]

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-11 10:56:04]

PTAL for mlamouri@ (since I see you didn't looked at the tests thoroughly and
maybe you want to look more?)

Addressed comments & improved the integration tests.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html (right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:7:
test(function() {
On 2016/10/10 20:45:09, whywhat wrote:
> is this change accidental? seems unrelated from the navigator.mediaSession on
> the first glance.

Aha! I found the indentation in this file is pretty mixed by mistake so I made
them consistent since this file is being touched.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:9:
assert_true(image != null);
On 2016/10/10 20:30:34, mlamouri wrote:
> `assert_not_equals(image, null)`?

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:19: exception
= false;
On 2016/10/10 20:30:34, mlamouri wrote:
> style: I might misunderstand the diff but is this mis-indented?

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:24: }
On 2016/10/10 20:30:34, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:45:
assert_metadata_equals(metadata, mojoMetadataToJS(mojoMetadata));
On 2016/10/10 20:30:34, mlamouri wrote:
> I don't think you should call `mojoMetadataToJS` in the test. If you add a
> `setMetadataCallback` you could make sure to convert the object to a JS object
> in the mock so the tests don't need to know how this work. It would also allow
> you to move the "utils.js" content in the mock. WDYT?

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html:9:
assert_true(metadata != null);
On 2016/10/10 20:30:34, mlamouri wrote:
> as above, assert_not_equals?

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasessionmetadata.html:59:
}, 'Test that passing unknown values to the dictionary is a no-op');
On 2016/10/10 20:45:09, whywhat wrote:
> looks like an accidental formatting change to me too.

Reverted

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/resources/mediasessionservice-mock.js
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/mediasessionservice-mock.js:30:
if (!!this.setMetadataStub) {
On 2016/10/10 20:30:34, mlamouri wrote:
> That's odd. Did you copy this behaviour from another mojo service mock? It
> sounds that having real methods in the mock would be better like
> `setMetadataCallback`.

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:12: for
(var i = 0; i < expected.artwork.length; i++) {
On 2016/10/10 20:30:34, mlamouri wrote:
> style: you can drop { }

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:18: return
String.fromCharCode.apply(this, mojoString16.data);
On 2016/10/10 20:45:09, whywhat wrote:
> nit: seems like everywhere else null is passed as the first argument

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:26: if (i >
0) {
On 2016/10/10 20:30:34, mlamouri wrote:
> style: ditto

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:27: sizes =
sizes + " ";
On 2016/10/10 20:45:09, whywhat wrote:
> nit: +=

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:30: sizes =
sizes + mojoSize.width.toString() + "x" + mojoSize.height.toString();
On 2016/10/10 20:45:09, whywhat wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/resources/utils.js:40: for
(var i = 0; i < mojoMetadata.artwork.length; i++) {
On 2016/10/10 20:30:34, mlamouri wrote:
> style: ditto

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp:13:
NavigatorMediaSession::NavigatorMediaSession(Navigator& navigator) {}
On 2016/10/10 20:30:34, mlamouri wrote:
> = default?

Done.

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

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:8:
#include "core/frame/DOMWindowProperty.h"
On 2016/10/10 20:30:35, mlamouri wrote:
> Do you need this include?

Removed.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:17: class
NavigatorMediaSession final
On 2016/10/10 20:45:10, whywhat wrote:
> nit: don't forget to write comments to new classes (although might be rather
> rudimentary in this case).

Done.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:8:
Exposed=(Window),
On 2016/10/10 20:45:10, whywhat wrote:
> What does this mean?

This means the partial interface is available to Window and not Worker. We do
not want workers use our API.

[commit-bot: I haz the power, 2016-10-11 11:00:32]

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

[commit-bot: I haz the power, 2016-10-11 11:00:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[whywhat, 2016-10-11 14:58:33]

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html (right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:7:
test(function() {
On 2016/10/11 at 10:56:03, Zhiqiang Zhang wrote:
> On 2016/10/10 20:45:09, whywhat wrote:
> > is this change accidental? seems unrelated from the navigator.mediaSession
on
> > the first glance.
> 
> Aha! I found the indentation in this file is pretty mixed by mistake so I made
them consistent since this file is being touched.

Usually reformatting is done in separate cls since now it's very hard to see
where you actually had to touch this file.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote:
> On 2016/10/10 20:45:09, whywhat wrote:
> > can't we just setup the mock up for each test like in
> > budget-service-mock.js-using tests?
> > the setup to add order to tests seems very unusual :/
> > There's also setup(function()) in testharness.js I believe, maybe it could
help.
> 
> Our test here is slightly different. It's because we are sending the metadata
through mojo and the browser end receives the metadata asynchronously. I looked
at what the budget service test do and seems doesn't fit our needs. Also,
setup() in testharness.js is synchronous :(
> 
> I think the tests in this file are very similar so I prefer not to split them
into multiple files. The reason why I run them one after another is to avoid any
chances of race. What I'm thinking is that multiple async_test could be executed
in parallel. There's an issue on testharness.js discussing executing async_tests
in sequence. https://github.com/w3c/testharness.js/issues/96
> I'll try do some cleanup and see if it feels better.

the budget service tests seem to just create a new mock for every promise test.
why can't we do that?
async + mojo is common, so I don't see how we're different here.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:8:
Exposed=(Window),
On 2016/10/11 at 10:56:04, Zhiqiang Zhang wrote:
> On 2016/10/10 20:45:10, whywhat wrote:
> > What does this mean?
> 
> This means the partial interface is available to Window and not Worker. We do
not want workers use our API.

Hm, we don't have that on Presentation I think, should we add that there
(Mounir?)

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

Patchset #5 (id:100001) has been deleted

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

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

[commit-bot: I haz the power, 2016-10-11 16:56:23]

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-11 16:59:05]

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html (right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediaimage.html:7:
test(function() {
On 2016/10/11 at 14:58:33, whywhat wrote:
> On 2016/10/11 at 10:56:03, Zhiqiang Zhang wrote:
> > On 2016/10/10 20:45:09, whywhat wrote:
> > > is this change accidental? seems unrelated from the navigator.mediaSession
on
> > > the first glance.
> > 
> > Aha! I found the indentation in this file is pretty mixed by mistake so I
made them consistent since this file is being touched.
> 
> Usually reformatting is done in separate cls since now it's very hard to see
where you actually had to touch this file.

OK, reverted the indenting.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
On 2016/10/11 at 14:58:33, whywhat wrote:
> On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote:
> > On 2016/10/10 20:45:09, whywhat wrote:
> > > can't we just setup the mock up for each test like in
> > > budget-service-mock.js-using tests?
> > > the setup to add order to tests seems very unusual :/
> > > There's also setup(function()) in testharness.js I believe, maybe it could
help.
> > 
> > Our test here is slightly different. It's because we are sending the
metadata through mojo and the browser end receives the metadata asynchronously.
I looked at what the budget service test do and seems doesn't fit our needs.
Also, setup() in testharness.js is synchronous :(
> > 
> > I think the tests in this file are very similar so I prefer not to split
them into multiple files. The reason why I run them one after another is to
avoid any chances of race. What I'm thinking is that multiple async_test could
be executed in parallel. There's an issue on testharness.js discussing executing
async_tests in sequence. https://github.com/w3c/testharness.js/issues/96
> > I'll try do some cleanup and see if it feels better.
> 
> the budget service tests seem to just create a new mock for every promise
test. why can't we do that?
> async + mojo is common, so I don't see how we're different here.

I think in our case, all these tests are used for testing if the Metadata is
correctly passed to content/browser. These tests are isomorphic so I would
prefer not to split them into separate files. Ideally it's possible write more
similar tests in the future, and then we need to write new files, and each file
would have the same boilerplate code. If we change a bit in the API in the
future, all these files need to be modified. I think the budget tests was split
into multiple files since it's not likely to have more similar tests.

So I think it's fine since we have a good degree of test isolation, since the
tests execute one after another and there is no chance they have overlapping
lifetimes.

[commit-bot: I haz the power, 2016-10-11 17:07:50]

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

[commit-bot: I haz the power, 2016-10-11 17:07:51]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

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

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

[commit-bot: I haz the power, 2016-10-11 17:11:35]

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-11 18:58:41]

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

[commit-bot: I haz the power, 2016-10-11 18:58:42]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-10-11 18:59:45]

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
On 2016/10/11 at 16:59:05, Zhiqiang Zhang wrote:
> On 2016/10/11 at 14:58:33, whywhat wrote:
> > On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote:
> > > On 2016/10/10 20:45:09, whywhat wrote:
> > > > can't we just setup the mock up for each test like in
> > > > budget-service-mock.js-using tests?
> > > > the setup to add order to tests seems very unusual :/
> > > > There's also setup(function()) in testharness.js I believe, maybe it
could help.
> > > 
> > > Our test here is slightly different. It's because we are sending the
metadata through mojo and the browser end receives the metadata asynchronously.
I looked at what the budget service test do and seems doesn't fit our needs.
Also, setup() in testharness.js is synchronous :(
> > > 
> > > I think the tests in this file are very similar so I prefer not to split
them into multiple files. The reason why I run them one after another is to
avoid any chances of race. What I'm thinking is that multiple async_test could
be executed in parallel. There's an issue on testharness.js discussing executing
async_tests in sequence. https://github.com/w3c/testharness.js/issues/96
> > > I'll try do some cleanup and see if it feels better.
> > 
> > the budget service tests seem to just create a new mock for every promise
test. why can't we do that?
> > async + mojo is common, so I don't see how we're different here.
> 
> I think in our case, all these tests are used for testing if the Metadata is
correctly passed to content/browser. These tests are isomorphic so I would
prefer not to split them into separate files. Ideally it's possible write more
similar tests in the future, and then we need to write new files, and each file
would have the same boilerplate code. If we change a bit in the API in the
future, all these files need to be modified. I think the budget tests was split
into multiple files since it's not likely to have more similar tests.
> 
> So I think it's fine since we have a good degree of test isolation, since the
tests execute one after another and there is no chance they have overlapping
lifetimes.

First, I don't think there's that much boilerplate. At least the boiler plate
will be familiar to the readers compared to inventing a concept of running tests
in sequence with test harness.

Second, I should've been more specific about the budget tests:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
has two tests that create a new mock each. There're a couple of other test files
using the same technique (get-budget.html and get-cost.html).

So you should be able to just do something like this...

async_test(function(test) {
  MediaSessionServiceMock.then(t.step_func(function() {
      var metadata = ...;
      mock.setMetadataStub = t.step_func_done(m) {
        assert_metadata_equals(metadata, mojoMetadataToJS(m));
      };
      navigator.mediaSession.metadata = metadata;
    }),
    test.unreached_func("Setting up MediaSession service mock failed!"));
}, "test that MediaMetadata is correctly propagated");

...for each test.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:43:
mock.setMetadataStub = function(mojoMetadata) {
I believe you can replace it with step_func(function(mojoMetadata)) at least (or
step_func_done even).

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

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

[commit-bot: I haz the power, 2016-10-11 19:37:49]

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-11 19:39:05]

Addressed Anton's comments.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html
(right):

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:28:
return mediaSessionServiceMock.then(m => {
On 2016/10/11 18:59:45, whywhat wrote:
> On 2016/10/11 at 16:59:05, Zhiqiang Zhang wrote:
> > On 2016/10/11 at 14:58:33, whywhat wrote:
> > > On 2016/10/10 at 21:21:18, Zhiqiang Zhang wrote:
> > > > On 2016/10/10 20:45:09, whywhat wrote:
> > > > > can't we just setup the mock up for each test like in
> > > > > budget-service-mock.js-using tests?
> > > > > the setup to add order to tests seems very unusual :/
> > > > > There's also setup(function()) in testharness.js I believe, maybe it
> could help.
> > > > 
> > > > Our test here is slightly different. It's because we are sending the
> metadata through mojo and the browser end receives the metadata
asynchronously.
> I looked at what the budget service test do and seems doesn't fit our needs.
> Also, setup() in testharness.js is synchronous :(
> > > > 
> > > > I think the tests in this file are very similar so I prefer not to split
> them into multiple files. The reason why I run them one after another is to
> avoid any chances of race. What I'm thinking is that multiple async_test could
> be executed in parallel. There's an issue on testharness.js discussing
executing
> async_tests in sequence. https://github.com/w3c/testharness.js/issues/96
> > > > I'll try do some cleanup and see if it feels better.
> > > 
> > > the budget service tests seem to just create a new mock for every promise
> test. why can't we do that?
> > > async + mojo is common, so I don't see how we're different here.
> > 
> > I think in our case, all these tests are used for testing if the Metadata is
> correctly passed to content/browser. These tests are isomorphic so I would
> prefer not to split them into separate files. Ideally it's possible write more
> similar tests in the future, and then we need to write new files, and each
file
> would have the same boilerplate code. If we change a bit in the API in the
> future, all these files need to be modified. I think the budget tests was
split
> into multiple files since it's not likely to have more similar tests.
> > 
> > So I think it's fine since we have a good degree of test isolation, since
the
> tests execute one after another and there is no chance they have overlapping
> lifetimes.
> 
> First, I don't think there's that much boilerplate. At least the boiler plate
> will be familiar to the readers compared to inventing a concept of running
tests
> in sequence with test harness.
> 
> Second, I should've been more specific about the budget tests:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> has two tests that create a new mock each. There're a couple of other test
files
> using the same technique (get-budget.html and get-cost.html).
> 
> So you should be able to just do something like this...
> 
> async_test(function(test) {
>   MediaSessionServiceMock.then(t.step_func(function() {
>       var metadata = ...;
>       mock.setMetadataStub = t.step_func_done(m) {
>         assert_metadata_equals(metadata, mojoMetadataToJS(m));
>       };
>       navigator.mediaSession.metadata = metadata;
>     }),
>     test.unreached_func("Setting up MediaSession service mock failed!"));
> }, "test that MediaMetadata is correctly propagated");
> 
> ...for each test.

OK. I didn't found any other tests running async_tests in sequence. I'm
splitting the tests now, and moved them to a separate mojo/ directory.

https://codereview.chromium.org/2401013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/mediasession/mediasession-integration-test.html:43:
mock.setMetadataStub = function(mojoMetadata) {
On 2016/10/11 18:59:45, whywhat wrote:
> I believe you can replace it with step_func(function(mojoMetadata)) at least
(or
> step_func_done even).

Done.

[commit-bot: I haz the power, 2016-10-11 19:48:51]

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

[commit-bot: I haz the power, 2016-10-11 19:48:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Zhiqiang Zhang (Slow), 2016-10-12 09:39:54]

I'm land the patch now since all the comments are addressed.

Anton mentioned [Exposed=Window] but that's minor and we can fix it later if
it's a real problem. I remember I asked Mounir before and he said I should use
it.

[Zhiqiang Zhang (Slow), 2016-10-12 09:40:03]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-12 09:40:04]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2401013002/#ps160001
(title: "split mojo tests")

[commit-bot: I haz the power, 2016-10-12 09:40:14]

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-12 09:58:57]

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490,653894
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490,653894
==========

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

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-10-12 10:01:04]

Description was changed from

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490,653894
==========

to

==========
Make MediaSession per frame as an attribute of Navigator

This CL makes MediaSession non user-constructible, and make it as
an attribute of window.navigator, thus MediaSession is one object
per frame. This CL also adds back the removed browser tests as
layout tests using a mock blink::mojom::MediaSessionService in js.
This CL also made MediaSession enabled in tests so no need for
virtual tests anymore.

Spec URL: https://wicg.github.io/mediasession/

BUG=643190,646490,653894

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

[commit-bot: I haz the power, 2016-10-12 10:01:06]

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