Closed
Bug 1018933
Opened 10 years ago
Closed 10 years ago
[webvtt] Subtitles not displayed in web-platform-tests WebVTT reftests
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: jgraham, Assigned: reyre)
References
()
Details
Attachments
(2 files, 3 obsolete files)
2.68 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
Details | Diff | Splinter Review |
On Tue, May 27, 2014 at 8:18 PM, Richard Eyre <rick.eyre@hotmail.com> wrote: > I'm thinking that the 'honor user preferences for automatic track selection' > [1] algorithm is running after the script on the test page and resetting the > mode value to 'disabled' since there is no default attribute on the track. A > more fail safe way to do this would be to set a default attribute on the > track and the browser will enable the track itself. Otherwise, we need to > listen to 'loadedmetadata' or some other video event before setting the mode > through script. On Tue, May 27, 2014 at 2:36 PM, Philip Jägenstedt <philipj@opera.com>wrote: > The 'honor user preferences for automatic track selection' algorithm > can only enable tracks, it doesn't disable them. Also, "If any of the > text tracks in candidates have a text track mode set to showing, abort > these steps." It seems like it would be possible to change all the tests here to use a different mechanism for enabling the subtitles, but there is also a bug that needs to be fixed.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rick.eyre
Assignee | ||
Comment 1•10 years ago
|
||
I remember you showed me a site where you could run these tests, James. Could you point me towards them again?
Reporter | ||
Comment 2•10 years ago
|
||
http://w3c-test.org/webvtt/rendering/cues-with-video/processing-model/ Or, if you want to check them out and run locally, see the instructions at [1] (it's intended to be relatively straightforward). [1] https://github.com/w3c/web-platform-tests
Assignee | ||
Comment 3•10 years ago
|
||
Great, thanks James! I should be able to look into this this week.
Assignee | ||
Comment 4•10 years ago
|
||
The issue was that 'GetTrack()' was being called before we 'BindToTree' was being called. This caused an issue where two TextTracks would be created. The first one when 'GetTrack' was called from script, and the second when 'BindToTree' triggered the creation of a new Track. The reftests weren't working because two TextTracks were being added and the TextTrack the it was trying to manipulate for the tests was the incorrect one.
Attachment #8447691 -
Flags: review?(giles)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8447692 -
Flags: review?(giles)
Comment 6•10 years ago
|
||
Comment on attachment 8447692 [details] [diff] [review] Part 2: Add regression test Review of attachment 8447692 [details] [diff] [review]: ----------------------------------------------------------------- Looks correct to me, but I can't get the test to fail with current nightly. Can we make the regression more robust?
Attachment #8447692 -
Flags: review?(giles) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8447691 [details] [diff] [review] Part 1: HTMLTrackElement should not create another TextTrack if one already exists Review of attachment 8447691 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment nit addressed. ::: content/html/content/src/HTMLTrackElement.cpp @@ +229,5 @@ > } > > + // We may already have a TextTrack at this point if GetTrack() has already > + // been called. This happens, for instance, if script tries to get the > + // TextTrack before it's HTMLTrackElement has been bound to the DOM tree. s/it's HTMLTrackElement/its mTrackElement/ Or at least fix the 'its'.
Attachment #8447691 -
Flags: review?(giles) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8447692 -
Attachment is obsolete: true
Attachment #8449926 -
Flags: review?(giles)
Assignee | ||
Comment 9•10 years ago
|
||
This should be more robust. Can you confirm Ralph?
Attachment #8449926 -
Attachment is obsolete: true
Attachment #8449926 -
Flags: review?(giles)
Attachment #8449927 -
Flags: review?(giles)
Assignee | ||
Comment 10•10 years ago
|
||
Fixed up the code comment nits. Carrying forward r=rillian
Attachment #8447691 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
Comment on attachment 8449927 [details] [diff] [review] Part 2: Add regression test Review of attachment 8449927 [details] [diff] [review]: ----------------------------------------------------------------- This one fails for me without the fix. Thanks for finding a better test.
Attachment #8449927 -
Flags: review?(giles) → review+
Comment 12•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=14c7f93562c8
Assignee | ||
Comment 13•10 years ago
|
||
Thanks for review Ralph! Thanks for the try push as well, I pushed it last night though and that one looks green: https://tbpl.mozilla.org/?tree=Try&rev=68a888e6d704
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/49c338731007 https://hg.mozilla.org/integration/mozilla-inbound/rev/6460b4e4452b
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/49c338731007 https://hg.mozilla.org/mozilla-central/rev/6460b4e4452b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•