[webkit-reviews] review denied: [Bug 94395] [Chrome] Create a toggle button for closed captions : [Attachment 159564] changes based on review

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Anna Cavender
<annacc at chromium.org>'s request for review:
Bug 94395: [Chrome] Create a toggle button for closed captions
https://bugs.webkit.org/show_bug.cgi?id=94395

Attachment 159564: changes based on review
https://bugs.webkit.org/attachment.cgi?id=159564&action=review

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


> Source/WebCore/html/HTMLMediaElement.cpp:1304
> +	       m_closedCaptionsVisible = track->mode() == TextTrack::SHOWING;

What if there is more than one text track?

> Source/WebCore/html/HTMLMediaElement.cpp:3920
> +    bool hasClosedCaptions = m_player && m_player->hasClosedCaptions();

WebKit style is to use early returns when possible:

    if (m_player && m_player->hasClosedCaptions())
	return true;

> Source/WebCore/html/HTMLMediaElement.cpp:3924
> +	   hasClosedCaptions |= m_textTracks->length() > 0;

The "> 0" is unnecessary. 

I think it is slightly clearer to include the three tests on the same line:

    if (RuntimeEnabledFeatures::webkitVideoTrackEnabled() && m_textTracks &&
m_textTracks->length())
	return true;