Bug 101669 – Support list of tracks in caption media controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dean Jackson]

Our closed caption media controls should support a list of tracks.

[Radar WebKit Bug Importer]

<rdar://problem/12667870>

[Dean Jackson]

Created attachment 173381 [details]
Patch

[Eric Carlson]

Comment on attachment 173381 [details]
Patch

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

Marking r+, but you should probably let the EWS bot chew on it for to make sure all platforms compile. It may also be necessary to update some test results.

> Source/WebCore/css/mediaControlsQuickTime.css:319
> +    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADYAAAA9CAYAAADmgpoeAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyJpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMC1jMDYwIDYxLjEzNDc3NywgMjAxMC8wMi8xMi0xNzozMjowMCAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNSBNYWNpbnRvc2giIHhtcE1NOkluc3RhbmNlSUQ9InhtcC5paWQ6RTBEQkI5QzgyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiIHhtcE1NOkRvY3VtZW50SUQ9InhtcC5kaWQ6RTBEQkI5QzkyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiPiA8eG1wTU06RGVyaXZlZEZyb20gc3RSZWY6aW5zdGFuY2VJRD0ieG1wLmlpZDpFMERCQjlDNjIxNzgxMUUyQkREN0FGMjc0RDA3NkRGMCIgc3RSZWY6ZG9jdW1lbnRJRD0ieG1wLmRpZDpFMERCQjlDNzIxNzgxMUUyQkREN0FGMjc0RDA3NkRGMCIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2V0IGVuZD0iciI/PqpWz0UAAAHPSURBVHja7JqBbYQwDEXdDRghI7BBGSEjZARGyAaMwAg3AtcJMkJug4xAUymqTggVO1yJbZ0lCyGd0D3Z/Pw4fICAWNfV5IvN+Zmzz2k2P7mX61fOx9M9X6Ccy0qLOWfHGcqt9Ji4V6oGyr2hGkD1GqG6nFEVVAGbNEIN6qAKWKCsU1KgRgLUIgXqRzASEiqydhQbME+oVi8FyhCqNYKUKGZVz3v1VC1MpLJlUVetUWO1AkgKgnUaJEFh161ZWrW8OsEoYJhqeWlQDlmtThpY1FitQWu1bhqVELsgi1NCr65aBNEYrmibefNnQrFBpuJ5tum2pFgdjOOeKMqFFA33n1CUQWXAwJXnHko8B1dAgkNOn/wVLzkVbnnBrNBcpWBUOH9i7bpxHontjsWQm0nLeSaxO8hEiFFsubCGmpZEnnH5lmCU8fNvSyLbsK0vJB7vLMhK8xiAEsXEN3MalXDUby3aOI1Kc5xeAMZve1L5mYKMo6CTLRmBa5xsSd4HDMQzY1kzjYqWFHMoTm1JB1KCuHCLG4QGdvuuC72kBYlx4OYTSI2D7c0MkuOPtc2C9NgRkgQaYkdIJtASm5F2rwnMsHfyJ+V/0gjWcWjDbwEGAN9/NX8H7V/FAAAAAElFTkSuQmCC');

This would look much better when scaled if it was SVG instead of PNG. Fine to do in a follow up.

> Source/WebCore/css/mediaControlsQuickTime.css:327
> +    background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADYAAAA9CAYAAADmgpoeAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyJpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4OnhtcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuMC1jMDYwIDYxLjEzNDc3NywgMjAxMC8wMi8xMi0xNzozMjowMCAgICAgICAgIj4gPHJkZjpSREYgeG1sbnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjpEZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpDcmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNSBNYWNpbnRvc2giIHhtcE1NOkluc3RhbmNlSUQ9InhtcC5paWQ6RTBEQkI5QzgyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiIHhtcE1NOkRvY3VtZW50SUQ9InhtcC5kaWQ6RTBEQkI5QzkyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiPiA8eG1wTU06RGVyaXZlZEZyb20gc3RSZWY6aW5zdGFuY2VJRD0ieG1wLmlpZDpFMERCQjlDNjIxNzgxMUUyQkREN0FGMjc0RDA3NkRGMCIgc3RSZWY6ZG9jdW1lbnRJRD0ieG1wLmRpZDpFMERCQjlDNzIxNzgxMUUyQkREN0FGMjc0RDA3NkRGMCIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2V0IGVuZD0iciI/PqpWz0UAAAHPSURBVHja7JqBbYQwDEXdDRghI7BBGSEjZARGyAaMwAg3AtcJMkJug4xAUymqTggVO1yJbZ0lCyGd0D3Z/Pw4fICAWNfV5IvN+Zmzz2k2P7mX61fOx9M9X6Ccy0qLOWfHGcqt9Ji4V6oGyr2hGkD1GqG6nFEVVAGbNEIN6qAKWKCsU1KgRgLUIgXqRzASEiqydhQbME+oVi8FyhCqNYKUKGZVz3v1VC1MpLJlUVetUWO1AkgKgnUaJEFh161ZWrW8OsEoYJhqeWlQDlmtThpY1FitQWu1bhqVELsgi1NCr65aBNEYrmibefNnQrFBpuJ5tum2pFgdjOOeKMqFFA33n1CUQWXAwJXnHko8B1dAgkNOn/wVLzkVbnnBrNBcpWBUOH9i7bpxHontjsWQm0nLeSaxO8hEiFFsubCGmpZEnnH5lmCU8fNvSyLbsK0vJB7vLMhK8xiAEsXEN3MalXDUby3aOI1Kc5xeAMZve1L5mYKMo6CTLRmBa5xsSd4HDMQzY1kzjYqWFHMoTm1JB1KCuHCLG4QGdvuuC72kBYlx4OYTSI2D7c0MkuOPtc2C9NgRkgQaYkdIJtASm5F2rwnMsHfyJ+V/0gjWcWjDbwEGAN9/NX8H7V/FAAAAAElFTkSuQmCC'), -webkit-gradient(linear, left top, left bottom, color-stop(0, rgb(79, 112, 245)), color-stop(1, rgb(28, 66, 245)));

Ditto.

> Source/WebCore/html/shadow/MediaControlElements.cpp:903
> +    setDisplayType(mediaController()->muted() ? MediaHideClosedCaptionsButton : MediaShowClosedCaptionsButton);

Oops :-)

> Source/WebCore/html/shadow/MediaControlElements.cpp:977
> +    RefPtr<Element> captionsSection = doc->createElement(sectionTag, ec);
> +    RefPtr<Element> captionsHeader = doc->createElement(h3Tag, ec);
> +    captionsHeader->appendChild(doc->createTextNode("Closed Captions"));
> +    captionsSection->appendChild(captionsHeader);
> +    RefPtr<Element> captionsList = doc->createElement(ulTag, ec);

Because you don't check these exceptions, it would be better to use ASSERT_NO_EXCEPTION.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1007
> +        AtomicString labelText = track->label();
> +        if (labelText.isNull() || labelText.isEmpty())
> +            labelText = displayNameForLanguageLocale(track->language());

Subtitle tracks should use language, captions should use label.

> Source/WebCore/html/shadow/MediaControls.h:73
> +    virtual void toggleClosedCaptionTrackList() = 0;

I wouldn't declare this pure virtual so other ports don't *need* to implement it until they are ready.

[Dean Jackson]

Comment on attachment 173381 [details]
Patch

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

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1007
>> +            labelText = displayNameForLanguageLocale(track->language());
> 
> Subtitle tracks should use language, captions should use label.

After some discussion on IRC, we decided that if a label has been specified then it should be used in all cases. It would be quite common for multiple subtitle tracks with the same language, but slightly different content e.g. English v English for hearing impaired.

[Dean Jackson]

Created attachment 173390 [details]
Patch for build test

[Dean Jackson]

Created attachment 173398 [details]
Rebased patch for build test

[Early Warning System Bot]

Comment on attachment 173398 [details]
Rebased patch for build test

Attachment 173398 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14794148

[Early Warning System Bot]

Comment on attachment 173398 [details]
Rebased patch for build test

Attachment 173398 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14779606

[Dean Jackson]

Created attachment 173404 [details]
Rebased patch take 3

[Build Bot]

Comment on attachment 173404 [details]
Rebased patch take 3

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

[Dean Jackson]

Created attachment 173413 [details]
Rebased patch take 4

[Silvia Pfeiffer]

I've waited with preparing such a patch for Chromium until I've redone the way the controls work, see bug 88871. Right now it's really easy to break other ports.

[Silvia Pfeiffer]

(In reply to comment #4)
> 
> After some discussion on IRC, we decided that if a label has been specified then it should be used in all cases. It would be quite common for multiple subtitle tracks with the same language, but slightly different content e.g. English v English for hearing impaired.

FWIW I agree with this decision - the label has been added to allow for human-provided description of tracks no matter what the tracks are.

[WebKit Review Bot]

Comment on attachment 173413 [details]
Rebased patch take 4

Attachment 173413 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14660584

[Roger Fong]

Also broke the Windows tests:
http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/29777

[Silvia Pfeiffer]

BTW: I think it should be possible to create a layout test for this. I would define multiple <track> elements and initiate a click on the captions button (see controls-drag-timebar.html for an example) and see if the menu is displayed as expected.

[Dean Jackson]

Committed r134154: <http://trac.webkit.org/changeset/134154>

[Dean Jackson]

(In reply to comment #16)
> BTW: I think it should be possible to create a layout test for this. I would define multiple <track> elements and initiate a click on the captions button (see controls-drag-timebar.html for an example) and see if the menu is displayed as expected.

Thanks Silvia. I'll add a test in http://wkb.ug/101670

[Silvia Pfeiffer]

(In reply to comment #18)
> (In reply to comment #16)
> > BTW: I think it should be possible to create a layout test for this. I would define multiple <track> elements and initiate a click on the captions button (see controls-drag-timebar.html for an example) and see if the menu is displayed as expected.
> 
> Thanks Silvia. I'll add a test in http://wkb.ug/101670

Thanks, much appreciated!

[WebKit Review Bot]

Re-opened since this is blocked by bug 101919

[Zan Dobersek]

For what it's worth, I've applied both the patch in bug #88871 and the last patch attached here, and the crashes that induced the rollout in bug #101919 are not reproducible.

[Dean Jackson]

Committed r134507: <http://trac.webkit.org/changeset/134507>