Bug 103413 - TextTrackCue's .endTime property should fire a TypeError when NaN is assigned
Summary: TextTrackCue's .endTime property should fire a TypeError when NaN is assigned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://w3c-test.org/html/tests/submis...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-11-27 08:14 PST by Antoine Quint
Modified: 2012-12-05 07:26 PST (History)
8 users (show)

See Also:


Attachments
Patch (16.15 KB, patch)
2012-12-04 08:16 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (16.02 KB, patch)
2012-12-05 06:20 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2012-11-27 08:14:53 PST
We're failing the Opera-submitted test at http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackCue/endTime.html due to not triggering a TypeError when trying to set .endTime to NaN, which is not an appropriate value.
Comment 1 Radar WebKit Bug Importer 2012-11-27 08:15:07 PST
<rdar://problem/12758008>
Comment 2 Antoine Quint 2012-11-27 08:29:42 PST
We run into the same issue for .startTime as shown in http://w3c-test.org/html/tests/submission/Opera/media/interfaces/TextTrackCue/startTime.html.
Comment 3 Antoine Quint 2012-12-04 08:16:51 PST
Created attachment 177486 [details]
Patch
Comment 4 Eric Carlson 2012-12-04 08:54:22 PST
Comment on attachment 177486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177486&action=review

> Source/WebCore/html/track/TextTrackCue.cpp:273
> +    if (WTF::double_conversion::Double(value).IsSpecial()) {

I don't see Double() being used to test this anywhere else in WebCore, and while I don't know enough about it to know that it is a problem I would rather we stick with the more commonly used isinf() and isnan(). Note that you don't need to test for negative vs. positive infinity as inf() returns true for either (eg. see MediaTime::createWithDouble).

> Source/WebCore/html/track/TextTrackCue.cpp:290
> +    if (WTF::double_conversion::Double(value).IsSpecial()) {

Ditto.

> LayoutTests/media/track/opera/interfaces/TextTrackCue/endTime-expected.txt:3
> +PASS TextTrackCue.endTime, script-created cue 
> +TIMEOUT TextTrackCue.endTime, parsed cue Test timed out

TIMEOUT? ChangeLog should note this if it is intentional.

> LayoutTests/media/track/opera/interfaces/TextTrackCue/startTime-expected.txt:3
> +PASS TextTrackCue.startTime, script-created cue 
> +TIMEOUT TextTrackCue.startTime, parsed cue Test timed out

Ditto.
Comment 5 Antoine Quint 2012-12-05 06:11:42 PST
(In reply to comment #4)
> (From update of attachment 177486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177486&action=review
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:273
> > +    if (WTF::double_conversion::Double(value).IsSpecial()) {
> 
> I don't see Double() being used to test this anywhere else in WebCore, and while I don't know enough about it to know that it is a problem I would rather we stick with the more commonly used isinf() and isnan(). Note that you don't need to test for negative vs. positive infinity as inf() returns true for either (eg. see MediaTime::createWithDouble).

Will adopt the isnan() and is infinite() methods from wtf/MathExtras.h in an updated patch.

> > LayoutTests/media/track/opera/interfaces/TextTrackCue/endTime-expected.txt:3
> > +PASS TextTrackCue.endTime, script-created cue 
> > +TIMEOUT TextTrackCue.endTime, parsed cue Test timed out
> 
> TIMEOUT? ChangeLog should note this if it is intentional.

It is intentional and the LayoutTests/ChangeLog mentioned the blocking bug (https://bugs.webkit.org/show_bug.cgi?id=103258).
Comment 6 Antoine Quint 2012-12-05 06:20:43 PST
Created attachment 177737 [details]
Patch
Comment 7 WebKit Review Bot 2012-12-05 07:26:16 PST
Comment on attachment 177737 [details]
Patch

Clearing flags on attachment: 177737

Committed r136684: <http://trac.webkit.org/changeset/136684>
Comment 8 WebKit Review Bot 2012-12-05 07:26:21 PST
All reviewed patches have been landed.  Closing bug.