Element::recalcStyle() overly reattach()-es InsertionPoints.

Source/core/dom/shadow/ContentDistributor.cpp

Index: Source/core/dom/shadow/ContentDistributor.cpp
diff --git a/Source/core/dom/shadow/ContentDistributor.cpp b/Source/core/dom/shadow/ContentDistributor.cpp
index 95db1751219ac81bd038c27c41c112ce980fd92e..c14c30b14e86e08376e73b30a51975a2ccc93c69 100644
--- a/Source/core/dom/shadow/ContentDistributor.cpp
+++ b/Source/core/dom/shadow/ContentDistributor.cpp
@@ -265,7 +265,7 @@ void ContentDistributor::distribute(Element* host)
     }
 }
 
-bool ContentDistributor::invalidate(Element* host)
+bool ContentDistributor::invalidate(Element* host, Vector<RefPtr<Node> >& nodesNeedingReattach)
 {
     ASSERT(needsInvalidation());
     bool needsReattach = (m_validity == Undetermined) || !m_nodeToInsertionPoint.isEmpty();
@@ -275,7 +275,10 @@ bool ContentDistributor::invalidate(Element* host)
             scope->setInsertionPointAssignedTo(0);
             const Vector<RefPtr<InsertionPoint> >& insertionPoints = scope->ensureInsertionPointList(root);
             for (size_t i = 0; i < insertionPoints.size(); ++i) {
-                needsReattach = needsReattach || true;
+                needsReattach = true;
+                for (Node* child = insertionPoints[i]->firstChild(); child; child = child->nextSibling())
+                    nodesNeedingReattach.append(child);
+
                 insertionPoints[i]->clearDistribution();
 
                 // After insertionPoint's distribution is invalidated, its reprojection should also be invalidated.
@@ -345,9 +348,19 @@ void ContentDistributor::distributeNodeChildrenTo(InsertionPoint* insertionPoint
 void ContentDistributor::ensureDistribution(ShadowRoot* shadowRoot)
 {
     ASSERT(shadowRoot);
+    return ensureDistribution(shadowRoot->host());
+}
+
+void ContentDistributor::ensureDistribution(ElementShadow* shadow)

[esprehn, 2013/06/14 08:32:29]

I'd prefer if this was just a method of Element itself.

element->ensureDistribtion(). Internally it can check for shadow() and such. All
the overloads make this confusing.

[Hajime Morrita, 2013/06/14 09:59:14]

makes sense. will do.

+{
+    ASSERT(shadow);
+    return ensureDistribution(shadow->host());
+}
 
+void ContentDistributor::ensureDistribution(Element* host)
+{
     Vector<ElementShadow*, 8> elementShadows;
-    for (Element* current = shadowRoot->host(); current; current = current->shadowHost()) {
+    for (Element* current = host; current; current = current->shadowHost()) {
         ElementShadow* elementShadow = current->shadow();
         if (!elementShadow->distributor().needsDistribution())
             break;
@@ -362,12 +375,15 @@ void ContentDistributor::ensureDistribution(ShadowRoot* shadowRoot)
 
 void ContentDistributor::invalidateDistribution(Element* host)
 {
+    Vector<RefPtr<Node> > nodesNeedingReattach;

[esprehn, 2013/06/14 08:32:29]

These shouldn't need to be RefPtr. I don't think anything below can run script?
If it can then then Node* isn't safe.

[Hajime Morrita, 2013/06/14 09:59:14]

It shouldn't run any script. WIll turn to raw pointer.

     bool didNeedInvalidation = needsInvalidation();
-    bool needsReattach = didNeedInvalidation ? invalidate(host) : false;
+    bool needsReattach = didNeedInvalidation ? invalidate(host, nodesNeedingReattach) : false;
 
     if (needsReattach && host->attached()) {
         for (Node* n = host->firstChild(); n; n = n->nextSibling())
-            n->lazyReattach();
+            n->lazyReattachIfAttached();
+        for (size_t i = 0; i < nodesNeedingReattach.size(); ++i)
+            nodesNeedingReattach[i]->lazyReattachIfAttached();
         host->setNeedsStyleRecalc();
     }
 
@@ -428,6 +444,14 @@ void ContentDistributor::willAffectSelector(Element* host)
     invalidateDistribution(host);
 }
 
+void ContentDistributor::setNeedsStyleRecalcIfDistributedTo(Element* host, InsertionPoint* insertionPoint)
+{
+    for (NodeInsertionPointMap::iterator i = m_nodeToInsertionPoint.begin(); i != m_nodeToInsertionPoint.end(); ++i) {
+        if (i->value == insertionPoint)
+            const_cast<Node*>(i->key)->setNeedsStyleRecalc(SyntheticStyleChange);

[esprehn, 2013/06/14 08:32:29]

Can you just remove the const from the key in the NodeInsertionPointMap type? It
seems unnecessarily restrictive.

Also lets try InlineStyleChange instead of SyntheticStyleChange. I don't think
you need to trigger RenderLayer updates for identical styles.

+    }
+}
+
 void ContentDistributor::didShadowBoundaryChange(Element* host)
 {
     setValidity(Undetermined);