[webkit-reviews] review denied: [Bug 62886] Basic display of WebVTT captions : [Attachment 119786] Proposed patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Eric Carlson <eric.carlson at apple.com>'s
request for review:
Bug 62886: Basic display of WebVTT captions
https://bugs.webkit.org/show_bug.cgi?id=62886

Attachment 119786: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=119786&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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.