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

Should it be possible to set the MediaMetadata attributes? #116

Closed
foolip opened this issue Dec 7, 2015 · 14 comments
Closed

Should it be possible to set the MediaMetadata attributes? #116

foolip opened this issue Dec 7, 2015 · 14 comments

Comments

@foolip
Copy link
Member

foolip commented Dec 7, 2015

The metadata proposal in #115 is one where MediaMetadata object are immutable, once created they can only be passed around.

The main reason for this is so that there can be no question about what the following does:

var session = new MediaSession();
session.metadata = new MediaMetadata({title: 'foo' });
session.metadata.title = 'bar'; // no-op if title is readonly

Making the attributes mutable would admittedly make the code simpler if only a single field has to be updated, as otherwise one has to keep the MediaMetadataInit dictionary object around, mutate that, and create a new MediaMetadata object.

If the attributes are mutable, a MediaMetadata object would need a way to notify the MediaSession object on which it is set, so that it updates the metadata at the next microtask checkpoint, or similar.

Issue opened in response to feedback from @mounirlamouri in #115 (diff)

@foolip
Copy link
Member Author

foolip commented Dec 7, 2015

@annevk @domenic, this is a bit of an API design question, do either of you have any thoughts on this?

@domenic
Copy link

domenic commented Dec 7, 2015

I'd say both designs are valid. As usual the mutable version adds more complexity, both for implementation and for authors to keep track of. But it does give some convenience.

I guess I would add mutability and a corresponding pointer back to the parent MediaSession object only if I expected it to be a common use case to update a single property at once. I'm not really sure whether that's true in general. Although you could imagine only updating title if playing an album from the same artist, in practice I think the code will more likely be resetting the metadata on every new track.

@foolip
Copy link
Member Author

foolip commented Dec 7, 2015

One of the more plausible cases where you would update only the title could be an audiobook with many chapters. You might not have separate audio files for each chapter, just a time cue to update the in-page and external UI.

I'm happy to leave this issue open and seek out feedback on this when developers get to play with the API.

@mounirlamouri
Copy link
Member

I'm not sure why this would be more complex for authors: it gives them more flexibility which they can use if they want to.

Regarding implementation complexity, I think we could say that the UA could start a microtask to update the metadata so we don't end up updating them x times.

@beaufortfrancois
Copy link
Contributor

beaufortfrancois commented May 13, 2016

The microtask @mounirlamouri suggested seems the right thing to do there. I even would remove the MediaMetadata object so have something like that:

function updateMetadata(event) {
  sharedSession.metadata.title = event.target == audio1 ? "Chapter 1" : "Chapter 2";
}

audio1.addEventListener("play", updateMetadata);
audio2.addEventListener("play", updateMetadata); 

@foolip
Copy link
Member Author

foolip commented May 13, 2016

sharedSession.metadata would still have to be a MediaMetadata object or similar, if it's just a plain object then there's no reasonable way for the implementation to even know that an attribute was changed, and feature detection would not be possible.

What I think is not good about having sharedSession.metadata be a constant object is that it becomes a lot less clean to replace the metadata with something different, as you have to reset all attributes to their original state, and that means you have to know what all the attributes are.

@beaufortfrancois
Copy link
Contributor

I understand your point about resetting the session.
Could we at least give the option to the developer to update only what is needed if wanted however?

@mounirlamouri
Copy link
Member

mounirlamouri commented May 18, 2016

I'm not entirely sure I followed.

What I would imagine is something like:

partial interface MediaSession {
  attribute MediaMetadata? metadata;
};

interface MediaMetadata {
  attribute DOMString title;
  attribute DOMString artist;
  attribute DOMString album;
};

(Note: I would see a benefit in having DOMString?).

Reset could be done with session.metadata = null;. Ideally, one could reset each property by setting it to null too. What would be the downside of this approach?

@beaufortfrancois
Copy link
Contributor

Nice! DomString? would be cool as well!
And with #129, it would be something like:

partial interface MediaSession {
  attribute MediaMetadata? metadata;
};

interface MediaMetadata {
  attribute DOMString? title;
  attribute DOMString? artist;
  attribute DOMString? album;
  attribute FrozenArray<MediaArtwork>? artworks;
};

and changing artwork would be as simple as:

sharedMediaSession.metadata.artworks = [{ src: "podcast.jpg" }];

@mounirlamouri
Copy link
Member

@foolip @doomdavve would you be fine with this?

@foolip
Copy link
Member Author

foolip commented Jun 9, 2016

My preference is to let MediaMetadata objects be entirely immutable and add that extra complexity only once web developers complain about the ergonomics of the API.

As for DOMString?, isn't the empty string sufficient? That's the most common way of representing a missing value in web APIs I think.

There might be an extra problem with having artwork be mutable. https://heycam.github.io/webidl/#idl-frozen-array doesn't say, but what's supposed to happen when you set a FrozenArray<T> property? A sequence<T> in an init dict is at least a pattern I know and understand.

@mounirlamouri
Copy link
Member

@beaufortfrancois WDYT of following the proposal @foolip made in the comment above?

@beaufortfrancois
Copy link
Contributor

My preference is to let MediaMetadata objects be entirely immutable and add that extra complexity only once web developers complain about the ergonomics of the API.

The "extra" complexity added seems to me less than the convenience it would bring to web developers.
Especially if artwork takes time to download on a spotty connection, I'd be happy to set MediaMetadata as soon as possible and update artwork solely later when resource comes back with mediaSession.metadata.artworks = [{ src: "artwork.jpg" }];

@mounirlamouri mounirlamouri added P2 and removed P3 labels Dec 15, 2016
@mounirlamouri mounirlamouri self-assigned this Dec 15, 2016
MXEBot pushed a commit to mirror/chromium that referenced this issue Dec 17, 2016
Spec discussion: w3c/mediasession#116

BUG=674710
R=zqzhang@chromium.org

Review-Url: https://codereview.chromium.org/2584703002
Cr-Commit-Position: refs/heads/master@{#439207}
@mounirlamouri
Copy link
Member

Fixed by #156

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

No branches or pull requests

5 participants