Rename variables in ContainerNode::checkForSiblingStyleChanges() for clarity

Source/core/dom/ContainerNode.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2013 Apple Inc. All rights reserved.
  *
  * 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 1133 matching lines @@
 
         if (childRulesChanged && hasDirectAdjacentRules)
             forceCheckOfNextElementCount = document.styleEngine()->maxDirectAdjacentSelectors();
         else if (forceCheckOfNextElementCount)
             --forceCheckOfNextElementCount;
 
         forceCheckOfAnyElementSibling = forceCheckOfAnyElementSibling || (childRulesChanged && hasIndirectAdjacentRules);
     }
 }
 
-void ContainerNode::checkForSiblingStyleChanges(SiblingCheckType changeType, Node* beforeChange, Node* afterChange)
+void ContainerNode::checkForSiblingStyleChanges(SiblingCheckType changeType, Node* nodeBeforeChange, Node* nodeAfterChange)

[Inactive, 2014/07/10 01:05:11]

Possible improvement idea: it seems like we are calling this method for any kind
of child change (Element insertion / removal, non-Element insertion / removal,
Text change). Based on the types of invalidations, it seems to me that we only
need to cause invalidation in case of _Element_ insertion removal?

For e.g., if childrenAffectedByForwardPositionalRules() is true and this is a
Text child insertion, I believe we are unnecessarily calling
setNeedsStyleRecalc(SubtreeStyleChange) on the parent / container node.

 {
     if (!inActiveDocument() || document().hasPendingForcedStyleRecalc() || styleChangeType() >= SubtreeStyleChange)
         return;
 
     if (needsStyleRecalc() && childrenAffectedByPositionalRules())
         return;
 
     // Forward positional selectors include nth-child, nth-of-type, first-of-type and only-of-type.
     // The indirect adjacent selector is the ~ selector.
     // Backward positional selectors include nth-last-child, nth-last-of-type, last-of-type and only-of-type.
     // We have to invalidate everything following the insertion point in the forward and indirect adjacent case,
     // and everything before the insertion point in the backward case.
     // |afterChange| is 0 in the parser callback case, so we won't do any work for the forward case if we don't have to.
     // For performance reasons we just mark the parent node as changed, since we don't want to make childrenChanged O(n^2) by crawling all our kids
     // here. recalcStyle will then force a walk of the children when it sees that this has happened.
-    if (((childrenAffectedByForwardPositionalRules() || childrenAffectedByIndirectAdjacentRules()) && afterChange)
-        || (childrenAffectedByBackwardPositionalRules() && beforeChange)) {
+    if (((childrenAffectedByForwardPositionalRules() || childrenAffectedByIndirectAdjacentRules()) && nodeAfterChange)

[Inactive, 2014/07/10 01:05:11]

Possible improvement idea: it seems like we may be causing too much invalidation
here if there are only non-Element nodes after the change.

Ideally, this would be an "&& elementAfterChange" check.

+        || (childrenAffectedByBackwardPositionalRules() && nodeBeforeChange)) {

[Inactive, 2014/07/10 01:05:11]

Possible improvement idea: it seems like we may be causing too much invalidation
here if there are only non-Element nodes before the change.

Ideally, this would be an "&& elementBeforeChange" check.

         setNeedsStyleRecalc(SubtreeStyleChange);
         return;
     }
 
     // :first-child. In the parser callback case, we don't have to check anything, since we were right the first time.
     // In the DOM case, we only need to do something if |afterChange| is not 0.
     // |afterChange| is 0 in the parser case, so it works out that we'll skip this block.
-    if (childrenAffectedByFirstChildRules() && afterChange) {
-        // Find our new first child.
-        Element* newFirstChild = ElementTraversal::firstWithin(*this);
-        RenderStyle* newFirstChildStyle = newFirstChild ? newFirstChild->renderStyle() : 0;
+    if (childrenAffectedByFirstChildRules() && nodeAfterChange) {
+        ASSERT(changeType != FinishedParsingChildren);
+        // Find our new first child element.
+        Element* firstChildElement = ElementTraversal::firstChild(*this);
+        RenderStyle* firstChildElementStyle = firstChildElement ? firstChildElement->renderStyle() : 0;
 
-        // Find the first element node following |afterChange|
-        Node* firstElementAfterInsertion = afterChange->isElementNode() ? afterChange : ElementTraversal::nextSibling(*afterChange);
-        RenderStyle* firstElementAfterInsertionStyle = firstElementAfterInsertion ? firstElementAfterInsertion->renderStyle() : 0;
+        // Find the first element after the change.
+        Element* elementAfterChange = nodeAfterChange->isElementNode() ? toElement(nodeAfterChange) : ElementTraversal::nextSibling(*nodeAfterChange);
+        RenderStyle* elementAfterChangeStyle = elementAfterChange ? elementAfterChange->renderStyle() : 0;
 
-        // This is the insert/append case.
-        if (newFirstChild != firstElementAfterInsertion && firstElementAfterInsertionStyle && firstElementAfterInsertionStyle->firstChildState())
-            firstElementAfterInsertion->setNeedsStyleRecalc(SubtreeStyleChange);
+        // This is the element insertion as first child element case.
+        if (firstChildElement != elementAfterChange && elementAfterChangeStyle && elementAfterChangeStyle->firstChildState())
+            elementAfterChange->setNeedsStyleRecalc(SubtreeStyleChange);
 
-        // We also have to handle node removal.
-        if (changeType == SiblingRemoved && newFirstChild == firstElementAfterInsertion && newFirstChild && (!newFirstChildStyle || !newFirstChildStyle->firstChildState()))
-            newFirstChild->setNeedsStyleRecalc(SubtreeStyleChange);
+        // This is the first child element removal case.
+        if (changeType == SiblingRemoved && firstChildElement == elementAfterChange && firstChildElement && (!firstChildElementStyle || !firstChildElementStyle->firstChildState()))

[Inactive, 2014/07/10 01:05:11]

Possible improvement idea: Seems like this could be an "else if".

+            firstChildElement->setNeedsStyleRecalc(SubtreeStyleChange);
     }
 
     // :last-child. In the parser callback case, we don't have to check anything, since we were right the first time.
     // In the DOM case, we only need to do something if |afterChange| is not 0.
-    if (childrenAffectedByLastChildRules() && beforeChange) {
-        // Find our new last child.
-        Node* newLastChild = ElementTraversal::lastChild(*this);
-        RenderStyle* newLastChildStyle = newLastChild ? newLastChild->renderStyle() : 0;
+    if (childrenAffectedByLastChildRules() && nodeBeforeChange) {
+        // Find our new last child element.
+        Element* lastChildElement = ElementTraversal::lastChild(*this);
+        RenderStyle* lastChildElementStyle = lastChildElement ? lastChildElement->renderStyle() : 0;
 
-        // Find the last element node going backwards from |beforeChange|
-        Node* lastElementBeforeInsertion = beforeChange->isElementNode() ? beforeChange : ElementTraversal::previousSibling(*beforeChange);
-        RenderStyle* lastElementBeforeInsertionStyle = lastElementBeforeInsertion ? lastElementBeforeInsertion->renderStyle() : 0;
+        // Find the last element before the change.
+        Element* elementBeforeChange = nodeBeforeChange->isElementNode() ? toElement(nodeBeforeChange) : ElementTraversal::previousSibling(*nodeBeforeChange);
+        RenderStyle* elementBeforeChangeStyle = elementBeforeChange ? elementBeforeChange->renderStyle() : 0;
 
-        if (newLastChild != lastElementBeforeInsertion && lastElementBeforeInsertionStyle && lastElementBeforeInsertionStyle->lastChildState())
-            lastElementBeforeInsertion->setNeedsStyleRecalc(SubtreeStyleChange);
+        // This is the element insertion as last child element case.
+        if (lastChildElement != elementBeforeChange && elementBeforeChangeStyle && elementBeforeChangeStyle->lastChildState())
+            elementBeforeChange->setNeedsStyleRecalc(SubtreeStyleChange);
 
-        // We also have to handle node removal. The parser callback case is similar to node removal as well in that we need to change the last child
+        // This is the last child element removal case. The parser callback case is similar to node removal as well in that we need to change the last child
         // to match now.
-        if ((changeType == SiblingRemoved || changeType == FinishedParsingChildren) && newLastChild == lastElementBeforeInsertion && newLastChild && (!newLastChildStyle || !newLastChildStyle->lastChildState()))
-            newLastChild->setNeedsStyleRecalc(SubtreeStyleChange);
+        if ((changeType == SiblingRemoved || changeType == FinishedParsingChildren) && lastChildElement == elementBeforeChange && lastChildElement && (!lastChildElementStyle || !lastChildElementStyle->lastChildState()))

[Inactive, 2014/07/10 01:05:11]

Possible improvement idea: Seems like this could be an "else if".

+            lastChildElement->setNeedsStyleRecalc(SubtreeStyleChange);
     }
 
-    // The + selector. We need to invalidate the first element following the insertion point. It is the only possible element
+    // The + selector. We need to invalidate the first element following the change. It is the only possible element
     // that could be affected by this DOM change.
-    if (childrenAffectedByDirectAdjacentRules() && afterChange) {
-        if (Node* firstElementAfterInsertion = afterChange->isElementNode() ? afterChange : ElementTraversal::nextSibling(*afterChange))
-            firstElementAfterInsertion->setNeedsStyleRecalc(SubtreeStyleChange);
+    if (childrenAffectedByDirectAdjacentRules() && nodeAfterChange) {
+        if (Element* elementAfterChange = nodeAfterChange->isElementNode() ? toElement(nodeAfterChange) : ElementTraversal::nextSibling(*nodeAfterChange))
+            elementAfterChange->setNeedsStyleRecalc(SubtreeStyleChange);
     }
 }
 
 PassRefPtrWillBeRawPtr<TagCollection> ContainerNode::getElementsByTagName(const AtomicString& localName)
 {
     if (localName.isNull())
         return nullptr;
 
     if (document().isHTMLDocument())
         return ensureRareData().ensureNodeLists().addCache<HTMLTagCollection>(*this, HTMLTagCollectionType, localName);
@@ skipping 62 matching lines @@
         return true;
 
     if (node->isElementNode() && toElement(node)->shadow())
         return true;
 
     return false;
 }
 #endif
 
 } // namespace WebCore