VTTCue constructor should use 'double' type for startTime / endTime
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Feb 2018 19:36:47 +0000 (19:36 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Feb 2018 19:36:47 +0000 (19:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182988

Reviewed by Eric Carlson.

Source/WebCore:

VTTCue constructor should use 'double' type for startTime / endTime, not
'unrestricted double':
- https://w3c.github.io/webvtt/#the-vttcue-interface

Otherwise, we end up potentially returning NaN for TextTrackCue.startTime / endTime,
even though those correctly use type 'double':
- https://html.spec.whatwg.org/multipage/media.html#texttrackcue

The new behavior is consistent with Firefox and Chrome.

No new tests, updated existing test.

* bindings/js/JSDOMConvertNumbers.h:
(WebCore::JSConverter<IDLDouble>::convert):
Add assertion to make sure our implementation never tries to return NaN
for an IDL attribute of type 'double'. This would be invalid as per Web
IDL spec and would crash if the NaN being returned was impure as JSValue
could not store it as a double.

* html/track/VTTCue.idl:
Update constructor parameters to use 'double' type instead of 'unrestricted
double', as per:
- https://w3c.github.io/webvtt/#the-vttcue-interface

LayoutTests:

Update existing test to reflect behavior change.

* media/track/track-add-remove-cue-expected.txt:
* media/track/track-add-remove-cue.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@228886 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/media/track/track-add-remove-cue-expected.txt
LayoutTests/media/track/track-add-remove-cue.html
LayoutTests/media/track/track-cue-negative-timestamp-expected.txt
LayoutTests/media/track/track-cue-negative-timestamp.html
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMConvertNumbers.h
Source/WebCore/html/track/VTTCue.idl

index 97545b3..841d67d 100644 (file)
@@ -1,3 +1,15 @@
+2018-02-21  Chris Dumez  <cdumez@apple.com>
+
+        VTTCue constructor should use 'double' type for startTime / endTime
+        https://bugs.webkit.org/show_bug.cgi?id=182988
+
+        Reviewed by Eric Carlson.
+
+        Update existing test to reflect behavior change.
+
+        * media/track/track-add-remove-cue-expected.txt:
+        * media/track/track-add-remove-cue.html:
+
 2018-02-21  Ms2ger  <Ms2ger@gmail.com>
 
         Test gardening.
index 766397a..6834799 100644 (file)
@@ -48,13 +48,9 @@ EXPECTED (newCue.size == '100') OK
 EXPECTED (newCue.align == 'middle') OK
 
 *** Create an old-style cue with an id.
-RUN(oldStyleCue = new VTTCue('sausage-cue', 33, 3.4, 'Sausage?'))
-EXPECTED (oldStyleCue.id == '') OK
-EXPECTED (oldStyleCue.startTime.toString() == 'NaN') OK
-EXPECTED (oldStyleCue.endTime == '33') OK
-*** Make sure the old-style cue is not inserted because its start time is not a number.
-EXPECTED (testTrack.track.cues.length == '5') OK
-RUN(testTrack.track.addCue(oldStyleCue))
+RUN(invalidCue = new VTTCue('sausage-cue', 33, 3.4, 'Sausage?'))
+TypeError: The provided value is non-finite
+EXPECTED (window.invalidCue == 'undefined') OK
 EXPECTED (testTrack.track.cues.length == '5') OK
 
 *** Remove a cue created with addCue().
index 7d50cf3..0e38c77 100644 (file)
                 testExpected("newCue.align", "middle");
 
                 consoleWrite("<br>*** Create an old-style cue with an id.");
-                run("oldStyleCue = new VTTCue('sausage-cue', 33, 3.4, 'Sausage?')");
-                testExpected("oldStyleCue.id", "");
-                testExpected("oldStyleCue.startTime.toString()", "NaN");
-                testExpected("oldStyleCue.endTime", 33);
-                consoleWrite("*** Make sure the old-style cue is not inserted because its start time is not a number.");
-                testExpected("testTrack.track.cues.length", 5);
-                run("testTrack.track.addCue(oldStyleCue)");
+                run("invalidCue = new VTTCue('sausage-cue', 33, 3.4, 'Sausage?')");
+                testExpected("window.invalidCue", undefined);
+
                 testExpected("testTrack.track.cues.length", 5);
 
                 consoleWrite("<br>*** Remove a cue created with addCue().");
index cbf7872..72e5a5f 100644 (file)
@@ -3,13 +3,13 @@ Tests negative timestamps.
 
 Test that cues with negative startTime are not added:
 EXPECTED (testTrack.track.cues.length == '4') OK
-RUN(textCue = new VTTCue('sausage-cue', -3439332606, 3.4, 'Sausage?'))
+RUN(textCue = new VTTCue(-3439332606, 3.4, 'Sausage?'))
 RUN(testTrack.track.addCue(textCue))
 EXPECTED (testTrack.track.cues.length == '4') OK
 
 Test that cues with negative startTime and negative endTime are not added:
 EXPECTED (testTrack.track.cues.length == '4') OK
-RUN(textCue = new VTTCue('pepperoni-cue', -110, -3.4, 'Pepperoni?'))
+RUN(textCue = new VTTCue(-110, -3.4, 'Pepperoni?'))
 RUN(testTrack.track.addCue(textCue))
 EXPECTED (testTrack.track.cues.length == '4') OK
 
index a779922..b33def2 100644 (file)
             {
                 consoleWrite("Test that cues with negative startTime are not added:");
                 testExpected("testTrack.track.cues.length", 4);
-                run("textCue = new VTTCue('sausage-cue', -3439332606, 3.4, 'Sausage?')");
+                run("textCue = new VTTCue(-3439332606, 3.4, 'Sausage?')");
                 run("testTrack.track.addCue(textCue)");
                 testExpected("testTrack.track.cues.length", 4);
 
                 consoleWrite("<br>Test that cues with negative startTime and negative endTime are not added:");
                 testExpected("testTrack.track.cues.length", 4);
-                run("textCue = new VTTCue('pepperoni-cue', -110, -3.4, 'Pepperoni?')");
+                run("textCue = new VTTCue(-110, -3.4, 'Pepperoni?')");
                 run("testTrack.track.addCue(textCue)");
                 testExpected("testTrack.track.cues.length", 4);
 
index 2293972..93b1681 100644 (file)
@@ -1,3 +1,34 @@
+2018-02-21  Chris Dumez  <cdumez@apple.com>
+
+        VTTCue constructor should use 'double' type for startTime / endTime
+        https://bugs.webkit.org/show_bug.cgi?id=182988
+
+        Reviewed by Eric Carlson.
+
+        VTTCue constructor should use 'double' type for startTime / endTime, not
+        'unrestricted double':
+        - https://w3c.github.io/webvtt/#the-vttcue-interface
+
+        Otherwise, we end up potentially returning NaN for TextTrackCue.startTime / endTime,
+        even though those correctly use type 'double':
+        - https://html.spec.whatwg.org/multipage/media.html#texttrackcue
+
+        The new behavior is consistent with Firefox and Chrome.
+
+        No new tests, updated existing test.
+
+        * bindings/js/JSDOMConvertNumbers.h:
+        (WebCore::JSConverter<IDLDouble>::convert):
+        Add assertion to make sure our implementation never tries to return NaN
+        for an IDL attribute of type 'double'. This would be invalid as per Web
+        IDL spec and would crash if the NaN being returned was impure as JSValue
+        could not store it as a double.
+
+        * html/track/VTTCue.idl:
+        Update constructor parameters to use 'double' type instead of 'unrestricted
+        double', as per:
+        - https://w3c.github.io/webvtt/#the-vttcue-interface
+
 2018-02-21  Zalan Bujtas  <zalan@apple.com>
 
         [RenderTreeBuilder] Move RenderTextFragment::willBeRemoved() mutation logic to RenderTreeBuilder
index 756cd67..ee9c900 100644 (file)
@@ -360,6 +360,7 @@ template<> struct JSConverter<IDLDouble> {
 
     static JSC::JSValue convert(Type value)
     {
+        ASSERT(!std::isnan(value));
         return JSC::jsNumber(value);
     }
 };
index 24a4ca9..5d0459e 100644 (file)
@@ -25,7 +25,7 @@
 
 [
     Conditional=VIDEO_TRACK,
-    Constructor(unrestricted double startTime, unrestricted double endTime, DOMString text),
+    Constructor(double startTime, double endTime, DOMString text),
     ConstructorCallWith=ScriptExecutionContext,
     JSGenerateToJSObject,
     JSGenerateToNativeObject,