Our closed caption media controls should support a list of tracks.
<rdar://problem/12667870>
Created attachment 173381 [details] Patch
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.
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.
Created attachment 173390 [details] Patch for build test
Created attachment 173398 [details] Rebased patch for build test
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
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
Created attachment 173404 [details] Rebased patch take 3
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
Created attachment 173413 [details] Rebased patch take 4
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.
(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.
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
Also broke the Windows tests: http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/29777
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.
Committed r134154: <http://trac.webkit.org/changeset/134154>
(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
(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!
Re-opened since this is blocked by bug 101919
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.
Committed r134507: <http://trac.webkit.org/changeset/134507>