Changeset 132941 in webkit

Timestamp:

Oct 30, 2012 1:25:52 PM (11 years ago)

Author:

Antti Koivisto

Message:

Avoid unnecessary style recalcs on class attribute mutation
​https://bugs.webkit.org/show_bug.cgi?id=100776

Reviewed by Andreas Kling.

There is no need to invalidate element style on class attribute change if neither the added
or removed classes featured in any active stylesheet.

• css/RuleFeature.cpp:

(WebCore::RuleFeatureSet::add):
(WebCore::RuleFeatureSet::clear):
(WebCore::RuleFeatureSet::reportMemoryUsage):

• css/RuleFeature.h:

(RuleFeatureSet):

• css/RuleSet.cpp:

(WebCore::collectFeaturesFromSelector):

Collect classes mentioned in CSS selectors the same way ids and attribute names are
already collected.

• css/StyleResolver.cpp:

(WebCore::StyleResolver::hasSelectorForClass):

Add a method to test if a given class name is mentioned anywhere in stylehseets.

(WebCore):

• css/StyleResolver.h:
• css/StyleScopeResolver.h:

(WebCore):

• dom/Element.cpp:

(WebCore::collectAddedAndRemovedClasses):
(WebCore):
(WebCore::Element::classAttributeChanged):

Figure out which classes were added and removed. Test if they are present in any style
rule and invalidate the style only if they are.

• dom/SpaceSplitString.cpp:

(WebCore::SpaceSplitStringData::add):
(WebCore::SpaceSplitStringData::remove):
(WebCore::SpaceSplitString::add):
(WebCore::SpaceSplitString::remove):

Added bool return value to indicate if anything was actually removed. Reorganized
the code a bit to avoid unnecessary uniquing when nothing changes.

• dom/SpaceSplitString.h:

(SpaceSplitStringData):
(SpaceSplitString):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/RuleFeature.cpp (modified) (3 diffs)
• css/RuleFeature.h (modified) (3 diffs)
• css/RuleSet.cpp (modified) (1 diff)
• css/StyleResolver.cpp (modified) (1 diff)
• css/StyleResolver.h (modified) (1 diff)
• css/StyleScopeResolver.h (modified) (1 diff)
• dom/Element.cpp (modified) (1 diff)
• dom/SpaceSplitString.cpp (modified) (2 diffs)
• dom/SpaceSplitString.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-10-30  Antti Koivisto  <antti@apple.com>
+
+        Avoid unnecessary style recalcs on class attribute mutation
+        https://bugs.webkit.org/show_bug.cgi?id=100776
+
+        Reviewed by Andreas Kling.
+
+        There is no need to invalidate element style on class attribute change if neither the added
+        or removed classes featured in any active stylesheet.
+
+        * css/RuleFeature.cpp:
+        (WebCore::RuleFeatureSet::add):
+        (WebCore::RuleFeatureSet::clear):
+        (WebCore::RuleFeatureSet::reportMemoryUsage):
+        * css/RuleFeature.h:
+        (RuleFeatureSet):
+        * css/RuleSet.cpp:
+        (WebCore::collectFeaturesFromSelector):
+        
+            Collect classes mentioned in CSS selectors the same way ids and attribute names are
+            already collected.
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::hasSelectorForClass):
+        
+            Add a method to test if a given class name is mentioned anywhere in stylehseets.
+
+        (WebCore):
+        * css/StyleResolver.h:
+        * css/StyleScopeResolver.h:
+        (WebCore):
+        * dom/Element.cpp:
+        (WebCore::collectAddedAndRemovedClasses):
+        (WebCore):
+        (WebCore::Element::classAttributeChanged):
+        
+            Figure out which classes were added and removed. Test if they are present in any style
+            rule and invalidate the style only if they are.
+
+        * dom/SpaceSplitString.cpp:
+        (WebCore::SpaceSplitStringData::add):
+        (WebCore::SpaceSplitStringData::remove):
+        (WebCore::SpaceSplitString::add):
+        (WebCore::SpaceSplitString::remove):
+        
+            Added bool return value to indicate if anything was actually removed. Reorganized 
+            the code a bit to avoid unnecessary uniquing when nothing changes.
+
+        * dom/SpaceSplitString.h:
+        (SpaceSplitStringData):
+        (SpaceSplitString):
+
 2012-10-30  Robert Hogan  <robert@webkit.org>
 

trunk/Source/WebCore/css/RuleFeature.cpp

@@ -39 +39 @@
 void RuleFeatureSet::add(const RuleFeatureSet& other)
 {
-    HashSet<AtomicStringImpl*>::iterator end = other.idsInRules.end();
-    for (HashSet<AtomicStringImpl*>::iterator it = other.idsInRules.begin(); it != end; ++it)
+    HashSet<AtomicStringImpl*>::const_iterator end = other.idsInRules.end();
+    for (HashSet<AtomicStringImpl*>::const_iterator it = other.idsInRules.begin(); it != end; ++it)
         idsInRules.add(*it);
+    end = other.classesInRules.end();
+    for (HashSet<AtomicStringImpl*>::const_iterator it = other.classesInRules.begin(); it != end; ++it)
+        classesInRules.add(*it);
     end = other.attrsInRules.end();
-    for (HashSet<AtomicStringImpl*>::iterator it = other.attrsInRules.begin(); it != end; ++it)
+    for (HashSet<AtomicStringImpl*>::const_iterator it = other.attrsInRules.begin(); it != end; ++it)
         attrsInRules.add(*it);
     siblingRules.append(other.siblingRules);
@@ -54 +57 @@
 {
     idsInRules.clear();
+    classesInRules.clear();
     attrsInRules.clear();
     siblingRules.clear();
@@ -65 +69 @@
     MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::CSS);
     info.addMember(idsInRules);
+    info.addMember(classesInRules);
     info.addMember(attrsInRules);
     info.addMember(siblingRules);

trunk/Source/WebCore/css/RuleFeature.h

@@ -32 +32 @@
 class StyleRule;
 
-class RuleFeature {
-public:
+struct RuleFeature {
     RuleFeature(StyleRule* rule, unsigned selectorIndex, bool hasDocumentSecurityOrigin)
         : rule(rule)
@@ -45 +44 @@
 };
 
-class RuleFeatureSet {
-public:
+struct RuleFeatureSet {
     RuleFeatureSet()
         : usesFirstLineRules(false)
@@ -56 +54 @@
     void reportMemoryUsage(MemoryObjectInfo*) const;
     HashSet<AtomicStringImpl*> idsInRules;
+    HashSet<AtomicStringImpl*> classesInRules;
     HashSet<AtomicStringImpl*> attrsInRules;
     Vector<RuleFeature> siblingRules;

trunk/Source/WebCore/css/RuleSet.cpp

@@ -162 +162 @@
     if (selector->m_match == CSSSelector::Id)
         features.idsInRules.add(selector->value().impl());
-    if (selector->isAttributeSelector())
+    else if (selector->m_match == CSSSelector::Class)
+        features.classesInRules.add(selector->value().impl());
+    else if (selector->isAttributeSelector())
         features.attrsInRules.add(selector->attribute().localName().impl());
     switch (selector->pseudoType()) {

trunk/Source/WebCore/css/StyleResolver.cpp

@@ -4255 +4255 @@
 }
 
+bool StyleResolver::hasSelectorForClass(const AtomicString& classValue) const
+{
+    if (classValue.isEmpty())
+        return false;
+    return m_features.classesInRules.contains(classValue.impl());
+}
+
 bool StyleResolver::hasSelectorForId(const AtomicString& idValue) const
 {

trunk/Source/WebCore/css/StyleResolver.h

@@ -242 +242 @@
 
     bool hasSelectorForId(const AtomicString&) const;
+    bool hasSelectorForClass(const AtomicString&) const;
     bool hasSelectorForAttribute(const AtomicString&) const;
 

trunk/Source/WebCore/css/StyleScopeResolver.h

@@ -40 +40 @@
 class Element;
 class RuleSet;
-class RuleFeatureSet;
 class ShadowRoot;
 class StyleRuleHost;
+struct RuleFeatureSet;
 
 #if ENABLE(STYLE_SCOPED) || ENABLE(SHADOW_DOM)

trunk/Source/WebCore/dom/Element.cpp

@@ -766 +766 @@
 }
 
+static void collectAddedAndRemovedClasses(Vector<AtomicString, 4>& addedAndRemovedClasses, SpaceSplitString& oldClasses, const SpaceSplitString& newClasses)
+{
+    for (unsigned i = 0; i < newClasses.size(); ++i) {
+        if (oldClasses.remove(newClasses[i]))
+            continue;
+        addedAndRemovedClasses.append(newClasses[i]);
+    }
+    for (unsigned i = 0; i < oldClasses.size(); ++i)
+        addedAndRemovedClasses.append(oldClasses[i]);
+}
+
 void Element::classAttributeChanged(const AtomicString& newClassString)
 {
+    bool shouldInvalidateStyle = attached() && document()->styleResolverIfExists();
+    Vector<AtomicString, 4> addedAndRemovedClasses;
+
     if (classStringHasClassName(newClassString)) {
+        const ElementAttributeData* attributeData = ensureAttributeData();
         const bool shouldFoldCase = document()->inQuirksMode();
-        ensureAttributeData()->setClass(newClassString, shouldFoldCase);
-    } else if (attributeData())
+        SpaceSplitString oldClasses = attributeData->classNames();
+        attributeData->setClass(newClassString, shouldFoldCase);
+        if (shouldInvalidateStyle)
+            collectAddedAndRemovedClasses(addedAndRemovedClasses, oldClasses, mutableAttributeData()->classNames());
+    } else if (attributeData()) {
+        if (shouldInvalidateStyle) {
+            const SpaceSplitString& oldClasses = attributeData()->classNames();
+            for (unsigned i = 0; i < oldClasses.size(); ++i)
+                addedAndRemovedClasses.append(oldClasses[i]);
+        }
         mutableAttributeData()->clearClass();
+    }
 
     if (DOMTokenList* classList = optionalClassList())
         static_cast<ClassList*>(classList)->reset(newClassString);
 
-    setNeedsStyleRecalc();
+    if (!shouldInvalidateStyle)
+        return;
+    for (unsigned i = 0; i < addedAndRemovedClasses.size(); ++i) {
+        if (document()->styleResolverIfExists()->hasSelectorForClass(addedAndRemovedClasses[i])) {
+            setNeedsStyleRecalc();
+            break;
+        }
+    }
 }
 

trunk/Source/WebCore/dom/SpaceSplitString.cpp

@@ -108 +108 @@
 {
     ASSERT(hasOneRef());
+    ASSERT(!contains(string));
+    m_vector.append(string);
+}
+
+void SpaceSplitStringData::remove(unsigned index)
+{
+    ASSERT(hasOneRef());
+    m_vector.remove(index);
+}
+
+void SpaceSplitString::add(const AtomicString& string)
+{
+    // FIXME: add() does not allow duplicates but createVector() does.
     if (contains(string))
         return;
-
-    m_vector.append(string);
-}
-
-void SpaceSplitStringData::remove(const AtomicString& string)
-{
-    ASSERT(hasOneRef());
-    size_t position = 0;
-    while (position < m_vector.size()) {
-        if (m_vector[position] == string)
-            m_vector.remove(position);
-        else
-            ++position;
-    }
-}
-
-void SpaceSplitString::add(const AtomicString& string)
-{
     ensureUnique();
     if (m_data)
@@ -133 +128 @@
 }
 
-void SpaceSplitString::remove(const AtomicString& string)
-{
-    ensureUnique();
-    if (m_data)
-        m_data->remove(string);
+bool SpaceSplitString::remove(const AtomicString& string)
+{
+    if (!m_data)
+        return false;
+    unsigned i = 0;
+    bool changed = false;
+    while (i < m_data->size()) {
+        if ((*m_data)[i] == string) {
+            if (!changed)
+                ensureUnique();
+            m_data->remove(i);
+            changed = true;
+            continue;
+        }
+        ++i;
+    }
+    return changed;
 }
 

trunk/Source/WebCore/dom/SpaceSplitString.h

@@ -48 +48 @@
 
         void add(const AtomicString&);
-        void remove(const AtomicString&);
+        void remove(unsigned index);
 
         bool isUnique() const { return m_keyString.isNull(); } 
@@ -77 +77 @@
         bool containsAll(const SpaceSplitString& names) const { return !names.m_data || (m_data && m_data->containsAll(*names.m_data)); }
         void add(const AtomicString&);
-        void remove(const AtomicString&);
+        bool remove(const AtomicString&);
 
         size_t size() const { return m_data ? m_data->size() : 0; }