Bug 106034

Summary: MediaControls::show() should make controls opaque
Product: WebKit Reporter: Min Qin <qinmin>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, eric.carlson, feature-media-reviews, ojan.autocc, s.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Min Qin 2013-01-03 13:56:00 PST
MediaControls::show() should make controls opaque
Comment 1 Min Qin 2013-01-03 14:26:27 PST
Created attachment 181221 [details]
Patch
Comment 2 Eric Carlson 2013-01-04 09:44:15 PST
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.
Comment 3 Min Qin 2013-01-04 10:12:13 PST
Created attachment 181322 [details]
Patch
Comment 4 Min Qin 2013-01-04 10:18:12 PST
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.
Comment 5 Min Qin 2013-01-04 10:20:49 PST
Created attachment 181324 [details]
Patch
Comment 6 Eric Carlson 2013-01-04 12:05:46 PST
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.
Comment 7 Min Qin 2013-01-04 12:21:55 PST
Created attachment 181355 [details]
Patch
Comment 8 Min Qin 2013-01-04 12:22:40 PST
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
Comment 9 Min Qin 2013-01-04 15:41:03 PST
Created attachment 181398 [details]
Patch
Comment 10 WebKit Review Bot 2013-01-05 12:50:35 PST
Comment on attachment 181398 [details]
Patch

Clearing flags on attachment: 181398

Committed r138902: <http://trac.webkit.org/changeset/138902>
Comment 11 WebKit Review Bot 2013-01-05 12:50:39 PST
All reviewed patches have been landed.  Closing bug.