Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChunMinChang
Copy link
Member

@ChunMinChang ChunMinChang commented Oct 11, 2019

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 #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 #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

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
@chrisn chrisn mentioned this pull request Aug 10, 2023
@youennf
Copy link
Contributor

youennf commented Sep 12, 2023

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.
And we probably want to use sequence instead of list.

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).
In theory, the same frozen array should be returned given it is an attribute.

I had a try in Chrome, Firefox and Safari. It seems only Firefox is returning the same frozen array.
I guess we can do that clean-up as a follow-up.

@marcoscaceres
Copy link
Member

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 mediaObject !== artowork[n]. .

Copy link
Member

@marcoscaceres marcoscaceres left a 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.

@youennf
Copy link
Contributor

youennf commented Oct 4, 2023

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.
This seems best to address it as a follow-up and with a new issue.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants