Do not use SubtreeStyleChange for reattachment of plugin renderers.

Source/core/html/HTMLPlugInElement.cpp

 /**
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Stefan Schimanski (1Stein@gmx.de)
  * Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 229 matching lines @@
 
     if (usePlaceholderContent())
         return new RenderBlockFlow(this);
 
     return new RenderEmbeddedObject(this);
 }
 
 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();
     if (useFallbackContent())
         return;
 
     setNeedsWidgetUpdate(true);
     if (inDocument())
-        setNeedsStyleRecalc(SubtreeStyleChange);
+        triggerReattachThroughStyleRecalcHack();
 }
 
 void HTMLPlugInElement::resetInstance()
 {
     m_pluginWrapper.clear();
 }
 
 SharedPersistent<v8::Object>* HTMLPlugInElement::pluginWrapper()
 {
     LocalFrame* frame = document().frame();
@@ skipping 313 matching lines @@
 
 void HTMLPlugInElement::setUsePlaceholderContent(bool use)
 {
     if (use != m_usePlaceholderContent) {
         m_usePlaceholderContent = use;
         lazyReattachIfAttached();
     }
 }
 
 }