Bug 94395 – [Chrome] Create a toggle button for closed captions

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

This patch will create a button that toggles any captions or subtitles on or off.

[Eric Carlson]

(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]

(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]

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]

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

[Anna Cavender]

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

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

[Eric Carlson]

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]

> > 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 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]

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 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]

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 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 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 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]

Created attachment 159769 [details]
fixes plus tests included

[Eric Carlson]

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 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]

Created attachment 159807 [details]
new updateTextTrackControls function

[Eric Carlson]

(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]

(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]

Created attachment 160227 [details]
Patch

removed an assignment of closedCaptionsVisible and added tests

[Eric Carlson]

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 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]

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 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]

Created attachment 160333 [details]
revisions based on erics review

[Early Warning System Bot]

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 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]

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

[WebKit Review Bot]

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]

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 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]

Created attachment 160487 [details]
minor revisions

[Anna Cavender]

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 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]

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 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 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 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]

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

[WebKit Review Bot]

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]

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 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 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]

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

[WebKit Review Bot]

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 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]

Created attachment 161088 [details]
fix typo

[Anna Cavender]

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

[Eric Carlson]

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 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 on attachment 161088 [details]
fix typo

Clearing flags on attachment: 161088

Committed r127035: <http://trac.webkit.org/changeset/127035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.