Reland "Implement FullScreen using top layer."

Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946
Committed: https://crrev.com/d666f503ae854fac360cc70da1f5971a6724546a
Cr-Commit-Position: refs/heads/master@{#405478}

[foolip, 2016-06-29 14:32:43]

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

[commit-bot: I haz the power, 2016-06-29 14:33:16]

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-06-29 15:53:52]

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

[commit-bot: I haz the power, 2016-06-29 15:53:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2016-06-29 20:11:00]

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

[foolip, 2016-06-29 20:11:01]

dsinclair@, this isn't ready for review, many additional changes are needed, but
perhaps you can help me understand a TODO to begin with?

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:186: //
TODO(dsinclair): We should find a way to get this standardized. crbug.com/534924
dsinclair@, can you fill me in about the purpose of this? Is there any
connection to https://bugs.chromium.org/p/chromium/issues/detail?id=543946 and
https://github.com/whatwg/fullscreen/issues/19 where the question is whether
top-layer should end up on top of scrollbars?

[dsinclair, 2016-06-29 20:14:47]

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:186: //
TODO(dsinclair): We should find a way to get this standardized. crbug.com/534924
On 2016/06/29 20:11:00, Philip Jägenstedt wrote:
> dsinclair@, can you fill me in about the purpose of this? Is there any
> connection to https://bugs.chromium.org/p/chromium/issues/detail?id=543946 and
> https://github.com/whatwg/fullscreen/issues/19 where the question is whether
> top-layer should end up on top of scrollbars?


I'm not sure if this is what causes the scrollbar bug or not, I'm not too
familiar with the viewport stuff. The TODO was that we, preferrably, want a way
for CSS to expose the inner viewport instead of forcing it here.

[foolip, 2016-06-30 13:03:13]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

BUG=240576
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

BUG=240576
==========

[foolip, 2016-06-30 14:33:24]

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:186: //
TODO(dsinclair): We should find a way to get this standardized. crbug.com/534924
On 2016/06/29 20:14:47, dsinclair wrote:
> On 2016/06/29 20:11:00, Philip Jägenstedt wrote:
> > dsinclair@, can you fill me in about the purpose of this? Is there any
> > connection to https://bugs.chromium.org/p/chromium/issues/detail?id=543946
and
> > https://github.com/whatwg/fullscreen/issues/19 where the question is whether
> > top-layer should end up on top of scrollbars?
> 
> 
> I'm not sure if this is what causes the scrollbar bug or not, I'm not too
> familiar with the viewport stuff. The TODO was that we, preferrably, want a
way
> for CSS to expose the inner viewport instead of forcing it here.

I tried reverting this and running test to understand what the difference is,
but no tests failed :)

Is the visual viewport the pinch zoom viewport, or something else? There are
some tricks in FullscreenController to save and restore scale and
scroll/viewport offsets, is this related?

[foolip, 2016-07-05 15:49:07]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

BUG=240576
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.


:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

BUG=240576, 543946
==========

[foolip, 2016-07-05 15:49:16]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.


:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

BUG=240576, 543946
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

BUG=240576, 543946
==========

[foolip, 2016-07-06 13:19:43]

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html
(right):

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html:15:
background-color: black;
This change seems to mask an actual regression in the CL that I'm investigating.
AFAICT, the ::backdrop element simply doesn't work for video elements, and also,
it seems, iframes. Maybe others too. Because no backdrop is painted, instead one
simply sees the page behind the (transparent) fullscreen element.

dsinclair@, does this sound familiar at all, is it a problem you encountered?

[dsinclair, 2016-07-06 13:32:31]

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:186: //
TODO(dsinclair): We should find a way to get this standardized. crbug.com/534924
On 2016/06/30 14:33:24, Philip Jägenstedt wrote:
> On 2016/06/29 20:14:47, dsinclair wrote:
> > On 2016/06/29 20:11:00, Philip Jägenstedt wrote:
> > > dsinclair@, can you fill me in about the purpose of this? Is there any
> > > connection to https://bugs.chromium.org/p/chromium/issues/detail?id=543946
> and
> > > https://github.com/whatwg/fullscreen/issues/19 where the question is
whether
> > > top-layer should end up on top of scrollbars?
> > 
> > 
> > I'm not sure if this is what causes the scrollbar bug or not, I'm not too
> > familiar with the viewport stuff. The TODO was that we, preferrably, want a
> way
> > for CSS to expose the inner viewport instead of forcing it here.
> 
> I tried reverting this and running test to understand what the difference is,
> but no tests failed :)
> 
> Is the visual viewport the pinch zoom viewport, or something else? There are
> some tricks in FullscreenController to save and restore scale and
> scroll/viewport offsets, is this related?


I was never really clear on viewport. There is an inner and outer viewport used
for pinch-zoom. I'm not sure how those relate to the visual viewport, if at all.
You could try asking bokan@ as he did a lot of viewport work.

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html
(right):

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html:15:
background-color: black;
On 2016/07/06 13:19:43, Philip Jägenstedt wrote:
> This change seems to mask an actual regression in the CL that I'm
investigating.
> AFAICT, the ::backdrop element simply doesn't work for video elements, and
also,
> it seems, iframes. Maybe others too. Because no backdrop is painted, instead
one
> simply sees the page behind the (transparent) fullscreen element.
> 
> dsinclair@, does this sound familiar at all, is it a problem you encountered?


This is the expected file, so this is what the cl should look like when it's
working. This shouldn't effect anything in the actual CL.

[foolip, 2016-07-06 14:14:52]

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2107233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:186: //
TODO(dsinclair): We should find a way to get this standardized. crbug.com/534924
On 2016/07/06 13:32:31, dsinclair wrote:
> On 2016/06/30 14:33:24, Philip Jägenstedt wrote:
> > On 2016/06/29 20:14:47, dsinclair wrote:
> > > On 2016/06/29 20:11:00, Philip Jägenstedt wrote:
> > > > dsinclair@, can you fill me in about the purpose of this? Is there any
> > > > connection to
https://bugs.chromium.org/p/chromium/issues/detail?id=543946
> > and
> > > > https://github.com/whatwg/fullscreen/issues/19 where the question is
> whether
> > > > top-layer should end up on top of scrollbars?
> > > 
> > > 
> > > I'm not sure if this is what causes the scrollbar bug or not, I'm not too
> > > familiar with the viewport stuff. The TODO was that we, preferrably, want
a
> > way
> > > for CSS to expose the inner viewport instead of forcing it here.
> > 
> > I tried reverting this and running test to understand what the difference
is,
> > but no tests failed :)
> > 
> > Is the visual viewport the pinch zoom viewport, or something else? There are
> > some tricks in FullscreenController to save and restore scale and
> > scroll/viewport offsets, is this related?
> 
> 
> I was never really clear on viewport. There is an inner and outer viewport
used
> for pinch-zoom. I'm not sure how those relate to the visual viewport, if at
all.
> You could try asking bokan@ as he did a lot of viewport work.

Thanks, I'll do that if I can't figure this out.

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html
(right):

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html:15:
background-color: black;
On 2016/07/06 13:32:31, dsinclair wrote:
> On 2016/07/06 13:19:43, Philip Jägenstedt wrote:
> > This change seems to mask an actual regression in the CL that I'm
> investigating.
> > AFAICT, the ::backdrop element simply doesn't work for video elements, and
> also,
> > it seems, iframes. Maybe others too. Because no backdrop is painted, instead
> one
> > simply sees the page behind the (transparent) fullscreen element.
> > 
> > dsinclair@, does this sound familiar at all, is it a problem you
encountered?
> 
> 
> This is the expected file, so this is what the cl should look like when it's
> working. This shouldn't effect anything in the actual CL.

Right, but what I noticed when trying to simplify some test expectations is that
if the background-color:black is removed from here, the test fails, but I think
it shouldn't, because fullscreen.css has
:not(:root):-webkit-full-screen::backdrop { background: black }.

I'm in the debugger now and it looks the ::backdrop element isn't ever added to
the top layer in Element::createPseudoElementIfNeeded when the fullscreen
element is a video, iframe or img, so maybe it has something to do with replaced
content.

If this doesn't sound at all familiar I'll just keep investigating, just thought
this might have been a deliberate workaround for the issue.

[dsinclair, 2016-07-06 14:17:38]

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html
(right):

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html:15:
background-color: black;
On 2016/07/06 14:14:51, Philip Jägenstedt wrote:
> On 2016/07/06 13:32:31, dsinclair wrote:
> > On 2016/07/06 13:19:43, Philip Jägenstedt wrote:
> > > This change seems to mask an actual regression in the CL that I'm
> > investigating.
> > > AFAICT, the ::backdrop element simply doesn't work for video elements, and
> > also,
> > > it seems, iframes. Maybe others too. Because no backdrop is painted,
instead
> > one
> > > simply sees the page behind the (transparent) fullscreen element.
> > > 
> > > dsinclair@, does this sound familiar at all, is it a problem you
> encountered?
> > 
> > 
> > This is the expected file, so this is what the cl should look like when it's
> > working. This shouldn't effect anything in the actual CL.
> 
> Right, but what I noticed when trying to simplify some test expectations is
that
> if the background-color:black is removed from here, the test fails, but I
think
> it shouldn't, because fullscreen.css has
> :not(:root):-webkit-full-screen::backdrop { background: black }.
> 
> I'm in the debugger now and it looks the ::backdrop element isn't ever added
to
> the top layer in Element::createPseudoElementIfNeeded when the fullscreen
> element is a video, iframe or img, so maybe it has something to do with
replaced
> content.
> 
> If this doesn't sound at all familiar I'll just keep investigating, just
thought
> this might have been a deliberate workaround for the issue.


It wasn't deliberate. It sounds like there is an issue if the ::backdrop isn't
getting applied. I was trying to match the expectation to what was produced and
what was produced looked right to me.

[foolip, 2016-07-06 15:20:28]

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html
(right):

https://codereview.chromium.org/2107233002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fullscreen/full-screen-stacking-context-expected.html:15:
background-color: black;
On 2016/07/06 14:17:38, dsinclair wrote:
> On 2016/07/06 14:14:51, Philip Jägenstedt wrote:
> > On 2016/07/06 13:32:31, dsinclair wrote:
> > > On 2016/07/06 13:19:43, Philip Jägenstedt wrote:
> > > > This change seems to mask an actual regression in the CL that I'm
> > > investigating.
> > > > AFAICT, the ::backdrop element simply doesn't work for video elements,
and
> > > also,
> > > > it seems, iframes. Maybe others too. Because no backdrop is painted,
> instead
> > > one
> > > > simply sees the page behind the (transparent) fullscreen element.
> > > > 
> > > > dsinclair@, does this sound familiar at all, is it a problem you
> > encountered?
> > > 
> > > 
> > > This is the expected file, so this is what the cl should look like when
it's
> > > working. This shouldn't effect anything in the actual CL.
> > 
> > Right, but what I noticed when trying to simplify some test expectations is
> that
> > if the background-color:black is removed from here, the test fails, but I
> think
> > it shouldn't, because fullscreen.css has
> > :not(:root):-webkit-full-screen::backdrop { background: black }.
> > 
> > I'm in the debugger now and it looks the ::backdrop element isn't ever added
> to
> > the top layer in Element::createPseudoElementIfNeeded when the fullscreen
> > element is a video, iframe or img, so maybe it has something to do with
> replaced
> > content.
> > 
> > If this doesn't sound at all familiar I'll just keep investigating, just
> thought
> > this might have been a deliberate workaround for the issue.
> 
> 
> It wasn't deliberate. It sounds like there is an issue if the ::backdrop isn't
> getting applied. I was trying to match the expectation to what was produced
and
> what was produced looked right to me.

I have a hard time deciding what would make most sense, but in any case the
handling of background will change from

:-webkit-full-screen {
    background-color: white;
}

video:-webkit-full-screen, audio:-webkit-full-screen {
    background-color: transparent !important;
}

to just

:not(:root):-webkit-full-screen::backdrop {
    background: black;
}

So in a lot of cases it will go from white to black, and this is such a case.
It's just that the ::backdrop pseudo doesn't actually work for video elements,
if it did then the line I've commented on wouldn't be needed.

Maybe I'll push back against the spec regarding the specific colors, but in any
case ::backdrop needs to work.

I was right to suspect replaced content, in
StyleResolver::createPseudoElementIfNeeded it bails out because
!canHaveGeneratedChildren(*parentLayoutObject), which happens for media
elements, some input elements and all LayoutReplaced things. It can be "fixed"
by just bypassing the check for PseudoIdBackdrop, but I doubt that's sensible.
I'll create a separate review for that I think.

[foolip, 2016-07-07 16:04:57]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

BUG=240576, 543946
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

BUG=240576, 543946
==========

[foolip, 2016-07-07 16:05:28]

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

[commit-bot: I haz the power, 2016-07-07 16:06:14]

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

[foolip, 2016-07-07 16:07:01]

This is now ready for real review, PTAL or suggest who would be a good reviewer
for each bit. There's more to be done but I think this change is a chunk that at
least has a chance of reaching stable on its own, although I plan to follow up
with more CLs of course.

PS1 is the original CL, with each relevant change is subsequent patchsets.

[dsinclair, 2016-07-07 16:09:30]

dsinclair@chromium.org changed reviewers:
+ eae@chromium.org, esprehn@chromium.org

[dsinclair, 2016-07-07 16:09:31]

+esprehn and +eae. Not sure if they're the correct reviewers, but are a good
starting point at least.

[foolip, 2016-07-07 16:11:56]

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:893: if (pseudoId
!= PseudoIdBackdrop && !canHaveGeneratedChildren(*parentLayoutObject))
The comment is written with confidence, but please don't assume that it's
entirely true, I'm concerned that this may actually still be dangerous for some
case I haven't thought about.

[commit-bot: I haz the power, 2016-07-07 20:39:24]

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

[commit-bot: I haz the power, 2016-07-07 20:39:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eae, 2016-07-12 17:09:49]

Overall this looks great. Two quick questions though.

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:893: if (pseudoId
!= PseudoIdBackdrop && !canHaveGeneratedChildren(*parentLayoutObject))
On 2016/07/07 16:11:55, Philip Jägenstedt wrote:
> The comment is written with confidence, but please don't assume that it's
> entirely true, I'm concerned that this may actually still be dangerous for
some
> case I haven't thought about.

Can we add asserts to ensure that our assumptions here hold true?

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5308: for (auto it =
m_topLayerElements.rbegin(); it != m_topLayerElements.rend(); ++it) {
Why are we iterating backwards here?

[foolip, 2016-07-12 20:19:48]

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:893: if (pseudoId
!= PseudoIdBackdrop && !canHaveGeneratedChildren(*parentLayoutObject))
On 2016/07/12 17:09:49, eae wrote:
> On 2016/07/07 16:11:55, Philip Jägenstedt wrote:
> > The comment is written with confidence, but please don't assume that it's
> > entirely true, I'm concerned that this may actually still be dangerous for
> some
> > case I haven't thought about.
> 
> Can we add asserts to ensure that our assumptions here hold true?

Good, idea, at some point it should be possible to assert that the layout object
created for the backdrop doesn't become the child of a layout object for which
canHaveGeneratedChildren returns true. I'll look into it tomorrow.

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5308: for (auto it =
m_topLayerElements.rbegin(); it != m_topLayerElements.rend(); ++it) {
On 2016/07/12 17:09:49, eae wrote:
> Why are we iterating backwards here?

This is merely because the end of the m_topLayerElements vector is treated as
the top of the top layer stack. In Document::addToTopLayer one can see this, and
I assume it's because pushing/popping at the end of the vector is more
efficient. I'll document this in Document.h.

[eae, 2016-07-12 20:25:04]

On 2016/07/12 20:19:48, Philip Jägenstedt wrote:
>
https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:893: if
(pseudoId
> != PseudoIdBackdrop && !canHaveGeneratedChildren(*parentLayoutObject))
> On 2016/07/12 17:09:49, eae wrote:
> > On 2016/07/07 16:11:55, Philip Jägenstedt wrote:
> > > The comment is written with confidence, but please don't assume that it's
> > > entirely true, I'm concerned that this may actually still be dangerous for
> > some
> > > case I haven't thought about.
> > 
> > Can we add asserts to ensure that our assumptions here hold true?
> 
> Good, idea, at some point it should be possible to assert that the layout
object
> created for the backdrop doesn't become the child of a layout object for which
> canHaveGeneratedChildren returns true. I'll look into it tomorrow.
> 
>
https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2107233002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Document.cpp:5308: for (auto it =
> m_topLayerElements.rbegin(); it != m_topLayerElements.rend(); ++it) {
> On 2016/07/12 17:09:49, eae wrote:
> > Why are we iterating backwards here?
> 
> This is merely because the end of the m_topLayerElements vector is treated as
> the top of the top layer stack. In Document::addToTopLayer one can see this,
and
> I assume it's because pushing/popping at the end of the vector is more
> efficient. I'll document this in Document.h.

Ah, that makes sense. A comment would be very helpful!

Thanks


LGTM given the above.

[foolip, 2016-07-13 15:47:08]

Patchset #7 (id:120001) has been deleted

[foolip, 2016-07-14 09:54:34]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

BUG=240576, 543946
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946
==========

[foolip, 2016-07-14 09:55:56]

OK, I've made the requested changes and will CQ, but I'd still appreciate a
sanity check of the added DCHECK, if there's another place that might catch more
problems.

[foolip, 2016-07-14 09:56:07]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-07-14 09:56:07]

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/2107233002/#ps260001
(title: "new untested DCHECK")

[commit-bot: I haz the power, 2016-07-14 09:56:23]

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-14 12:34:01]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946
==========

[commit-bot: I haz the power, 2016-07-14 12:34:04]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-07-14 12:34:09]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 12:35:42]

Description was changed from

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946
==========

to

==========
Reland "Implement FullScreen using top layer."

This reverts commit 2460a95281959d1b4dd941fed10998adf8dcb63c.

Notable changes on top of previous attempt:

full-screen-iframe-ua-style.html is changed to use overflow property
instead, as position is no longer changed on ancestors.

:root:-webkit-full-screen-ancestor { overflow: hidden !important; } is
retained, as the previous attempt regressed on Vimeo (crbug.com/543946)
and it's not clear yet how to handle this in the spec:
https://github.com/whatwg/fullscreen/issues/19

Make ::backdrop work for replaced content (video, iframe, etc.) by
changing StyleResolver::createPseudoElementIfNeeded to allow creating
the ::backdrop pseudo-element even if the parent doesn't support
children. With this change, full-screen-stacking-context.html was
simplified and full-screen-video-has-backdrop.html was
simplified+renamed.

The will-change override is dropped, as full-screen-will-change.html
passes without it.

BUG=240576, 543946

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

[commit-bot: I haz the power, 2016-07-14 12:35:44]

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

[eae, 2016-07-14 16:25:36]

LGTM, thanks!

[foolip, 2016-07-22 03:35:13]

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

The reason for reverting is: Multiple regressions around the fullscreen
transition becoming visibly worse, as well as a suspected leak.

BUG=628554,628605,628662,630191.