Bug 106653 – CC Button doesn't always show up

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

Original issue on Chromium bug tracker: https://code.google.com/p/chromium/issues/detail?id=150897

[Victor Carbune]

Created attachment 182319 [details]
Added extra update for CC button visibility

I think this could actually be in MediaControls::closedCaptionTracksChanged(), but I'm not sure if other ports are affected by the bug

[Eric Carlson]

Comment on attachment 182319 [details]
Added extra update for CC button visibility

View in context: https://bugs.webkit.org/attachment.cgi?id=182319&action=review

> Source/WebCore/html/shadow/MediaControlsChromium.cpp:231
> +void MediaControlsChromium::closedCaptionTracksChanged()
> +{
> +    if (!m_toggleClosedCaptionsButton)
> +        return;
> +
> +    if (m_mediaController->hasClosedCaptions())
> +        m_toggleClosedCaptionsButton->show();
> +    else
> +        m_toggleClosedCaptionsButton->hide();
> +}

This seems like the correct default behavior, so I think it belongs in the base class. Any port that does not want this behavior can override and not call the base method.

[Victor Carbune]

Created attachment 182405 [details]
Modified base class

[Eric Carlson]

Comment on attachment 182405 [details]
Modified base class

Thank you Victor!

[WebKit Review Bot]

Comment on attachment 182405 [details]
Modified base class

Attachment 182405 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15822105

New failing tests:
fast/text/international/hindi-spacing.html

[Silvia Pfeiffer]

> LayoutTests/media/video-controls-captions.html:48
> +                consoleWrite("** Caption button should not be visible as there are no captions track.");

Nit: English grammar correction
"as there are no caption tracks"

Thanks for moving this to the base class! I think we should do as much as possible there and leave ports to override it if they disagree (as Eric says).

[Victor Carbune]

Created attachment 182466 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182466 [details]
Patch for landing

Clearing flags on attachment: 182466

Committed r139547: <http://trac.webkit.org/changeset/139547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Some of the new checks appear to fail on EFL port. I'm not sure why yet:
--- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/media/video-controls-captions-expected.txt
+++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/media/video-controls-captions-actual.txt
@@ -25,8 +25,8 @@
 ** Remove DOM node representing the track element **
 
 ** Caption button should not be visible as there are no caption tracks.
-EXPECTED (captionsButtonCoordinates[0] <= '0') OK
-EXPECTED (captionsButtonCoordinates[1] <= '0') OK
+EXPECTED (captionsButtonCoordinates[0] <= '0'), OBSERVED '330' FAIL
+EXPECTED (captionsButtonCoordinates[1] <= '0'), OBSERVED '328' FAIL
 
 ** Add a text track through JS to the video element **