MediaControls::show() should make controls opaque
Created attachment 181221 [details] Patch
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.
Created attachment 181322 [details] Patch
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.
Created attachment 181324 [details] Patch
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.
Created attachment 181355 [details] Patch
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
Created attachment 181398 [details] Patch
Comment on attachment 181398 [details] Patch Clearing flags on attachment: 181398 Committed r138902: <http://trac.webkit.org/changeset/138902>
All reviewed patches have been landed. Closing bug.