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 36 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.
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.
Do input validation here. I.e. throw TypeError on negative startTime.
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.
... and NaN (similar to the steps for
setPositionState
)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.
Hi @chrisn , this part has been deleted and rewritten. Can you PTAL at the latest patchset?
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.
https://webidl.spec.whatwg.org/#js-double already throws on NaN
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.
Ah, you're right, it's
unrestricted double
that doesn't. Sorry for the noise.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 looks good but requires fixing the convert artwork algorithm algorithm to expect
artwork
instead ofinput
(right now it still says "For each entry in input’s artwork, perform the following steps:") (plus fixing the other call to that 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.
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?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.
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".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.
Changed to "For each entry in input (which is a MediaImage list), perform the following steps:"
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 is good as is I think but it could be made slightly clearer by splitting in two steps:
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.