Bug 62886

Because of the complexity of rendering WebVTT and because the rendering section of the spec is still in flux, I think it makes sense to implement a simple text track renderer using default positioning and style only. FWIW, this is the approach that Microsoft has taken for the beta (at least) of IE 10.

<rdar://problem/10320025>

Created attachment 119786 [details] Proposed patch

Attachment 119786 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1 Source/WebCore/html/shadow/MediaControlElements.h:500: The parameter name "arena" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 119786 [details] Proposed patch Attachment 119786 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10933352

Comment on attachment 119786 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=119786&action=review r- due to using innerHTML. We should at the very least have tests which show the effect of using it. > Source/WebCore/html/HTMLMediaElement.cpp:1007 > + HTMLTrackElement* trackElement; > + for (Node* node = firstChild(); node; node = node->nextSibling()) { > + if (!node->hasTagName(trackTag)) > + continue; > + trackElement = static_cast<HTMLTrackElement*>(node); > + if (trackElement->track() != track) > + continue; I don't think it is necessary to have the trackElement outside of the loop. > Source/WebCore/html/HTMLMediaElement.h:70 > +typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree; Don't want the space after PODIntervalTree. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:379 > + StringBuilder cues; > + for (size_t i = 0; i < visibleCues.size(); ++i) { > + if (i > 0 && cues.length()) > + cues.append("<br>"); > + > + TextTrackCue* cue = visibleCues[i].data(); > + ASSERT(cue->isActive()); > + if (!cue->track() || cue->track()->mode() != TextTrack::SHOWING) > + continue; > + > + cues.append(cue->getCueAsSource()); > + } > + > + ExceptionCode e; > + m_textTrackDisplay->setInnerHTML(cues.toString(), e); > + if (cues.length()) > + m_textDisplayContainer->show(); > + else > + m_textDisplayContainer->hide(); I think it would be nicer/safer to use programatic DOM APIs (appendChild, etc) instead of building a string and using innerHTML. If you do need to parse the cues as HTML, I would recommend doing it one cue at a time and not as a concatenated string.

Created attachment 119795 [details] Updated patch

(In reply to comment #6) > (From update of attachment 119786 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119786&action=review > > r- due to using innerHTML. We should at the very least have tests which show the effect of using it. > > > Source/WebCore/html/HTMLMediaElement.cpp:1007 > > + HTMLTrackElement* trackElement; > > + for (Node* node = firstChild(); node; node = node->nextSibling()) { > > + if (!node->hasTagName(trackTag)) > > + continue; > > + trackElement = static_cast<HTMLTrackElement*>(node); > > + if (trackElement->track() != track) > > + continue; > > I don't think it is necessary to have the trackElement outside of the loop. > Fixed. > > Source/WebCore/html/HTMLMediaElement.h:70 > > +typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree; > > Don't want the space after PODIntervalTree. > Fixed. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:379 > > + StringBuilder cues; > > + for (size_t i = 0; i < visibleCues.size(); ++i) { > > + if (i > 0 && cues.length()) > > + cues.append("<br>"); > > + > > + TextTrackCue* cue = visibleCues[i].data(); > > + ASSERT(cue->isActive()); > > + if (!cue->track() || cue->track()->mode() != TextTrack::SHOWING) > > + continue; > > + > > + cues.append(cue->getCueAsSource()); > > + } > > + > > + ExceptionCode e; > > + m_textTrackDisplay->setInnerHTML(cues.toString(), e); > > + if (cues.length()) > > + m_textDisplayContainer->show(); > > + else > > + m_textDisplayContainer->hide(); > > I think it would be nicer/safer to use programatic DOM APIs (appendChild, etc) instead of building a string and using innerHTML. If you do need to parse the cues as HTML, I would recommend doing it one cue at a time and not as a concatenated string. Fixed. Thanks!

Comment on attachment 119795 [details] Updated patch Attachment 119795 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10935368

Comment on attachment 119795 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=119795&action=review r=me, but please consider adding a test with tags in the content. (Also, make sure all the parts of the test are there.) > LayoutTests/media/track/track-cue-rendering.html:65 > + testTrack = document.querySelectorAll('track')[0]; Nit: This could just use querySelector('track') if you so felt.

Created attachment 119806 [details] Updated, one more time.

Comment on attachment 119806 [details] Updated, one more time. Remove r? because Sam already r+'d the previous version. This one is just so the ews bots can tell me if the Chromium specific code compiles.

Comment on attachment 119806 [details] Updated, one more time. Attachment 119806 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10932449

Created attachment 119820 [details] One more patch for the EWS bot

Comment on attachment 119820 [details] One more patch for the EWS bot Attachment 119820 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10921510

Created attachment 119822 [details] YAP

Comment on attachment 119822 [details] YAP Attachment 119822 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10942012

Created attachment 119867 [details] Updated patch Updated to fix a problem the Chromium EWS bot found.

http://trac.webkit.org/changeset/103242