Bug 80660 - Support W3C Full Screen API proposal
Summary: Support W3C Full Screen API proposal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL: http://dvcs.w3.org/hg/fullscreen/raw-...
Keywords: InRadar
: 68817 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-03-08 17:59 PST by Jer Noble
Modified: 2015-04-27 14:35 PDT (History)
9 users (show)

See Also:


Attachments
Patch (38.62 KB, patch)
2012-03-08 23:10 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (40.71 KB, patch)
2012-03-09 08:36 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (40.54 KB, patch)
2012-03-09 08:43 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (42.85 KB, patch)
2012-03-09 16:09 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (42.55 KB, patch)
2012-03-12 12:30 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (43.67 KB, patch)
2012-03-12 22:39 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (43.67 KB, patch)
2012-03-13 00:25 PDT, Jer Noble
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-03-08 17:59:06 PST
Support W3C Full Screen API proposal
Comment 1 Jer Noble 2012-03-08 18:00:14 PST
The current Full Screen API proposal can be found here: <http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html>
Comment 2 Jer Noble 2012-03-08 23:10:46 PST
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 3 WebKit Review Bot 2012-03-08 23:53:39 PST
Comment on attachment 130986 [details]
Patch

Attachment 130986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894459
Comment 4 Jer Noble 2012-03-09 08:36:26 PST
Created attachment 131041 [details]
Patch

Added accessors to RuntimeEnabledFeatures for the new .idl functions.
Comment 5 WebKit Review Bot 2012-03-09 08:38:30 PST
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.
Comment 6 Jer Noble 2012-03-09 08:43:19 PST
Created attachment 131042 [details]
Patch

Removed the (empty and extraneous) ChangeLog entry.
Comment 7 WebKit Review Bot 2012-03-09 09:43:45 PST
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
Comment 8 Jer Noble 2012-03-09 16:09:00 PST
Created attachment 131128 [details]
Patch
Comment 9 WebKit Review Bot 2012-03-09 18:14:03 PST
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
Comment 10 Jer Noble 2012-03-12 12:30:35 PDT
Created attachment 131380 [details]
Patch
Comment 11 Build Bot 2012-03-12 21:21:09 PDT
Comment on attachment 131380 [details]
Patch

Attachment 131380 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11949241
Comment 12 Jer Noble 2012-03-12 22:39:05 PDT
Created attachment 131548 [details]
Patch

Fixed windows 'possible uninitialized variable use' compile error.
Comment 13 Jer Noble 2012-03-12 22:40:05 PDT
<rdar://problem/10725086>
Comment 14 Gustavo Noronha (kov) 2012-03-12 23:16:35 PDT
Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11948328
Comment 15 WebKit Review Bot 2012-03-12 23:30:51 PDT
Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11949273
Comment 16 Build Bot 2012-03-12 23:57:30 PDT
Comment on attachment 131548 [details]
Patch

Attachment 131548 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11948351
Comment 17 Jer Noble 2012-03-13 00:25:07 PDT
Created attachment 131563 [details]
Patch

One final compilation fix.
Comment 18 Alexey Proskuryakov 2012-03-16 10:19:01 PDT
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 19 Jer Noble 2012-03-16 10:31:05 PDT
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!
Comment 20 Jer Noble 2012-03-16 11:12:32 PDT
Committed r111028: <http://trac.webkit.org/changeset/111028>
Comment 21 Darin Fisher (:fishd, Google) 2012-03-30 10:23:56 PDT
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?
Comment 22 Jeremy Apthorp 2012-04-02 18:34:50 PDT
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).
Comment 23 Jer Noble 2012-04-02 20:01:43 PDT
(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.
Comment 24 Jer Noble 2015-04-27 14:35:10 PDT
*** Bug 68817 has been marked as a duplicate of this bug. ***