[webkit-reviews] review denied: [Bug 94395] [Chrome] Create a toggle button for closed captions : [Attachment 160340] quick fix for ports that dont have track enabled

[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 160340: quick fix for ports that dont have track enabled
https://bugs.webkit.org/attachment.cgi?id=160340&action=review

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


Closer, but the CC button should only change the state of caption and subtitle
tracks.

> Source/WebCore/html/HTMLMediaElement.cpp:2796
> +    if (m_closedCaptionsVisible)
> +	   return true;

Why return true for 'chapter' and 'metadata' tracks?

> Source/WebCore/html/HTMLMediaElement.cpp:3960
> +	   m_ignoreDefaultTracks = !m_closedCaptionsVisible;

"m_ignoreDefaultTracks" is a misleading name. It really means "don't show me
any captions", so maybe "m_doNotDisplayCaptions" or
"m_disableCaptionDisplay",or just "m_disableCaptions"?

> Source/WebCore/html/HTMLMediaElement.cpp:3962
> +	   // Check whether existing caption/subtitle tracks should change
visibility.

This comment is incorrect. It *is* probably worth having a comment about why
the flag is being cleared because the link between that and showing/hiding
tracks isn't necessarily obvious.

> Source/WebCore/html/HTMLMediaElement.cpp:3967
> +	   for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +	       if (!node->hasTagName(trackTag))
> +		   continue;
> +	       HTMLTrackElement* trackElement =
static_cast<HTMLTrackElement*>(node);
> +	       trackElement->setHasBeenConfigured(false);

This forces *all* tracks to be reconfigured, not just captions and subtitles.

> Source/WebCore/html/HTMLMediaElement.cpp:3970
> +	   configureNewTextTracks();

"configureNewTextTracks" isn't a great name any more because it no longer only
configures new tracks. When you revise this, can you please remove "New" from
the name?