-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update type of MediaMetadata's artwork #243
Conversation
Since the entries in the MediaMetadata's `artwork` are frozen in the current spec [1], the type of the attribute `artwork` must be `FrozenArray<object>` rather than `FrozenArray<MediaImage>`. Otherwise the entries of artwork can not be frozen [2]. This change will address issue w3c#237 The `artwork` in `MediaMetadataInit` and `MediaMetadata` will be clearly different after changing the `artwork` in `MediaMetadata` to `FrozenArray<object>`, hence the _getter_, _setter_ of `artwork` and the _convert artwork algorithm_ should be updated to match the change. This change will address issue w3c#176 [1] https://github.com/web-platform-tests/wpt/blob/801a2b3b5e1cd0192f31890ddf9ee7b4d0ad9e89/mediasession/mediametadata.html#L148 [2] https://tc39.es/ecma262/#sec-object.freeze
PR looks good to me overall. Boris mentionned using https://heycam.github.io/webidl/#es-to-sequence to do the first conversion in the setter. Also, this is a preexisting issue, but it is unclear whether we create a new frozen array each time the getter is called or whether the same frozen array is returned (except it gets regenerated after the setter is called). I had a try in Chrome, Firefox and Safari. It seems only Firefox is returning the same frozen array. |
I don't think this pull request addresses the issues outlined by @jan-ivar in #237. We should go back and look the questions/use cases jan-ivar posted there and see how we can address them via an interface. This only partially addresses the problem but would be very confusing if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that this doesn't address the core problems outlined in the issue.
I think the first step is to correct the spec so that it describes in a sensible way the original intent and what implementations (roughly) do. To address @jan-ivar's feedback, we might have to change the API shape, which is not as easy as getting the spec in a consistent state. @marcoscaceres, is it ok if I update this PR according my feedback in #243 (comment) and file the new issue to track @jan-ivar's improvements. |
@youennf, as some time has passed, what should we do with this PR? |
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
The plan is to update it and align with implementations as much as we can. Then, we should file another issue to keep track of @jan-ivar API change request. |
Closing in favor of #343 |
Since the entries in the MediaMetadata's
artwork
are frozen in thecurrent spec [1], the type of the attribute
artwork
must beFrozenArray<object>
rather thanFrozenArray<MediaImage>
. Otherwisethe entries of artwork can not be frozen [2]. This change will address
issue #237.
The
artwork
inMediaMetadataInit
andMediaMetadata
will be clearlydifferent after changing the
artwork
inMediaMetadata
toFrozenArray<object>
, hence the getter, setter ofartwork
and theconvert artwork algorithm should be updated to match the change. This
change will address issue #176.
[1] https://github.com/web-platform-tests/wpt/blob/801a2b3b5e1cd0192f31890ddf9ee7b4d0ad9e89/mediasession/mediametadata.html#L148
[2] https://tc39.es/ecma262/#sec-object.freeze
Preview | Diff