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

Manifest-icon-like metadata artworks. #129

Merged
merged 4 commits into from
Jul 4, 2016
Merged

Conversation

xxyzzzq
Copy link
Contributor

@xxyzzzq xxyzzzq commented May 31, 2016

Make the artwork spec more like WebApp manifest icons, which has src,
type and sizes fields. The texts are based on pull request #126

Fixes #56

Make the artwork spec more like WebApp manifest icons, which has `src`,
`type` and `sizes` fields.

Fixes w3c#58
@@ -1123,15 +1136,21 @@ interface MediaMetadata {
readonly attribute DOMString title;
readonly attribute DOMString artist;
readonly attribute DOMString album;
readonly attribute sequence<MediaArtwork> artworks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use FrozenArray here while things are readonly (I don't think it should be readonly but that's orthogonal ;)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -1136,20 +1136,20 @@ interface MediaMetadata {
readonly attribute DOMString title;
readonly attribute DOMString artist;
readonly attribute DOMString album;
readonly attribute sequence<MediaArtwork> artworks;
readonly attribute FrozenArray<MediaArtwork> artworks;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

artwork is a mass noun so it does not need to be pluralized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mounirlamouri
Copy link
Member

With Eric comment applied this lgtm.

@foolip
Copy link
Member

foolip commented Jun 8, 2016

It doesn't look like one can do anything with a MediaArtwork instance, so should it have a constructor? Presumably manifests of this form will make sense for things other than Media Session as well, but the name MediaArtwork probably won't make sense in that context.

Assuming there's still the ambition of having this for notifications as well, where can the shared bits be put so that both specs can use them?

@foolip
Copy link
Member

foolip commented Jun 8, 2016

What I'm getting at here is that maybe it would be nice if artwork were a FrozenArray<SomeDictionary> artwork where SomeDictionary wouldn't have an interface object (because it's not an interface). But the trouble is of course that then you could do session.metadata.artwork[0].src = 'bla' and that wouldn't do anything useful. Is it possible to just freeze those objects as well, is that a pattern that exists anywhere in the platform? @annevk @domenic

@foolip
Copy link
Member

foolip commented Jun 8, 2016

Also CC @doomdavve

@mounirlamouri
Copy link
Member

I don't think using dictionaries this way would be legal. To quote WebIDL: "Dictionaries must not be used as the type of an attribute, constant or exception field."

@@ -132,6 +132,9 @@ urlPrefix: http://www.w3.org/TR/page-visibility/; spec: PAGE-VISIBILITY
type: enum; urlPrefix: #pv-page-
text: visible
text: hidden
urlPrefix: http://www.w3.org/TR/appmanifest/; spec: appmanifest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to find good URL, filed w3c/manifest#469 instead.

In the meantime, can you use https here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@foolip
Copy link
Member

foolip commented Jun 8, 2016

I don't think using dictionaries this way would be legal. To quote WebIDL: "Dictionaries must not be used as the type of an attribute, constant or exception field."

Strictly speaking it wouldn't violate that, but it would be (unacceptably) weird if session.metadata.artwork[0].src = 'bla' changed the src property without that taking effect.

Since the internal object will have to be a copy of what is passed (with src resolved), perhaps we could just say in prose that these new objects are frozen?

<li>
If the image format is supported, use the image as the artwork for
display. (Otherwise the user agent can select another <a>prefered
artwork</a> and try again.)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's common practice to try again with another source when an image resource fails to decode or turns out to be unsupported. Is there precedent for this? In notifications, we just fall back to having nothing or a default resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we say "the user agent may have fallback behavior such as displaying an default image as artwork."

@domenic
Copy link

domenic commented Jun 8, 2016

is that a pattern that exists anywhere in the platform? @annevk @domenic

Yes, it is used by Notification.prototype.actions: https://notifications.spec.whatwg.org/#dom-notification-actions (but see also whatwg/notifications#74 for a wording tweak).

@mvano
Copy link

mvano commented Jun 8, 2016

For context, I'm making a similar change in the notifications spec: whatwg/notifications#76

@domenic: I think we could stop calling freeze (or SetIntegrityLevel) there by using an interface with readonly attributes instead of a dictionary.

@domenic
Copy link

domenic commented Jun 8, 2016

Yes, but that would be semantically different... The whole question is whether we should be creating dummy interfaces to work around how Web IDL makes it awkward to have frozen dictionary instances, or whether we should just use ES's primitives to reuse the dictionaries we already have.

artworks->artwork
several foolip's comments
added more links
@xxyzzzq xxyzzzq merged commit 869835c into w3c:master Jul 4, 2016
@xxyzzzq xxyzzzq deleted the artwork_icon branch July 4, 2016 15:54
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.

6 participants