WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
94395
[Chrome] Create a toggle button for closed captions
https://bugs.webkit.org/show_bug.cgi?id=94395
Summary
[Chrome] Create a toggle button for closed captions
Anna Cavender
Reported
2012-08-17 16:28:06 PDT
This patch will create a button that toggles any captions or subtitles on or off.
Attachments
Patch
(9.91 KB, patch)
2012-08-19 15:24 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
changes based on review
(9.81 KB, patch)
2012-08-20 17:08 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-08
(450.92 KB, application/zip)
2012-08-20 23:56 PDT
,
WebKit Review Bot
no flags
Details
fixes plus tests included
(17.01 KB, patch)
2012-08-21 14:19 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
new updateTextTrackControls function
(16.60 KB, patch)
2012-08-21 16:59 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Patch
(16.25 KB, patch)
2012-08-23 13:17 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-05
(619.59 KB, application/zip)
2012-08-23 14:19 PDT
,
WebKit Review Bot
no flags
Details
revisions based on erics review
(17.53 KB, patch)
2012-08-23 22:49 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
quick fix for ports that dont have track enabled
(17.53 KB, patch)
2012-08-23 23:25 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-08
(677.93 KB, application/zip)
2012-08-24 03:11 PDT
,
WebKit Review Bot
no flags
Details
minor revisions
(20.28 KB, patch)
2012-08-24 13:18 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-03
(313.19 KB, application/zip)
2012-08-24 18:27 PDT
,
WebKit Review Bot
no flags
Details
change test to use testRunner and remove default attr
(22.30 KB, patch)
2012-08-27 16:15 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-07
(326.31 KB, application/zip)
2012-08-27 17:55 PDT
,
WebKit Review Bot
no flags
Details
adding support for CC in RenderThemeChromiumSkia
(23.69 KB, patch)
2012-08-28 14:55 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
fix typo
(23.71 KB, patch)
2012-08-28 16:43 PDT
,
Anna Cavender
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
2012-08-19 09:40:53 PDT
(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.
Anna Cavender
Comment 2
2012-08-19 13:46:56 PDT
(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.
Silvia Pfeiffer
Comment 3
2012-08-19 14:55:09 PDT
I don't think there will be much overlap in code: Anna's code is in the Chromium version of the video controls. :-)
Silvia Pfeiffer
Comment 4
2012-08-19 14:56:21 PDT
I mean: at least it would be worth attaching the patch so it's possible to see the overlap.
Anna Cavender
Comment 5
2012-08-19 15:24:11 PDT
Created
attachment 159306
[details]
Patch PTAL, feel free to compare to what you already have and we can better coordinate efforts :-D
Early Warning System Bot
Comment 6
2012-08-19 15:40:55 PDT
Comment on
attachment 159306
[details]
Patch
Attachment 159306
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13542107
Early Warning System Bot
Comment 7
2012-08-19 15:41:27 PDT
Comment on
attachment 159306
[details]
Patch
Attachment 159306
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/13539335
Build Bot
Comment 8
2012-08-19 15:51:28 PDT
Comment on
attachment 159306
[details]
Patch
Attachment 159306
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/13531730
Eric Carlson
Comment 9
2012-08-19 19:09:57 PDT
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?
Silvia Pfeiffer
Comment 10
2012-08-19 21:29:17 PDT
> > 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. :-(
Anna Cavender
Comment 11
2012-08-20 10:38:01 PDT
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.
Anna Cavender
Comment 12
2012-08-20 17:08:13 PDT
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.
WebKit Review Bot
Comment 13
2012-08-20 23:56:18 PDT
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
WebKit Review Bot
Comment 14
2012-08-20 23:56:22 PDT
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
Eric Carlson
Comment 15
2012-08-21 08:19:40 PDT
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;
Anna Cavender
Comment 16
2012-08-21 09:11:27 PDT
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.
Anna Cavender
Comment 17
2012-08-21 09:55:15 PDT
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.
Anna Cavender
Comment 18
2012-08-21 14:19:52 PDT
Created
attachment 159769
[details]
fixes plus tests included
Eric Carlson
Comment 19
2012-08-21 14:48:16 PDT
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?
Anna Cavender
Comment 20
2012-08-21 16:53:10 PDT
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.
Anna Cavender
Comment 21
2012-08-21 16:59:31 PDT
Created
attachment 159807
[details]
new updateTextTrackControls function
Eric Carlson
Comment 22
2012-08-22 10:42:34 PDT
(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.
Anna Cavender
Comment 23
2012-08-23 13:02:28 PDT
(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.
Anna Cavender
Comment 24
2012-08-23 13:17:43 PDT
Created
attachment 160227
[details]
Patch removed an assignment of closedCaptionsVisible and added tests
Eric Carlson
Comment 25
2012-08-23 13:51:16 PDT
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.
WebKit Review Bot
Comment 26
2012-08-23 14:19:42 PDT
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
WebKit Review Bot
Comment 27
2012-08-23 14:19:45 PDT
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
Anna Cavender
Comment 28
2012-08-23 22:47:39 PDT
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.
Anna Cavender
Comment 29
2012-08-23 22:49:43 PDT
Created
attachment 160333
[details]
revisions based on erics review
Early Warning System Bot
Comment 30
2012-08-23 23:07:23 PDT
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
Early Warning System Bot
Comment 31
2012-08-23 23:07:52 PDT
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
Anna Cavender
Comment 32
2012-08-23 23:25:37 PDT
Created
attachment 160340
[details]
quick fix for ports that dont have track enabled
WebKit Review Bot
Comment 33
2012-08-24 03:11:08 PDT
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
WebKit Review Bot
Comment 34
2012-08-24 03:11:12 PDT
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
Eric Carlson
Comment 35
2012-08-24 09:12:54 PDT
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?
Anna Cavender
Comment 36
2012-08-24 13:18:40 PDT
Created
attachment 160487
[details]
minor revisions
Anna Cavender
Comment 37
2012-08-24 13:20:17 PDT
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.
WebKit Review Bot
Comment 38
2012-08-24 18:27:41 PDT
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
WebKit Review Bot
Comment 39
2012-08-24 18:27:45 PDT
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
Eric Carlson
Comment 40
2012-08-25 13:06:23 PDT
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!
Eric Carlson
Comment 41
2012-08-25 16:04:28 PDT
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.
Anna Cavender
Comment 42
2012-08-27 15:31:05 PDT
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.
Anna Cavender
Comment 43
2012-08-27 16:15:18 PDT
Created
attachment 160846
[details]
change test to use testRunner and remove default attr
WebKit Review Bot
Comment 44
2012-08-27 17:55:43 PDT
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
WebKit Review Bot
Comment 45
2012-08-27 17:55:47 PDT
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
Victor Carbune
Comment 46
2012-08-28 11:06:12 PDT
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?
Anna Cavender
Comment 47
2012-08-28 13:13:13 PDT
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.
Anna Cavender
Comment 48
2012-08-28 14:55:44 PDT
Created
attachment 161066
[details]
adding support for CC in RenderThemeChromiumSkia
WebKit Review Bot
Comment 49
2012-08-28 16:09:09 PDT
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
Peter Beverloo (cr-android ews)
Comment 50
2012-08-28 16:27:58 PDT
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
Anna Cavender
Comment 51
2012-08-28 16:43:31 PDT
Created
attachment 161088
[details]
fix typo
Anna Cavender
Comment 52
2012-08-29 10:08:08 PDT
Yay, bots are a happy shade of green. Please take one last look. Thanks!
Eric Carlson
Comment 53
2012-08-29 11:18:15 PDT
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.
Anna Cavender
Comment 54
2012-08-29 12:45:59 PDT
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!
WebKit Review Bot
Comment 55
2012-08-29 13:02:49 PDT
Comment on
attachment 161088
[details]
fix typo Clearing flags on attachment: 161088 Committed
r127035
: <
http://trac.webkit.org/changeset/127035
>
WebKit Review Bot
Comment 56
2012-08-29 13:02:58 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug