Clean up some OOPIF-related naming and comments in Fullscreen.

Clean up some OOPIF-related naming and comments in Fullscreen.

m_forCrossProcessAncestor was intended to mean that fullscreen was
entered for an iframe container which is an ancestor of the actual
fullscreen element, which also mirrors the
:-webkit-full-screen-ancestor style that the container will gain.  But
on the flipside, this naming can also be confusing: if an ancestor of
a cross-process iframe goes fullscreen, that iframe doesn't need to
know about this at all, and instead it only needs to know when an
element in a descendant frame goes fullscreen.  It might make more
sense to name this flag to reflect where the actual fullscreen element
is, rather than how m_fullscreenElement relates to it, so this CL
renames the flag to m_forCrossProcessDescendant and updates comments.

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

[alexmos, 2016-07-12 23:28:48]

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

[alexmos, 2016-07-12 23:28:48]

Philip, please take a look.  This is the renaming that we discussed.  I also
moved a stale comment on didEnterFullscreenForElement to a better place.  I
guess one other reason for the old naming was that it matched the
:-webkit-full-screen-ancestor pseudoclass which needs to be applied if this flag
is true (i.e., current frame's fullscreen element is ancestor of actual
fullscreen element).  But your argument still seems compelling enough to rename
it, as was the fact that the comment on requestFullscreen() already used the
word "descendant". :)

[foolip, 2016-07-13 08:58:14]

LGTM, thanks! If you get conflicts just try again, and if I see this in CQ I'll
hold off things that would conflict.

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

https://codereview.chromium.org/2147803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:508: // TODO(alexmos): When
|m_forCrossProcessDescendant| is true, some of
I actually think this comment is wrong, the fullscreen element in each document
needs to get the same treatment. I may be missing something, but it doesn't
matter much, because this would go away with the top layer rewrite.

https://codereview.chromium.org/2147803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:516: // that contains the
actual fullscreen element.   Hence, it must also set
I'm really looking forward to getting rid of :-webkit-full-screen-ancestor :)

[alexmos, 2016-07-13 17:52:18]

Thanks!

[also +site-isolation-reviews, which I forgot to include initially]

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

https://codereview.chromium.org/2147803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:508: // TODO(alexmos): When
|m_forCrossProcessDescendant| is true, some of
On 2016/07/13 08:58:14, foolip wrote:
> I actually think this comment is wrong, the fullscreen element in each
document
> needs to get the same treatment. I may be missing something, but it doesn't
> matter much, because this would go away with the top layer rewrite.

The thinking behind the comment was that when all frames are in the same
process, the LayoutFullScreen::wrapLayoutObject happens only once in the deepest
document, but with OOPIFs, it currently happens once for the deepest document in
each process.  I thought doing it in the other (ancestor) processes isn't really
necessary (though it doesn't hurt), as these other ancestors are just stretching
the inner fullscreened iframe to fill up their entire viewport (via
setContainsFullScreenElement and iframe:-webkit-full-screen) -- I actually had a
prototype working where I skipped doing it, and things still seemed to work
afaict.  Maybe there are other gotchas with this that I wasn't aware of.  But
you're right that at this point it doesn't matter, as this will go away with top
layer, so it's probably not worth spending time to finish that CL.

https://codereview.chromium.org/2147803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:516: // that contains the
actual fullscreen element.   Hence, it must also set
On 2016/07/13 08:58:14, foolip wrote:
> I'm really looking forward to getting rid of :-webkit-full-screen-ancestor :)

That sounds great. :)  I guess that's because it isn't part of the spec anymore?
 Do we have/need counters that existing web content isn't relying on it?  I see
that Mozilla just did this too, which is exciting:
https://www.fxsitecompat.com/en-CA/docs/2016/moz-full-screen-ancestor-has-bee...

[alexmos, 2016-07-13 17:52:25]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-07-13 17:52:53]

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-07-13 18:03:26]

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

[commit-bot: I haz the power, 2016-07-13 18:03:27]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexmos, 2016-07-13 18:36:28]

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

[alexmos, 2016-07-13 18:36:29]

bokan@: can you please review the trivial rename in
Source/web/FullscreenController.cpp for OWNERS?

[bokan, 2016-07-13 18:37:18]

rslgtm

[bokan, 2016-07-13 18:37:30]

rs lgtm, rather

[alexmos, 2016-07-13 19:42:03]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-07-13 19:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2147803002/#ps20001
(title: "Resolve conflicts")

[commit-bot: I haz the power, 2016-07-13 19:44:40]

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-07-13 21:26:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-13 21:27:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 21:28:13]

Description was changed from

==========
Clean up some OOPIF-related naming and comments in Fullscreen.

m_forCrossProcessAncestor was intended to mean that fullscreen was
entered for an iframe container which is an ancestor of the actual
fullscreen element, which also mirrors the
:-webkit-full-screen-ancestor style that the container will gain.  But
on the flipside, this naming can also be confusing: if an ancestor of
a cross-process iframe goes fullscreen, that iframe doesn't need to
know about this at all, and instead it only needs to know when an
element in a descendant frame goes fullscreen.  It might make more
sense to name this flag to reflect where the actual fullscreen element
is, rather than how m_fullscreenElement relates to it, so this CL
renames the flag to m_forCrossProcessDescendant and updates comments.

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

to

==========
Clean up some OOPIF-related naming and comments in Fullscreen.

m_forCrossProcessAncestor was intended to mean that fullscreen was
entered for an iframe container which is an ancestor of the actual
fullscreen element, which also mirrors the
:-webkit-full-screen-ancestor style that the container will gain.  But
on the flipside, this naming can also be confusing: if an ancestor of
a cross-process iframe goes fullscreen, that iframe doesn't need to
know about this at all, and instead it only needs to know when an
element in a descendant frame goes fullscreen.  It might make more
sense to name this flag to reflect where the actual fullscreen element
is, rather than how m_fullscreenElement relates to it, so this CL
renames the flag to m_forCrossProcessDescendant and updates comments.

BUG=550497

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

[commit-bot: I haz the power, 2016-07-13 21:28:14]

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

[foolip, 2016-07-13 21:40:30]

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

https://codereview.chromium.org/2147803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:516: // that contains the
actual fullscreen element.   Hence, it must also set
On 2016/07/13 17:52:18, alexmos wrote:
> On 2016/07/13 08:58:14, foolip wrote:
> > I'm really looking forward to getting rid of :-webkit-full-screen-ancestor
:)
> 
> That sounds great. :)  I guess that's because it isn't part of the spec
anymore?
>  Do we have/need counters that existing web content isn't relying on it?  I
see
> that Mozilla just did this too, which is exciting:
>
https://www.fxsitecompat.com/en-CA/docs/2016/moz-full-screen-ancestor-has-bee...

It was completely necessary before in order to get the ancestor elements out of
the way, but now the ancestor elements won't be ancestor layout objects so there
won't be much useful you can do with :-webkit-full-screen-ancestor. The use
counter is https://www.chromestatus.com/metrics/feature/timeline/popularity/628
but I don't think that's a good predictor of risk, I'd have to search
httparchive before attempting a removal.

The last remaining thing that depends on it is to remove scrollbars, but I think
that could be done without the selector.