Ensure WebView plugins run their lifecycle even absent main frame Layout

Ensure WebView plugins run their lifecycle even absent main frame Layout

The PluginView method layoutIfNeeded causes WebView plugins to update
their entire lifecycle, but we only call that if their layout is
dirtied. In the presence of some style changes, they can need
invalidation or compositing updates without layout, and our recursive
lifecycle update missed WebView plugins as it only recurses through
the Frame tree.

This change always updates child plugins when
FrameView::updateStyleAndLayoutIfNeededRecursive is called.

BUG=545039
Committed: https://crrev.com/eec0a93f429b9797678ca67f8359ea65a6496ca3
Cr-Commit-Position: refs/heads/master@{#376114}

[leviw_travelin_and_unemployed, 2016-02-17 23:28:28]

leviw@chromium.org changed reviewers:
+ chrishtr@chromium.org, eae@chromium.org

[eae, 2016-02-17 23:48:04]

LGTM w/comment

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:2629: for (const
RefPtrWillBeMember<Widget>& child : *viewChildren) {
Why is this only needed for WebView plugins and not other widgets? A comment to
that effect would be helpful.

[chrishtr, 2016-02-17 23:49:18]

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:10:
document.getElementsByTagName("style")[0].disabled = true;
Change to appending a style sheet?

Also add a comment that this just needs to force full-document style recalc. And
add comments
about what the purpose of each of these weird lines is.

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:2627: // that owns them
needed layout.
Add a TODO to fix this in a better way.

https://codereview.chromium.org/1708923002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:2631:
toPluginView(child.get())->layoutIfNeeded();
Add a note that this actually runs the entire lifecycle on the plugin.

[leviw_travelin_and_unemployed, 2016-02-17 23:59:15]

PTAL.

I added comments and was able to take a little bit of crazy out of the test case

[chrishtr, 2016-02-18 00:01:45]

lgtm

[eae, 2016-02-18 00:01:58]

Thanks, LGTM

[eae, 2016-02-18 00:02:05]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-02-18 00:04:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708923002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708923002/20001

[esprehn, 2016-02-18 00:19:38]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-02-18 00:19:39]

https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):

https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:11:
document.appendChild(document.createElement("style"));
does this actually trigger a full recalc? I thought we have an empty style
optimization?

[commit-bot: I haz the power, 2016-02-18 01:06:40]

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

[commit-bot: I haz the power, 2016-02-18 01:06:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leviw_travelin_and_unemployed, 2016-02-18 03:16:22]

On 2016/02/18 at 00:19:39, esprehn wrote:
>
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):
> 
>
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:11:
document.appendChild(document.createElement("style"));
> does this actually trigger a full recalc? I thought we have an empty style
optimization?

I reverted to the old code that disabled the stylesheet. I actually tested on an
older build of Chrome and found the newer test flaky while this one appeared to
work 100% of the time.

Thoughts or can I move forward?

[esprehn, 2016-02-18 03:27:09]

On 2016/02/18 at 03:16:22, leviw wrote:
> On 2016/02/18 at 00:19:39, esprehn wrote:
> >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> > File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):
> > 
> >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> >
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:11:
document.appendChild(document.createElement("style"));
> > does this actually trigger a full recalc? I thought we have an empty style
optimization?
> 
> I reverted to the old code that disabled the stylesheet. I actually tested on
an older build of Chrome and found the newer test flaky while this one appeared
to work 100% of the time.
> 
> Thoughts or can I move forward?

I guess I don't understand what you're trying to trigger. inserting an empty
sheet should be a no-op, I see that the optimization for that appears to be
gone... sigh.

If you just want to trigger a style recalc appending a div is enough, or doing
document.body.style.color = "red" or something.

I worry this test will break when we add that optimization back.

[esprehn, 2016-02-18 03:27:43]

https://codereview.chromium.org/1708923002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):

https://codereview.chromium.org/1708923002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:3:
html:first-letter { color: papayawhip; }
why does the first letter matter? what does it cause?

[leviw_travelin_and_unemployed, 2016-02-18 03:31:15]

On 2016/02/18 at 03:27:09, esprehn wrote:
> On 2016/02/18 at 03:16:22, leviw wrote:
> > On 2016/02/18 at 00:19:39, esprehn wrote:
> > >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> > > File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):
> > > 
> > >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> > >
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:11:
document.appendChild(document.createElement("style"));
> > > does this actually trigger a full recalc? I thought we have an empty style
optimization?
> > 
> > I reverted to the old code that disabled the stylesheet. I actually tested
on an older build of Chrome and found the newer test flaky while this one
appeared to work 100% of the time.
> > 
> > Thoughts or can I move forward?
> 
> I guess I don't understand what you're trying to trigger. inserting an empty
sheet should be a no-op, I see that the optimization for that appears to be
gone... sigh.
> 
> If you just want to trigger a style recalc appending a div is enough, or doing
document.body.style.color = "red" or something.
> 
> I worry this test will break when we add that optimization back.


Honestly, it's just a reduced version of the cluster-fuzz test. Why
first-letter? The issue doesn't repro without it. I tried setting an inline
style of the body and the embed, neither tickled the issue.

[esprehn, 2016-02-18 03:40:00]

On 2016/02/18 at 03:31:15, leviw wrote:
> On 2016/02/18 at 03:27:09, esprehn wrote:
> > On 2016/02/18 at 03:16:22, leviw wrote:
> > > On 2016/02/18 at 00:19:39, esprehn wrote:
> > > >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> > > > File
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html
(right):
> > > > 
> > > >
https://codereview.chromium.org/1708923002/diff/20001/third_party/WebKit/Layo...
> > > >
third_party/WebKit/LayoutTests/fast/plugins/webview-plugin-updates-without-layout.html:11:
document.appendChild(document.createElement("style"));
> > > > does this actually trigger a full recalc? I thought we have an empty
style optimization?
> > > 
> > > I reverted to the old code that disabled the stylesheet. I actually tested
on an older build of Chrome and found the newer test flaky while this one
appeared to work 100% of the time.
> > > 
> > > Thoughts or can I move forward?
> > 
> > I guess I don't understand what you're trying to trigger. inserting an empty
sheet should be a no-op, I see that the optimization for that appears to be
gone... sigh.
> > 
> > If you just want to trigger a style recalc appending a div is enough, or
doing document.body.style.color = "red" or something.
> > 
> > I worry this test will break when we add that optimization back.
> 
> 
> Honestly, it's just a reduced version of the cluster-fuzz test. Why
first-letter? The issue doesn't repro without it. I tried setting an inline
style of the body and the embed, neither tickled the issue.

Hmm I think all you're doing is removing the generated PseudoElement when you
disable the sheet. If you put a span in the page and call remove () that's
probably similar.

Anyway lgtm

[leviw_travelin_and_unemployed, 2016-02-18 05:15:11]

> Hmm I think all you're doing is removing the generated PseudoElement when you
disable the sheet. If you put a span in the page and call remove () that's
probably similar.

I attempted this and it also didn't trigger the issue. I've found a slew of
things to improve while working on this issue, and following-up on this will be
added to the list :p

In the meantime, I'm stoked to kill the top crasher :) Thanks for your review!

[leviw_travelin_and_unemployed, 2016-02-18 05:15:13]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2016-02-18 05:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1708923002/#ps40001
(title: "Revert to disabling stylesheet")

[commit-bot: I haz the power, 2016-02-18 05:15:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708923002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708923002/40001

[commit-bot: I haz the power, 2016-02-18 07:34:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-18 07:35:50]

Description was changed from

==========
Ensure WebView plugins run their lifecycle even absent main frame Layout

The PluginView method layoutIfNeeded causes WebView plugins to update
their entire lifecycle, but we only call that if their layout is
dirtied. In the presence of some style changes, they can need
invalidation or compositing updates without layout, and our recursive
lifecycle update missed WebView plugins as it only recurses through
the Frame tree.

This change always updates child plugins when
FrameView::updateStyleAndLayoutIfNeededRecursive is called.

BUG=545039
==========

to

==========
Ensure WebView plugins run their lifecycle even absent main frame Layout

The PluginView method layoutIfNeeded causes WebView plugins to update
their entire lifecycle, but we only call that if their layout is
dirtied. In the presence of some style changes, they can need
invalidation or compositing updates without layout, and our recursive
lifecycle update missed WebView plugins as it only recurses through
the Frame tree.

This change always updates child plugins when
FrameView::updateStyleAndLayoutIfNeededRecursive is called.

BUG=545039

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

[commit-bot: I haz the power, 2016-02-18 07:35:51]

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