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

The freezing in the artwork getter is broken #237

Closed
bzbarsky opened this issue Sep 17, 2019 · 4 comments · Fixed by #343
Closed

The freezing in the artwork getter is broken #237

bzbarsky opened this issue Sep 17, 2019 · 4 comments · Fixed by #343
Labels
metadata P1 TPAC2024 Topic for discussion at TPAC 2024
Milestone

Comments

@bzbarsky
Copy link

In https://w3c.github.io/mediasession/#dom-mediametadata-artwork there is this step:

Call Object.freeze on image, to prevent accidental mutation by scripts.

But image is an IDL dictionary here, not a JS value, so you can't call Object.freeze on it.

Maybe this means to say that you convert the dictionary to a JS object, then freeze it? That would be a viable option if this is the defined behavior, but then the artwork attribute should return FrozenArray<object>, because when you're creating the FrozenArray it's done from a list of objects, not a list of MediaImage dictionaries.

@jan-ivar
Copy link
Member

jan-ivar commented Oct 2, 2019

How does this spec imagine people change metadata? It would help if the examples showed this.

Here's what I get in Chrome:

navigator.mediaSession.metadata = new MediaMetadata({artist:"Ƭ̵̬̊", artwork:[{src:"A.png"}]})
navigator.mediaSession.metadata.artist = "Prince"
navigator.mediaSession.metadata.artwork[0].src = "B.png"
navigator.mediaSession.metadata.artist // Prince
navigator.mediaSession.metadata.artwork[0].src // "http://example.com/A.png"

artwork[0].src was not updated. That seems to spec and broken. How do I change artwork?

The other thing that's odd is there's no way to add or remove artwork. How do I do that?

If things are meant to be changed solely through the constructor, I'd make MediaImage an interface, and all attributes readonly.

@jan-ivar
Copy link
Member

jan-ivar commented Oct 2, 2019

Or make MediaImage an interface and remove the freezing, and support

navigator.mediaSession.metadata.artwork[0].src = "B.png"

@ChunMinChang
Copy link
Member

ChunMinChang commented Oct 7, 2019

So there are two different issues here:

  1. By the behavior defined in the current spec, the artwork in MediaMetadata must be FrozenArray<object> rather than FrozenArray<MediaImage>, so the item in the artwork can be frozen.
  2. We probably should make MediaImage be an interface instead of dictionary

ChunMinChang added a commit to ChunMinChang/mediasession that referenced this issue Oct 11, 2019
Since the entries in the MediaMetadata's `artwork` are frozen in the
current spec, the type of the attribute `artwork` must be
`FrozenArray<object>` rather than `FrozenArray<MediaImage>`, otherwise
the entries of artwork can not be frozen[1]. 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://tc39.es/ecma262/#sec-object.freeze
ChunMinChang added a commit to ChunMinChang/mediasession that referenced this issue 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 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
ChunMinChang added a commit to ChunMinChang/mediasession that referenced this issue 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 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
@youennf youennf added this to the V1 milestone Mar 14, 2023
@marcoscaceres marcoscaceres added the TPAC2024 Topic for discussion at TPAC 2024 label Aug 1, 2024
youennf pushed a commit to youennf/mediasession that referenced this issue Sep 26, 2024
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
youennf added a commit to youennf/mediasession that referenced this issue Sep 26, 2024
Fixes w3c#237

Introduce a slot where we store the FrozenArray so that the getter always returns the same object.
Make sure the slot is reset when the setter is called.
youennf added a commit to youennf/mediasession that referenced this issue Sep 27, 2024
Fixes w3c#237

Introduce a slot where we store the FrozenArray so that the getter always returns the same object.
Make sure the slot is reset when the setter is called.
youennf added a commit to youennf/mediasession that referenced this issue Sep 27, 2024
Fixes w3c#237

Introduce a slot where we store the FrozenArray so that the getter always returns the same object.
Make sure the slot is reset when the setter is called.
youennf added a commit to youennf/mediasession that referenced this issue Sep 27, 2024
Fixes w3c#237

Introduce a slot where we store the FrozenArray so that the getter always returns the same object.
Make sure the slot is reset when the setter is called.
@youennf
Copy link
Contributor

youennf commented Sep 27, 2024

PR available at #343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata P1 TPAC2024 Topic for discussion at TPAC 2024
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants