Closed
Bug 903030
Opened 6 years ago
Closed 6 years ago
[webvtt] Add tests for TextTrackCue validation code
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Not set
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: reyre, Assigned: reyre)
References
Details
Attachments
(1 file, 5 obsolete files)
2.92 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
We need to have tests that test whether or not the TextTrackCue is validating it's data correctly.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee: nobody → mv.nsaad
Comment 1•6 years ago
|
||
This WIP only addresses the changes made on bug903425. Future patches will include validation for bug882299. -Added a new TextTrackCue -Validated enum ranged values such as "lr", "rl", "". -Validated that values outside of the enum range aren't accepted.
Attachment #803774 -
Flags: feedback?(rick.eyre)
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 803774 [details] [diff] [review] WIP 01 Review of attachment 803774 [details] [diff] [review]: ----------------------------------------------------------------- Other then that looks good :)! ::: content/media/test/test_texttrackcue.html @@ +81,5 @@ > is(cue.endTime, 4, "Cue's end time should be 4."); > is(cue.text, "foo", "Cue's text should be foo."); > > + // bug903425 - Validating DirectionSetting enum > + var ttcue = new TextTrackCue(5, 6, "Validating Vertical attribute."); Move this up and use 'cue' instead of creating a new cue. It should work the same. @@ +90,5 @@ > + ttcue.vertical = "rl"; > + is(ttcue.vertical, "rl", "TextTrackCue.Vertical should be rl (right to left)."); > + ttcue.vertical = "NonEnumValue"; > + isnot(ttcue.vertical, "NonEnumValue", "Values outside of the enum shouldn't be accepted."); > + is(ttcue.vertical, "rl", "After an invalid value, the last valid value for TextTrackCue.Vertical should be kept."); We can just check that ttcue.vertical is "rl" instead of checking that it's not "NonEnumValue" and then checking if it's "rl".
Attachment #803774 -
Flags: feedback?(rick.eyre) → feedback+
Comment 3•6 years ago
|
||
> Comment on attachment 803774 [details] [diff] [review]
> WIP 01
>
> Review of attachment 803774 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> We can just check that ttcue.vertical is "rl" instead of checking that it's
> not "NonEnumValue" and then checking if it's "rl".
By doing what you suggested, we wouldn't be testing if values that are outside of the enum are being ignored.
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Marcus Saad (:msaad) from comment #3) > By doing what you suggested, we wouldn't be testing if values that are > outside of the enum are being ignored. We still would be. Instead of checking if first that align != "NonEnumValue" and then checking that align == "rl" we can just check that it's still "rl". The fact that it's still "rl" means that it's not "NonEnumValue" and that everything worked the way it was supposed too. so: ttcue.align = "rl"; is(ttcue.align, "rl"); ttcue.align = "NonEnumValue"; is(ttcue.align, "rl", "Should still be rl after setting bogus value.");
Comment 5•6 years ago
|
||
I misunderstood you, will be posting an update soon.
Comment 6•6 years ago
|
||
Addressed Reyre's previous comments.
Attachment #803774 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #804127 -
Attachment filename: 0001-Bug903030-Adding-Validation-tests-for-TextTrackCue.patch → WIP 02
Comment 7•6 years ago
|
||
Fixed up the patch and addressed the changes suggested.
Attachment #804127 -
Attachment is obsolete: true
Attachment #804136 -
Flags: feedback?(rick.eyre)
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 804136 [details] [diff] [review] WIP 03 Review of attachment 804136 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/test/test_texttrackcue.html @@ -84,4 @@ > is(cue.startTime, 3.999, "Cue's start time should be 3.999."); > is(cue.endTime, 4, "Cue's end time should be 4."); > is(cue.text, "foo", "Cue's text should be foo."); > - Keep this empty line here please.
Attachment #804136 -
Flags: feedback?(rick.eyre) → feedback+
Comment 9•6 years ago
|
||
Fixed the space that was accidentally removed.
Attachment #804136 -
Attachment is obsolete: true
Comment 10•6 years ago
|
||
Rick, I've added validation for bug882299 too. Spec is a little messed up as we all know, but I did my best to understand the line algorithm. It depends on the values that snapToLines are set. Thanks for your feedback!
Attachment #805277 -
Attachment is obsolete: true
Attachment #8347655 -
Flags: feedback?(rick.eyre)
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8347655 [details] [diff] [review] WIP 5 Review of attachment 8347655 [details] [diff] [review]: ----------------------------------------------------------------- Looking good do far, but we also need to test position, size, align, and getCueAsHTML() as well. ::: content/media/test/test_texttrackcue.html @@ +58,5 @@ > + is(cue.vertical, "rl", "VTTCue.Vertical should be rl (right to left)."); > + cue.vertical = "NonEnumValue"; > + is(cue.vertical, "rl", "Should still be rl after setting arbitrary value."); > + // bug882299 - Validating Long or Autokeyword union of TextTrackCue.Line > + is(cue.line, "auto", "VTTCue.line should be initiated with autokeyword"); I think we shouldn't add the line tests until bug 882299 lands as line won't even work until then. I can take care of landing those tests when I get the patch ready. I can do snapToLines at that time too as there closely related.
Attachment #8347655 -
Flags: feedback?(rick.eyre) → feedback-
Assignee | ||
Comment 12•6 years ago
|
||
Marcus, do you still have time to do this? If not I can take it from you if you don't mind.
Assignee | ||
Comment 13•6 years ago
|
||
I'm going to work on this since I haven't heard back from Marcus in a while and we need to get some more test coverage since the pref is being turned on soon.
Assignee: mv.nsaad → rick.eyre
Assignee | ||
Comment 14•6 years ago
|
||
The code in the test could do with a major refactoring, but I think we should tackle that in bug 983182.
Attachment #8347655 -
Attachment is obsolete: true
Attachment #8399596 -
Flags: review?(cpearce)
Updated•6 years ago
|
Attachment #8399596 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 15•6 years ago
|
||
Try looks good https://tbpl.mozilla.org/?tree=Try&rev=a51633b44ea4
Keywords: checkin-needed
Comment 16•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c011937283fc
Keywords: checkin-needed
Comment 17•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c011937283fc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 18•6 years ago
|
||
The following changeset is now in Firefox Nightly: > c011937283fc Bug 903030 - Add tests for TextTrackCue validation code. r=cpearce Nightly Build Information: ID: 20140402030201 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central Download Links: > Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2 > Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2 > Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2 > Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg > Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe > Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe Previous Nightly Build Information: ID: 20140401030203 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
You need to log in
before you can comment on or make changes to this bug.
Description
•