OOPIF: Process all local ancestors when requesting and exiting fullscreen.

OOPIF: Process all local ancestors when requesting and exiting fullscreen.

Currently, requestFullscreen, fullyExitFullscreen, and exitFullscreen
perform several walks over ancestor frames, stopping as soon as they
hit a remote frame.  This means that for hierarchies like A-B-A, when
fullscreen is entered from the bottom frame, the top frame won't
update its fullscreen element stack or queue up fullscreenchange
events.  This CL fixes this by accounting for local ancestors
separated by remote frames in all of these walks.

BUG=550497
Committed: https://crrev.com/59e00ee5e5bdb631ed9617e3e028f76b06b35267
Cr-Commit-Position: refs/heads/master@{#401424}

[alexmos, 2016-06-06 22:23:48]

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

[alexmos, 2016-06-06 22:23:49]

Philip, can you please take a look?

This CL helps do the right thing with fullscreen and OOPIFs where there's a
remote ancestor frame in between local ancestors.  With the current
implementation, if we have a hierarchy like A-B-A-B-C, and we enter fullscreen
for an element in C, we'll end up calling requestFullscreen once in each process
(on bottom three frames), and without these changes, we never updated the
fullscreen element stack or fired fullscreenchange events on the top two frames.
 I already had a test lined up for this from previous work (no need to review
that - I can ask creis@ to review that separately if the changes in
Fullscreen.cpp make sense).

Daniel: CC-ing you so you can also take a look if you have time, in particular
at the changes related to topDocument.

https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:327: Document& doc =
topmostDocument(document);
topDocument() was wrong here - see https://crbug.com/617677.  I think this
actually needs the topmost local frame's Document, but given all the confusion
about topDocument(), it'd be good to try and remove topDocument() altogether
rather than trying to change it, so I just did this in a local helper.  (If it
turns out that other places need this, we can move it to be available more
broadly.)

https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:400: // TODO(alexmos): Deal
with nested fullscreen cases.
It appears that cross-frame nested fullscreen has issues without OOPIF as well;
I filed https://crbug.com/617369 for that.  The change here will ensure that
we'll pop all the relevant container elements off the stack in all processes
when exiting fullscreen from something like the bottom frame of a A-B-A
hierarchy (where B is a remote frame).  The nested case that it won't handle
(yet) is if the middle frame B has another fullscreen element on the stack that
should enter fullscreen (without the tab actually going out of fullscreen mode).
 The fix for that probably depends on how https://crbug.com/617369 is fixed for
the non-OOPIF case, so I left it alone for the purposes of this CL.

[foolip, 2016-06-09 13:12:36]

This is my first close encounter with OOPIF, fun!

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:124: static Document&
topmostDocument(Document& document)
Can you name this so that it's clear that it's not the top-level browsing
context's document, but the locally topmost document? Spelling out the
difference between this and Document::topDocument() would be good, because
without this CL leading me to think about A-B-A cases I'm not sure I would
understand how it's different.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:257: Frame* currentFrame =
document()->frame();
The spec quotes are a lot let useful when the names don't match, thinking about
ways to minimize the difference.

First option, could the docs list be populated with all the documents for local
frames even if there are remote frames with documents in between them per spec?
Then much of the following logic would remain unchanged, and one would only need
to convince oneself that it's not observable.

Second option, if a Frame is equivalent to a browsing context in spec terms,
change the spec to talk about browsing contexts instead. But I think it'd still
be unnatural to say "Frame* browsingContext" so it still wouldn't match.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:289: if
(currentFrame->isRemoteFrame())
Assuming that this isn't reverted to a list of only local documents, can you
move this to before step 3 and document why the observable behavior will still
match what the spec says?

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:324: // For OOPIF, use the
Document of the topmost local ancestor frame.  Each
"Use the document... because OOPIF something something" so that it doesn't sound
like this code path is only for OOPIF. The reader might notice the lack of "if
(oopif)" but still doubt what's going on, as I am now in a first reading :)

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:400: // TODO(alexmos): Deal
with nested fullscreen cases.
What cases is that?

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:405: if (frame) {
OK, so this just skips over the remote frames and otherwise does what the spec
says. Some symmetry with that for the requestFullscreen code path is what I'm
hoping for above, if it can be had.

[alexmos, 2016-06-09 21:36:55]

Thanks for the comments!  Can you please take another look?

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:124: static Document&
topmostDocument(Document& document)
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> Can you name this so that it's clear that it's not the top-level browsing
> context's document, but the locally topmost document? Spelling out the
> difference between this and Document::topDocument() would be good, because
> without this CL leading me to think about A-B-A cases I'm not sure I would
> understand how it's different.

Renamed to topmostLocalAncestor and added a comment explaining how it's
different from top frame's document.  Let me know if that name sounds better.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:257: Frame* currentFrame =
document()->frame();
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> The spec quotes are a lot let useful when the names don't match, thinking
about
> ways to minimize the difference.
> 
> First option, could the docs list be populated with all the documents for
local
> frames even if there are remote frames with documents in between them per
spec?
> Then much of the following logic would remain unchanged, and one would only
need
> to convince oneself that it's not observable.
> 
> Second option, if a Frame is equivalent to a browsing context in spec terms,
> change the spec to talk about browsing contexts instead. But I think it'd
still
> be unnatural to say "Frame* browsingContext" so it still wouldn't match.

The main reason I originally kept ancestors as Frames is for the check in step
4.3 on the following document's container (the old topElement !=
followingDoc->localOwner() check).  With an A-B-A2 hierarchy, we need B's owner
when checking A's topElement (B won't have a document in A's process, but it
will have a local owner()).  With Frames, this is straightforward: the ancestors
list is {A,B,A2}, followingFrame is B, and we can just use its owner. If we keep
the ancestor list as documents (your first option), the list will be {A, A2},
followingDoc is A2, and we'll need to do another traversal from A2 to get B's
owner.

That said, I was worried about departing from spec comments myself.  I switched
to the latter approach (your first option) -- this better follows the spec
quotes and results in fewer overall changes to requestFullscreen, at the cost of
a bit more complexity in the 4.3 check.  Let me know if you think this is
better.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:289: if
(currentFrame->isRemoteFrame())
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> Assuming that this isn't reverted to a list of only local documents, can you
> move this to before step 3 and document why the observable behavior will still
> match what the spec says?

I did revert to a list of only local documents, so this is gone now.  I also
added a note about how this affects observable behavior and the spec to step 3's
comment.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:324: // For OOPIF, use the
Document of the topmost local ancestor frame.  Each
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> "Use the document... because OOPIF something something" so that it doesn't
sound
> like this code path is only for OOPIF. The reader might notice the lack of "if
> (oopif)" but still doubt what's going on, as I am now in a first reading :)

Rephrased the comment - let me know if it's better. :)

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:400: // TODO(alexmos): Deal
with nested fullscreen cases.
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> What cases is that?

I described it in a comment on my original message, which probably got lost :) :
https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...

Basically, it's about fixing https://crbug.com/617369, both for OOPIF and
non-OOPIF cases.

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:405: if (frame) {
On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> OK, so this just skips over the remote frames and otherwise does what the spec
> says. Some symmetry with that for the requestFullscreen code path is what I'm
> hoping for above, if it can be had.

Let me know if you feel this still needs improvement after I revamped the
requestFullscreen path.

[foolip, 2016-06-22 16:23:58]

LGTM!

Last week was BlinkOn, but sorry for also being very slow this week, still
trying to perfect my inbox management skills :)

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:124: static Document&
topmostDocument(Document& document)
On 2016/06/09 21:36:55, alexmos wrote:
> On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> > Can you name this so that it's clear that it's not the top-level browsing
> > context's document, but the locally topmost document? Spelling out the
> > difference between this and Document::topDocument() would be good, because
> > without this CL leading me to think about A-B-A cases I'm not sure I would
> > understand how it's different.
> 
> Renamed to topmostLocalAncestor and added a comment explaining how it's
> different from top frame's document.  Let me know if that name sounds better.

That's great, and the comments throughout this CL were just great. I will try to
emulate :)

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:257: Frame* currentFrame =
document()->frame();
On 2016/06/09 21:36:55, alexmos wrote:
> On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> > The spec quotes are a lot let useful when the names don't match, thinking
> about
> > ways to minimize the difference.
> > 
> > First option, could the docs list be populated with all the documents for
> local
> > frames even if there are remote frames with documents in between them per
> spec?
> > Then much of the following logic would remain unchanged, and one would only
> need
> > to convince oneself that it's not observable.
> > 
> > Second option, if a Frame is equivalent to a browsing context in spec terms,
> > change the spec to talk about browsing contexts instead. But I think it'd
> still
> > be unnatural to say "Frame* browsingContext" so it still wouldn't match.
> 
> The main reason I originally kept ancestors as Frames is for the check in step
> 4.3 on the following document's container (the old topElement !=
> followingDoc->localOwner() check).  With an A-B-A2 hierarchy, we need B's
owner
> when checking A's topElement (B won't have a document in A's process, but it
> will have a local owner()).  With Frames, this is straightforward: the
ancestors
> list is {A,B,A2}, followingFrame is B, and we can just use its owner. If we
keep
> the ancestor list as documents (your first option), the list will be {A, A2},
> followingDoc is A2, and we'll need to do another traversal from A2 to get B's
> owner.
> 
> That said, I was worried about departing from spec comments myself.  I
switched
> to the latter approach (your first option) -- this better follows the spec
> quotes and results in fewer overall changes to requestFullscreen, at the cost
of
> a bit more complexity in the 4.3 check.  Let me know if you think this is
> better.

Looks great :)

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:400: // TODO(alexmos): Deal
with nested fullscreen cases.
On 2016/06/09 21:36:55, alexmos wrote:
> On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> > What cases is that?
> 
> I described it in a comment on my original message, which probably got lost :)
:
>
https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...
> 
> Basically, it's about fixing https://crbug.com/617369, both for OOPIF and
> non-OOPIF cases.

Can you link to it here, unless it's too specific or something?

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:405: if (frame) {
On 2016/06/09 21:36:55, alexmos wrote:
> On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> > OK, so this just skips over the remote frames and otherwise does what the
spec
> > says. Some symmetry with that for the requestFullscreen code path is what
I'm
> > hoping for above, if it can be had.
> 
> Let me know if you feel this still needs improvement after I revamped the
> requestFullscreen path.

I think the symmetry is there now. What still feels a bit strange is that one
enters the web-exposed requestFullscreen code path in multiple processes, but
unless that actually causes any trouble it's probably just fine :)

[alexmos, 2016-06-22 18:04:22]

Thanks for reviewing!

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2040973003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:400: // TODO(alexmos): Deal
with nested fullscreen cases.
On 2016/06/22 16:23:58, Philip Jägenstedt wrote:
> On 2016/06/09 21:36:55, alexmos wrote:
> > On 2016/06/09 13:12:36, Philip Jägenstedt wrote:
> > > What cases is that?
> > 
> > I described it in a comment on my original message, which probably got lost
:)
> :
> >
>
https://codereview.chromium.org/2040973003/diff/20001/third_party/WebKit/Sour...
> > 
> > Basically, it's about fixing https://crbug.com/617369, both for OOPIF and
> > non-OOPIF cases.
> 
> Can you link to it here, unless it's too specific or something?

Done.

[alexmos, 2016-06-22 18:08:37]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2016-06-22 18:08:38]

Charlie, could you please sanity-check the small change in
site_per_process_interactive_browsertest.cc?  I'm basically removing a couple of
TODOs which now work, and adding coverage for both browser-initiated and
renderer-initiated escape paths to the ABA test, as they trigger different paths
on the Blink side (and this CL touched both of them).

[Charlie Reis, 2016-06-22 18:41:23]

Test LGTM.

[alexmos, 2016-06-22 20:33:04]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-06-22 20:33:05]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2040973003/#ps100001
(title: "Add bug reference for nested fullscreen")

[commit-bot: I haz the power, 2016-06-22 20:33:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2040973003/100001

[commit-bot: I haz the power, 2016-06-22 22:04:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-22 22:06:29]

Description was changed from

==========
OOPIF: Process all local ancestors when requesting and exiting fullscreen.

Currently, requestFullscreen, fullyExitFullscreen, and exitFullscreen
perform several walks over ancestor frames, stopping as soon as they
hit a remote frame.  This means that for hierarchies like A-B-A, when
fullscreen is entered from the bottom frame, the top frame won't
update its fullscreen element stack or queue up fullscreenchange
events.  This CL fixes this by accounting for local ancestors
separated by remote frames in all of these walks.

BUG=550497
==========

to

==========
OOPIF: Process all local ancestors when requesting and exiting fullscreen.

Currently, requestFullscreen, fullyExitFullscreen, and exitFullscreen
perform several walks over ancestor frames, stopping as soon as they
hit a remote frame.  This means that for hierarchies like A-B-A, when
fullscreen is entered from the bottom frame, the top frame won't
update its fullscreen element stack or queue up fullscreenchange
events.  This CL fixes this by accounting for local ancestors
separated by remote frames in all of these walks.

BUG=550497

Committed: https://crrev.com/59e00ee5e5bdb631ed9617e3e028f76b06b35267
Cr-Commit-Position: refs/heads/master@{#401424}
==========

[commit-bot: I haz the power, 2016-06-22 22:06:30]

Patchset 6 (id:??) landed as
https://crrev.com/59e00ee5e5bdb631ed9617e3e028f76b06b35267
Cr-Commit-Position: refs/heads/master@{#401424}