Media Session: use dictionary instead of an interface for MediaImage.

Media Session: use dictionary instead of an interface for MediaImage.

Using an interface makes the usage of MediaImage needlessly more
complicated than needed. A dictionary should keep the feature working
roughly the same way and improve developer's ergonomics.

Spec change: https://github.com/WICG/mediasession/pull/162

BUG=676995
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2612003002
Cr-Commit-Position: refs/heads/master@{#441962}
Committed: https://chromium.googlesource.com/chromium/src/+/31c6ef91f565a59348aad61bfadd3c954a3b0552

[mlamouri (slow - plz ping), 2017-01-04 12:48:34]

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

[commit-bot: I haz the power, 2017-01-04 12:48:40]

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-04 12:50:30]

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

[commit-bot: I haz the power, 2017-01-04 12:50:30]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mlamouri (slow - plz ping), 2017-01-04 12:55:59]

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

[commit-bot: I haz the power, 2017-01-04 12:56:06]

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

[mlamouri (slow - plz ping), 2017-01-04 12:58:35]

zqzhang@, PTAL

This is mostly removing code because MediaImage implementation is now done by
the bindings code.

I can think of two open questions:
- should we do something similar to
https://notifications.spec.whatwg.org/#dom-notification-actions to freeze the
artwork?
- should we expose the resolved url in mediametadata.artwork?

[commit-bot: I haz the power, 2017-01-04 14:32:26]

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

[commit-bot: I haz the power, 2017-01-04 14:32:27]

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-04 15:54:45]

> I can think of two open questions:

Yeah, I have the same questions writing the spec :)

> - should we do something similar to
> https://notifications.spec.whatwg.org/#dom-notification-actions to freeze the
> artwork?

Maybe yes. It would not make sense if we only freeze the array but not it's
elements.

> - should we expose the resolved url in mediametadata.artwork?

I suggest not to expose the resolved url in mediametadata.artwork.
Suppose we have
```
var images = [ {...}, {...} ];
navigator.mediaSession.artwork = images;
```
Then checking the equality of |navigator.mediaSession.artwork| and |images|
would return false. This feels unnatural.

[Zhiqiang Zhang (Slow), 2017-01-04 16:05:24]

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaImage.idl (right):

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:1: // Copyright
2016 The Chromium Authors. All rights reserved.
nit: Maybe we could merge it into MediaMetadata.idl

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

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:26: m_artwork =
metadata.artwork();
Create copies of MediaImages and freeze them if we decide to do so.

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

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadataSanitizer.cpp:66:
KURL url = KURL(ParsedURLString, image.src());
Resolve to absolute URL if we decide to do it here.

[mlamouri (slow - plz ping), 2017-01-05 15:36:03]

Description was changed from

==========
Media Session: use dictionary instead of an interface for MediaImage.

Using an interface makes the usage of MediaImage needlessly more
complicated than needed. A dictionary should keep the feature working
roughly the same way and improve developer's ergonomics.

BUG=676995
R=zqzhang@chromium.org
==========

to

==========
Media Session: use dictionary instead of an interface for MediaImage.

Using an interface makes the usage of MediaImage needlessly more
complicated than needed. A dictionary should keep the feature working
roughly the same way and improve developer's ergonomics.

Spec change: https://github.com/WICG/mediasession/pull/162

BUG=676995
R=zqzhang@chromium.org
==========

[mlamouri (slow - plz ping), 2017-01-05 15:40:20]

I think this is ready. It is implementing what we discussed in the spec issue.

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaImage.idl (right):

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/01/04 at 16:05:24, Zhiqiang Zhang wrote:
> nit: Maybe we could merge it into MediaMetadata.idl

In modules_idl_files, we list this file as a dictionary so I don't think we can
merge things. Also, more files doesn't cost much :)

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

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:26: m_artwork =
metadata.artwork();
On 2017/01/04 at 16:05:24, Zhiqiang Zhang wrote:
> Create copies of MediaImages and freeze them if we decide to do so.

Done.

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

https://codereview.chromium.org/2612003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadataSanitizer.cpp:66:
KURL url = KURL(ParsedURLString, image.src());
On 2017/01/04 at 16:05:24, Zhiqiang Zhang wrote:
> Resolve to absolute URL if we decide to do it here.

I did it when we set. Though, this code assumes it is an absolute URL already.

[mlamouri (slow - plz ping), 2017-01-05 15:41:08]

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

[commit-bot: I haz the power, 2017-01-05 15:41:27]

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 15:54:34]

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

[commit-bot: I haz the power, 2017-01-05 15:54:35]

Dry run: Try jobs failed on following builders:
  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), 2017-01-05 16:03:08]

lgtm w/ nits

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

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:8: USVString src;
+required?

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:9: DOMString
sizes;
= "" ?

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:10: DOMString
type;
= "" ?

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

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:48:
Vector<v8::Local<v8::Value>> result(m_artwork.size());
Why not freeze them in the ctor?

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:104:
exceptionState.throwTypeError("'" + image.src() +
Should this be URIError? I currently use it in the spec PR.

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

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

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:48:
Vector<v8::Local<v8::Value>> result(m_artwork.size());
On 2017/01/05 16:03:08, Zhiqiang Zhang wrote:
> Why not freeze them in the ctor?

I see it's following the pattern of Notification.cpp :) Please ignore

[mlamouri (slow - plz ping), 2017-01-05 19:28:22]

Comments applied. Thanks for the review! :)

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

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:8: USVString src;
On 2017/01/05 at 16:03:07, Zhiqiang Zhang wrote:
> +required?

Done.

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:9: DOMString
sizes;
On 2017/01/05 at 16:03:07, Zhiqiang Zhang wrote:
> = "" ?

Done.

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaImage.idl:10: DOMString
type;
On 2017/01/05 at 16:03:07, Zhiqiang Zhang wrote:
> = "" ?

Done.

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

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:48:
Vector<v8::Local<v8::Value>> result(m_artwork.size());
On 2017/01/05 at 16:06:40, Zhiqiang Zhang wrote:
> On 2017/01/05 16:03:08, Zhiqiang Zhang wrote:
> > Why not freeze them in the ctor?
> 
> I see it's following the pattern of Notification.cpp :) Please ignore

Ack :)

https://codereview.chromium.org/2612003002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaMetadata.cpp:104:
exceptionState.throwTypeError("'" + image.src() +
On 2017/01/05 at 16:03:08, Zhiqiang Zhang wrote:
> Should this be URIError? I currently use it in the spec PR.

TypeError is the one we use when something doesn't match the expected type. I
think it's fair to use TypeError and it matches some other specs (like
Presentation API). Any reason why you used URIError?

(I will not block the CL on your reply and we can always change the code/spec
after the fact, it's a one-liner :))

[mlamouri (slow - plz ping), 2017-01-05 19:28:23]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-01-05 19:28:57]

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 21:17:02]

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

[commit-bot: I haz the power, 2017-01-05 21:17:03]

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_...)

[mlamouri (slow - plz ping), 2017-01-06 10:56:27]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-01-06 10:56:43]

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 11:01:00]

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

[commit-bot: I haz the power, 2017-01-06 11:01:04]

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_...)

[mlamouri (slow - plz ping), 2017-01-06 13:34:51]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-01-06 13:34:51]

The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/2612003002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-06 13:35:04]

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 17:28:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483709691442570,
"parent_rev": "1c53fd3cf915d7700bbb2702061e90845c6a120d", "commit_rev":
"31c6ef91f565a59348aad61bfadd3c954a3b0552"}

[commit-bot: I haz the power, 2017-01-06 17:28:33]

Description was changed from

==========
Media Session: use dictionary instead of an interface for MediaImage.

Using an interface makes the usage of MediaImage needlessly more
complicated than needed. A dictionary should keep the feature working
roughly the same way and improve developer's ergonomics.

Spec change: https://github.com/WICG/mediasession/pull/162

BUG=676995
R=zqzhang@chromium.org
==========

to

==========
Media Session: use dictionary instead of an interface for MediaImage.

Using an interface makes the usage of MediaImage needlessly more
complicated than needed. A dictionary should keep the feature working
roughly the same way and improve developer's ergonomics.

Spec change: https://github.com/WICG/mediasession/pull/162

BUG=676995
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2612003002
Cr-Commit-Position: refs/heads/master@{#441962}
Committed:
https://chromium.googlesource.com/chromium/src/+/31c6ef91f565a59348aad61bfadd...
==========

[commit-bot: I haz the power, 2017-01-06 17:28:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/31c6ef91f565a59348aad61bfadd...