Make Fullscreen::requestFullscreen and exitFullscreen static

Make Fullscreen::requestFullscreen and exitFullscreen static

Making requestFullscreen and exitFullscreen async like the spec will
mean that no state on the Fullscreen instances will be modified
synchronously in the request/exit calls, and the instances may in fact
end up not being created at all.

The code arguably ends up looking worse with this change alone, but it
does make it very clear that all events are put in the queue of the
requesting document, and not for each document involved.

This should not be observable at all, and makes future CLs smaller.

BUG=402421, 402376
Committed: https://crrev.com/2d25b500542df1129457767aef5ec97b31ca695e
Cr-Commit-Position: refs/heads/master@{#417446}

[foolip, 2016-09-08 15:00:51]

The CQ bit was checked by foolip@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 15:01:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 16:28:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 16:28:15]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-09-08 17:53:52]

foolip@chromium.org changed reviewers:
+ eae@chromium.org

[foolip, 2016-09-08 17:53:54]

PTAL and CQ?

[eae, 2016-09-08 19:28:39]

I have a couple of concerns and questions but nothing major. Feel free to land
once addressed, LGTM

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:224: Document& doc =
element.document();
s/doc/document/

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:407: while (currentDoc) {
currentDoc is never changed (other than to null) in the while loop. This smells
bad. How about using while(true) with a break statement instead?

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:465: Element*
currentFullScreenElement = currentFullScreenElementFrom(doc);
const?

[foolip, 2016-09-08 20:00:58]

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:224: Document& doc =
element.document();
On 2016/09/08 19:28:38, eae wrote:
> s/doc/document/

This was actually deliberate to match the comments from the spec, but the
current spec doesn't have that local variable, so I'll go back to document and
it'll look alright a few CLs later.

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:407: while (currentDoc) {
On 2016/09/08 19:28:38, eae wrote:
> currentDoc is never changed (other than to null) in the while loop. This
smells
> bad. How about using while(true) with a break statement instead?

currentDoc is never used after the loop, so I'll see about making it a for loop
to make that clear as well.

[foolip, 2016-09-08 22:19:01]

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:407: while (currentDoc) {
On 2016/09/08 20:00:58, foolip wrote:
> On 2016/09/08 19:28:38, eae wrote:
> > currentDoc is never changed (other than to null) in the while loop. This
> smells
> > bad. How about using while(true) with a break statement instead?
> 
> currentDoc is never used after the loop, so I'll see about making it a for
loop
> to make that clear as well.

Oh wait, currentDoc actually is changed in the loop. I went with a for loop
still, that keeps it close to the comments. (This will all change soon anyway.)

https://codereview.chromium.org/2325663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:465: Element*
currentFullScreenElement = currentFullScreenElementFrom(doc);
On 2016/09/08 19:28:38, eae wrote:
> const?

ChromeClient::exitFullscreenForElement takes a non-const pointer, so that
doesn't compile. I think that's right, because it will eventually mutate some
state via Element::setContainsFullScreenElement and similar.

[foolip, 2016-09-08 22:20:10]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-09-08 22:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2325663002/#ps20001
(title: "doc->document, for loop")

[commit-bot: I haz the power, 2016-09-08 22:20:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 00:01:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-09 00:03:35]

Description was changed from

==========
Make Fullscreen::requestFullscreen and exitFullscreen static

Making requestFullscreen and exitFullscreen async like the spec will
mean that no state on the Fullscreen instances will be modified
synchronously in the request/exit calls, and the instances may in fact
end up not being created at all.

The code arguably ends up looking worse with this change alone, but it
does make it very clear that all events are put in the queue of the
requesting document, and not for each document involved.

This should not be observable at all, and makes future CLs smaller.

BUG=402421,402376
==========

to

==========
Make Fullscreen::requestFullscreen and exitFullscreen static

Making requestFullscreen and exitFullscreen async like the spec will
mean that no state on the Fullscreen instances will be modified
synchronously in the request/exit calls, and the instances may in fact
end up not being created at all.

The code arguably ends up looking worse with this change alone, but it
does make it very clear that all events are put in the queue of the
requesting document, and not for each document involved.

This should not be observable at all, and makes future CLs smaller.

BUG=402421,402376

Committed: https://crrev.com/2d25b500542df1129457767aef5ec97b31ca695e
Cr-Commit-Position: refs/heads/master@{#417446}
==========

[commit-bot: I haz the power, 2016-09-09 00:03:36]

Patchset 2 (id:??) landed as
https://crrev.com/2d25b500542df1129457767aef5ec97b31ca695e
Cr-Commit-Position: refs/heads/master@{#417446}