Bug 106653 - CC Button doesn't always show up
Summary: CC Button doesn't always show up
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 43668 106743
  Show dependency treegraph
 
Reported: 2013-01-11 04:07 PST by Victor Carbune
Modified: 2013-01-13 04:24 PST (History)
7 users (show)

See Also:


Attachments
Added extra update for CC button visibility (7.57 KB, patch)
2013-01-11 04:26 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Modified base class (7.65 KB, patch)
2013-01-11 13:23 PST, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch for landing (7.64 KB, patch)
2013-01-12 03:42 PST, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2013-01-11 04:07:31 PST
Original issue on Chromium bug tracker: https://code.google.com/p/chromium/issues/detail?id=150897
Comment 1 Victor Carbune 2013-01-11 04:26:32 PST
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
Comment 2 Eric Carlson 2013-01-11 10:17:24 PST
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.
Comment 3 Victor Carbune 2013-01-11 13:23:41 PST
Created attachment 182405 [details]
Modified base class
Comment 4 Eric Carlson 2013-01-11 13:28:15 PST
Comment on attachment 182405 [details]
Modified base class

Thank you Victor!
Comment 5 WebKit Review Bot 2013-01-11 17:25:24 PST
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
Comment 6 Silvia Pfeiffer 2013-01-11 17:36:04 PST
> 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).
Comment 7 Victor Carbune 2013-01-12 03:42:34 PST
Created attachment 182466 [details]
Patch for landing
Comment 8 WebKit Review Bot 2013-01-12 04:06:19 PST
Comment on attachment 182466 [details]
Patch for landing

Clearing flags on attachment: 182466

Committed r139547: <http://trac.webkit.org/changeset/139547>
Comment 9 WebKit Review Bot 2013-01-12 04:06:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Chris Dumez 2013-01-13 04:06:18 PST
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 **