Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update index.bs with ChapterInformation #308
Changes from 13 commits
2e39d54
40b6030
1c90b91
58fcc52
ac87a02
690f342
af180ff
bf940ad
9d60ac7
3e3ee8c
283feab
2869d20
a1825ea
140971c
27cc4db
a43f58f
7690976
edeaf6d
9edbfae
7209afa
75b6dac
7a1501e
6fbcc9a
9d365de
7b13db7
f93c6a8
49d55ec
37f5f38
b1d9923
9271fe4
4234faa
1a9cd3a
0dcf14e
95ffe0b
4a1cb19
adde728
ea885c3
9959bcf
8bf0611
76e40d7
1b8b564
966b2cc
009a61b
ae06c70
2100bfc
3f4cfa1
63c64c7
dbf1354
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :]
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :]
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
inMediaMetadata
, so I also use it in theChapterInformation
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. ForMediaImage
, it might be better to submit a separate pr under the corresponding issue.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.