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
Changes from 5 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
81 changes: 81 additions & 0 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<MediaImage> artwork;
attribute FrozenArray<ChapterInformation> chapterInfo;
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
};

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

Choose a reason for hiding this comment

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

If we introduce a ChapterInformationInit, this could be a ChapterInformationInit instead (which would allow passing either ChapterInformationInit or ChapterInformation 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. Shall I also use ChapterInformationInit in the MediaMetadata interface declaration(L951)?

};
</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,74 @@ invoked, the user agent MUST run the following steps:
</ol>
</p>

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

<pre class="idl">

jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
dictionary ChapterInformation {
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 :]

attribute FrozenArray&lt;MediaImage> artwork;
};
</pre>

The <dfn dict-member for="ChapterInformation">title</dfn> <a>dictionary member</a> is
used to specify the {{ChapterInformation}} object's {{MediaImage/title}}.

The <dfn dict-member for="ChapterInformation">startTime</dfn> <a>dictionary member</a>
is used to specify the {{ChapterInformation}} object's {{MediaImage/startTime}} in
seconds. It should not be negative.
Copy link
Member

Choose a reason for hiding this comment

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

Avoid "should" and describe what MUST happen if it is negative.

This should be done in a ChapterInformation(init) constructor (similar to MediaMetadata(init)), which should throw TypeError on invalid input, as well as run 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.


The <dfn dict-member for="ChapterInformation">artwork</dfn> <a>dictionary member</a>
steimelchrome marked this conversation as resolved.
Show resolved Hide resolved
is used to specify the {{ChapterInformation}} object's {{MediaImage/artwork}}. On
getting, it MUST return the result of the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

Dictionary members cannot have getters or setters.

This section appears to conflate the dictionary members with the interface attributes, which is confusing. We should describe them separately like we do for MediaMetadata.

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 have changed it to an interface now as another comment suggested. I also followed the MediaMetadata 's description and rewrote this part in the latest commit :]

<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>frozenArtwork</var> be an empty list of type {{MediaImage}}.
Copy link
Member

Choose a reason for hiding this comment

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

Still kinda not great that we have ImageResource and MediaImage...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are they referring to the same thing? I see we are using MediaImage in MediaMetadata, so I also use it in the ChapterInformation.

</li>
<li>
For each <var>entry</var> in the {{ChapterInformation}}'s <a
for="ChapterInformation">artwork images</a>, perform the following steps:
<ol>
<li>
Let <var>image</var> be a new {{MediaImage}}.
Copy link
Member

@jan-ivar jan-ivar Jan 26, 2024

Choose a reason for hiding this comment

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

Creating new objects in a getter is an anti-pattern, especially in a frozen array. It violates § 6.1. Attributes should behave like data properties. See also crbug 94665.

I understand this is copied from https://w3c.github.io/mediasession/#dom-mediametadata-artwork but that is also wrong and redundant since the MediaMetadata(init) already runs "the convert artwork algorithm with init’s artwork as input and set metadata’s artwork images as the result if it succeeded."

IOW we ran convert (instantiated objects) on set, so no need to run it again (instantiating more objects) on every get. The attribute getter can simply return artwork images.

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:
"The artwork attribute reflects the ChapterInformation's artwork images. On getting, it MUST return the ChapterInformation's artwork. On setting, it MUST run the convert artwork algorithm with the new value as input, and set the ChapterInformation's artwork images as the result if it succeeded."
Let me know what you think.

</li>
<li>
Set <var>image</var>'s {{MediaImage/src}} to <var>entry</var>'s
{{MediaImage/src}}.
</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>
<!-- XXX IDL dictionaries are usually returned by value, so don't need
to be immutable. But FrozenArray reifies the dictionaries to mutable JS
objects accessed by reference, so we explicitly freeze them. It would be
better to do this with IDL primitives instead of JS - see
https://www.w3.org/Bugs/Public/show_bug.cgi?id=29004 -->
<li>
Call <a lt="freeze">Object.freeze</a> on <var>image</var>, to prevent
Copy link
Member

Choose a reason for hiding this comment

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

This seems not great... yeah, maybe this should be an interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have changed ChapterInformation to an interface in this pr. For MediaImage , it might be better to submit a separate pr under the corresponding issue.

accidental mutation by scripts.
</li>
<li>
Append <var>image</var> to <var>frozenArtwork</var>.
</li>
</ol>
</li>
<li>
<a>Create a frozen array</a> from <var>frozenArtwork</var>.
jiajiabingcheng marked this conversation as resolved.
Show resolved Hide resolved
</li>
</ol>

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.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be in the constructor 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.

Updated.



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

<pre class="idl">
Expand Down