[webkit-reviews] review granted: [Bug 106034] MediaControls::show() should make controls opaque : [Attachment 181324] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Min Qin
<qinmin at chromium.org>'s request for review:
Bug 106034: MediaControls::show() should make controls opaque
https://bugs.webkit.org/show_bug.cgi?id=106034

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181324&action=review


> Source/WebCore/ChangeLog:9
> +	   If the user exits fullscreen while the timer expires, webkit will
call mediaControls::show(). However, show() actually displays nothing as the
control is transparent.

Nit: this line is still twice as long as the others.

> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:21
> +	   function oncanplaythrough() {

Nit: a function's opening brace should be on a new line.

> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:47
> +	       if (opacity < 1) {
> +		   failTest("Media control is not opaque.");
> +	       } else {
> +		   runWithKeyDown(function(){ video.webkitRequestFullscreen();
});

Nit: a single line if statement doesn't need a brace.

> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:51
> +	   function onfullscreenchange() {

Nit: a function's opening brace should be on a new line.

> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:64
> +		       if (opacity < 1) {
> +			   failTest("Media control is not opaque.");
> +		       } else {
> +			   endTest();
> +		       }

Nit: a single line if statement doesn't need a brace.