Bug 88187

Corrected blocking dependency, sorry.

Created attachment 145481 [details] Preliminary patch

Created attachment 145488 [details] Corrected patch

Just realized that according to the spec, the ProcessingInstructions nodes must have the original timestamp, not the actual value (this seems to require to parse it each time, for now)

Comment on attachment 145488 [details] Corrected patch View in context: https://bugs.webkit.org/attachment.cgi?id=145488&action=review > Source/WebCore/ChangeLog:5 > +2012-06-03 Victor Carbune <vcarbune@adobe.com> > + > + Need a short description and bug URL (OOPS!) > + > + Reviewed by NOBODY (OOPS!). Oops, > Source/WebCore/ChangeLog:36 > +2012-06-03 Victor Carbune <victor@rosedu.org> > + > + Basic support for timestamps within a TextTrackCue > + https://bugs.webkit.org/show_bug.cgi?id=88187 > + > + Implemented support for timestamps within a TextTrackCue. > + This enables rendering functionality for Karaoke and Paint-on captions. > + > + Reviewed by NOBODY (OOPS!). > + you have two ChangeLog entries. > Source/WebCore/css/mediaControls.css:237 > +video::-webkit-media-text-track-future-nodes { Do we display future nodes? > Source/WebCore/css/mediaControls.css:242 > + color: green; > + background-color: rgba(0, 0, 0, 0.8); If so, why are they green? > Source/WebCore/html/track/TextTrackCue.cpp:579 > + for (Node *child = referenceTree->firstChild(); child; child = child->nextSibling()) { > + if (child->nodeName() == timestampTag) { > + unsigned int position = 0; > + String timestamp = child->nodeValue(); > + > + double timestampTime = WebVTTParser::create(0, m_scriptExecutionContext)->collectTimeStamp(timestamp, &position); > + ASSERT(timestampTime != -1); > + > + if (timestampTime > movieTime) > + isPastNode = false; > + } Should you always set "isPastNode", or are timestamps guaranteed to be in ascending time order? > Source/WebCore/html/track/WebVTTParser.cpp:386 > - m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size()))); > + m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size()))); Is the only difference here an extra space? If so, it is unnecessary. > Source/WebCore/html/track/WebVTTParser.h:97 > + double collectTimeStamp(const String&, unsigned*); Because we are making this a public method, I think we should give it a better name, eg. something like "parseTimeStamp"?

Created attachment 146418 [details] Updated patch

(In reply to comment #5) > (From update of attachment 145488 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145488&action=review > > > Source/WebCore/ChangeLog:5 > > +2012-06-03 Victor Carbune <vcarbune@adobe.com> > > + > > + Need a short description and bug URL (OOPS!) > > + > > + Reviewed by NOBODY (OOPS!). > > Oops, > > > Source/WebCore/ChangeLog:36 > > +2012-06-03 Victor Carbune <victor@rosedu.org> > > + > > + Basic support for timestamps within a TextTrackCue > > + https://bugs.webkit.org/show_bug.cgi?id=88187 > > + > > + Implemented support for timestamps within a TextTrackCue. > > + This enables rendering functionality for Karaoke and Paint-on captions. > > + > > + Reviewed by NOBODY (OOPS!). > > + > > you have two ChangeLog entries. Sorry, fixed. > > Source/WebCore/css/mediaControls.css:237 > > +video::-webkit-media-text-track-future-nodes { > > Do we display future nodes? They have visibility:none, so they are not visible, but are attached to the display tree and rendered there. > > Source/WebCore/css/mediaControls.css:242 > > + color: green; > > + background-color: rgba(0, 0, 0, 0.8); > > If so, why are they green? Oops, it was mostly for testing. Removed. > > Source/WebCore/html/track/TextTrackCue.cpp:579 > > + for (Node *child = referenceTree->firstChild(); child; child = child->nextSibling()) { > > + if (child->nodeName() == timestampTag) { > > + unsigned int position = 0; > > + String timestamp = child->nodeValue(); > > + > > + double timestampTime = WebVTTParser::create(0, m_scriptExecutionContext)->collectTimeStamp(timestamp, &position); > > + ASSERT(timestampTime != -1); > > + > > + if (timestampTime > movieTime) > > + isPastNode = false; > > + } > > Should you always set "isPastNode", or are timestamps guaranteed to be in ascending time order? Yes, timestamps are guaranteed to be in ascending time order in the spec. > > Source/WebCore/html/track/WebVTTParser.cpp:386 > > - m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size()))); > > + m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size()))); > > Is the only difference here an extra space? If so, it is unnecessary. Fixed. > > Source/WebCore/html/track/WebVTTParser.h:97 > > + double collectTimeStamp(const String&, unsigned*); > > Because we are making this a public method, I think we should give it a better name, eg. something like "parseTimeStamp"? I would suggest keeping it the same, mostly because "collect" is the term used in the specification (and by the other remaining private methods).

Comment on attachment 146418 [details] Updated patch Clearing flags on attachment: 146418 Committed r119907: <http://trac.webkit.org/changeset/119907>

All reviewed patches have been landed. Closing bug.

The new test is failing on EFL port. The diff looks like: -Current time: 2.25 +Current time: 2.249999761581421 -Current time: 2.75 +Current time: 2.750000238418579 -Current time: 3.25 +Current time: 3.249999761581421 -Current time: 3.75 +Current time: 3.750000238418579 Looks like a rounding issue.

The new test appears to be failing for GTK port as well. I opened Bug 88725 to track the issue.