Do not use SubtreeStyleChange for reattachment of plugin renderers.

Do not use SubtreeStyleChange for reattachment of plugin renderers.

The essence of this CL is to use LocalStyleChange instead of
SubtreeStyleChange to trigger style recalc of the embedded content renderer.
This is done to avoid sibling subtree recalculations when the determined
embedded content type changes.

The setNeedsStyleRecalc calls in question are related to a hack to trigger
re-attachment of embedded content renderers. To make that clear the
setNeedsStyleRecalc has been wrapped in a more descriptive method.

R=esprehn@chromium.org
BUG=408957

[rune, 2014-09-16 18:53:18]

PTAL

[esprehn, 2014-09-19 04:15:08]

Sigh, so many hacks. I guess this is okay, lgtm. Would be nice if we could just
detach instead though.

https://codereview.chromium.org/521603002/diff/1/LayoutTests/fast/dom/HTMLObj...
File LayoutTests/fast/dom/HTMLObjectElement/data-attr-reattach-recalc.html
(right):

https://codereview.chromium.org/521603002/diff/1/LayoutTests/fast/dom/HTMLObj...
LayoutTests/fast/dom/HTMLObjectElement/data-attr-reattach-recalc.html:4: object
+ div { color: green; }
Have we seen this in the wild?

https://codereview.chromium.org/521603002/diff/1/Source/core/html/HTMLPlugInE...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/521603002/diff/1/Source/core/html/HTMLPlugInE...
Source/core/html/HTMLPlugInElement.cpp:264: // reattachWhitespaceSiblings() will
be called after a reattach that is done from
Can we just rebaseline those tests? What did it actually break?

https://codereview.chromium.org/521603002/diff/1/Source/core/html/HTMLPlugInE...
Source/core/html/HTMLPlugInElement.cpp:278: // No styles to recalc, just to
trigger a call to willRecalcStyle() :-(
FIXME:

[rune, 2014-09-19 11:22:00]

https://codereview.chromium.org/521603002/diff/1/Source/core/html/HTMLPlugInE...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/521603002/diff/1/Source/core/html/HTMLPlugInE...
Source/core/html/HTMLPlugInElement.cpp:264: // reattachWhitespaceSiblings() will
be called after a reattach that is done from
On 2014/09/19 04:15:08, esprehn wrote:
> Can we just rebaseline those tests? What did it actually break?

We regressed in an paint invalidation test where there is an inline display
plugin which has a whitespace renderer sibling inside body. Invalidating text
renderers invalidates its parent [1] which in this case is body. So, the
difference is that this willRecalcStyle reattach() hack does not call
reattachWhiteSpaceSiblings while using lazyReattachIfAttached() will. The
missing reattachWhiteSpaceSiblings here is really a bug, but I don't think it's
observable with the current code.

This is really a reattachWhiteSpaceSiblings problem. First, do we really need to
do the actual detach()/attach() in order to figure out if it was necessary? And
secondly, do whitespace renderers really need to do any paint invalidations at
all?

I'm leaning towards dropping this CL and use lazyReattachIfAttached() instead.
That means rebaselining the test in question and accepting the extra
invalidation from the whitespace renderer and not fix the bug this CL is trying
to fix. Reattachment causes sibling tree style invalidation the same way a
SubtreeStyleChange does. The right way to fix the bug is to introduce sibling
invalidation sets and use those instead of propagating SubtreeStyleChange to
sibling trees based on >= SubtreeStyleChange in
ContainerNode::checkForSiblingStyleChanges.

I'll try remove this hack completely in a different CL and see what you think.

[rune, 2014-09-23 11:42:27]

On 2014/09/19 11:22:00, rune wrote:

> I'll try remove this hack completely in a different CL and see what you think.

Here:

https://codereview.chromium.org/596773002/

[rune, 2014-09-29 20:39:51]

On 2014/09/23 11:42:27, rune wrote:
> On 2014/09/19 11:22:00, rune wrote:
> 
> > I'll try remove this hack completely in a different CL and see what you
think.
> 
> Here:
> 
> https://codereview.chromium.org/596773002/

Closed this one as the proper re-attach is about to land.