Support W3C Full Screen API proposal
The current Full Screen API proposal can be found here: <http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html>
Created attachment 130986 [details] Patch More detailed ChangeLogs to come; I just wanted to give the EWS bots a chance to chew on this patch.
Comment on attachment 130986 [details] Patch Attachment 130986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11894459
Created attachment 131041 [details] Patch Added accessors to RuntimeEnabledFeatures for the new .idl functions.
Attachment 131041 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1 Source/WebCore/ChangeLog:7: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 131042 [details] Patch Removed the (empty and extraneous) ChangeLog entry.
Comment on attachment 131042 [details] Patch Attachment 131042 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11892623 New failing tests: fullscreen/full-screen-remove-ancestor-after.html fullscreen/video-controls-override.html fullscreen/full-screen-cancel.html fullscreen/full-screen-zIndex-after.html fullscreen/full-screen-remove-ancestor.html fullscreen/full-screen-placeholder.html fullscreen/full-screen-remove-children.html fullscreen/full-screen-remove.html
Created attachment 131128 [details] Patch
Comment on attachment 131128 [details] Patch Attachment 131128 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11903957 New failing tests: fullscreen/full-screen-twice.html fullscreen/full-screen-css.html
Created attachment 131380 [details] Patch
Comment on attachment 131380 [details] Patch Attachment 131380 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11949241
Created attachment 131548 [details] Patch Fixed windows 'possible uninitialized variable use' compile error.
<rdar://problem/10725086>
Comment on attachment 131548 [details] Patch Attachment 131548 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11948328
Comment on attachment 131548 [details] Patch Attachment 131548 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11949273
Comment on attachment 131548 [details] Patch Attachment 131548 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11948351
Created attachment 131563 [details] Patch One final compilation fix.
Comment on attachment 131563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131563&action=review > Source/WebCore/dom/Document.cpp:5105 > + // 2. Let doc be element's node document. (i.e. "this") > + Document* doc = this; We prefer to not abbreviate words in variable names. > Source/WebCore/dom/Document.cpp:5148 > + } while (++current != docs.end()); So, Deque iterator is not invalidated for any operations you do above? > Source/WebCore/dom/Document.cpp:5485 > +void Document::addDocumentToEventQueue(Document* doc) Please have "fullscreen" somewhere in this name. It's super confusing otherwise. As elsewhere, "doc" is an unwelcome abbreviation. > Source/WebKit/mac/WebView/WebView.mm:6287 > - (BOOL)_supportsFullScreenForElement:(const WebCore::Element*)element withKeyboard:(BOOL)withKeyboard I'm not sure if I fully understand why this function no longer looks at its withKeyboard argument. But if it doesn't need to look at it, can it be just removed? > Source/WebKit2/UIProcess/WebFullScreenManagerProxy.cpp:-94 > void WebFullScreenManagerProxy::supportsFullScreen(bool withKeyboard, bool& supports) > { > - if (withKeyboard) Ditto.
Comment on attachment 131563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131563&action=review >> Source/WebCore/dom/Document.cpp:5105 >> + Document* doc = this; > > We prefer to not abbreviate words in variable names. I know, I was just trying to hew to the spec. I'll change it though. :) >> Source/WebCore/dom/Document.cpp:5148 >> + } while (++current != docs.end()); > > So, Deque iterator is not invalidated for any operations you do above? Nope. the "docs" queue isn't modified in the loop. >> Source/WebCore/dom/Document.cpp:5485 >> +void Document::addDocumentToEventQueue(Document* doc) > > Please have "fullscreen" somewhere in this name. It's super confusing otherwise. > > As elsewhere, "doc" is an unwelcome abbreviation. Yep. I'll fix this as well. >> Source/WebKit/mac/WebView/WebView.mm:6287 >> - (BOOL)_supportsFullScreenForElement:(const WebCore::Element*)element withKeyboard:(BOOL)withKeyboard > > I'm not sure if I fully understand why this function no longer looks at its withKeyboard argument. But if it doesn't need to look at it, can it be just removed? It probably can, but I'd prefer to do it in a separate patch. It should be removed both from here and WebKit2 (as you point out below), which is a big enough change to warrant doing in a new bug. Filed bug# 81367. Thanks!
Committed r111028: <http://trac.webkit.org/changeset/111028>
Perhaps it is time to factor fullscreen code out into a separate controller class? It seems really unfortunate to clutter Document.cpp up with all of this code. Am I crazy for thinking this way?
After this patch I no longer see 'webkitfullscreenchange' events when the user exits fullscreen. Is this intentional? It breaks many websites (e.g. http://www.youtube.com/watch?v=0QRO3gKj3qw&html5=1, http://vimeo.com/36258512, http://sublimevideo.net/demo).
(In reply to comment #22) > After this patch I no longer see 'webkitfullscreenchange' events when the user exits fullscreen. Is this intentional? It breaks many websites (e.g. http://www.youtube.com/watch?v=0QRO3gKj3qw&html5=1, http://vimeo.com/36258512, http://sublimevideo.net/demo). Yep, there's a bug on it awaiting review: bug #82755.
*** Bug 68817 has been marked as a duplicate of this bug. ***