[webkit-reviews] review granted: [Bug 133350] Perserve caption selection in fullscreen. : [Attachment 232206] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jeremy Jones
<jeremyj-wk at apple.com>'s request for review:
Bug 133350: Perserve caption selection in fullscreen.
https://bugs.webkit.org/show_bug.cgi?id=133350

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

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


> Source/WebCore/ChangeLog:8
> +	   Use the logic from the line player to calculate the selected caption
index.

Nit: "from the line player" -> "from the inline player"

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:297
> +    if (trackList && m_mediaElement->document().page() &&
m_mediaElement->mediaControlsHost()) {

This should be an early return.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:303
> +	   Vector<String> legibleOptions;

Nit: I had to read through the code to figure out what "legibleOptions" meant,
so something like "trackDisplayNames" might be better.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:326
> +	   if (offIndex && displayMode ==
MediaControlsHost::forcedOnlyKeyword() && !trackMenuItemSelected) {

Nit: "!trackMenuItemSelected" is cheaper than "displayMode ==
MediaControlsHost::forcedOnlyKeyword()" so it should be first.