[webkit-reviews] review granted: [Bug 122218] Update TextTrack API to current spec : [Attachment 223467] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Brendan Long
<self at brendanlong.com>'s request for review:
Bug 122218: Update TextTrack API to current spec
https://bugs.webkit.org/show_bug.cgi?id=122218

Attachment 223467: Patch
https://bugs.webkit.org/attachment.cgi?id=223467&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223467&action=review


Nice, thanks for taking this on!

> Source/WebCore/html/shadow/MediaControlElements.cpp:1345
> +	   if (cue->cueType() == TextTrackCue::WebVTT || cue->cueType() ==
TextTrackCue::Generic)
> +	       toVTTCue(cue)->setFontSize(m_fontSize,
m_videoDisplaySize.size(), m_fontSizeIsImportant);

Minor nit: this would be slightly cleaner with a "continue" if the cue is *not*
generic or vtt.

> LayoutTests/media/track/track-vttcue.html:23
> +		   run("inbandCue = cues[0]");

Nit: "inbandCue" is not a great variable name because it is not an "in-band"
cue, ie. a cue from an in-band text track. Maybe "cueFromVTTFile" (long), or
"loadedCue" (potentially humorous), or "trackCue"?