Changeset 143112 in webkit


Ignore:
Timestamp:
Feb 16, 2013 2:57:02 PM (11 years ago)
Author:
akling@apple.com
Message:

Element: Avoid unrelated attribute synchronization on other attribute access.
<​http://webkit.org/b/110025>

Reviewed by Darin Adler.

We've been extremely trigger happy with re-serializing the style attribute (and SVG animatables)
whenever any Element attribute API was used. This patch narrows this down to (almost always)
only synchronizing an attribute when someone specifically wants to read/update it.

Also removed two more confusing ElementData accessors:

  • Element::elementDataWithSynchronizedAttributes()
  • Element::ensureElementDataWithSynchronizedAttributes()
  • dom/Element.h:
  • dom/Element.cpp:

(WebCore::Element::hasAttributes):
(WebCore::Element::hasEquivalentAttributes):
(WebCore::Element::cloneAttributesFromElement):
(WebCore::Element::synchronizeAllAttributes):

Renamed updateInvalidAttributes() to synchronizeAllAttributes().
This function should only be used when we need every single attribute to be up-to-date.

(WebCore::Element::synchronizeAttribute):

Broke out logic for synchronizing a specific attribute, given either a full QualifiedName
or a localName.

(WebCore::Element::setSynchronizedLazyAttribute):

Don't call ensureUniqueElementData() indisciminately here. This avoids converting the attribute
storage when re-serializing the inline style yields the same CSS text that was already in the
style attribute.

(WebCore::Element::hasAttribute):
(WebCore::Element::hasAttributeNS):
(WebCore::Element::getAttribute):
(WebCore::Element::getAttributeNode):
(WebCore::Element::getAttributeNodeNS):
(WebCore::Element::setAttribute):
(WebCore::Element::setAttributeNode):
(WebCore::Element::removeAttributeNode):

Only synchronize the attribute in question.

  • dom/Node.cpp:

(WebCore::Node::compareDocumentPosition):

Call synchronizeAllAttributes() when comparing two Attr nodes on the same Element instead
of relying on the side-effects of another function doing this.

Location:
trunk/Source/WebCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r143108 r143112  
     12013-02-16  Andreas Kling  <akling@apple.com>
     2
     3        Element: Avoid unrelated attribute synchronization on other attribute access.
     4        <http://webkit.org/b/110025>
     5
     6        Reviewed by Darin Adler.
     7
     8        We've been extremely trigger happy with re-serializing the style attribute (and SVG animatables)
     9        whenever any Element attribute API was used. This patch narrows this down to (almost always)
     10        only synchronizing an attribute when someone specifically wants to read/update it.
     11
     12        Also removed two more confusing ElementData accessors:
     13
     14            - Element::elementDataWithSynchronizedAttributes()
     15            - Element::ensureElementDataWithSynchronizedAttributes()
     16
     17        * dom/Element.h:
     18        * dom/Element.cpp:
     19        (WebCore::Element::hasAttributes):
     20        (WebCore::Element::hasEquivalentAttributes):
     21        (WebCore::Element::cloneAttributesFromElement):
     22        (WebCore::Element::synchronizeAllAttributes):
     23
     24            Renamed updateInvalidAttributes() to synchronizeAllAttributes().
     25            This function should only be used when we need every single attribute to be up-to-date.
     26
     27        (WebCore::Element::synchronizeAttribute):
     28
     29            Broke out logic for synchronizing a specific attribute, given either a full QualifiedName
     30            or a localName.
     31
     32        (WebCore::Element::setSynchronizedLazyAttribute):
     33
     34            Don't call ensureUniqueElementData() indisciminately here. This avoids converting the attribute
     35            storage when re-serializing the inline style yields the same CSS text that was already in the
     36            style attribute.
     37
     38        (WebCore::Element::hasAttribute):
     39        (WebCore::Element::hasAttributeNS):
     40        (WebCore::Element::getAttribute):
     41        (WebCore::Element::getAttributeNode):
     42        (WebCore::Element::getAttributeNodeNS):
     43        (WebCore::Element::setAttribute):
     44        (WebCore::Element::setAttributeNode):
     45        (WebCore::Element::removeAttributeNode):
     46
     47            Only synchronize the attribute in question.
     48
     49        * dom/Node.cpp:
     50        (WebCore::Node::compareDocumentPosition):
     51
     52            Call synchronizeAllAttributes() when comparing two Attr nodes on the same Element instead
     53            of relying on the side-effects of another function doing this.
     54
    1552013-02-16  Seokju Kwon  <seokju.kwon@gmail.com>
    256

  • trunk/Source/WebCore/dom/Element.cpp

    r143089 r143112  
    9595using namespace HTMLNames;
    9696using namespace XMLNames;
     97
     98static inline bool shouldIgnoreAttributeCase(const Element* e)
     99{
     100    return e && e->document()->isHTMLDocument() && e->isHTMLElement();
     101}
    97102   
    98103class StyleResolverParentPusher {
    … …  
    332337}
    333338
    334 const AtomicString& Element::getAttribute(const QualifiedName& name) const
     339void Element::synchronizeAllAttributes() const
    335340{
    336341    if (!elementData())
    337         return nullAtom;
    338 
    339     if (UNLIKELY(name == styleAttr && elementData()->m_styleAttributeIsDirty))
     342        return;
     343    if (elementData()->m_styleAttributeIsDirty)
    340344        updateStyleAttribute();
    341 
     345#if ENABLE(SVG)
     346    if (elementData()->m_animatedSVGAttributesAreDirty)
     347        updateAnimatedSVGAttribute(anyQName());
     348#endif
     349}
     350
     351inline void Element::synchronizeAttribute(const QualifiedName& name) const
     352{
     353    if (!elementData())
     354        return;
     355    if (UNLIKELY(name == styleAttr && elementData()->m_styleAttributeIsDirty)) {
     356        updateStyleAttribute();
     357        return;
     358    }
    342359#if ENABLE(SVG)
    343360    if (UNLIKELY(elementData()->m_animatedSVGAttributesAreDirty))
    344361        updateAnimatedSVGAttribute(name);
    345362#endif
    346 
     363}
     364
     365inline void Element::synchronizeAttribute(const AtomicString& localName) const
     366{
     367    // This version of synchronizeAttribute() is streamlined for the case where you don't have a full QualifiedName,
     368    // e.g when called from DOM API.
     369    if (!elementData())
     370        return;
     371    if (elementData()->m_styleAttributeIsDirty && equalPossiblyIgnoringCase(localName, styleAttr.localName(), shouldIgnoreAttributeCase(this))) {
     372        updateStyleAttribute();
     373        return;
     374    }
     375#if ENABLE(SVG)
     376    if (elementData()->m_animatedSVGAttributesAreDirty) {
     377        // We're not passing a namespace argument on purpose. SVGNames::*Attr are defined w/o namespaces as well.
     378        updateAnimatedSVGAttribute(QualifiedName(nullAtom, localName, nullAtom));
     379    }
     380#endif
     381}
     382
     383const AtomicString& Element::getAttribute(const QualifiedName& name) const
     384{
     385    if (!elementData())
     386        return nullAtom;
     387    synchronizeAttribute(name);
    347388    if (const Attribute* attribute = getAttributeItem(name))
    348389        return attribute->value();
    … …  
    699740}
    700741
    701 static inline bool shouldIgnoreAttributeCase(const Element* e)
    702 {
    703     return e && e->document()->isHTMLDocument() && e->isHTMLElement();
    704 }
    705 
    706 const AtomicString& Element::getAttribute(const AtomicString& name) const
     742const AtomicString& Element::getAttribute(const AtomicString& localName) const
    707743{
    708744    if (!elementData())
    709745        return nullAtom;
    710 
    711     bool ignoreCase = shouldIgnoreAttributeCase(this);
    712 
    713     // Update the 'style' attribute if it's invalid and being requested:
    714     if (elementData()->m_styleAttributeIsDirty && equalPossiblyIgnoringCase(name, styleAttr.localName(), ignoreCase))
    715         updateStyleAttribute();
    716 
    717 #if ENABLE(SVG)
    718     if (elementData()->m_animatedSVGAttributesAreDirty) {
    719         // We're not passing a namespace argument on purpose. SVGNames::*Attr are defined w/o namespaces as well.
    720         updateAnimatedSVGAttribute(QualifiedName(nullAtom, name, nullAtom));
    721     }
    722 #endif
    723 
    724     if (const Attribute* attribute = elementData()->getAttributeItem(name, ignoreCase))
     746    synchronizeAttribute(localName);
     747    if (const Attribute* attribute = elementData()->getAttributeItem(localName, shouldIgnoreAttributeCase(this)))
    725748        return attribute->value();
    726749    return nullAtom;
    … …  
    732755}
    733756
    734 void Element::setAttribute(const AtomicString& name, const AtomicString& value, ExceptionCode& ec)
    735 {
    736     if (!Document::isValidName(name)) {
     757void Element::setAttribute(const AtomicString& localName, const AtomicString& value, ExceptionCode& ec)
     758{
     759    if (!Document::isValidName(localName)) {
    737760        ec = INVALID_CHARACTER_ERR;
    738761        return;
    739762    }
    740763
    741     const AtomicString& localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
    742 
    743     size_t index = ensureElementDataWithSynchronizedAttributes()->getAttributeItemIndex(localName, false);
    744     const QualifiedName& qName = index != notFound ? attributeItem(index)->name() : QualifiedName(nullAtom, localName, nullAtom);
     764    synchronizeAttribute(localName);
     765    const AtomicString& caseAdjustedLocalName = shouldIgnoreAttributeCase(this) ? localName.lower() : localName;
     766
     767    size_t index = elementData() ? elementData()->getAttributeItemIndex(caseAdjustedLocalName, false) : notFound;
     768    const QualifiedName& qName = index != notFound ? attributeItem(index)->name() : QualifiedName(nullAtom, caseAdjustedLocalName, nullAtom);
    745769    setAttributeInternal(index, qName, value, NotInSynchronizationOfLazyAttribute);
    746770}
    … …  
    748772void Element::setAttribute(const QualifiedName& name, const AtomicString& value)
    749773{
    750     setAttributeInternal(ensureElementDataWithSynchronizedAttributes()->getAttributeItemIndex(name), name, value, NotInSynchronizationOfLazyAttribute);
     774    synchronizeAttribute(name);
     775    size_t index = elementData() ? elementData()->getAttributeItemIndex(name) : notFound;
     776    setAttributeInternal(index, name, value, NotInSynchronizationOfLazyAttribute);
    751777}
    752778
    753779void Element::setSynchronizedLazyAttribute(const QualifiedName& name, const AtomicString& value)
    754780{
    755     setAttributeInternal(ensureUniqueElementData()->getAttributeItemIndex(name), name, value, InSynchronizationOfLazyAttribute);
     781    size_t index = elementData() ? elementData()->getAttributeItemIndex(name) : notFound;
     782    setAttributeInternal(index, name, value, InSynchronizationOfLazyAttribute);
    756783}
    757784
    … …  
    10331060bool Element::hasAttributes() const
    10341061{
    1035     updateInvalidAttributes();
     1062    synchronizeAllAttributes();
    10361063    return elementData() && elementData()->length();
    10371064}
    … …  
    10391066bool Element::hasEquivalentAttributes(const Element* other) const
    10401067{
    1041     const ElementData* elementData = elementDataWithSynchronizedAttributes();
    1042     const ElementData* otherElementData = other->elementDataWithSynchronizedAttributes();
    1043     if (elementData == otherElementData)
     1068    synchronizeAllAttributes();
     1069    other->synchronizeAllAttributes();
     1070    if (elementData() == other->elementData())
    10441071        return true;
    1045     if (elementData)
    1046         return elementData->isEquivalent(otherElementData);
    1047     if (otherElementData)
    1048         return otherElementData->isEquivalent(elementData);
     1072    if (elementData())
     1073        return elementData()->isEquivalent(other->elementData());
     1074    if (other->elementData())
     1075        return other->elementData()->isEquivalent(elementData());
    10491076    return true;
    10501077}
    … …  
    16861713    }
    16871714
    1688     updateInvalidAttributes();
     1715    synchronizeAllAttributes();
    16891716    UniqueElementData* elementData = ensureUniqueElementData();
    16901717
    … …  
    17231750    ASSERT(document() == attr->document());
    17241751
    1725     const ElementData* elementData = elementDataWithSynchronizedAttributes();
    1726     ASSERT(elementData);
    1727 
    1728     size_t index = elementData->getAttributeItemIndex(attr->qualifiedName());
     1752    synchronizeAttribute(attr->qualifiedName());
     1753
     1754    size_t index = elementData()->getAttributeItemIndex(attr->qualifiedName());
    17291755    if (index == notFound) {
    17301756        ec = NOT_FOUND_ERR;
    … …  
    18141840}
    18151841
    1816 PassRefPtr<Attr> Element::getAttributeNode(const AtomicString& name)
    1817 {
    1818     const ElementData* elementData = elementDataWithSynchronizedAttributes();
    1819     if (!elementData)
     1842PassRefPtr<Attr> Element::getAttributeNode(const AtomicString& localName)
     1843{
     1844    if (!elementData())
    18201845        return 0;
    1821     const Attribute* attribute = elementData->getAttributeItem(name, shouldIgnoreAttributeCase(this));
     1846    synchronizeAttribute(localName);
     1847    const Attribute* attribute = elementData()->getAttributeItem(localName, shouldIgnoreAttributeCase(this));
    18221848    if (!attribute)
    18231849        return 0;
    … …  
    18271853PassRefPtr<Attr> Element::getAttributeNodeNS(const AtomicString& namespaceURI, const AtomicString& localName)
    18281854{
    1829     const ElementData* elementData = elementDataWithSynchronizedAttributes();
    1830     if (!elementData)
     1855    if (!elementData())
    18311856        return 0;
    1832     const Attribute* attribute = elementData->getAttributeItem(QualifiedName(nullAtom, localName, namespaceURI));
     1857    QualifiedName qName(nullAtom, localName, namespaceURI);
     1858    synchronizeAttribute(qName);
     1859    const Attribute* attribute = elementData()->getAttributeItem(qName);
    18331860    if (!attribute)
    18341861        return 0;
    … …  
    18361863}
    18371864
    1838 bool Element::hasAttribute(const AtomicString& name) const
     1865bool Element::hasAttribute(const AtomicString& localName) const
    18391866{
    18401867    if (!elementData())
    18411868        return false;
    1842 
    1843     // This call to String::lower() seems to be required but
    1844     // there may be a way to remove it.
    1845     AtomicString localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
    1846     return elementDataWithSynchronizedAttributes()->getAttributeItem(localName, false);
     1869    synchronizeAttribute(localName);
     1870    return elementData()->getAttributeItem(shouldIgnoreAttributeCase(this) ? localName.lower() : localName, false);
    18471871}
    18481872
    18491873bool Element::hasAttributeNS(const AtomicString& namespaceURI, const AtomicString& localName) const
    18501874{
    1851     const ElementData* elementData = elementDataWithSynchronizedAttributes();
    1852     if (!elementData)
     1875    if (!elementData())
    18531876        return false;
    1854     return elementData->getAttributeItem(QualifiedName(nullAtom, localName, namespaceURI));
     1877    QualifiedName qName(nullAtom, localName, namespaceURI);
     1878    synchronizeAttribute(qName);
     1879    return elementData()->getAttributeItem(qName);
    18551880}
    18561881
    … …  
    27442769        detachAllAttrNodesFromElement();
    27452770
    2746     other.updateInvalidAttributes();
     2771    other.synchronizeAllAttributes();
    27472772    if (!other.m_elementData) {
    27482773        m_elementData.clear();

  • trunk/Source/WebCore/dom/Element.h

    r143054 r143112  
    370370
    371371    const ElementData* elementData() const { return m_elementData.get(); }
    372     const ElementData* elementDataWithSynchronizedAttributes() const;
    373     const ElementData* ensureElementDataWithSynchronizedAttributes() const;
    374372    UniqueElementData* ensureUniqueElementData();
     373
     374    void synchronizeAllAttributes() const;
    375375
    376376    // Clones attributes only.
    … …  
    648648    void didRemoveAttribute(const QualifiedName&);
    649649
    650     void updateInvalidAttributes() const;
     650    void synchronizeAttribute(const QualifiedName&) const;
     651    void synchronizeAttribute(const AtomicString& localName) const;
    651652
    652653    void scrollByUnits(int units, ScrollGranularity);
    … …  
    775776}
    776777
    777 inline const ElementData* Element::elementDataWithSynchronizedAttributes() const
    778 {
    779     updateInvalidAttributes();
    780     return elementData();
    781 }
    782 
    783 inline const ElementData* Element::ensureElementDataWithSynchronizedAttributes() const
    784 {
    785     updateInvalidAttributes();
    786     if (elementData())
    787         return elementData();
    788     return const_cast<Element*>(this)->ensureUniqueElementData();
    789 }
    790 
    791778inline void Element::updateName(const AtomicString& oldName, const AtomicString& newName)
    792779{
    … …  
    900887    ASSERT(elementData());
    901888    return elementData()->getAttributeItem(name);
    902 }
    903 
    904 inline void Element::updateInvalidAttributes() const
    905 {
    906     if (!elementData())
    907         return;
    908 
    909     if (elementData()->m_styleAttributeIsDirty)
    910         updateStyleAttribute();
    911 
    912 #if ENABLE(SVG)
    913     if (elementData()->m_animatedSVGAttributesAreDirty)
    914         updateAnimatedSVGAttribute(anyQName());
    915 #endif
    916889}
    917890

  • trunk/Source/WebCore/dom/Node.cpp

    r142858 r143112  
    17541754        // We are comparing two attributes on the same node. Crawl our attribute map and see which one we hit first.
    17551755        Element* owner1 = attr1->ownerElement();
    1756         owner1->elementDataWithSynchronizedAttributes(); // Force update invalid attributes.
     1756        owner1->synchronizeAllAttributes();
    17571757        unsigned length = owner1->attributeCount();
    17581758        for (unsigned i = 0; i < length; ++i) {
Note: See TracChangeset for help on using the changeset viewer.