Bug 106034 – MediaControls::show() should make controls opaque

• Component: Media
• Status: RESOLVED FIXED

[Min Qin]

MediaControls::show() should make controls opaque

[Min Qin]

Created attachment 181221 [details]
Patch

[Eric Carlson]

Comment on attachment 181221 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        When a video enters fullscreen, webkit starts a timer to make the control transparent when the timer expires. If the user exits fullscreen while the timer expires, webkit will call mediaControls::show(). However, show() actually displays nothing as the control is transparent. The user had to move his mouse outside the video rect and move it back in order to show the controls. Simply clicking/moving the mouse inside the video rect will not make the control opaque.

Please wrap this line, not every code editor is set to auto-wrap.

> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:11
> +        var fadeoutTime = 4000;

Four seconds is a very long time for a layout test. Does it need to be this long?

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

Nit: It seems strange to have some functions declared as a variable with an anonymous function and some with a function declaration.

[Min Qin]

Created attachment 181322 [details]
Patch

[Min Qin]

Comment on attachment 181221 [details]
Patch

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

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:11
>> +        var fadeoutTime = 4000;
> 
> Four seconds is a very long time for a layout test. Does it need to be this long?

Currently the webkit fullscreen timer is set to 3 seconds to fade out the media control. And we need some time for the fading animation. 
I can file a seperate bug later to make this webkit fullscreen timeout value configurable.

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:21
>> +        var oncanplaythrough = function() {
> 
> Nit: It seems strange to have some functions declared as a variable with an anonymous function and some with a function declaration.

Done, changed everything to function declaration.

[Min Qin]

Created attachment 181324 [details]
Patch

[Eric Carlson]

Comment on attachment 181324 [details]
Patch

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.

[Min Qin]

Created attachment 181355 [details]
Patch

[Min Qin]

Comment on attachment 181324 [details]
Patch

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.

fixed

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:21
>> +        function oncanplaythrough() {
> 
> Nit: a function's opening brace should be on a new line.

fixed

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:47
>> +                runWithKeyDown(function(){ video.webkitRequestFullscreen(); });
> 
> Nit: a single line if statement doesn't need a brace.

fixed

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:51
>> +        function onfullscreenchange() {
> 
> Nit: a function's opening brace should be on a new line.

fixed

>> LayoutTests/media/video-controls-visible-exiting-fullscreen.html:64
>> +                    }
> 
> Nit: a single line if statement doesn't need a brace.

fixed

[Min Qin]

Created attachment 181398 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181398 [details]
Patch

Clearing flags on attachment: 181398

Committed r138902: <http://trac.webkit.org/changeset/138902>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.