Bug 106289 – [Chromium] Support list of tracks in caption media controls

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Media

[Silvia Pfeiffer]

Same as bug 101669 but for Chromium.

[Silvia Pfeiffer]

Created attachment 190478 [details]
WIP patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Early Warning System Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16807132

[Early Warning System Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16663255

[WebKit Review Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16783181

New failing tests:
media/video-controls-captions.html

[WebKit Review Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16663320

New failing tests:
media/video-controls-captions.html

[Build Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16803090

New failing tests:
media/track/track-user-preferences.html

[Build Bot]

Comment on attachment 190478 [details]
WIP patch

Attachment 190478 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16791114

New failing tests:
media/track/track-user-preferences.html

[Eric Carlson]

Comment on attachment 190478 [details]
WIP patch

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

> Source/WebCore/ChangeLog:18
> +        * css/CSSDefaultStyleSheets.cpp:
> +        (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement):
> +        Allow mediaControlsStyleSheet to also style by siblingRules.

A longer comment explaining why this is necessary would be nice.

> Source/WebCore/html/shadow/MediaControlElements.cpp:868
> +        if (!(track->kind() == track->captionsKeyword() || track->kind() == track->subtitlesKeyword()))
> +            continue;

Nit: this is slightly awkward, why not "if (!captions && !subtitles)"?

> Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:49
> +static const char* textTracksOffAttrValue = "-1"; // This must match HTMLMediaElement::textTracksOffIndex()

Why not just call HTMLMediaElement::textTracksOffIndex() (besides the fact that the old code did it ;-) )

> Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:96
> +    // Create first list item to turn captions/subtitles off.

This comment just tells us what the code does, which is obvious by inspection.

> Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:109
> +        if (!(track->kind() == track->captionsKeyword() || track->kind() == track->subtitlesKeyword()))

Nit: this is slightly odd, why not "if (!captions && !subtitles)"?

> Source/WebCore/html/shadow/MediaControlElementsChromium.cpp:142
> +        // Use the language name as the display name for the track, if available.
> +        String labelText = displayNameForLanguageLocale(track->language());
> +
> +        // If the developer provided a label for the track, add it to display name.
> +        if (!(track->label().isNull() || track->label().isEmpty())) {
> +            if (labelText.isNull() || labelText.isEmpty())
> +                labelText = track->label();
> +            else  {
> +                labelText.append(" - ");
> +                labelText.append(track->label());
> +            }
> +        }
> +
> +        // If track still has no display name, add "No Label".
> +        if (labelText.isNull() || labelText.isEmpty())
> +            labelText = textTrackNoLabelText();
> +
> +        // Mark actual caption tracks with a special keyword.
> +        if (track->kind() == track->captionsKeyword()) {
> +            labelText.append(" [");
> +            labelText.append(textTrackCaptionsMarker());
> +            labelText.append("]");
> +        }

Couldn't you get rid of this entire function if you put the name generation logic into an override of CaptionsUserPreferences::displayNameForTrack?

[Silvia Pfeiffer]

Eric, thanks. That's all great feedback - will apply.

>> Source/WebCore/ChangeLog:18
>> +        * css/CSSDefaultStyleSheets.cpp:
>> +        (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement):
>> +        Allow mediaControlsStyleSheet to also style by siblingRules.
>
>A longer comment explaining why this is necessary would be nice. 

This is to allow me to use
video::-webkit-media-controls-closed-captions-track-list li:first-child
in the mediaControlsChromium.css style sheet.
I could instead move this into a layout() function, though I thought this was easier to read.
What do you think?


Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.)

[Eric Carlson]

(In reply to comment #10)
> Eric, thanks. That's all great feedback - will apply.
> 
> >> Source/WebCore/ChangeLog:18
> >> +        * css/CSSDefaultStyleSheets.cpp:
> >> +        (WebCore::CSSDefaultStyleSheets::ensureDefaultStyleSheetsForElement):
> >> +        Allow mediaControlsStyleSheet to also style by siblingRules.
> >
> >A longer comment explaining why this is necessary would be nice. 
> 
> This is to allow me to use
> video::-webkit-media-controls-closed-captions-track-list li:first-child
> in the mediaControlsChromium.css style sheet.
> I could instead move this into a layout() function, though I thought this was easier to read.
> What do you think?
> 
I think this is fine, I was actually just requesting a longer explanation in the ChangeLog.

> 
> Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.)

It stills confuses me too, so you would probably be better off asking someone that understands renderers+theme better.

[Silvia Pfeiffer]

(In reply to comment #11)
> (In reply to comment #10)
> > Another architecture question: What do you think about the new paintMediaClosedCaptionsContainer() function? Would that be better in a MediaClosedCaptionsContainer::layout() function and overloaded for Chromium? (That whole painting pipeline with adjustxxx, layout(), paintxxx(), formatxxx functions still confuses me.)
> 
> It stills confuses me too, so you would probably be better off asking someone that understands renderers+theme better.


Can somebody with renderer experience provide some advice?

I've got a caption menu that is displayed when the CC button on the video controls is clicked. It needs custom styling just before it's being shown:
* adapt it's right offset to line up with the video controls right edge
* adapt the controls top right corner to not be rounded but align with the menu

I currently did this in a paintXXX function as part of paintMediaControlsPart, but this updates after painting, so we see an actual position change of the menu.

I'd prefer using  the adjustMediaControlStyle() function that Blackberry uses for this kind of stuff. However, we haven't used this in Chromium yet and I wonder if there is a reason.