Sync requestFullscreen() and exitFullscreen() algorithms with the spec

Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests --gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376, 402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e1d42d636990425056ede44086ef49a1f8c6a0a5
Cr-Commit-Position: refs/heads/master@{#439607}

[foolip, 2016-12-13 11:32:21]

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

[foolip, 2016-12-13 11:32:22]

Description was changed from

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
==========

to

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-12-13 11:32:58]

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

[foolip, 2016-12-13 11:42:00]

foolip@chromium.org changed reviewers:
+ alexmos@chromium.org, eae@chromium.org, esprehn@chromium.org,
mlamouri@chromium.org

[foolip, 2016-12-13 11:42:00]

Here it is, the long awaited timing fix for Fullscreen, which is what got in the
way of top layer last time:
https://codereview.chromium.org/2168373002

It's a big CL, but parts that could be easily separated I've already split over
the past few weeks.

eae@, esprehn@, can either or both of you review everything?

alexmos@, can you search for "OOPIF" and "PrefixedForCrossProcessDescendant" to
see if I made a mess of it?

mlamouri@, any feedback much appreciated.

When reviewing, please don't assume that the spec makes sense. It is very much
open to change, I believe this is the first attempt to follow the current spec
to the letter, and there are already TODOs for some things that won't work.

There is one known issue I have not yet addressed, and that's the TODO in
FullscreenController.cpp. That couldn't previously happen, as
requestFullscreen() would synchronously modify the fullscreen element stack and
removing that element would then trigger an exit.

[commit-bot: I haz the power, 2016-12-13 12:57:18]

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

[commit-bot: I haz the power, 2016-12-13 12:57:19]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-12-13 13:57:29]

I will make this a priority for my review session tomorrow. Let me know if you
want/need me to have a look earlier.

[eae, 2016-12-14 00:19:24]

This is great.

LGTM w/nits & suggestions

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:250: if
(Fullscreen::fullscreenElementStackSizeFrom(*document) == 1)
This is a little hard to follow, could you move down the description of the
condition (single element with flag set) to be closer to the if statement?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:256: // 4. Return |docs|.
Unnecessary comment.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:429: // 4. If any of the
following conditions are false, then set |error| to
Comment doesn't match the logic anymore (it's been reversed).

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:826:
document()->styleEngine().ensureUAStyleForFullscreen();
Is document() guaranteed to be non-null here?

[mlamouri (slow - plz ping), 2016-12-14 16:24:12]

lgtm. As usual with CLs big like that, it's likely my review is very superficial
and I'm missing details.

Note that handling all of this from the browser process might be fairly simpler
:)

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:250: if
(Fullscreen::fullscreenElementStackSizeFrom(*document) == 1)
On 2016/12/14 at 00:19:24, eae wrote:
> This is a little hard to follow, could you move down the description of the
condition (single element with flag set) to be closer to the if statement?

+1

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:256: // 4. Return |docs|.
On 2016/12/14 at 00:19:24, eae wrote:
> Unnecessary comment.

I guess the idea is to keep track of the spec steps.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:260: // Creates a non-bubbling
event with |document| as its target.
Does it have to be cancellable? The one you create isn't.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:271: // connected to
|document|, then |document| is used as the target instead.
ditto

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:306: }
Doesn't seem very useful.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:463: LocalFrame& frame =
*document.frame();
nit: Maybe you could move this all the way up because otherwise, a naive and
short attention span reader like me will be scared of a non-null check. Not sure
if it would make sense because I only look at an unified diff. Feel free to
ignore :)

edit: I see that you do something similar for `runTaskForRequest` with
`document` and `element`.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:531: if (frame->owner() &&
frame->owner()->isLocal()) {
early break for readability:
```
if (!frame->owner() || !frame->owner()->isLocal())
  continue;
```

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:724: if (exitDoc == &doc &&
exitType == ExitType::Fully) {
Maybe an early return? I know, I'm obsessed :)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:776: DCHECK(document() ==
&oldNode.document());
DCHECK_EQ?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:826:
document()->styleEngine().ensureUAStyleForFullscreen();
On 2016/12/14 at 00:19:24, eae wrote:
> Is document() guaranteed to be non-null here?

Would it make sense to return Document& if you always use document() without
null check? I can't find a place where you null-check it.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:862: bool
shouldCreatePlaceholder = layoutObject && layoutObject->isBox();
Is this variable added just for readability? You could move the check in the
condition check.

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.h:133: void
fullscreenElementChanged(Element*, Element*, RequestType);
I would name the two different `Element*` parameters here because otherwise one
has to guess it is old/new unless they look at the implementation.

[foolip, 2016-12-14 17:01:59]

On 2016/12/14 16:24:12, mlamouri wrote:
> lgtm. As usual with CLs big like that, it's likely my review is very
superficial
> and I'm missing details.

Thanks for the review all the same!

> Note that handling all of this from the browser process might be fairly
simpler
> :)

Nested fullscreen scenarios involving multiple processes are what make this
hard, at least for me. Say there are processes A, B and C and a nested iframes
in ABCA order. When entering fullscreen, the browser process would have to
notify each frame in order that it's entered fullscreen and wait for
confirmation, since the pending fullscreen elements can be moved/removed at any
time. But that rules out the possibility of a flicker-free transition, where all
documents get the fullscreen style on the same animation frame.

As it is now, it's actually possible that both A documents enter fullscreen but
it turns out that B or C failed, and it'll just end up in a broken state.

Even if the browser process notifies all frames (individually) without waiting,
then if the IPC queue is not fully emptied all at once, then the two A documents
might still be handled in different animation frames. (But maybe that IPC queue
problem can't happen?)

I'm hopeful there is a design for all of this that would work, but I don't know
what it is yet.

alexmos@ ^

[alexmos, 2016-12-15 07:58:53]

Very nice.  Looks good overall, a few comments below, and I'll need to think
more about the nested fullscreen cases with OOPIFs -- there's one thought in
comments, but I'm not sure what the design should really be yet.  Haven't looked
at the layout test changes, so hoping that someone else has. :)

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:246: //
https://crbug.com/617369.
Does this CL help with problematic nested cases involving <iframe> elements
without OOPIFs?  I checked the repro in https://crbug.com/617369, and (4a) seems
to be fixed, but (4b) is now fully exiting fullscreen instead of popping the
<div> off the stack and keeping the <iframe> fullscreened -- better than old
broken behavior, but I wonder what it'll take to fix this, and if I'm
understanding the nested behavior correctly.  Will the iframe fullscreen flag
help here?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:610:
collectDocumentsToUnfullscreen(doc, exitType);
Seems a bit inefficient to collect all these docs here only to throw them away
and only use the last one below, but I don't see an easy way of improving on it.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:615: // OOPIF: Use
|isLocalRoot| as oppposed to |isMainFrame|, so that if the
nit: s/oppposed/opposed/

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:620: if (!docs.isEmpty() &&
docs.back()->frame()->isLocalRoot())
I wonder if instead of isLocalRoot() this should be comparing docs.back() to
topmostLocalAncestor(doc)?

The difference is that with a A1-B1-A2-B2 hierarchy, both B1 and B2 will have
isLocalRoot() set to true, but B1 is the topmostLocalAncestor.  So if we did
exitFullscreen for an element in B2 and collectDocumentsToUnfullscreen returned
{B2}, seems like we shouldn't be resizing (if, for example, there was nested
fullscreen in B1).  I know it won't fix all the nested fullscreen problems with
OOPIFs, but might be just a bit saner? :)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:625: 
Is it worth DCHECKing somewhere that if exitType is Fully, then resize is true?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:677: // element in their
fullscreen element stacks, thereby erring on the side of
One thing we could do for these nested fullscreen cases with OOPIFs is replicate
the fullscreen element stack length, so that we can query it for remote frames
as well.  This will let us stop on the right frame in places like this, though I
haven't thought through all the implications, races, etc.  I'll keep thinking
about this.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Fullscreen.h (left):

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.h:113: bool
forCrossProcessDescendant() { return m_forCrossProcessDescendant; }
See my note in FullscreenController::enterFullscreen about this.

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.h:72:
PrefixedForCrossProcessDescendant,
When we add plumbing for the request type in OOPIF fullscreen requests (in
willEnterFullscreen), seems like we will end up adding an
UnprefixedForCrossProcessDescendant here, and having to check for that in
addition to Unprefixed in places like createChangeEvent.  Was that how you
wanted to do that eventually?  I guess if we kept the forCrossProcessDescendant
bit separate from this, that would also need to be in the fullscreen element
stack, which seems ugly.

FWIW, I started working on a CL for plumbing the request type for OOPIFs a while
back, but got distracted with other things before finishing.  We should figure
out how to finish it before unprefixed fullscreen ships - when do you think that
might happen?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:165:
webFrameClient(frame).enterFullscreen();
I was surprised this happens unconditionally now.  This used to be gated on
forCrossProcessDescendant(), but looks like that changed in your
https://codereview.chromium.org/2550703002.  It seems this will cause redundant
ToggleFullscreen IPCs to be sent to the browser process from all the
cross-process ancestors, which might trigger more redundant WillEnterFullscreen
IPCs to ancestor renderers. It appears things are still working, probably
because we eventually realize that we're already in fullscreen mode both on
browser and renderer sides, but it'd still be nice to avoid sending those
redundant IPCs here somehow.  Is there a way to still check whether we're making
a request for a cross-process descendant here?

[foolip, 2016-12-19 17:36:57]

Thank you all for your comments. I think I've addressed it all. Further comments
welcome, but if I don't hear anything I'll land this sometime before Santa Claus
comes around.

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:246: //
https://crbug.com/617369.
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> Does this CL help with problematic nested cases involving <iframe> elements
> without OOPIFs?  I checked the repro in https://crbug.com/617369, and (4a)
seems
> to be fixed, but (4b) is now fully exiting fullscreen instead of popping the
> <div> off the stack and keeping the <iframe> fullscreened -- better than old
> broken behavior, but I wonder what it'll take to fix this, and if I'm
> understanding the nested behavior correctly.  Will the iframe fullscreen flag
> help here?

The iframe fullscreen flag is unrelated, it's merely to signal that
iframe.requestFullscreen() was called, as opposed to the iframe automatically
going fullscreen for a descendent.

I tried to not change things for OOPIF beyond keeping the tests passing, but
occasionally had to decide on something and here erred on the side of exiting.
It's still totally possible to end up in weird states, perhaps more than there
are bugs for. Even if the number of elements on the fullscreen element stack
were replicated on RemoteFrame, it might still be stale.

The spec (and this CL) tries very hard to make all the observable changes and
fire events at a single point, but OOPIF makes this impossible. I'm not sure if
it's possible to make the difference unobservable, but spec changes may well be
needed.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:250: if
(Fullscreen::fullscreenElementStackSizeFrom(*document) == 1)
On 2016/12/14 16:24:12, mlamouri wrote:
> On 2016/12/14 at 00:19:24, eae wrote:
> > This is a little hard to follow, could you move down the description of the
> condition (single element with flag set) to be closer to the if statement?
> 
> +1

Done.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:256: // 4. Return |docs|.
On 2016/12/14 16:24:12, mlamouri wrote:
> On 2016/12/14 at 00:19:24, eae wrote:
> > Unnecessary comment.
> 
> I guess the idea is to keep track of the spec steps.

Yeah, admittedly it gets a bit silly at points, but it's the best way I've found
to have confidence that one matches the spec and it also makes it clear if the
spec has changed since the implementation.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:260: // Creates a non-bubbling
event with |document| as its target.
On 2016/12/14 16:24:12, mlamouri wrote:
> Does it have to be cancellable? The one you create isn't.

The spec uses https://dom.spec.whatwg.org/#concept-event-fire and bubbles and
cancelable are false by default.
LayoutTests/imported/wpt/fullscreen/api/element-request-fullscreen-manual.html
and two other tests asserts that it is so.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:306: }
On 2016/12/14 16:24:12, mlamouri wrote:
> Doesn't seem very useful.

It made the error event case a bit more compact, but I've dropped the helper.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:429: // 4. If any of the
following conditions are false, then set |error| to
On 2016/12/14 00:19:24, eae (ooo until 2017-01-03) wrote:
> Comment doesn't match the logic anymore (it's been reversed).

It's the forCrossProcessDescendant case that makes this confusing, I set error
to true first and then back to false if there were no errors. I've gone back and
forth multiple times while working on this CL, and now I've split it out into a
requestFullscreenConditionsMet helper instead. (The hardest part by far was
naming it.)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:463: LocalFrame& frame =
*document.frame();
On 2016/12/14 16:24:12, mlamouri wrote:
> nit: Maybe you could move this all the way up because otherwise, a naive and
> short attention span reader like me will be scared of a non-null check. Not
sure
> if it would make sense because I only look at an unified diff. Feel free to
> ignore :)
> 
> edit: I see that you do something similar for `runTaskForRequest` with
> `document` and `element`.

This is indeed something to worry about, at various points I've gotten it wrong
and maybe there are still missing checks. I added some DCHECKs runTaskForRequest
and runTaskForExit to assert what I think is guaranteed by
PageAnimator::serviceScriptedAnimations.

Here, moving LocalFrame to the top would make it more obvious why
document.frame() can't be null, but also less obvious where document.frame() is
used. So I suppose I will leave it as-is. (There will be another chance to
change this when I renumber and tweak to align with
https://github.com/whatwg/fullscreen/pull/68 and other spec changes.)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:531: if (frame->owner() &&
frame->owner()->isLocal()) {
On 2016/12/14 16:24:12, mlamouri wrote:
> early break for readability:
> ```
> if (!frame->owner() || !frame->owner()->isLocal())
>   continue;
> ```

Done.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:610:
collectDocumentsToUnfullscreen(doc, exitType);
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> Seems a bit inefficient to collect all these docs here only to throw them away
> and only use the last one below, but I don't see an easy way of improving on
it.

Yeah. The spec uses the same primitive both before and after the resize, and so
it's nice to have confidence that it matches, so I haven't added an
shouldWeResize() that does the checks without the collecting.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:615: // OOPIF: Use
|isLocalRoot| as oppposed to |isMainFrame|, so that if the
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> nit: s/oppposed/opposed/

Done.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:620: if (!docs.isEmpty() &&
docs.back()->frame()->isLocalRoot())
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> I wonder if instead of isLocalRoot() this should be comparing docs.back() to
> topmostLocalAncestor(doc)?
> 
> The difference is that with a A1-B1-A2-B2 hierarchy, both B1 and B2 will have
> isLocalRoot() set to true, but B1 is the topmostLocalAncestor.  So if we did
> exitFullscreen for an element in B2 and collectDocumentsToUnfullscreen
returned
> {B2}, seems like we shouldn't be resizing (if, for example, there was nested
> fullscreen in B1).  I know it won't fix all the nested fullscreen problems
with
> OOPIFs, but might be just a bit saner? :)

Done, this was a not-well-thought-out change compared to previously, where the
currentDoc loop in exitFullscreen would have continued all the way up and not
exit in the A1-B1-A2-B2 case you've described. I have commented on
https://crbug.com/617369 and have requested a test for this situation, but will
boldly suggest that we make this change as long as it doesn't regress the
existing tests, as I found the existing coverage to actually be quite good and
it caught a number "simplifications" I tried to make.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:625: 
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> Is it worth DCHECKing somewhere that if exitType is Fully, then resize is
true?

Yes, this is currently true because Fullscreen::fullyExitFullscreen uses
topmostLocalAncestor, so I'll add a DCHECK here with a comment for why it's
going to fail eventually.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:677: // element in their
fullscreen element stacks, thereby erring on the side of
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> One thing we could do for these nested fullscreen cases with OOPIFs is
replicate
> the fullscreen element stack length, so that we can query it for remote frames
> as well.  This will let us stop on the right frame in places like this, though
I
> haven't thought through all the implications, races, etc.  I'll keep thinking
> about this.

Yeah, I think that is the obvious next thing to try. It's possible it could be
made observably indistinguishable from what the spec says, but I'm just not
sure. We should meet and brainstorm about this some time, maybe BlinkOn?

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:724: if (exitDoc == &doc &&
exitType == ExitType::Fully) {
On 2016/12/14 16:24:12, mlamouri wrote:
> Maybe an early return? I know, I'm obsessed :)

This one I think would be less clear and actually longer with an early continue.
But fear not, it'll get rewritten after
https://github.com/whatwg/fullscreen/pull/65 to an if-else where an early
continue will come more naturally!

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:776: DCHECK(document() ==
&oldNode.document());
On 2016/12/14 16:24:12, mlamouri wrote:
> DCHECK_EQ?

Done, and verified no other missed opportunities to DCHECK_EQ in Fullscreen.cpp
or FullscreenController.cpp.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:826:
document()->styleEngine().ensureUAStyleForFullscreen();
On 2016/12/14 16:24:12, mlamouri wrote:
> On 2016/12/14 at 00:19:24, eae wrote:
> > Is document() guaranteed to be non-null here?
> 
> Would it make sense to return Document& if you always use document() without
> null check? I can't find a place where you null-check it.

We get here from pushFullscreenElementStack or popFullscreenElementStack, and in
both cases the Fullscreen instance came from Fullscreen::from(Document&), so
there should definitely be a document around. However, Fullscreen::document() is
implemented in terms of lifecycleContext(), and after
Fullscreen::contextDestroyed() is called it can return null.

There are places, this included, where at least I can't find a strong guarantee
that document() is non-null, so I'll add some null checks and think about
perhaps making changes that would make them redundant separately. (Given the
timing changes in this CL, the chances of running into a null document() should
increase, if it is at all possible.)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:862: bool
shouldCreatePlaceholder = layoutObject && layoutObject->isBox();
On 2016/12/14 16:24:12, mlamouri wrote:
> Is this variable added just for readability? You could move the check in the
> condition check.

This is moved code and getting rid of it is the next CL, which is based on an
old (reverted) CL, so I'll leave this to make everything look as familiar as it
can to someone comparing.

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.h:72:
PrefixedForCrossProcessDescendant,
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> When we add plumbing for the request type in OOPIF fullscreen requests (in
> willEnterFullscreen), seems like we will end up adding an
> UnprefixedForCrossProcessDescendant here, and having to check for that in
> addition to Unprefixed in places like createChangeEvent.  Was that how you
> wanted to do that eventually?  I guess if we kept the
forCrossProcessDescendant
> bit separate from this, that would also need to be in the fullscreen element
> stack, which seems ugly.
> 
> FWIW, I started working on a CL for plumbing the request type for OOPIFs a
while
> back, but got distracted with other things before finishing.  We should figure
> out how to finish it before unprefixed fullscreen ships - when do you think
that
> might happen?

Having both bits of information in RequestType was nifty and indeed the reason I
did this. Rather than adding UnprefixedForCrossProcessDescendant I think this
will have to be turned into a bitfield of size two to represent all four
combinations.

I'll be OOO until January, but I hope to ship unprefixed fullscreen in M57 or
M58, so we'll have to revisit this soon. (First though, moving to top layer.
Getting rid of :-webkit-full-screen-ancestor would also simplify things a bit,
and it's not clear yet if that'll happen before unprefixing.)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.h:133: void
fullscreenElementChanged(Element*, Element*, RequestType);
On 2016/12/14 16:24:12, mlamouri wrote:
> I would name the two different `Element*` parameters here because otherwise
one
> has to guess it is old/new unless they look at the implementation.

Done, all args named. (I imitated ChromeClient::focusedNodeChanged, but guessing
RequestType is even harder than the from/to element.)

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:165:
webFrameClient(frame).enterFullscreen();
On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> I was surprised this happens unconditionally now.  This used to be gated on
> forCrossProcessDescendant(), but looks like that changed in your
> https://codereview.chromium.org/2550703002.  It seems this will cause
redundant
> ToggleFullscreen IPCs to be sent to the browser process from all the
> cross-process ancestors, which might trigger more redundant
WillEnterFullscreen
> IPCs to ancestor renderers. It appears things are still working, probably
> because we eventually realize that we're already in fullscreen mode both on
> browser and renderer sides, but it'd still be nice to avoid sending those
> redundant IPCs here somehow.  Is there a way to still check whether we're
making
> a request for a cross-process descendant here?

I dropped that check in https://codereview.chromium.org/2550703002/ to get rid
of FullscreenController::m_provisionalFullscreenElement, although I didn't stop
to wonder if sending the IPC twice would break things.

It would be easy to pass through forCrossProcessDescendant and in some quick
testing it looks like none of FullscreenController::enterFullscreen except
`m_state = State::EnteringFullscreen` is actually needed to pass
SitePerProcessInteractiveBrowserTest.Fullscreen*, so I wonder if the m_initial*
state matters here.

Since the redundant IPC is already happening I won't avoid it in this CL, but
I've filed https://crbug.com/675475 for what I think is the root problem here.
I've had significant difficulty understanding how things currently actually
work. Perhaps it would be simple enough if documented exhaustively, but it'd
still be pretty weird that WebViewImpl owns the controller when there's more
than one WebViewImpl.

[foolip, 2016-12-19 17:40:28]

On 2016/12/19 17:36:57, foolip wrote:
> Thank you all for your comments. I think I've addressed it all. Further
comments
> welcome, but if I don't hear anything I'll land this sometime before Santa
Claus
> comes around.

Note that in Sweden, Santa comes on the December 24, around 4pm or so :)

[foolip, 2016-12-19 17:40:35]

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

[commit-bot: I haz the power, 2016-12-19 17:40:48]

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-12-19 21:28:38]

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

[commit-bot: I haz the power, 2016-12-19 21:28:39]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-12-19 22:53:27]

LGTM, thanks!

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

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:677: // element in their
fullscreen element stacks, thereby erring on the side of
On 2016/12/19 17:36:57, foolip (OOO til Jan. 2) wrote:
> On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> > One thing we could do for these nested fullscreen cases with OOPIFs is
> replicate
> > the fullscreen element stack length, so that we can query it for remote
frames
> > as well.  This will let us stop on the right frame in places like this,
though
> I
> > haven't thought through all the implications, races, etc.  I'll keep
thinking
> > about this.
> 
> Yeah, I think that is the obvious next thing to try. It's possible it could be
> made observably indistinguishable from what the spec says, but I'm just not
> sure. We should meet and brainstorm about this some time, maybe BlinkOn?

sgtm.  I'm planning to be there.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:165:
webFrameClient(frame).enterFullscreen();
On 2016/12/19 17:36:57, foolip (OOO til Jan. 2) wrote:
> On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> > I was surprised this happens unconditionally now.  This used to be gated on
> > forCrossProcessDescendant(), but looks like that changed in your
> > https://codereview.chromium.org/2550703002.  It seems this will cause
> redundant
> > ToggleFullscreen IPCs to be sent to the browser process from all the
> > cross-process ancestors, which might trigger more redundant
> WillEnterFullscreen
> > IPCs to ancestor renderers. It appears things are still working, probably
> > because we eventually realize that we're already in fullscreen mode both on
> > browser and renderer sides, but it'd still be nice to avoid sending those
> > redundant IPCs here somehow.  Is there a way to still check whether we're
> making
> > a request for a cross-process descendant here?
> 
> I dropped that check in https://codereview.chromium.org/2550703002/ to get rid
> of FullscreenController::m_provisionalFullscreenElement, although I didn't
stop
> to wonder if sending the IPC twice would break things.
> 
> It would be easy to pass through forCrossProcessDescendant and in some quick
> testing it looks like none of FullscreenController::enterFullscreen except
> `m_state = State::EnteringFullscreen` is actually needed to pass
> SitePerProcessInteractiveBrowserTest.Fullscreen*, so I wonder if the
m_initial*
> state matters here.
> 
> Since the redundant IPC is already happening I won't avoid it in this CL, but
> I've filed https://crbug.com/675475 for what I think is the root problem here.
> I've had significant difficulty understanding how things currently actually
> work. Perhaps it would be simple enough if documented exhaustively, but it'd
> still be pretty weird that WebViewImpl owns the controller when there's more
> than one WebViewImpl.

Agreed about no need to fix in this CL since this is already happening.

Yes, it'd be nice to move at least the state tracking from FullscreenController
to the browser process, though I'm not sure what would happen to the code
dealing with video overlays, page scale, etc.  It's interesting that the only
thing needed for the tests right now is the m_state change; it probably helped
that you refactored it to get rid of m_provisionalFullscreenElement, etc. :)  I
don't remember if saving/restoring the m_initial* sizing vars in OOPIF cases was
important for something.  scrollOffset clearly doesn't matter, as it only deals
with local main frames, but not sure about page scale and visual viewport
offset.

I'll mention that another tricky case I remember now was opening devtools when
in fullscreen from an OOPIF.  Unfortunately, I don't think we have a test for
that - I should probably add it.

[foolip, 2016-12-19 23:07:54]

Thanks alexmos@, I'll go ahead and land now. Given the size of the change I'm
half expecting a revert, so perhaps this isn't the last time you see it :)

esprehn@, if you find anything you want to comment on in this CL, please just
comment after the fact and I'll fix it.

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2573773002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:165:
webFrameClient(frame).enterFullscreen();
On 2016/12/19 22:53:26, alexmos (OOO Dec 20 to Dec 27) wrote:
> On 2016/12/19 17:36:57, foolip (OOO til Jan. 2) wrote:
> > On 2016/12/15 07:58:53, alexmos (OOO Dec 20 to Dec 27) wrote:
> > > I was surprised this happens unconditionally now.  This used to be gated
on
> > > forCrossProcessDescendant(), but looks like that changed in your
> > > https://codereview.chromium.org/2550703002.  It seems this will cause
> > redundant
> > > ToggleFullscreen IPCs to be sent to the browser process from all the
> > > cross-process ancestors, which might trigger more redundant
> > WillEnterFullscreen
> > > IPCs to ancestor renderers. It appears things are still working, probably
> > > because we eventually realize that we're already in fullscreen mode both
on
> > > browser and renderer sides, but it'd still be nice to avoid sending those
> > > redundant IPCs here somehow.  Is there a way to still check whether we're
> > making
> > > a request for a cross-process descendant here?
> > 
> > I dropped that check in https://codereview.chromium.org/2550703002/ to get
rid
> > of FullscreenController::m_provisionalFullscreenElement, although I didn't
> stop
> > to wonder if sending the IPC twice would break things.
> > 
> > It would be easy to pass through forCrossProcessDescendant and in some quick
> > testing it looks like none of FullscreenController::enterFullscreen except
> > `m_state = State::EnteringFullscreen` is actually needed to pass
> > SitePerProcessInteractiveBrowserTest.Fullscreen*, so I wonder if the
> m_initial*
> > state matters here.
> > 
> > Since the redundant IPC is already happening I won't avoid it in this CL,
but
> > I've filed https://crbug.com/675475 for what I think is the root problem
here.
> > I've had significant difficulty understanding how things currently actually
> > work. Perhaps it would be simple enough if documented exhaustively, but it'd
> > still be pretty weird that WebViewImpl owns the controller when there's more
> > than one WebViewImpl.
> 
> Agreed about no need to fix in this CL since this is already happening.
> 
> Yes, it'd be nice to move at least the state tracking from
FullscreenController
> to the browser process, though I'm not sure what would happen to the code
> dealing with video overlays, page scale, etc.  It's interesting that the only
> thing needed for the tests right now is the m_state change; it probably helped
> that you refactored it to get rid of m_provisionalFullscreenElement, etc. :) 
I
> don't remember if saving/restoring the m_initial* sizing vars in OOPIF cases
was
> important for something.  scrollOffset clearly doesn't matter, as it only
deals
> with local main frames, but not sure about page scale and visual viewport
> offset.

Yeah, the saving/restoring of m_initial* state can't be entirely unaware of the
global fullscreen state, but perhaps it can be triggered by a state transition
in the browser process in a way that the renderer doesn't need to always know
the current global state. (Or if it does, that it's at least merely a mirroring
of the global state, not separately maintained.)

The video overlay stuff should be possible to move into HTMLVideoElement with
enough willpower, it was just convenient (and low risk) to leave on
FullscreenController since it deals entirely with WebViewImpl.

> I'll mention that another tricky case I remember now was opening devtools when
> in fullscreen from an OOPIF.  Unfortunately, I don't think we have a test for
> that - I should probably add it.

Ah, that's certainly not something I've noticed, a test for whatever it was
would be nice or I'll probably break it :)

[foolip, 2016-12-19 23:08:05]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-12-19 23:08:05]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2573773002/#ps40001
(title: "address feedback")

[commit-bot: I haz the power, 2016-12-19 23:09:11]

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-12-19 23:16:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482188885380770,
"parent_rev": "63336c1207fb4d460734d59e9fd93fe6799e82e2", "commit_rev":
"3f69d436c5461a76f1b6f620654f475db245fe5a"}

[commit-bot: I haz the power, 2016-12-19 23:17:05]

Description was changed from

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2573773002
==========

[commit-bot: I haz the power, 2016-12-19 23:17:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-19 23:20:36]

Description was changed from

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2573773002
==========

to

==========
Sync requestFullscreen() and exitFullscreen() algorithms with the spec

The central change is the timing for when the fullscreen element stack
is modified and when the events fire. This makes it possible to make
webkitCurrentFullScreenElement an alias of fullscreenElement, as the old
notion of a single fullscreen element is gone.

Previously, fullscreen requests did:
 1. In Fullscreen::requestFullscreen(), synchronously modify
    m_fullscreenElementStack, visible in document.fullscreenElement, but
    not document.webkitCurrentFullScreenElement. Also enqueue events to
    fire, without starting the timer to fire them.
 2. As soon as the resize happens, in Fullscreen::didEnterFullscreen(),
    set m_currentFullScreenElement, affecting :-webkit-full-screen
    document.webkitCurrentFullScreenElement. Start the event timer.
 3. When the timer fires, events are dispatched. If the tree has changed
    since the events were enqueued, additional events may be fired.

And similarly for exit. For errors, requestFullscreen() would itself
start the timer to fire the events.

Now, fullscreen requests will:
 1. In Fullscreen::requestFullscreen(), append to a list of pending
    requests.
 2. When the resize happens, enqueue an animation frame task for each
    pending request.
 3. In the animation frame task, apply all changes, decide which events
    to fire and then fire them.

TEST=run-webkit-tests fullscreen/ imported/wpt/fullscreen/
     webkit_unit_tests --gtest_filter=*Fullscreen*
     interactive_ui_tests
--gtest_filter=SitePerProcessInteractiveBrowserTest.Fullscreen*
BUG=402376,402421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e1d42d636990425056ede44086ef49a1f8c6a0a5
Cr-Commit-Position: refs/heads/master@{#439607}
==========

[commit-bot: I haz the power, 2016-12-19 23:20:38]

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

[stgao, 2016-12-20 20:41:32]

By any chance, will this CL make TabCaptureApiTest.FullscreenEvents flaky?

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

[foolip, 2016-12-21 08:21:37]

On 2016/12/20 20:41:32, stgao (slow on Monday) wrote:
> By any chance, will this CL make TabCaptureApiTest.FullscreenEvents flaky?
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

That test didn't show up while working on this CL, but it does change the timing
of the fullscreenchange event, so it's quite plausible. If this CL isn't
reverted, I'll take a look at the test when I'm back in January.

[jeffcarp, 2016-12-21 22:07:34]

On 2016/12/21 at 08:21:37, foolip wrote:
> On 2016/12/20 20:41:32, stgao (slow on Monday) wrote:
> > By any chance, will this CL make TabCaptureApiTest.FullscreenEvents flaky?
> > 
> >
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> >
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> >
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
> That test didn't show up while working on this CL, but it does change the
timing of the fullscreenchange event, so it's quite plausible. If this CL isn't
reverted, I'll take a look at the test when I'm back in January.

WPT changes exported: https://github.com/w3c/web-platform-tests/pull/4381

[stgao, 2017-01-14 08:13:02]

On 2016/12/21 08:21:37, foolip_slow_very_sorry wrote:
> On 2016/12/20 20:41:32, stgao (slow on Monday) wrote:
> > By any chance, will this CL make TabCaptureApiTest.FullscreenEvents flaky?
> > 
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
> That test didn't show up while working on this CL, but it does change the
timing
> of the fullscreenchange event, so it's quite plausible. If this CL isn't
> reverted, I'll take a look at the test when I'm back in January.

This CL seems to be the exact culprit as shown below. Mind investigating and
fixing the test flakiness?

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

[foolip, 2017-01-26 18:44:11]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2654083006/ by foolip@chromium.org.

The reason for reverting is: Delaying fullscreen element stack changes until the
animation frame after resize, and thus after resize-triggered scroll events,
caused a regression with twitter video embeds that may require spec changes to
fix:
https://github.com/whatwg/fullscreen/issues/74

BUG=680467.

[foolip, 2017-01-27 06:30:03]

On 2017/01/14 08:13:02, stgao (OOO until Feb 23) wrote:
> On 2016/12/21 08:21:37, foolip_slow_very_sorry wrote:
> > On 2016/12/20 20:41:32, stgao (slow on Monday) wrote:
> > > By any chance, will this CL make TabCaptureApiTest.FullscreenEvents flaky?
> > > 
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > > 
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > > 
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> > That test didn't show up while working on this CL, but it does change the
> timing
> > of the fullscreenchange event, so it's quite plausible. If this CL isn't
> > reverted, I'll take a look at the test when I'm back in January.
> 
> This CL seems to be the exact culprit as shown below. Mind investigating and
> fixing the test flakiness?
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

I am now reverting this CL, but have filed
https://bugs.chromium.org/p/chromium/issues/detail?id=685940 to investigate this
before relanding.