Summary: | [GTK] youtube HTML5 videos in fullscreen, after <Esc>, can't go fullscreen again | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Víctor M. Jáquez L. <vjaquez> | ||||||||||||||||||
Component: | WebKitGTK | Assignee: | Víctor M. Jáquez L. <vjaquez> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, calvaris, cgarcia, commit-queue, dbates, gustavo, mrobinson, pnormand, rniwa, thorton | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Víctor M. Jáquez L.
2014-01-15 13:47:36 PST
I still don't discard that it may be a bug in the youtube's player. I'll do that tomorrow. Is this with their HTML5 or Flash player? (In reply to comment #2) > Is this with their HTML5 or Flash player? HTML5 oddly enough, the bad behavior is only shown in webkit2, not in webkit1 with GtkLaunch. Using GtkLaunch the behavoir is as expected. Created attachment 221475 [details]
testcase
this test web page is to corner the bug
The error is simple: when pressing <Esc> to cancel fullscreen, the event webkitfullscreenchange is not emitted. Again, this only happens in webkit2. The thing is this: When pressing the <Esc>, at the function Document::dispatchFullScreenChangeOrErrorEvent(), the queue is empty, hence the event webkitfullscreenchange is not dispatched. Why is it empty? Created attachment 221661 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API This odd: running the layout test inside MiniBrowser, GtkLaunch or DTR, it works as expected. But, running it with WKTR, it times out, as if the eventSender doesn't send the keyDown correctly. Comment on attachment 221661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review Looks good to me. Added some nits. > Source/WebKit2/ChangeLog:10 > + This is a nice description of the fix, but I miss some context, like the actual explanation of why it was failing (I understand it should be related to the event emission but it isn't a direct outcome of the description) > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 > +} I guess the function does not make sense at all without the FULLSCREEN_API. It might be better to guard it all (after all the caller is already guarded) > LayoutTests/fullscreen/full-screen-escape.html:1 > +<body> It's a good practice to start the file with <!DOCTYPE html> > LayoutTests/fullscreen/full-screen-escape.html:37 > + runWithKeyDown(function(){span.webkitRequestFullScreen()}); Add some spaces between the call and the brackets, also after the parentheses. Created attachment 221666 [details]
Patch
Comment on attachment 221661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review >> Source/WebKit2/ChangeLog:10 >> + > > This is a nice description of the fix, but I miss some context, like the actual explanation of why it was failing (I understand it should be related to the event emission but it isn't a direct outcome of the description) done >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1072 >> +} > > I guess the function does not make sense at all without the FULLSCREEN_API. It might be better to guard it all (after all the caller is already guarded) I'm following what the other functions related with full screen do: keep the function signature but guard the content. >> LayoutTests/fullscreen/full-screen-escape.html:1 >> +<body> > > It's a good practice to start the file with <!DOCTYPE html> done >> LayoutTests/fullscreen/full-screen-escape.html:37 >> + runWithKeyDown(function(){span.webkitRequestFullScreen()}); > > Add some spaces between the call and the brackets, also after the parentheses. done (In reply to comment #11) > (From update of attachment 221661 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221661&action=review Thanks for the promptly review! Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6274908631859200 New failing tests: fullscreen/full-screen-escape.html Created attachment 221673 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6693187779297280 New failing tests: fullscreen/full-screen-escape.html Created attachment 221675 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review > LayoutTests/fullscreen/full-screen-escape.html:16 > + var fullscreenChanged = function(event) > + { > + if (callback) > + callback(event) > + }; Not self-consistent with curly brace placement below. > LayoutTests/fullscreen/full-screen-escape.html:25 > + setTimeout(exitFullScreen, 100); Why the 100 millisecond timeout? Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review >> LayoutTests/fullscreen/full-screen-escape.html:25 >> + setTimeout(exitFullScreen, 100); > > Why the 100 millisecond timeout? This is an arbitrary value, just to make a pause before exiting the full screen mode. (In reply to comment #20) > (From update of attachment 221666 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review > > >> LayoutTests/fullscreen/full-screen-escape.html:25 > >> + setTimeout(exitFullScreen, 100); > > > > Why the 100 millisecond timeout? > > This is an arbitrary value, just to make a pause before exiting the full screen mode. Why is the pause necessary? Comment on attachment 221666 [details] Patch Attachment 221666 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5110401599537152 New failing tests: fullscreen/full-screen-escape.html Created attachment 221679 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 221666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221666&action=review >>>> LayoutTests/fullscreen/full-screen-escape.html:25 >>>> + setTimeout(exitFullScreen, 100); >>> >>> Why the 100 millisecond timeout? >> >> This is an arbitrary value, just to make a pause before exiting the full screen mode. > > Why is the pause necessary? Actually, it is not necessary, just inlining exitFullScreen in there should be enough. What worries me is this test doesn't work in any platform: in safari doesn't because eventSent.keyDown doesn't work, and in gtk, the same function doesn't work in fullscreen mode (to add the bug is in my todo). Perhaps I should remove the test. Since none of the platforms implements correctly the sendEvent.keyDown in WKTR when an element is in full screen mode, I'll move out the test into ManualTests Created attachment 221748 [details]
Patch
Comment on attachment 221748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221748&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1071 > + WebFullScreenManagerProxy* fullScreenManagerProxy = webkitWebViewBase->priv->pageProxy->fullScreenManager(); > + fullScreenManagerProxy->requestExitFullScreen(); > +#endif This can be one line. Created attachment 222146 [details]
Patch
Comment on attachment 222146 [details] Patch Clearing flags on attachment: 222146 Committed r162724: <http://trac.webkit.org/changeset/162724> All reviewed patches have been landed. Closing bug. Comment on attachment 222146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222146&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:48 > +void webkitWebViewBaseRequestExitFullScreen(WebKitWebViewBase*); Why are you adding private API that is only used in WebKitWebViewBase.cpp? This should be a static method, or you could even avoid the method entirely that is used only once and use priv->pageProxy->fullScreenManager()->requestExitFullScreen(); directly Comment on attachment 222146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222146&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h:48 >> +void webkitWebViewBaseRequestExitFullScreen(WebKitWebViewBase*); > > Why are you adding private API that is only used in WebKitWebViewBase.cpp? This should be a static method, or you could even avoid the method entirely that is used only once and use priv->pageProxy->fullScreenManager()->requestExitFullScreen(); directly I followed the code structure saw in webkitWebViewBaseExitFullScreen(), that's why. |