Replace RenderFullscreen with top layer - Take II

Replace RenderFullscreen with top layer - Take II

This patch is strongly inspired by
https://codereview.chromium.org/134753003/
but is a complete rewrite of the change to:
- understand why every bits was needed (some were not).
- better document the differences we have with the
specification so that we can track and fix them.

The issue with re-attaching plugins went away due to
https://codereview.chromium.org/23618022, which moved
the plugin's lifecycle to the DOM. 

The tests' changes are because fullscreen doesn't center
by default anymore but stretches the element to the
viewport's size. Some tests were also not accounting
for the backdrop and where thus modified.

BUG=240576, 246077, 398599

[Julien - ping for review, 2014-12-13 00:08:29]

jchaffraix@chromium.org changed reviewers:
+ aelias@chromium.org, dsinclair@chromium.org, esprehn@chromium.org,
falken@chromium.org, philipj@opera.com

[Julien - ping for review, 2014-12-13 00:08:29]

Time to revive sync'ing fullscreen with the specification.

@falken: I removed some part of your change. Ideally you will want to check that
I didn't forget something. Review required.

@aelias / @philipj: it's an FYI as you did or will do some fullscreen work.
Reviews deeply appreciated.

@dsinclair: you're on the hook for the review.

@esprehn: FYI as the original reviewer. Review optional (there is some recalc
style change though)

[falken, 2014-12-15 08:19:51]

Thanks so much for reviving this. The challenge I had with the old patch was
preventing Flash videos to restart when they went fullscreen in the middle of
playback (more generally, keeping a plugin alive when added/removed from the top
layer, which triggers a reattach). Does this patch deal with that case?

Another BUG to you might add is 398599, this should fix that. There is a test
attached to the bug you can use as a layout test.

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fullscreen/full-...
File LayoutTests/fullscreen/full-screen-video-has-backdrop-expected.html
(right):

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fullscreen/full-...
LayoutTests/fullscreen/full-screen-video-has-backdrop-expected.html:13: height:
100%;
I don't understand this expectation, how do you see the green background if
object is 100% height and width?

https://codereview.chromium.org/788073004/diff/1/Source/core/css/fullscreen.css
File Source/core/css/fullscreen.css (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/css/fullscreen.c...
Source/core/css/fullscreen.css:31: /* Anything below is an extensions over what
the Fullscreen API (14 October 2014) mandates. */
"is an extension"

[rune, 2014-12-15 09:39:50]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2014-12-15 09:39:51]

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fast/css/invalid...
File LayoutTests/fast/css/invalidation/fullscreen-expected.txt (right):

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fast/css/invalid...
LayoutTests/fast/css/invalidation/fullscreen-expected.txt:7: PASS
internals.updateStyleAndReturnAffectedElementCount() is 3
This is due to a ::backdrop recalc?

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
This looks like a really big cannon. Which style changes is this supposed to
trigger?

[esprehn, 2014-12-15 09:45:32]

This won't work as-is, you'll make flash restart on reattach which breaks a
bunch of sites. I guess YouTube is fine if we assume the new HTML5 player...

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:5099: for (; it != end; ++it) {
Use C++11 for loop.

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Fullscreen.h
File Source/core/dom/Fullscreen.h (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Fullscreen.h...
Source/core/dom/Fullscreen.h:89: const
WillBeHeapVector<std::pair<RefPtrWillBeMember<Element>, RequestType> >&
fullScreenElementStack() { return m_fullScreenElementStack; }
Add ForTesting() for the function name?

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
On 2014/12/15 at 09:39:51, rune wrote:
> This looks like a really big cannon. Which style changes is this supposed to
trigger?

This is probably for -webkit-full-screen-ancestor, that thing already does huge
recalcs in Element.cpp, we should fix it.

[rune, 2014-12-15 09:56:41]

On 2014/12/15 at 09:45:32, esprehn wrote:

>
https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
> Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
> On 2014/12/15 at 09:39:51, rune wrote:
> > This looks like a really big cannon. Which style changes is this supposed to
trigger?
> 
> This is probably for -webkit-full-screen-ancestor, that thing already does
huge recalcs in Element.cpp, we should fix it.

Yes, I was wondering why this additional subtree recalc was done given the ones
already in the enter/exit fullscreen code and the one for fullscreen-ancestors
in Element.

This should just be a matter of making :-webkit-full-screen and
:-webkit-full-screen-ancestor have invalidation sets in the pseudo map and
schedule invalidation sets on the node chain.

[rune, 2014-12-15 10:08:20]

On 2014/12/15 at 09:56:41, rune wrote:
> On 2014/12/15 at 09:45:32, esprehn wrote:
> 
> >
https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
> > Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
> > On 2014/12/15 at 09:39:51, rune wrote:
> > > This looks like a really big cannon. Which style changes is this supposed
to trigger?
> > 
> > This is probably for -webkit-full-screen-ancestor, that thing already does
huge recalcs in Element.cpp, we should fix it.
> 
> Yes, I was wondering why this additional subtree recalc was done given the
ones already in the enter/exit fullscreen code and the one for
fullscreen-ancestors in Element.
> 
> This should just be a matter of making :-webkit-full-screen and
:-webkit-full-screen-ancestor have invalidation sets in the pseudo map and
schedule invalidation sets on the node chain.

Reported: https://code.google.com/p/chromium/issues/detail?id=442239

[Julien - ping for review, 2014-12-15 16:49:22]

On 2014/12/15 at 08:19:51, falken wrote:
> Thanks so much for reviving this. The challenge I had with the old patch was
preventing Flash videos to restart when they went fullscreen in the middle of
playback (more generally, keeping a plugin alive when added/removed from the top
layer, which triggers a reattach). Does this patch deal with that case?

This patch doesn't care about plugins because it was taken care of by
https://codereview.chromium.org/23618022. I have updated the description (it
should have been mentioned).

I have manually tested and regular <video> tags and they work.

> Another BUG to you might add is 398599, this should fix that. There is a test
attached to the bug you can use as a layout test.

Added, thanks!

[rune, 2014-12-15 16:58:31]

On 2014/12/15 at 10:08:20, rune wrote:
> On 2014/12/15 at 09:56:41, rune wrote:
> > On 2014/12/15 at 09:45:32, esprehn wrote:

> > > This is probably for -webkit-full-screen-ancestor, that thing already does
huge recalcs in Element.cpp, we should fix it.
> > 
> > Yes, I was wondering why this additional subtree recalc was done given the
ones already in the enter/exit fullscreen code and the one for
fullscreen-ancestors in Element.
> > 
> > This should just be a matter of making :-webkit-full-screen and
:-webkit-full-screen-ancestor have invalidation sets in the pseudo map and
schedule invalidation sets on the node chain.
> 
> Reported: https://code.google.com/p/chromium/issues/detail?id=442239

FYI:

https://codereview.chromium.org/807473002
https://codereview.chromium.org/803133002

Latter depending on the former, but lacks tests. Will do tomorrow.

[rune, 2014-12-15 17:02:13]

On 2014/12/15 at 16:58:31, rune wrote:

> https://codereview.chromium.org/807473002
> https://codereview.chromium.org/803133002
> 
> Latter depending on the former, but lacks tests. Will do tomorrow.

Both interfere with this CL as well. I can put them on hold until this CL lands.

[Julien - ping for review, 2014-12-15 17:02:43]

On 2014/12/15 at 09:45:32, esprehn wrote:
> This won't work as-is, you'll make flash restart on reattach which breaks a
bunch of sites. I guess YouTube is fine if we assume the new HTML5 player...

I will need to test but I thought that https://codereview.chromium.org/23618022
would have taken of the restarting (I thought I had a test case for plugins but
the site I was using to validate has a <video>).

[Julien - ping for review, 2014-12-15 17:02:53]

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fast/css/invalid...
File LayoutTests/fast/css/invalidation/fullscreen-expected.txt (right):

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fast/css/invalid...
LayoutTests/fast/css/invalidation/fullscreen-expected.txt:7: PASS
internals.updateStyleAndReturnAffectedElementCount() is 3
On 2014/12/15 09:39:51, rune wrote:
> This is due to a ::backdrop recalc?

Indeed.

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fullscreen/full-...
File LayoutTests/fullscreen/full-screen-video-has-backdrop-expected.html
(right):

https://codereview.chromium.org/788073004/diff/1/LayoutTests/fullscreen/full-...
LayoutTests/fullscreen/full-screen-video-has-backdrop-expected.html:13: height:
100%;
On 2014/12/15 08:19:50, falken wrote:
> I don't understand this expectation, how do you see the green background if
> object is 100% height and width?

The <object> has a transparent background, thus we see the backdrop. The width /
height are needed to match the sizing in the test case.

https://codereview.chromium.org/788073004/diff/1/Source/core/css/fullscreen.css
File Source/core/css/fullscreen.css (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/css/fullscreen.c...
Source/core/css/fullscreen.css:31: /* Anything below is an extensions over what
the Fullscreen API (14 October 2014) mandates. */
On 2014/12/15 08:19:50, falken wrote:
> "is an extension"

Changed to are "extensions"

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:5099: for (; it != end; ++it) {
On 2014/12/15 09:45:32, esprehn wrote:
> Use C++11 for loop.

Done

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Fullscreen.h
File Source/core/dom/Fullscreen.h (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Fullscreen.h...
Source/core/dom/Fullscreen.h:89: const
WillBeHeapVector<std::pair<RefPtrWillBeMember<Element>, RequestType> >&
fullScreenElementStack() { return m_fullScreenElementStack; }
On 2014/12/15 09:45:32, esprehn wrote:
> Add ForTesting() for the function name?

Removed the for-testing comment as it could be useful for ASSERTs too.

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
On 2014/12/15 09:45:32, esprehn wrote:
> On 2014/12/15 at 09:39:51, rune wrote:
> > This looks like a really big cannon. Which style changes is this supposed to
> trigger?
> 
> This is probably for -webkit-full-screen-ancestor, that thing already does
huge
> recalcs in Element.cpp, we should fix it.

FWIW this is copied from what Fullscreen is doing for consistency.

I would like to remove -webkit-full-screen-ancestor and
-webkit-full-screen-document but the change is already too big / disruptive so
that will wait until a follow-up patch.

[Julien - ping for review, 2014-12-15 21:32:03]

> I will need to test but I thought that
https://codereview.chromium.org/23618022 would have taken of the restarting (I
thought I had a test case for plugins but the site I was using to validate has a
<video>).

Testing on dailymotion, re-attaching the plugin restarts the video so the bug
isn't fixed.

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:5099: for (; it != end; ++it) {
On 2014/12/15 17:02:53, Julien Chaffraix - PST wrote:
> On 2014/12/15 09:45:32, esprehn wrote:
> > Use C++11 for loop.
> 
> Done

Actually I probably didn't do what asked for: we can't use range-based for as
it's a *reverse* iterator (it would call begin() and end() which would do a
normal iteration).

Instead I used 'auto' instead to simplify the loop.

[rune, 2014-12-16 14:22:14]

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
On 2014/12/15 17:02:53, Julien Chaffraix - PST wrote:
> On 2014/12/15 09:45:32, esprehn wrote:
> > On 2014/12/15 at 09:39:51, rune wrote:
> > > This looks like a really big cannon. Which style changes is this supposed
to
> > trigger?
> > 
> > This is probably for -webkit-full-screen-ancestor, that thing already does
> huge
> > recalcs in Element.cpp, we should fix it.
> 
> FWIW this is copied from what Fullscreen is doing for consistency.

But in Fullscreen (and Element) setNeedsStyleRecalc is a direct consequence of
changing Element state affecting :-webkit-full-screen and
:-webkit-full-screen-ancestor selectors. In what way does updateSize() affect
selector evaluation?

[Julien - ping for review, 2014-12-16 17:00:59]

On 2014/12/16 at 14:22:14, rune wrote:
>
https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
> File Source/web/FullscreenController.cpp (right):
> 
>
https://codereview.chromium.org/788073004/diff/1/Source/web/FullscreenControl...
> Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
> On 2014/12/15 17:02:53, Julien Chaffraix - PST wrote:
> > On 2014/12/15 09:45:32, esprehn wrote:
> > > On 2014/12/15 at 09:39:51, rune wrote:
> > > > This looks like a really big cannon. Which style changes is this
supposed to
> > > trigger?
> > > 
> > > This is probably for -webkit-full-screen-ancestor, that thing already does
> > huge
> > > recalcs in Element.cpp, we should fix it.
> > 
> > FWIW this is copied from what Fullscreen is doing for consistency.
> 
> But in Fullscreen (and Element) setNeedsStyleRecalc is a direct consequence of
changing Element state affecting :-webkit-full-screen and
:-webkit-full-screen-ancestor selectors. In what way does updateSize() affect
selector evaluation?

We would only need layout if it wasn't for some virtual viewport magic. Due to
how the outer/inner viewport work, we have to force the size of the fullscreen
element to the inner viewport, which is done in StyleAdjuster.cpp. I thought it
was clearer than the old code but would welcome a better design.

[rune, 2014-12-16 19:10:24]

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleAdjuster.cpp:187:
style->setHeight(Length(viewportSize.height(), Fixed));
This looks very weird to me. If I understand correctly, this means that you'll
get a reflow on every pinch zoom change and effectively you'll get a zoom
behavior for fullscreen on Android that behaves like a desktop type of zoom
where you scale the CSS pixel size.

What's the reasoning behind this behavior? I couldn't see anything in the spec
about this.

My immediate thought would be to size the CSS viewport to the initial viewport
(@viewport rule in the fullscreen.css for Android) and restrict zooming in the
viewport rule as well, if that's what we want.

[aelias_OOO_until_Jul13, 2014-12-16 19:34:33]

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleAdjuster.cpp:187:
style->setHeight(Length(viewportSize.height(), Fixed));
On 2014/12/16 19:10:24, rune wrote:
> This looks very weird to me. If I understand correctly, this means that you'll
> get a reflow on every pinch zoom change and effectively you'll get a zoom
> behavior for fullscreen on Android that behaves like a desktop type of zoom
> where you scale the CSS pixel size.
> 
> What's the reasoning behind this behavior? I couldn't see anything in the spec
> about this.

We don't support pinch zoom in fullscreen; page scale factor is always locked to
1 when fullscreen starts.  The reason is that for embedded videos on desktop
pages on Android, the outer viewport is sized to the content width of the page
(so that fixed-position elements are sized properly as they don't expect a small
pinched viewport).  We want the fullscreened video to be sized to the true
physical size of the screen instead.  But there wasn't any way to express this
in CSS.

[rune, 2014-12-16 19:54:25]

On 2014/12/16 at 19:34:33, aelias wrote:
>
https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
> Source/core/css/resolver/StyleAdjuster.cpp:187:
style->setHeight(Length(viewportSize.height(), Fixed));
> On 2014/12/16 19:10:24, rune wrote:
> > This looks very weird to me. If I understand correctly, this means that
you'll
> > get a reflow on every pinch zoom change and effectively you'll get a zoom
> > behavior for fullscreen on Android that behaves like a desktop type of zoom
> > where you scale the CSS pixel size.
> > 
> > What's the reasoning behind this behavior? I couldn't see anything in the
spec
> > about this.
> 
> We don't support pinch zoom in fullscreen; page scale factor is always locked
to 1 when fullscreen starts.  The reason is that for embedded videos on desktop
pages on Android, the outer viewport is sized to the content width of the page
(so that fixed-position elements are sized properly as they don't expect a small
pinched viewport).  We want the fullscreened video to be sized to the true
physical size of the screen instead.  But there wasn't any way to express this
in CSS.

Isn't that the same as "@viewport { width: auto; height: auto; min-zoom: 1;
max-zoom: 1; }" ?

[aelias_OOO_until_Jul13, 2014-12-16 20:07:31]

On 2014/12/16 at 19:54:25, rune wrote:
> > We don't support pinch zoom in fullscreen; page scale factor is always
locked to 1 when fullscreen starts.  The reason is that for embedded videos on
desktop pages on Android, the outer viewport is sized to the content width of
the page (so that fixed-position elements are sized properly as they don't
expect a small pinched viewport).  We want the fullscreened video to be sized to
the true physical size of the screen instead.  But there wasn't any way to
express this in CSS.
> 
> Isn't that the same as "@viewport { width: auto; height: auto; min-zoom: 1;
max-zoom: 1; }" ?

That might work.  I was thinking more in terms of finding a CSS property to size
just the width and the height of the fullscreen element without touching
anything else.  My only concern with that @viewport tag is that I think that
would would relayout the entire page temporarily?  That seems undesirable for
transition performance and for semitransparent backdrops.  (I wouldn't
necessarily block landing an @viewport-based approach because of those concerns,
particularly as semitransparent backdrops are already very buggy, but I thought
I'd mention it -- in the long run it is something we ought to fix.)

[rune, 2014-12-16 20:22:19]

On 2014/12/16 at 20:07:31, aelias wrote:
> On 2014/12/16 at 19:54:25, rune wrote:
> > > We don't support pinch zoom in fullscreen; page scale factor is always
locked to 1 when fullscreen starts.  The reason is that for embedded videos on
desktop pages on Android, the outer viewport is sized to the content width of
the page (so that fixed-position elements are sized properly as they don't
expect a small pinched viewport).  We want the fullscreened video to be sized to
the true physical size of the screen instead.  But there wasn't any way to
express this in CSS.
> > 
> > Isn't that the same as "@viewport { width: auto; height: auto; min-zoom: 1;
max-zoom: 1; }" ?
> 
> That might work.  I was thinking more in terms of finding a CSS property to
size just the width and the height of the fullscreen element without touching
anything else.  My only concern with that @viewport tag is that I think that
would would relayout the entire page temporarily?  That seems undesirable for
transition performance and for semitransparent backdrops.  (I wouldn't
necessarily block landing an @viewport-based approach because of those concerns,
particularly as semitransparent backdrops are already very buggy, but I thought
I'd mention it -- in the long run it is something we ought to fix.)

Yes, it would re-layout the page. The current code recalculates style for all
elements, which is even more expensive, but given this info it looks like we
should just recalculate the style for the fullscreen element since that's the
only element affected by the style adjustment.

[rune, 2014-12-16 20:25:52]

On 2014/12/16 at 17:00:59, jchaffraix wrote:
> On 2014/12/16 at 14:22:14, rune wrote:

> > But in Fullscreen (and Element) setNeedsStyleRecalc is a direct consequence
of changing Element state affecting :-webkit-full-screen and
:-webkit-full-screen-ancestor selectors. In what way does updateSize() affect
selector evaluation?
> 
> We would only need layout if it wasn't for some virtual viewport magic. Due to
how the outer/inner viewport work, we have to force the size of the fullscreen
element to the inner viewport, which is done in StyleAdjuster.cpp. I thought it
was clearer than the old code but would welcome a better design.

Given the discussion with aelias, I think we can use the StyleAdjuster trick for
now, but only a LocalStyleChange on the fullscreen element should be necessary.

[rune, 2014-12-16 20:33:38]

On 2014/12/16 at 20:25:52, rune wrote:
> On 2014/12/16 at 17:00:59, jchaffraix wrote:
> > On 2014/12/16 at 14:22:14, rune wrote:
> 
> > > But in Fullscreen (and Element) setNeedsStyleRecalc is a direct
consequence of changing Element state affecting :-webkit-full-screen and
:-webkit-full-screen-ancestor selectors. In what way does updateSize() affect
selector evaluation?
> > 
> > We would only need layout if it wasn't for some virtual viewport magic. Due
to how the outer/inner viewport work, we have to force the size of the
fullscreen element to the inner viewport, which is done in StyleAdjuster.cpp. I
thought it was clearer than the old code but would welcome a better design.
> 
> Given the discussion with aelias, I think we can use the StyleAdjuster trick
for now, but only a LocalStyleChange on the fullscreen element should be
necessary.

Also, in general, I think we should wrap setNeedsStyleRecalc in methods with
names that explains why it's done, or add a comment.

[philipj_slow, 2015-02-12 08:19:17]

Sorry for being late to the party, I'm actually very excited about this change!

Source/core/css/fullscreen.css, Source/core/dom/* and Source/web/* LGTM, but
others probably have a better grasp of the juicy bits in Source/core/rendering.

If there's anything I could do to help move this along, let me know.

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/fullscre...
File Source/core/css/fullscreen.css (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/fullscre...
Source/core/css/fullscreen.css:31: /* Anything below are extensions over what
the Fullscreen API (14 October 2014) mandates. */
Awesome to split this out, thanks!

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
Source/core/dom/Fullscreen.cpp:424: // FIXME: Why can we get here without
calling pushFullscreenElementStack?
Not sure, but possibly it's when Chromium toggles fullscreen. Unfortunately,
those requests don't originate at requestFullscreen().

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
File Source/core/dom/Fullscreen.h (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
Source/core/dom/Fullscreen.h:88: const
WillBeHeapVector<std::pair<RefPtrWillBeMember<Element>, RequestType> >&
fullScreenElementStack() { return m_fullScreenElementStack; }
Can you comment that this is exposed for tests only?

[esprehn, 2015-02-20 06:23:45]

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:736: if (pseudoId != BACKDROP &&
!parentRenderer->canHaveGeneratedChildren())
The parentRenderer for BACKDROP should be the RenderView. You should make that
return true from canHaveGeneratedChildren instead of giving it a special case.

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
File Source/core/dom/Fullscreen.h (right):

https://codereview.chromium.org/788073004/diff/20001/Source/core/dom/Fullscre...
Source/core/dom/Fullscreen.h:88: const
WillBeHeapVector<std::pair<RefPtrWillBeMember<Element>, RequestType> >&
fullScreenElementStack() { return m_fullScreenElementStack; }
On 2015/02/12 at 08:19:16, philipj_UTC7 wrote:
> Can you comment that this is exposed for tests only?

Just name it fullScreenElementStackForTesting()

https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
Doing a full document recalc every time we go in and out is very expensive. We
can do better than that...

[rune, 2015-02-23 10:23:31]

https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
On 2015/02/20 06:23:45, esprehn wrote:
> Doing a full document recalc every time we go in and out is very expensive. We
> can do better than that...

I prepared [1], but decided to await this CL since they'd collide.

[1] https://codereview.chromium.org/803133002/

[esprehn, 2015-02-23 16:41:19]

On 2015/02/23 at 10:23:31, rune wrote:
>
https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
> File Source/web/FullscreenController.cpp (right):
> 
>
https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
> Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
> On 2015/02/20 06:23:45, esprehn wrote:
> > Doing a full document recalc every time we go in and out is very expensive.
We
> > can do better than that...
> 
> I prepared [1], but decided to await this CL since they'd collide.
> 
> [1] https://codereview.chromium.org/803133002/

Ah okay, lgtm with my other nits fixed then. As long as we get rid of this full
document recalc soon. :)

[esprehn, 2015-02-23 16:41:24]

On 2015/02/23 at 10:23:31, rune wrote:
>
https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
> File Source/web/FullscreenController.cpp (right):
> 
>
https://codereview.chromium.org/788073004/diff/20001/Source/web/FullscreenCon...
> Source/web/FullscreenController.cpp:174:
document->setNeedsStyleRecalc(SubtreeStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::FullScreen));
> On 2015/02/20 06:23:45, esprehn wrote:
> > Doing a full document recalc every time we go in and out is very expensive.
We
> > can do better than that...
> 
> I prepared [1], but decided to await this CL since they'd collide.
> 
> [1] https://codereview.chromium.org/803133002/

Ah okay, lgtm with my other nits fixed then. As long as we get rid of this full
document recalc soon. :)

[dsinclair, 2015-05-07 20:47:31]

On 2014/12/15 at 21:32:03, jchaffraix wrote:
> > I will need to test but I thought that
https://codereview.chromium.org/23618022 would have taken of the restarting (I
thought I had a test case for plugins but the site I was using to validate has a
<video>).
> 
> Testing on dailymotion, re-attaching the plugin restarts the video so the bug
isn't fixed.
> 


What's the magic to repeat this test? I applied this patch to my local tree, but
when I go to dailymotion if I watch a video and fullscreen/unfullscreen
everything seems to be working correctly to me. Once I get a failing case I can
look into making the plugins not get re-created on reattach.

[falken, 2015-05-08 05:48:47]

On 2015/05/07 20:47:31, dsinclair wrote:
> On 2014/12/15 at 21:32:03, jchaffraix wrote:
> > > I will need to test but I thought that
> https://codereview.chromium.org/23618022 would have taken of the restarting (I
> thought I had a test case for plugins but the site I was using to validate has
a
> <video>).
> > 
> > Testing on dailymotion, re-attaching the plugin restarts the video so the
bug
> isn't fixed.
> > 
> 
> 
> What's the magic to repeat this test? I applied this patch to my local tree,
but
> when I go to dailymotion if I watch a video and fullscreen/unfullscreen
> everything seems to be working correctly to me. Once I get a failing case I
can
> look into making the plugins not get re-created on reattach.

I wonder if something changed to fix it, can you check with a 4 month old
checkout?

I would test by going to dailymotion and verifying the video is an <object> or
<embed>. Then seeing if clicking the fullscreen button during a playing video
causes it to restart. It may be the same thing, but also it's good to try
selecting the element or an ancestor <div> in the debugger, and then adding:

var x = $0;
document.addEventListener('keypress', function() { x.webkitRequestFullscreen();
// also webkitRequestFullScreen(); });

to the console, and then pressing a key to trigger fullscreen.

[dsinclair, 2015-05-08 18:51:24]

On 2015/05/08 at 05:48:47, falken wrote:
> On 2015/05/07 20:47:31, dsinclair wrote:
> > On 2014/12/15 at 21:32:03, jchaffraix wrote:
> > > > I will need to test but I thought that
> > https://codereview.chromium.org/23618022 would have taken of the restarting
(I
> > thought I had a test case for plugins but the site I was using to validate
has a
> > <video>).
> > > 
> > > Testing on dailymotion, re-attaching the plugin restarts the video so the
bug
> > isn't fixed.
> > > 
> > 
> > 
> > What's the magic to repeat this test? I applied this patch to my local tree,
but
> > when I go to dailymotion if I watch a video and fullscreen/unfullscreen
> > everything seems to be working correctly to me. Once I get a failing case I
can
> > look into making the plugins not get re-created on reattach.
> 
> I wonder if something changed to fix it, can you check with a 4 month old
checkout?
> 
> I would test by going to dailymotion and verifying the video is an <object> or
<embed>. Then seeing if clicking the fullscreen button during a playing video
causes it to restart. It may be the same thing, but also it's good to try
selecting the element or an ancestor <div> in the debugger, and then adding:
> 
> var x = $0;
> document.addEventListener('keypress', function() {
x.webkitRequestFullscreen(); // also webkitRequestFullScreen(); });
> 
> to the console, and then pressing a key to trigger fullscreen.


It's an <object>, playing flash. Could it be because I have pepper flash?

[falken, 2015-05-11 03:25:01]

On 2015/05/08 18:51:24, (OOO until May 19th) dsinclair wrote:
> On 2015/05/08 at 05:48:47, falken wrote:
> > On 2015/05/07 20:47:31, dsinclair wrote:
> > > On 2014/12/15 at 21:32:03, jchaffraix wrote:
> > > > > I will need to test but I thought that
> > > https://codereview.chromium.org/23618022 would have taken of the
restarting
> (I
> > > thought I had a test case for plugins but the site I was using to validate
> has a
> > > <video>).
> > > > 
> > > > Testing on dailymotion, re-attaching the plugin restarts the video so
the
> bug
> > > isn't fixed.
> > > > 
> > > 
> > > 
> > > What's the magic to repeat this test? I applied this patch to my local
tree,
> but
> > > when I go to dailymotion if I watch a video and fullscreen/unfullscreen
> > > everything seems to be working correctly to me. Once I get a failing case
I
> can
> > > look into making the plugins not get re-created on reattach.
> > 
> > I wonder if something changed to fix it, can you check with a 4 month old
> checkout?
> > 
> > I would test by going to dailymotion and verifying the video is an <object>
or
> <embed>. Then seeing if clicking the fullscreen button during a playing video
> causes it to restart. It may be the same thing, but also it's good to try
> selecting the element or an ancestor <div> in the debugger, and then adding:
> > 
> > var x = $0;
> > document.addEventListener('keypress', function() {
> x.webkitRequestFullscreen(); // also webkitRequestFullScreen(); });
> > 
> > to the console, and then pressing a key to trigger fullscreen.
> 
> 
> It's an <object>, playing flash. Could it be because I have pepper flash?

I actually don't know, but since the problem is the plugin gets destroyed and
recreated, I'd expect pepper flash to suffer the same fate as non-pepper flash.
Might be worth testing with/without pepper flash to see though.

[dsinclair, 2015-08-05 18:16:20]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org

[Julien - ping for review, 2015-10-01 15:39:00]

This is super seeded by Dan's patch in
https://codereview.chromium.org/1363023005