903030 - [webvtt] Add tests for TextTrackCue validation code

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

We need to have tests that test whether or not the TextTrackCue is validating it's data correctly.

Comment 1

[Marcus Saad (:msaad)]

Attachment: WIP 01

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.

Comment 2

[Rick Eyre (:reyre)]

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".

Comment 3

[Marcus Saad (:msaad)]

> 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.

Comment 4

[Rick Eyre (:reyre)]

(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

[Marcus Saad (:msaad)]

I misunderstood you, will be posting an update soon.

Comment 6

[Marcus Saad (:msaad)]

Attachment: 0001-Bug903030-Adding-Validation-tests-for-TextTrackCue.patch

Addressed Reyre's previous comments.

Comment 7

[Marcus Saad (:msaad)]

Attachment: WIP 03

Fixed up the patch and addressed the changes suggested.

Comment 8

[Rick Eyre (:reyre)]

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.

Comment 9

[Marcus Saad (:msaad)]

Attachment: WIP 04

Fixed the space that was accidentally removed.

Comment 10

[Marcus Saad (:msaad)]

Attachment: WIP 5

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!

Comment 11

[Rick Eyre (:reyre)]

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.

Comment 12

[Rick Eyre (:reyre)]

Marcus, do you still have time to do this? If not I can take it from you if you don't mind.

Comment 13

[Rick Eyre (:reyre)]

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.

Comment 14

[Rick Eyre (:reyre)]

Attachment: v1: Add tests for TextTrackCue validation code r=cpearce.

The code in the test could do with a major refactoring, but I think we should tackle that in bug 983182.

Comment 15

[Rick Eyre (:reyre)]

Try looks good https://tbpl.mozilla.org/?tree=Try&rev=a51633b44ea4

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c011937283fc

Comment 17

[[Back July 11] Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/c011937283fc

Comment 18

[Firefox Commit Bot]

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