Bug 80660 – Support W3C Full Screen API proposal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Support W3C Full Screen API proposal

[Jer Noble]

The current Full Screen API proposal can be found here: <http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html>

[Jer Noble]

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.

[WebKit Review Bot]

Comment on attachment 130986 [details]
Patch

Attachment 130986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894459

[Jer Noble]

Created attachment 131041 [details]
Patch

Added accessors to RuntimeEnabledFeatures for the new .idl functions.

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 131042 [details]
Patch

Removed the (empty and extraneous) ChangeLog entry.

[WebKit Review Bot]

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

[Jer Noble]

Created attachment 131128 [details]
Patch

[WebKit Review Bot]

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

[Jer Noble]

Created attachment 131380 [details]
Patch

[Build Bot]

Comment on attachment 131380 [details]
Patch

Attachment 131380 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11949241

[Jer Noble]

Created attachment 131548 [details]
Patch

Fixed windows 'possible uninitialized variable use' compile error.

[Jer Noble]

<rdar://problem/10725086>

[Gustavo Noronha (kov)]

Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11948328

[WebKit Review Bot]

Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11949273

[Build Bot]

Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11948351

[Jer Noble]

Created attachment 131563 [details]
Patch

One final compilation fix.

[Alexey Proskuryakov]

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.

[Jer Noble]

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!

[Jer Noble]

Committed r111028: <http://trac.webkit.org/changeset/111028>

[Darin Fisher (:fishd, Google)]

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?

[Jeremy Apthorp]

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).

[Jer Noble]

(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.

[Jer Noble]

*** Bug 68817 has been marked as a duplicate of this bug. ***