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 index.bs with ChapterInformation #308

Merged
merged 48 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
2e39d54
Update index.bs with ChapterInformation
jiajiabingcheng Dec 13, 2023
40b6030
Update index.bs
jiajiabingcheng Dec 14, 2023
1c90b91
Update index.bs
jiajiabingcheng Dec 14, 2023
58fcc52
Update index.bs resolve comments
jiajiabingcheng Dec 15, 2023
ac87a02
Update index.bs
jiajiabingcheng Dec 15, 2023
690f342
Update index.bs: add examples
jiajiabingcheng Dec 20, 2023
af180ff
Update explainer.md
jiajiabingcheng Dec 20, 2023
bf940ad
Update index.bs
jiajiabingcheng Dec 20, 2023
9d60ac7
Update index.bs
jiajiabingcheng Jan 23, 2024
3e3ee8c
Update index.bs
jiajiabingcheng Jan 23, 2024
283feab
Update explainer.md
jiajiabingcheng Jan 23, 2024
2869d20
Update explainer.md
jiajiabingcheng Jan 24, 2024
a1825ea
Update index.bs
jiajiabingcheng Jan 24, 2024
140971c
Update index.bs
jiajiabingcheng Jan 25, 2024
27cc4db
Update index.bs
jiajiabingcheng Jan 25, 2024
a43f58f
Update explainer.md
jiajiabingcheng Jan 25, 2024
7690976
Update index.bs
jiajiabingcheng Jan 26, 2024
edeaf6d
Update index.bs
jiajiabingcheng Jan 27, 2024
9edbfae
Update index.bs
jiajiabingcheng Jan 27, 2024
7209afa
Update index.bs
jiajiabingcheng Jan 27, 2024
75b6dac
Update index.bs
jiajiabingcheng Jan 27, 2024
7a1501e
Update index.bs
jiajiabingcheng Jan 27, 2024
6fbcc9a
Update explainer.md
jiajiabingcheng Jan 30, 2024
9d365de
Update explainer.md
jiajiabingcheng Jan 30, 2024
7b13db7
Update index.bs
jiajiabingcheng Jan 30, 2024
f93c6a8
Update index.bs
jiajiabingcheng Jan 30, 2024
49d55ec
Update index.bs
jiajiabingcheng Jan 30, 2024
37f5f38
Update index.bs
jiajiabingcheng Jan 30, 2024
b1d9923
Update index.bs
jiajiabingcheng Jan 31, 2024
9271fe4
Update explainer.md
jiajiabingcheng Feb 6, 2024
4234faa
Update index.bs
jiajiabingcheng Feb 6, 2024
1a9cd3a
Update index.bs
jiajiabingcheng Feb 6, 2024
0dcf14e
Update index.bs
jiajiabingcheng Feb 6, 2024
95ffe0b
Update index.bs
jiajiabingcheng Feb 6, 2024
4a1cb19
Update index.bs
jiajiabingcheng Feb 6, 2024
adde728
Update index.bs
jiajiabingcheng Feb 6, 2024
ea885c3
Update explainer.md
jiajiabingcheng Feb 6, 2024
9959bcf
Update index.bs
jiajiabingcheng Feb 6, 2024
8bf0611
Update index.bs
jiajiabingcheng Feb 13, 2024
76e40d7
Update index.bs
jiajiabingcheng Feb 22, 2024
1b8b564
Update index.bs
jiajiabingcheng Feb 22, 2024
966b2cc
Update explainer.md
jiajiabingcheng Feb 22, 2024
009a61b
Update explainer.md
jiajiabingcheng Feb 22, 2024
ae06c70
Update index.bs
jiajiabingcheng Feb 23, 2024
2100bfc
Update index.bs
jiajiabingcheng Feb 23, 2024
3f4cfa1
Update index.bs
jiajiabingcheng Feb 23, 2024
63c64c7
Update index.bs
jiajiabingcheng Mar 7, 2024
dbf1354
Update explainer.md
jiajiabingcheng Mar 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions explainer.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ interface MediaSession : EventTarget {
### The `MediaMetadata` interface

A `MediaMetadata` object can contain media metadata like title, artist, album
and album art. To set the metadata for a `MediaSession`, the page should create
a `MediaMetadata` object and assign it to a `MediaSession` object:
album art and video chapter information. To set the metadata for a `MediaSession`,
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
the page should create a `MediaMetadata` object and assign it to a `MediaSession`
object:

```javascript
navigator.mediaSession.metadata = new MediaMetadata(/* MediaMetadata constructor */);
Expand All @@ -123,13 +124,20 @@ interface MediaMetadata {
attribute DOMString artist;
attribute DOMString album;
attribute FrozenArray<MediaImage> artwork;
readonly attribute FrozenArray<ChapterInformation> chapterInfo;
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
};

dictionary MediaImage {
required USVString src;
DOMString sizes = "";
DOMString type = "";
};

interface ChapterInformation {
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
attribute DOMString title;
attribute double startTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these two be readonly?

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.

readonly attribute FrozenArray<MediaImage> artwork;
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
};
```

### The `MediaPositionState` dictionary
Expand Down
205 changes: 203 additions & 2 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -940,13 +940,15 @@ interface MediaMetadata {
attribute DOMString artist;
attribute DOMString album;
attribute FrozenArray&lt;MediaImage> artwork;
[SameObject] readonly attribute FrozenArray&lt;ChapterInformation> chapterInfo;
};

dictionary MediaMetadataInit {
DOMString title = "";
DOMString artist = "";
DOMString album = "";
sequence&lt;MediaImage> artwork = [];
sequence&lt;ChapterInformationInit> chapterInfo = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see ChapterInformationInit being defined 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.

It is defined at L1184, can I use it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole section is somehow redundant with web idl.
Since we use ChapterInformationInit, it seems to make sense to add it there as well, since it seems this would be the only one missing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add ChapterInformationInit for consistency here

};
</pre>

Expand All @@ -972,6 +974,11 @@ dictionary MediaMetadataInit {
images</dfn>.
</p>

<p>
A {{MediaMetadata}} has an associated list of <dfn for="MediaMetadata">
chapter information</dfn>.
</p>

<p>
A {{MediaMetadata}} is said to be an <dfn>empty metadata</dfn> if it is equal
to <code>null</code> or all the following conditions are true:
Expand All @@ -981,6 +988,8 @@ dictionary MediaMetadataInit {
<li>Its <a for=MediaMetadata>album</a> is the empty string.</li>
<li>Its <a for=MediaMetadata title='artwork image'>artwork images</a> length
is <code>0</code>.</li>
<li>Its <a for=MediaMetadata>chapter information</a> length
is <code>0</code>.</li>
</ul>
</p>

Expand Down Expand Up @@ -1011,6 +1020,10 @@ dictionary MediaMetadataInit {
<var>metadata</var>'s <a for="MediaMetadata">artwork images</a> as the
result if it succeeded.
</li>
<li>
Set <var>metadata</var>'s {{MediaMetadata/chapterInfo}} to
steimelchrome marked this conversation as resolved.
Show resolved Hide resolved
<var>init</var>'s {{MediaMetadataInit/chapterInfo}}.
Copy link
Member

Choose a reason for hiding this comment

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

Forgot an important piece... this is where we need to instantiate ChapterInformation objects, basically walk the sequence of chapters in the dictionary and create a list of ChapterInformation objects from it, then freeze that list and assign {{MediaMetadata/chapterInfo}} to the result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "Create a list of ChapterInformation from the sequence of chapters. Then freeze this list and set it to metadata’s chapterInfo to init’s chapterInfo."

WDYT?

</li>
<li>
Return <var>metadata</var>.
</li>
Expand Down Expand Up @@ -1157,6 +1170,180 @@ invoked, the user agent MUST run the following steps:
</ol>
</p>

<h2 id="the-chapterinformation-interface">The {{ChapterInformation}} interface</h2>

<pre class="idl">
interface ChapterInformation {
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
constructor(optional ChapterInformationInit init = {});
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
readonly attribute DOMString title;
readonly attribute double startTime;
[SameObject] readonly attribute FrozenArray&lt;MediaImage> artwork;
};

dictionary ChapterInformationInit {
DOMString title = "";
double startTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there are two chapter information with the same startTime?
Should we define which one is selected using the sequence order?
Also, it seems we somehow expect that the web page provides a sequence ordered by startTime but I would guess it is fine to not do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is unlikely that there will be more than one chapter with the same start time. However, if this does occur, we can simply use the chapter list as normal and sort it by start time after we have obtained it. Because the image sources are associated with each chapterInformation, there should be no problems regardless of the order in the list. Then in the UI, some chapters with the same start time will be displayed next to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this is an edge case, let's make sure that even these corner cases get implemented in an interoperable manner. So if we have to use list order to disambiguate, let's define it 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.

I'm thinking the website should be able to set whatever information is with the media. Even if there are two or more chapters with the same start time, there should be no order from them because they all refer to the same point in the video. Also, would it be better to leave this up to the client side? When the client receives a list of chapters, they can order them by start time first. Then, it is up to them whether or not to display them in any order. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey youennf, we're moving forward with submitting this pull request. Please feel free to open an issue later if you still have any additional questions about this change. Thanks :]

sequence&lt;MediaImage> artwork = [];
};

</pre>

<p>
A {{ChapterInformation}} object is a representation of metadata for an individual
chapter, such as the title of the section, its timestamp, and screenshot image data
of this section, that can be used by user agents to provide a customized user interface.
</p>

<p>
A {{ChapterInformation}} can have an associated <dfn for="ChapterInformation">
MediaMetadata</dfn>.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

This back pointer appears unused. Do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to media metadata.


<p>
A {{ChapterInformation}} has an associated <dfn for="ChapterInformation">title</dfn>
which is DOMString.
</p>

<p>
A {{ChapterInformation}} has an associated <dfn for="ChapterInformation">
startTime</dfn> which is double.
</p>

<p>
A {{ChapterInformation}} has an associated list of <dfn for="ChapterInformation">
artwork images</dfn>.
</p>

<p>
A {{ChapterInformation}} is said to be an <dfn>empty chapterInfo</dfn> if it is equal
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used? Did you mean to call it from empty metadata to make the following qualify as empty?

navigator.mediaSession.metadata = new MediaMetadata({chapterInfo: [{}]});

It might be better to not define an empty chapter since skipping a chapter affects the number of chapters.

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.

to `null` or all the following conditions are true:
<ul>
<li>Its <a for=ChapterInformation>title</a> is the empty string.</li>
<li>Its <a for=ChapterInformation>startTime</a> is <code>0</code>.</li>
<li>Its <a for=ChapterInformation title='artwork image'>artwork images</a> length
is <code>0</code>.</li>
</ul>
</p>

<p>
The <dfn constructor
for="ChapterInformation">ChapterInformation(<var>init</var>)</dfn>
constructor, when invoked, MUST run the following steps:
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved

<ol>
<li>
Copy link
Member

Choose a reason for hiding this comment

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

Do input validation here. I.e. throw TypeError on negative startTime.

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.

Copy link
Member

Choose a reason for hiding this comment

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

... and NaN (similar to the steps for setPositionState)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @chrisn , this part has been deleted and rewritten. Can you PTAL at the latest patchset?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right, it's unrestricted double that doesn't. Sorry for the noise.

Let <var>chapterInfo</var> be a new {{ChapterInformation}} object.
</li>
<li>
Set <var>chapterInfo</var>'s {{ChapterInformation/title}} to <var>init</var>'s
{{ChapterInformationInit/title}}.
</li>
<li>
Set <var>chapterInfo</var>'s {{ChapterInformation/startTime}} to <var>init</var>'s
{{ChapterInformationInit/startTime}}.
</li>
<li>
Run the <a>convert artwork algorithm</a> with <var>init</var>'s
{{ChapterInformationInit/artwork}} as <var>input</var> and set
Copy link
Member

Choose a reason for hiding this comment

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

This looks good but requires fixing the convert artwork algorithm algorithm to expect artwork instead of input (right now it still says "For each entry in input’s artwork, perform the following steps:") (plus fixing the other call to that algorithm).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we rephrase this paragraph to:
"Run the convert artwork algorithm with init’s artwork as input and create a frozen array from the result, then set it to chapterInfo’s artwork images." ?

Do we need to change the other places? For this sentence "When the convert artwork algorithm with input parameter is invoked, the user agent MUST run the following steps:", isn't input the init's artwork list which need to be parsed with the algorithm? And then it can generate the output list?

Copy link
Member

@jan-ivar jan-ivar Feb 23, 2024

Choose a reason for hiding this comment

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

Clicking on the link in "For each entry in input’s artwork" reveals the expected type of input to be MediaMetadataInit. I.e. A's B is a deref. To take a sequence, e.g. artwork instead of {artwork}, it needs to say "For each entry in input".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "For each entry in input (which is a MediaImage list), perform the following steps:"

<var>chapterInfo</var>'s <a for="ChapterInformation">artwork images</a> as the
result if it succeeded.
Copy link
Member

Choose a reason for hiding this comment

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

Set artwork images to the result of creating a frozen array from the result of the convert artwork algorithm.

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.

</li>
<li>
Return <var>chapterInfo</var>.
</li>
</ol>
</p>

When the <dfn>convert artwork algorithm</dfn> with <var>input</var> parameter is
invoked, the user agent MUST run the following steps:
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
<ol>
<li>
Let <var>output</var> be an empty list of type {{MediaImage}}.
</li>
<li>
For each <var>entry</var> in <var>input</var>'s
{{ChapterInformationInit/artwork}}, perform the following steps:
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
<ol>
<li>
Let <var>image</var> be a new {{MediaImage}}.
Copy link
Member

Choose a reason for hiding this comment

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

Please file an issue about MediaImage and leave a <!-- TODO: #issuenumber --> 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.

Can I use this issue number: #237 ?

</li>
<li>Let <var>baseURL</var> be the API base URL specified by the <a>entry
settings object</a>. </li>
<li>
<a lt="url parser">Parse</a> <var>entry</var>'s {{MediaImage/src}} using
<var>baseURL</var>. If it does not return failure, set
<var>image</var>'s {{MediaImage/src}} to the return value. Otherwise,
throw a <a exception>TypeError</a> and abort these steps.
</li>
<li>
Set <var>image</var>'s {{MediaImage/sizes}} to <var>entry</var>'s
{{MediaImage/sizes}}.
</li>
<li>
Set <var>image</var>'s {{MediaImage/type}} to <var>entry</var>'s
{{MediaImage/type}}.
</li>
<li>
Append <var>image</var> to the <var>output</var>.
</li>
</ol>
</li>
<li>
Return <var>output</var> as result.
</li>
</ol>

<p>
The <dfn attribute for="ChapterInformation">title</dfn> attribute
reflects the {{ChapterInformation}}'s <a for=ChapterInformation>title</a>. On getting,
it MUST return the {{ChapterInformation}}'s <a for=ChapterInformation>title</a>. On
setting, it MUST set the {{ChapterInformation}}'s <a for=ChapterInformation>title</a> to
the given value.
</p>

<p>
The <dfn attribute for="ChapterInformation">startTime</dfn> attribute
reflects the {{ChapterInformation}}'s <a for=ChapterInformation>startTime</a>. On getting,
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
it MUST return the {{ChapterInformation}}'s <a for=ChapterInformation>startTime</a>. On
setting, it MUST set the {{ChapterInformation}}'s <a for=ChapterInformation>startTime</a>
to the given value. If the <a for=ChapterInformation>startTime</a> is negative or greater than
<a dict-member for="MediaPositionState">duration</a>, throw a <a exception>TypeError</a>
Copy link
Member

Choose a reason for hiding this comment

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

Remove setter from readonly attribute.

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.

</p>

<p>
The <dfn attribute for="ChapterInformation">artwork</dfn>
attribute reflects the {{ChapterInformation}}'s <a for="ChapterInformation">artwork
images</a>. On getting, it MUST return the {{ChapterInformation}}'s <a for=ChapterInformation>
artwork</a>. On setting, it MUST run the <a>convert artwork algorithm</a> with the new value as
<var>input</var>, and set the {{ChapterInformation}}'s <a for="ChapterInformation">artwork
images</a> as the result if it succeeded.
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
</p>

<p>
When {{ChapterInformation}}'s <a for=ChapterInformation>title</a>, <a
for=ChapterInformation>startTime</a> or <a for=ChapterInformation>artwork images</a>
are modified, the user agent MUST run the following steps:
<ol>
<li>
If the instance has no associated [=ChapterInformation/media metadata=],
abort these steps.
</li>
<li>
Otherwise, <a>queue a task</a> to run the following substeps:
<ol>
<li>
If the instance no longer has an associated <a for=ChapterInformation>media
metadata</a>, abort these steps.
</li>
<li>
Otherwise, <a>in parallel</a>, run the <a>update metadata
algorithm</a>.
Copy link
Member

Choose a reason for hiding this comment

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

Queuing a task per chapter doesn't make sense to me. I would think this instead needs to be folded under the umbrella of MediaMetadata where this was copied from, around here, but that language doesn't make sense to me either:

image

...because these internal slots are never modified once initialized. IOW, the change doesn't happen here:

const newMetadata = new MediaMetadata({
  title: "Episode Title",
  artist: "Podcast Host",
  album: "Podcast Title",
  artwork: [{src: "podcast.jpg"}],
  chapterInfo: [
    {title: "Chapter 1", startTime: 0, artwork: [{src: "chapter1.jpg"}]},
    {title: "Chapter 2", startTime: 120, artwork: [{src: "chapter2.jpg"}]}
  ]
});

...but here:

navigator.mediaSession.metadata = newMetadata;

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.

</li>
</ol>
</li>
</ol>
</p>

<h2 id="the-mediaimage-dictionary">The {{MediaImage}} dictionary</h2>

<pre class="idl">
Expand Down Expand Up @@ -1300,14 +1487,18 @@ If disabled in the document, the User Agent MUST NOT select the document's media
title: "Episode Title",
artist: "Podcast Host",
album: "Podcast Title",
artwork: [{src: "podcast.jpg"}]
artwork: [{src: "podcast.jpg"}],
chapterInfo: [
{title: "Chapter 1", startTime: 0, artwork: [{src: "chapter1.jpg"}]},
{title: "Chapter 2", startTime: 120, artwork: [{src: "chapter2.jpg"}]}
]
});
</pre>

Alternatively, providing multiple <a for="MediaMetadata" title="artwork
image">artwork images</a> in the metadata can let the user agent be able to
select different artwork images for different display purposes and better fit
for different screens:
for different screens (the same for the artwork in {{MediaMetadata/chapterInfo}}):

<pre class="lang-javascript">
navigator.mediaSession.metadata = new MediaMetadata({
Expand All @@ -1321,6 +1512,16 @@ If disabled in the document, the User Agent MUST NOT select the document's media
{src: "podcast.png", sizes: "128x128", type: "image/png"},
{src: "podcast_hd.png", sizes: "256x256", type: "image/png"},
{src: "podcast.ico", sizes: "128x128 256x256", type: "image/x-icon"}
],
chapterInfo: [
{title: "Chapter 1", startTime: 0, artwork: [
{src: "chapter1_a.jpg", sizes: "128x128", type: "image/jpeg"},
{src: "chapter1_b.png", sizes: "256x256", type: "image/png"}
]},
{title: "Chapter 2", startTime: 120, artwork: [
{src: "chapter2_a.jpg", sizes: "128x128", type: "image/jpeg"},
{src: "chapter2_b.png", sizes: "256x256", type: "image/png"}
]}
]
});
</pre>
Expand Down