Do not use SubtreeStyleChange for reattachment of plugin renderers.

Source/core/html/HTMLPlugInElement.cpp

Index: Source/core/html/HTMLPlugInElement.cpp
diff --git a/Source/core/html/HTMLPlugInElement.cpp b/Source/core/html/HTMLPlugInElement.cpp
index b264749247dd2b43f68c10c62e7eb1c1977eb0c5..e7d3c27d81a62bed6b5dd748b7d26ddc71d51e06 100644
--- a/Source/core/html/HTMLPlugInElement.cpp
+++ b/Source/core/html/HTMLPlugInElement.cpp
@@ -247,10 +247,38 @@ RenderObject* HTMLPlugInElement::createRenderer(RenderStyle* style)
 void HTMLPlugInElement::willRecalcStyle(StyleRecalcChange)
 {
     // FIXME: Why is this necessary? Manual re-attach is almost always wrong.
+    //
+    // Attributes like type/classid/data determines the type of the embedded content
+    // rendered. If that type changes, the RenderObject needs to be re-attached.
+    // That is triggered by the triggerReattachThroughStyleRecalcHack() method below
+    // causing a style recalc, which in turn calls this method, where the renderer is
+    // re-attached below.
+    //
+    // There are possibly side-effects of doing that here, and not as part of re-attaching
+    // inside Element::recalcStyle.
+    //
+    // Alternatively, we could call lazyReattachIfAttached() where
+    // triggerReattachThroughStyleRecalcHack() is currently called.
+    //
+    // Doing that currently causes regressions in some tests because
+    // reattachWhitespaceSiblings() will be called after a reattach that is done from

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

Can we just rebaseline those tests? What did it actually break?

[rune, 2014/09/19 11:21:59]

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.

+    // Element::recalcStyle.
+    //
+    // The fact that HTMLObjectElement::updateWidgetInternal (called from a timer) is
+    // responsible for reattaching fallback content, while reattaching embedded content
+    // renderers happens here as part of style recalc also seems sketchy since they are
+    // both responses to determining the content type.
+
     if (!useFallbackContent() && !usePlaceholderContent() && needsWidgetUpdate() && renderer() && !isImageType())
         reattach();
 }
 
+void HTMLPlugInElement::triggerReattachThroughStyleRecalcHack()
+{
+    // No styles to recalc, just to trigger a call to willRecalcStyle() :-(

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

FIXME:

+    setNeedsStyleRecalc(LocalStyleChange);
+}
+
 void HTMLPlugInElement::finishParsingChildren()
 {
     HTMLFrameOwnerElement::finishParsingChildren();
@@ -259,7 +287,7 @@ void HTMLPlugInElement::finishParsingChildren()
 
     setNeedsWidgetUpdate(true);
     if (inDocument())
-        setNeedsStyleRecalc(SubtreeStyleChange);
+        triggerReattachThroughStyleRecalcHack();
 }
 
 void HTMLPlugInElement::resetInstance()