[webkit-reviews] review denied: [Bug 94395] [Chrome] Create a toggle button for closed captions : [Attachment 159306] Patch

[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 159306: Patch
https://bugs.webkit.org/attachment.cgi?id=159306&action=review

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


> Source/WebCore/html/HTMLMediaElement.cpp:784
> +	       if (m_textTracksWhenResourceSelectionBegan.size() > 0)
> +		   setClosedCaptionsVisible(true);

This is wrong. The spec says captions are not enabled by default, visibility is
to be controlled by user preferences and actions.

> Source/WebCore/html/HTMLMediaElement.cpp:3932
> +#if ENABLE(VIDEO_TRACK)
> +    if (RuntimeEnabledFeatures::webkitVideoTrackEnabled() && m_textTracks) {

> +	   for (unsigned i = 0; i < m_textTracks->length(); ++i) {
> +	       if (m_textTracks->item(i)->mode() != TextTrack::DISABLED) {
> +		   hasClosedCaptions = true;
> +		   break;
> +	       }
> +	   }
> +    }
> +#endif

This also seems wrong. The CC button is shown based on what this function
returns and it seems to me that we want a CC button if there are *any* text
tracks, disabled or not.

> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:174
> +    if (document->page()->theme()->supportsClosedCaptioning()) {
> +	   RefPtr<MediaControlToggleClosedCaptionsButtonElement>
toggleClosedCaptionsButton =
MediaControlToggleClosedCaptionsButtonElement::create(document);
> +	   m_toggleClosedCaptionsButton = toggleClosedCaptionsButton.get();
> +	   panel->appendChild(toggleClosedCaptionsButton.release(), ec, true);
> +	   if (ec)
> +	       return false;
> +    }

This, and much of the other code added to this file, is even more code
duplicated from MediaControlRootElement. 

What happened to the plan to create a base class?
https://bugs.webkit.org/show_bug.cgi?id=88871 has been sitting unattended for
more than two months.

> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:353
> +    m_toggleClosedCaptionsButton->hide();

Isn't it possible for m_toggleClosedCaptionsButton to be NULL?