Bug 94395

(In reply to comment #0) > This patch will create a button that toggles any captions or subtitles on or off. Don't start working on this if you haven't already, we have a patch for this that is almost ready for review.

(In reply to comment #1) > (In reply to comment #0) > > This patch will create a button that toggles any captions or subtitles on or off. > > Don't start working on this if you haven't already, we have a patch for this that is almost ready for review. Eric, my patch is ready to go, I was just waiting for the chromium changes to land first. Let me know what you would like me to do.

I don't think there will be much overlap in code: Anna's code is in the Chromium version of the video controls. :-)

I mean: at least it would be worth attaching the patch so it's possible to see the overlap.

Created attachment 159306 [details] Patch PTAL, feel free to compare to what you already have and we can better coordinate efforts :-D

Comment on attachment 159306 [details] Patch Attachment 159306 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13542107

Comment on attachment 159306 [details] Patch Attachment 159306 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13539335

Comment on attachment 159306 [details] Patch Attachment 159306 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13531730

Comment on attachment 159306 [details] Patch 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?

> > 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. It's still on my TODO list. I've been away for a month and then sick for a while. Sorry for the slowness. :-(

Comment on attachment 159306 [details] Patch Thanks for the quick feedback. Let me know what you think the best plan of action is ... View in context: https://bugs.webkit.org/attachment.cgi?id=159306&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:784 >> + setClosedCaptionsVisible(true); > > This is wrong. The spec says captions are not enabled by default, visibility is to be controlled by user preferences and actions. Thanks will remove this. >> Source/WebCore/html/HTMLMediaElement.cpp:3932 >> +#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. Yes, good call. Will set to true if m_textTracks->length() is greater than 0. >> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:174 >> + } > > 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. Saw that, but didn't realize there were plans to create a base class. How would you like me to proceed? (a) allow this duplication for now with the plan to create a base class (b) create a base class (Silvia? or me?), then come back to this patch. >> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:353 >> + m_toggleClosedCaptionsButton->hide(); > > Isn't it possible for m_toggleClosedCaptionsButton to be NULL? Good catch, will add a check.

Created attachment 159564 [details] changes based on review uploading a fresh patch, but do let me know if you'd like me to hold off until a base class is created.

Comment on attachment 159564 [details] changes based on review Attachment 159564 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13545391 New failing tests: media/track/track-cue-rendering-inner-timestamps.html media/track/track-cue-mutable-text.html

Created attachment 159627 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 159564 [details] changes based on review 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;

Comment on attachment 159564 [details] changes based on review Thanks again Eric. Question about changing a text track's mode .... 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? Whoops, good call! Seems like if this text track's mode just changed to SHOWING, we should make sure that m_closedCaptionsVisible is true. Does that seem right: setting a track's mode to SHOWING will turn on the captions button? Should the button turn off if all the text tracks are HIDDEN or DISABLED? >> 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; Done. >> 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; Done.

Comment on attachment 159564 [details] changes based on review 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? > > Whoops, good call! Seems like if this text track's mode just changed to SHOWING, we should make sure that m_closedCaptionsVisible is true. Does that seem right: setting a track's mode to SHOWING will turn on the captions button? Should the button turn off if all the text tracks are HIDDEN or DISABLED? On second thought, perhaps setting the mode should not affect the button at all. Removing this line altogether.

Created attachment 159769 [details] fixes plus tests included

Comment on attachment 159769 [details] fixes plus tests included View in context: https://bugs.webkit.org/attachment.cgi?id=159769&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4120 > void HTMLMediaElement::configureTextTrackDisplay() > return; > if (!hasMediaControls() && !createMediaControls()) > return; > - mediaControls()->updateTextTrackDisplay(); > + setClosedCaptionsVisible(m_haveVisibleTextTrack); > } > #endif I am sorry that I didn't notice this before, but calling setClosedCaptionsVisible() because it redraws the controls/captions as a side effect doesn't make sense even though it is expedient. Please put the code in HTMLMediaElement::setClosedCaptionsVisible that updates the controls into a function that you can call from here and there. > Source/WebCore/html/track/TextTrack.cpp:-150 > + if (mode != TextTrack::SHOWING && m_cues) > + for (size_t i = 0; i < m_cues->length(); ++i) > + m_cues->item(i)->removeDisplayTree(); > + > // If mode changes to disabled, remove this track's cues from the client > // because they will no longer be accessible from the cues() function. > if (mode == TextTrack::DISABLED && m_client && m_cues) > m_client->textTrackRemoveCues(this, m_cues.get()); > - > - if (mode != TextTrack::SHOWING && m_cues) > - for (size_t i = 0; i < m_cues->length(); ++i) > - m_cues->item(i)->removeDisplayTree(); What does this change do? Does it have anything to do with the toggle button? > LayoutTests/ChangeLog:11 > + The following tests now need to set the captions visibility before testing > + the rendering: Why is it necessary to changes tests in this way?

Comment on attachment 159769 [details] fixes plus tests included View in context: https://bugs.webkit.org/attachment.cgi?id=159769&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4120 >> #endif > > I am sorry that I didn't notice this before, but calling setClosedCaptionsVisible() because it redraws the controls/captions as a side effect doesn't make sense even though it is expedient. > > Please put the code in HTMLMediaElement::setClosedCaptionsVisible that updates the controls into a function that you can call from here and there. Will do. >> Source/WebCore/html/track/TextTrack.cpp:-150 >> - m_cues->item(i)->removeDisplayTree(); > > What does this change do? Does it have anything to do with the toggle button? No, I found a crash during testing that occurs when changing the mode to DISABLED. I will remove this from this patch and file a separate bug. >> LayoutTests/ChangeLog:11 >> + the rendering: > > Why is it necessary to changes tests in this way? I added a check for mediaElement->closedCaptionsVisible() in MediaControlTextTrackContainerElement::updateDisplay() and so these tests will no longer render the captions unless the visibility is explicitly set.

Created attachment 159807 [details] new updateTextTrackControls function

(In reply to comment #20) > (From update of attachment 159769 [details]) > >> LayoutTests/ChangeLog:11 > >> + the rendering: > > > > Why is it necessary to changes tests in this way? > > I added a check for mediaElement->closedCaptionsVisible() in MediaControlTextTrackContainerElement::updateDisplay() and so these tests will no longer render the captions unless the visibility is explicitly set. All of those tests have the 'default' attribute on the track element, so the is enabled by default. You did not update closedCaptionsVisible to account for out-of-band captions, so how can it be used in MediaControlTextTrackContainerElement::updateDisplay? I should have caught this earlier. If the tests did need to change to enable tracks before running, using webkitClosedCaptionsVisible is *definitely* the wrong way to do it because it is using a WebKit specific extension to test a spec feature.

(In reply to comment #22) > (In reply to comment #20) > > (From update of attachment 159769 [details] [details]) > > >> LayoutTests/ChangeLog:11 > > >> + the rendering: > > > > > > Why is it necessary to changes tests in this way? > > > > I added a check for mediaElement->closedCaptionsVisible() in MediaControlTextTrackContainerElement::updateDisplay() and so these tests will no longer render the captions unless the visibility is explicitly set. > > All of those tests have the 'default' attribute on the track element, so the is enabled by default. You did not update closedCaptionsVisible to account for out-of-band captions, so how can it be used in MediaControlTextTrackContainerElement::updateDisplay? I should have caught this earlier. > > If the tests did need to change to enable tracks before running, using webkitClosedCaptionsVisible is *definitely* the wrong way to do it because it is using a WebKit specific extension to test a spec feature. Aha! Turns out those Layout Tests were flakily failing because we were setting closedCaptionsVisible to false in HTMLMediaElement::prepareForLoad(), which can end up undoing the checks for defaults we do in configureNewTextTracks(). I don't understand why we would set m_closedCaptionsVisible to false in prepareForLoad(), so I've removed it (see the next patch) and that seems to solve all the problems I was seeing. Please let me know if this was an important thing to have and if so, advise me on how to ensure that m_closedCaptionsVisible properly reflects the out-of-band tracks that should be visible.

Created attachment 160227 [details] Patch removed an assignment of closedCaptionsVisible and added tests

Comment on attachment 160227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160227&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3927 > + if (RuntimeEnabledFeatures::webkitVideoTrackEnabled() && m_textTracks && m_textTracks->length()) > + return true; Thinking about this some more in light of my comment below, I now see that this is wrong because it will return true if a video has *any* text tracks, eg. 'metadata', 'menu', etc. This function needs to consult TextTrack::isRendered to see if there is a track that will render captions. > Source/WebCore/html/HTMLMediaElement.cpp:4129 > if (m_haveVisibleTextTrack == haveVisibleTextTrack) > return; > m_haveVisibleTextTrack = haveVisibleTextTrack; > + m_closedCaptionsVisible = m_haveVisibleTextTrack; > > if (!m_haveVisibleTextTrack && !hasMediaControls()) > return; > if (!hasMediaControls() && !createMediaControls()) > return; > - mediaControls()->updateTextTrackDisplay(); > + > + updateClosedCaptionsControls(); > } setClosedCaptionsVisible() is called when the user clicks the CC button in controls. When Safari has a video with an in-band captions open, the button causes captions draw or not draw. If the intention is do the same thing for out-of-band captions, which I think it is, I am not sure that this will work. Image a page with the following: <video src=trailer.mp4 controls > <track kind=captions src=english.vtt srclang=en> <track kind=captions src=french.vtt srclang=fr> </video> When the page loads no captions will be displayed because neither of the tracks has 'default', but the CC button should be enabled and the user will expect us to show the track that is most appropriate for their system language. I don't believe this patch will do anything in this case. I *think* the right way to handle this is to change HTMLMediaElement::userIsInterestedInThisTrackKind to return true when the user has asked us to show captions, and then to clear the "had been configured" flag on caption and subtitle tracks and call configureNewTextTracks(). This will cause the new logic added to userIsInterestedInThisTrackKind to be consulted and I think the right thing will happen.

Comment on attachment 160227 [details] Patch Attachment 160227 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13563825 New failing tests: media/video-controls-captions.html

Created attachment 160243 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160227&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:3927 >> + return true; > > Thinking about this some more in light of my comment below, I now see that this is wrong because it will return true if a video has *any* text tracks, eg. 'metadata', 'menu', etc. This function needs to consult TextTrack::isRendered to see if there is a track that will render captions. Good point about only considering tracks with relevant kinds, however TextTrack::isRendered requires the track to be SHOWING, and we want to make the CC button available if any track could possibly be rendered. I'll add a check for the tracks' kind. >> Source/WebCore/html/HTMLMediaElement.cpp:4129 >> } > > setClosedCaptionsVisible() is called when the user clicks the CC button in controls. When Safari has a video with an in-band captions open, the button causes captions draw or not draw. If the intention is do the same thing for out-of-band captions, which I think it is, I am not sure that this will work. Image a page with the following: > > <video src=trailer.mp4 controls > > <track kind=captions src=english.vtt srclang=en> > <track kind=captions src=french.vtt srclang=fr> > </video> > > When the page loads no captions will be displayed because neither of the tracks has 'default', but the CC button should be enabled and the user will expect us to show the track that is most appropriate for their system language. I don't believe this patch will do anything in this case. > > I *think* the right way to handle this is to change HTMLMediaElement::userIsInterestedInThisTrackKind to return true when the user has asked us to show captions, and then to clear the "had been configured" flag on caption and subtitle tracks and call configureNewTextTracks(). This will cause the new logic added to userIsInterestedInThisTrackKind to be consulted and I think the right thing will happen. Beautiful. Will revise setClosedCaptionsVisible() accordingly.

Created attachment 160333 [details] revisions based on erics review

Comment on attachment 160333 [details] revisions based on erics review Attachment 160333 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13592043

Comment on attachment 160333 [details] revisions based on erics review Attachment 160333 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13570989

Created attachment 160340 [details] quick fix for ports that dont have track enabled

Comment on attachment 160340 [details] quick fix for ports that dont have track enabled Attachment 160340 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13598064 New failing tests: media/video-controls-captions.html

Created attachment 160374 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160340 [details] quick fix for ports that dont have track enabled 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?

Created attachment 160487 [details] minor revisions

Comment on attachment 160340 [details] quick fix for ports that dont have track enabled Thanks again Eric! View in context: https://bugs.webkit.org/attachment.cgi?id=160340&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:2796 >> + return true; > > Why return true for 'chapter' and 'metadata' tracks? Done. That was just an oversight. >> 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"? Done. >> 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. Done. >> Source/WebCore/html/HTMLMediaElement.cpp:3967 >> + trackElement->setHasBeenConfigured(false); > > This forces *all* tracks to be reconfigured, not just captions and subtitles. Done. >> 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? Done.

Comment on attachment 160487 [details] minor revisions Attachment 160487 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13603307 New failing tests: media/video-controls-captions.html

Created attachment 160539 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160487 [details] minor revisions View in context: https://bugs.webkit.org/attachment.cgi?id=160487&action=review r+, although you will clearly have to figure why the test is failing. > Source/WebCore/html/HTMLMediaElement.cpp:3963 > + // Mark all track elements as not "configured" so that configureTextTracks() > + // will reconsider which tracks to display in light of new user preferences > + // (e.g. default tracks should not be displayed if the user has turned off > + // captions and non-default tracks should be displayed based on language > + // preferences if the user has turned captions on). Nice explanation, thanks!

Comment on attachment 160487 [details] minor revisions View in context: https://bugs.webkit.org/attachment.cgi?id=160487&action=review > LayoutTests/media/video-controls-captions.html:70 > + <video controls > > + <track src="track/captions-webvtt/captions-fast.vtt" kind="captions" default > > + </video> Why don't you leave out the 'default' attribute to make sure the CC button is visible but not enabled at first.

Comment on attachment 160487 [details] minor revisions View in context: https://bugs.webkit.org/attachment.cgi?id=160487&action=review >> LayoutTests/media/video-controls-captions.html:70 >> + </video> > > Why don't you leave out the 'default' attribute to make sure the CC button is visible but not enabled at first. Will do, thanks Eric.

Created attachment 160846 [details] change test to use testRunner and remove default attr

Comment on attachment 160846 [details] change test to use testRunner and remove default attr Attachment 160846 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13642162 New failing tests: media/video-controls-captions.html

Created attachment 160872 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160846 [details] change test to use testRunner and remove default attr View in context: https://bugs.webkit.org/attachment.cgi?id=160846&action=review > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:168 > + if (document->page()->theme()->supportsClosedCaptioning()) { I think you need to override the default return false in all the RenderThemeChromium* that support this. However, I don't think it's enough - the button doesn't show on my machine, but the test passes. Is there any other Chrome patch waiting to land? Or maybe some other places that need these kind of updates. > LayoutTests/media/video-controls-captions.html:10 > + if (window.testRunner) { I think you can remove this check - this is already done in video-test.js > LayoutTests/media/video-controls-captions.html:28 > + testRunner.notifyDone(); Nit: would be nice to log the exception that was catched, so we know what happened. > LayoutTests/media/video-controls-captions.html:49 > + failTest(); *Optional*: I would fail early and remove the if {} else {} > LayoutTests/media/video-controls-captions.html:71 > + checkCaptionsDisplay(); Nit: would you mind changing the indentation to be just 4 spaces?

Comment on attachment 160846 [details] change test to use testRunner and remove default attr Thanks so much. I'll try your suggestions and upload another patch. View in context: https://bugs.webkit.org/attachment.cgi?id=160846&action=review >> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:168 >> + if (document->page()->theme()->supportsClosedCaptioning()) { > > I think you need to override the default return false in all the RenderThemeChromium* that support this. > However, I don't think it's enough - the button doesn't show on my machine, but the test passes. Is there > any other Chrome patch waiting to land? Or maybe some other places that need these kind of updates. Thank you, Victor!! Looks like RenderThemeMac was already supporting closed captioning, so I didn't even realize I needed to do this. >> LayoutTests/media/video-controls-captions.html:10 >> + if (window.testRunner) { > > I think you can remove this check - this is already done in video-test.js Done. >> LayoutTests/media/video-controls-captions.html:28 >> + testRunner.notifyDone(); > > Nit: would be nice to log the exception that was catched, so we know what happened. Smart. Thanks. >> LayoutTests/media/video-controls-captions.html:49 >> + failTest(); > > *Optional*: I would fail early and remove the if {} else {} Done. >> LayoutTests/media/video-controls-captions.html:71 >> + checkCaptionsDisplay(); > > Nit: would you mind changing the indentation to be just 4 spaces? Done.

Created attachment 161066 [details] adding support for CC in RenderThemeChromiumSkia

Comment on attachment 161066 [details] adding support for CC in RenderThemeChromiumSkia Attachment 161066 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13665425

Comment on attachment 161066 [details] adding support for CC in RenderThemeChromiumSkia Attachment 161066 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13652592

Created attachment 161088 [details] fix typo

Yay, bots are a happy shade of green. Please take one last look. Thanks!

Comment on attachment 161088 [details] fix typo View in context: https://bugs.webkit.org/attachment.cgi?id=161088&action=review > LayoutTests/media/video-controls-captions.html:83 > + function trackLoaded() > + { > + checkCaptionsDisplay(); > + > + consoleWrite(""); > + consoleWrite("** Captions should not be visible after button is clicked again **"); > + clickCCButton(); > + checkCaptionsDisplay(); > + > + endTest(); > + } > + > + function loaded() > + { > + findMediaElement(); > + consoleWrite("Set the user language preference so that the track will be chosen when the CC button is clicked."); > + run("internals.setUserPreferredLanguages(['en'])"); > + > + waitForEvent('canplaythrough', startTest); > + > + video.src = findMediaFile('video', 'content/counting'); > + } Are you certain that body.load will fire before track.load? If not, this test will be flakey.

Comment on attachment 161088 [details] fix typo View in context: https://bugs.webkit.org/attachment.cgi?id=161088&action=review >> LayoutTests/media/video-controls-captions.html:83 >> + } > > Are you certain that body.load will fire before track.load? If not, this test will be flakey. Yes, track.load will only fire after the first button click. Thanks Eric!

Comment on attachment 161088 [details] fix typo Clearing flags on attachment: 161088 Committed r127035: <http://trac.webkit.org/changeset/127035>

All reviewed patches have been landed. Closing bug.