[webkit-reviews] review granted: [Bug 101669] Support list of tracks in caption media controls : [Attachment 173381] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 101669: Support list of tracks in caption media controls
https://bugs.webkit.org/show_bug.cgi?id=101669

Attachment 173381: Patch
https://bugs.webkit.org/attachment.cgi?id=173381&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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,iVBORw0KGgoAAAANSUhEUgAAADYAAAA9CAYAAADmgpoeAAAAGXRF
WHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyJpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAA
AADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4On
htcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuM
C1jMDYwIDYxLjEzNDc3NywgMjAxMC8wMi8xMi0xNzozMjowMCAgICAgICAgIj4gPHJkZjpSREYgeG1s
bnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjp
EZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC
8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c
3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpD
cmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNSBNYWNpbnRvc2giIHhtcE1NOkluc3RhbmNlSUQ
9InhtcC5paWQ6RTBEQkI5QzgyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiIHhtcE1NOkRvY3VtZW50SU
Q9InhtcC5kaWQ6RTBEQkI5QzkyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiPiA8eG1wTU06RGVyaXZlZ
EZyb20gc3RSZWY6aW5zdGFuY2VJRD0ieG1wLmlpZDpFMERCQjlDNjIxNzgxMUUyQkREN0FGMjc0RDA3
NkRGMCIgc3RSZWY6ZG9jdW1lbnRJRD0ieG1wLmRpZDpFMERCQjlDNzIxNzgxMUUyQkREN0FGMjc0RDA
3NkRGMCIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2
V0IGVuZD0iciI/PqpWz0UAAAHPSURBVHja7JqBbYQwDEXdDRghI7BBGSEjZARGyAaMwAg3AtcJMkJug
4xAUymqTggVO1yJbZ0lCyGd0D3Z/Pw4fICAWNfV5IvN+Zmzz2k2P7mX61fOx9M9X6Ccy0qLOWfHGcqt
9Ji4V6oGyr2hGkD1GqG6nFEVVAGbNEIN6qAKWKCsU1KgRgLUIgXqRzASEiqydhQbME+oVi8FyhCqNYK
UKGZVz3v1VC1MpLJlUVetUWO1AkgKgnUaJEFh161ZWrW8OsEoYJhqeWlQDlmtThpY1FitQWu1bhqVEL
sgi1NCr65aBNEYrmibefNnQrFBpuJ5tum2pFgdjOOeKMqFFA33n1CUQWXAwJXnHko8B1dAgkNOn/wVL
zkVbnnBrNBcpWBUOH9i7bpxHontjsWQm0nLeSaxO8hEiFFsubCGmpZEnnH5lmCU8fNvSyLbsK0vJB7v
LMhK8xiAEsXEN3MalXDUby3aOI1Kc5xeAMZve1L5mYKMo6CTLRmBa5xsSd4HDMQzY1kzjYqWFHMoTm1
JB1KCuHCLG4QGdvuuC72kBYlx4OYTSI2D7c0MkuOPtc2C9NgRkgQaYkdIJtASm5F2rwnMsHfyJ+V/0g
jWcWjDbwEGAN9/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,iVBORw0KGgoAAAANSUhEUgAAADYAAAA9CAYAAADmgpoeAAAAGXRF
WHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAyJpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAA
AADw/eHBhY2tldCBiZWdpbj0i77u/IiBpZD0iVzVNME1wQ2VoaUh6cmVTek5UY3prYzlkIj8+IDx4On
htcG1ldGEgeG1sbnM6eD0iYWRvYmU6bnM6bWV0YS8iIHg6eG1wdGs9IkFkb2JlIFhNUCBDb3JlIDUuM
C1jMDYwIDYxLjEzNDc3NywgMjAxMC8wMi8xMi0xNzozMjowMCAgICAgICAgIj4gPHJkZjpSREYgeG1s
bnM6cmRmPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5LzAyLzIyLXJkZi1zeW50YXgtbnMjIj4gPHJkZjp
EZXNjcmlwdGlvbiByZGY6YWJvdXQ9IiIgeG1sbnM6eG1wPSJodHRwOi8vbnMuYWRvYmUuY29tL3hhcC
8xLjAvIiB4bWxuczp4bXBNTT0iaHR0cDovL25zLmFkb2JlLmNvbS94YXAvMS4wL21tLyIgeG1sbnM6c
3RSZWY9Imh0dHA6Ly9ucy5hZG9iZS5jb20veGFwLzEuMC9zVHlwZS9SZXNvdXJjZVJlZiMiIHhtcDpD
cmVhdG9yVG9vbD0iQWRvYmUgUGhvdG9zaG9wIENTNSBNYWNpbnRvc2giIHhtcE1NOkluc3RhbmNlSUQ
9InhtcC5paWQ6RTBEQkI5QzgyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiIHhtcE1NOkRvY3VtZW50SU
Q9InhtcC5kaWQ6RTBEQkI5QzkyMTc4MTFFMkJERDdBRjI3NEQwNzZERjAiPiA8eG1wTU06RGVyaXZlZ
EZyb20gc3RSZWY6aW5zdGFuY2VJRD0ieG1wLmlpZDpFMERCQjlDNjIxNzgxMUUyQkREN0FGMjc0RDA3
NkRGMCIgc3RSZWY6ZG9jdW1lbnRJRD0ieG1wLmRpZDpFMERCQjlDNzIxNzgxMUUyQkREN0FGMjc0RDA
3NkRGMCIvPiA8L3JkZjpEZXNjcmlwdGlvbj4gPC9yZGY6UkRGPiA8L3g6eG1wbWV0YT4gPD94cGFja2
V0IGVuZD0iciI/PqpWz0UAAAHPSURBVHja7JqBbYQwDEXdDRghI7BBGSEjZARGyAaMwAg3AtcJMkJug
4xAUymqTggVO1yJbZ0lCyGd0D3Z/Pw4fICAWNfV5IvN+Zmzz2k2P7mX61fOx9M9X6Ccy0qLOWfHGcqt
9Ji4V6oGyr2hGkD1GqG6nFEVVAGbNEIN6qAKWKCsU1KgRgLUIgXqRzASEiqydhQbME+oVi8FyhCqNYK
UKGZVz3v1VC1MpLJlUVetUWO1AkgKgnUaJEFh161ZWrW8OsEoYJhqeWlQDlmtThpY1FitQWu1bhqVEL
sgi1NCr65aBNEYrmibefNnQrFBpuJ5tum2pFgdjOOeKMqFFA33n1CUQWXAwJXnHko8B1dAgkNOn/wVL
zkVbnnBrNBcpWBUOH9i7bpxHontjsWQm0nLeSaxO8hEiFFsubCGmpZEnnH5lmCU8fNvSyLbsK0vJB7v
LMhK8xiAEsXEN3MalXDUby3aOI1Kc5xeAMZve1L5mYKMo6CTLRmBa5xsSd4HDMQzY1kzjYqWFHMoTm1
JB1KCuHCLG4QGdvuuC72kBYlx4OYTSI2D7c0MkuOPtc2C9NgRkgQaYkdIJtASm5F2rwnMsHfyJ+V/0g
jWcWjDbwEGAN9/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.