Bug 101669

Summary:

Support list of tracks in caption media controls

Product:

WebKit

Reporter:

Dean Jackson <dino>

Component:

Media

Assignee:

Dean Jackson <dino>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, dglazkov, eric.carlson, feature-media-reviews, macpherson, menard, roger_fong, silviapf, webkit-bug-importer, webkit-ews, webkit.review.bot, zan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

101823, 101831, 101919

Bug Blocks:

88871, 101670

Attachments:

Description	Flags
Patch	none
Patch for build test	none
Rebased patch for build test	none
Rebased patch take 3	none
Rebased patch take 4	none

Dean Jackson

Reported 2012-11-08 17:13:13 PST

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

Attachments
Patch (33.32 KB, patch) 2012-11-09 15:16 PST, Dean Jackson	no flags	Details Formatted Diff Diff
Patch for build test (33.46 KB, patch) 2012-11-09 16:08 PST, Dean Jackson	no flags	Details Formatted Diff Diff
Rebased patch for build test (33.61 KB, patch) 2012-11-09 16:39 PST, Dean Jackson	no flags	Details Formatted Diff Diff
Rebased patch take 3 (33.65 KB, patch) 2012-11-09 17:00 PST, Dean Jackson	no flags	Details Formatted Diff Diff
Rebased patch take 4 (33.68 KB, patch) 2012-11-09 17:28 PST, Dean Jackson	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2012-11-08 17:14:08 PST

<rdar://problem/12667870>

Dean Jackson

Comment 2 2012-11-09 15:16:24 PST

Created attachment 173381 [details] Patch

Eric Carlson

Comment 3 2012-11-09 15:42:18 PST

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 4 2012-11-09 16:00:00 PST

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

Comment 5 2012-11-09 16:08:15 PST

Created attachment 173390 [details] Patch for build test

Dean Jackson

Comment 6 2012-11-09 16:39:43 PST

Created attachment 173398 [details] Rebased patch for build test

Early Warning System Bot

Comment 7 2012-11-09 16:50:00 PST

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 8 2012-11-09 16:50:06 PST

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

Comment 9 2012-11-09 17:00:37 PST

Created attachment 173404 [details] Rebased patch take 3

Build Bot

Comment 10 2012-11-09 17:14:30 PST

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

Comment 11 2012-11-09 17:28:46 PST

Created attachment 173413 [details] Rebased patch take 4

Silvia Pfeiffer

Comment 12 2012-11-09 18:12:27 PST

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

Comment 13 2012-11-09 18:14:04 PST

(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 14 2012-11-09 18:18:38 PST

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

Comment 15 2012-11-09 18:25:22 PST

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

Silvia Pfeiffer

Comment 16 2012-11-09 18:33:42 PST

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

Comment 17 2012-11-09 23:02:35 PST

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

Dean Jackson

Comment 18 2012-11-10 21:22:38 PST

(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

Comment 19 2012-11-11 04:27:28 PST

(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

Comment 20 2012-11-12 03:20:08 PST

Re-opened since this is blocked by bug 101919

Zan Dobersek

Comment 21 2012-11-13 09:20:57 PST

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

Comment 22 2012-11-13 16:37:23 PST

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

Note You need to log in before you can comment on or make changes to this bug.