Bug 70274 - WebVTT parser should not allow timestamps that have no spaces
: WebVTT parser should not allow timestamps that have no spaces
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-10-17 15:14 PST by
Modified: 2011-10-24 17:26 PST (History)


Attachments
Patch (2.24 KB, patch)
2011-10-17 15:32 PST, Anna Cavender
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.24 KB, patch)
2011-10-18 17:34 PST, Anna Cavender
no flags Review Patch | Details | Formatted Diff | Diff
adding check for tabs (2.30 KB, patch)
2011-10-18 18:07 PST, Anna Cavender
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-17 15:14:16 PST
Timestamps like this should not be allowed:

00:04:01.000-->00:03:00.500

They should instead be required to have at least one whitespace character between the startTime/endTime and the arrow:

00:04:01.000 --> 00:03:00.500
------- Comment #1 From 2011-10-17 15:32:23 PST -------
Created an attachment (id=111332) [details]
Patch
------- Comment #2 From 2011-10-17 15:36:08 PST -------
Attachment 111332 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   c0d4f37..d10d208  master     -> origin/master
    M    Source/WebCore/ChangeLog
    M    Source/WebCore/html/HTMLKeygenElement.h
    M    Source/WebCore/html/HTMLKeygenElement.cpp
r97658 = c627b94a38756dd0cde4d64c2288f36bff87ce20 (refs/remotes/trunk)
    A    LayoutTests/fast/mutation/observe-attributes-expected.txt
    A    LayoutTests/fast/mutation/observe-attributes.html
    M    LayoutTests/ChangeLog
    M    Source/WebCore/dom/MutationRecord.idl
    M    Source/WebCore/dom/MutationRecord.cpp
    M    Source/WebCore/dom/Element.cpp
    M    Source/WebCore/dom/Node.h
    M    Source/WebCore/dom/WebKitMutationObserver.h
    M    Source/WebCore/dom/NodeRareData.h
    M    Source/WebCore/dom/MutationRecord.h
    M    Source/WebCore/dom/Node.cpp
    M    Source/WebCore/dom/WebKitMutationObserver.cpp
    M    Source/WebCore/ChangeLog
    M    Source/WebCore/bindings/v8/V8Proxy.cpp
    M    Source/WebCore/WebCore.xcodeproj/project.pbxproj
r97659 = d10d208ef8dc738eb8d45c331840c61c2c779498 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2011-10-17 15:48:58 PST -------
Hmm, the spec at http://www.whatwg.org/specs/web-apps/current-work/webvtt.html currently states only the U+0020 SPACE character as a valid separator between the "-->" string and the timestamps. This accepts other white space as separators, too, in particular isASpace() accepts also U+0009 CHARACTER TABULATION (tab), U+000A LINE FEED (LF), U+000C FORM FEED (FF), and U+000D CARRIAGE RETURN.

I've just registered a bug on the spec to allow the TAB character as a separator, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=14487. But I do not believe the newline characters should be allowed.
------- Comment #4 From 2011-10-17 16:08:47 PST -------
I agree line terminators should not be allowed, but collectTimingsAndSettings() is only ever handed a single line (so it will not have any line terminators).  Perhaps the use of isASpace() is confusing here, would it be more clear to just check for ' ' and/or '\t' instead of using isASpace()?
------- Comment #5 From 2011-10-18 17:34:50 PST -------
Created an attachment (id=111538) [details]
Patch

How about we make sure that at least one space separates the time from the --> for now?  We can always update if more whitespace characters are allowed in the future.
------- Comment #6 From 2011-10-18 17:38:49 PST -------
Works for me. :-)
------- Comment #7 From 2011-10-18 18:07:36 PST -------
Created an attachment (id=111543) [details]
adding check for tabs

Ha, one of our tests (tc008-timings-no-hour-errors.vtt) checks for tabs, so at least WE think tabs should be allowed.  Adding that here.
------- Comment #8 From 2011-10-19 15:29:12 PST -------
(From update of attachment 111543 [details])
Clearing flags on attachment: 111543

Committed r97883: <http://trac.webkit.org/changeset/97883>
------- Comment #9 From 2011-10-19 15:29:16 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #10 From 2011-10-24 17:10:03 PST -------
As of a few hours ago, the spec now also tolerates tabs, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=14487 . We should add this.
------- Comment #11 From 2011-10-24 17:26:08 PST -------
Great!  We too allow both spaces and tabs, so looks like we are good to go.

A small note: the parser in the spec currently allows *0 or more* space or tab characters, not *1 or more* space or tab characters as specified in Ian's change.  Our implementation now enforces at least 1 space or tab character (so that [time]-->[time] without any spaces or tabs is not allowed).