- Revision
- 103452
- Author
- [email protected]
- Date
- 2011-12-21 14:34:29 -0800 (Wed, 21 Dec 2011)
Log Message
Make calls to willModifyAttribute and attributeChanged symmetrical
https://bugs.webkit.org/show_bug.cgi?id=74987
Reviewed by Ryosuke Niwa.
Previously, calls to Element::willModifyAttribute sometimes happened
in one method while calls to Element::attributeChanged happened in
another. This change makes them symmetrical for all the cases I know
about: setAttribute, removeAttribute, setNamedItem, removeNamedItem.
To accomplish this, NamedNodeMap::addAttribute, removeAttribute, and
replaceAttribute have been reduced to their pure functionality of
manipulating m_attributes, and their callers are left responsible for
properly notifying the Element of the underlying changes.
One other bit of refactoring was done: to simplify
Element::setAttribute, it now dispatches to
Element::removeAttributeInternal if the incoming value is null.
No new tests, no change in behavior.
* dom/Attribute.h:
* dom/Element.cpp:
(WebCore::Element::removeAttribute):
(WebCore::Element::removeAttributeInternal): Added, sharing code
between the two removeAttribute overloads.
(WebCore::Element::setAttributeInternal):
* dom/Element.h:
* dom/NamedNodeMap.cpp:
(WebCore::NamedNodeMap::setNamedItem):
(WebCore::NamedNodeMap::removeNamedItem):
(WebCore::NamedNodeMap::addAttribute):
(WebCore::NamedNodeMap::removeAttribute):
(WebCore::NamedNodeMap::replaceAttribute):
* svg/properties/SVGAnimatedPropertySynchronizer.h:
Call Element::setAttribute unless the attribute is already present,
and add a comment explaining why the code looks the way it does.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (103451 => 103452)
--- trunk/Source/WebCore/ChangeLog 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/ChangeLog 2011-12-21 22:34:29 UTC (rev 103452)
@@ -1,3 +1,43 @@
+2011-12-20 Adam Klein <[email protected]>
+
+ Make calls to willModifyAttribute and attributeChanged symmetrical
+ https://bugs.webkit.org/show_bug.cgi?id=74987
+
+ Reviewed by Ryosuke Niwa.
+
+ Previously, calls to Element::willModifyAttribute sometimes happened
+ in one method while calls to Element::attributeChanged happened in
+ another. This change makes them symmetrical for all the cases I know
+ about: setAttribute, removeAttribute, setNamedItem, removeNamedItem.
+
+ To accomplish this, NamedNodeMap::addAttribute, removeAttribute, and
+ replaceAttribute have been reduced to their pure functionality of
+ manipulating m_attributes, and their callers are left responsible for
+ properly notifying the Element of the underlying changes.
+
+ One other bit of refactoring was done: to simplify
+ Element::setAttribute, it now dispatches to
+ Element::removeAttributeInternal if the incoming value is null.
+
+ No new tests, no change in behavior.
+
+ * dom/Attribute.h:
+ * dom/Element.cpp:
+ (WebCore::Element::removeAttribute):
+ (WebCore::Element::removeAttributeInternal): Added, sharing code
+ between the two removeAttribute overloads.
+ (WebCore::Element::setAttributeInternal):
+ * dom/Element.h:
+ * dom/NamedNodeMap.cpp:
+ (WebCore::NamedNodeMap::setNamedItem):
+ (WebCore::NamedNodeMap::removeNamedItem):
+ (WebCore::NamedNodeMap::addAttribute):
+ (WebCore::NamedNodeMap::removeAttribute):
+ (WebCore::NamedNodeMap::replaceAttribute):
+ * svg/properties/SVGAnimatedPropertySynchronizer.h:
+ Call Element::setAttribute unless the attribute is already present,
+ and add a comment explaining why the code looks the way it does.
+
2011-12-21 Adrienne Walker <[email protected]>
Unreviewed, rolling out r103408.
Modified: trunk/Source/WebCore/dom/Attribute.h (103451 => 103452)
--- trunk/Source/WebCore/dom/Attribute.h 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/dom/Attribute.h 2011-12-21 22:34:29 UTC (rev 103452)
@@ -40,7 +40,6 @@
// The actual Attr with its value as a Text child is allocated only if needed.
class Attribute : public RefCounted<Attribute> {
friend class Attr;
- friend class NamedNodeMap;
public:
static PassRefPtr<Attribute> create(const QualifiedName& name, const AtomicString& value)
{
Modified: trunk/Source/WebCore/dom/Element.cpp (103451 => 103452)
--- trunk/Source/WebCore/dom/Element.cpp 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/dom/Element.cpp 2011-12-21 22:34:29 UTC (rev 103452)
@@ -193,13 +193,35 @@
if (!m_attributeMap)
return;
- size_t index = m_attributeMap->getAttributeItemIndex(name);
+ removeAttributeInternal(m_attributeMap->getAttributeItemIndex(name));
+}
+
+inline void Element::removeAttributeInternal(size_t index)
+{
+ ASSERT(m_attributeMap);
if (index == notFound)
return;
- willModifyAttribute(name, m_attributeMap->attributeItem(index)->value(), nullAtom);
+ InspectorInstrumentation::willModifyDOMAttr(document(), this);
+ RefPtr<Attribute> attribute = m_attributeMap->attributeItem(index);
+
+ if (!attribute->isNull())
+ willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
+
m_attributeMap->removeAttribute(index);
+
+ if (!attribute->isNull()) {
+ AtomicString savedValue = attribute->value();
+ attribute->setValue(nullAtom);
+ attributeChanged(attribute.get());
+ attribute->setValue(savedValue);
+ }
+
+ InspectorInstrumentation::didRemoveDOMAttr(document(), this, attribute->name().localName());
+
+ if (!isSynchronizingStyleAttribute())
+ dispatchSubtreeModifiedEvent();
}
void Element::setBooleanAttribute(const QualifiedName& name, bool value)
@@ -641,22 +663,25 @@
inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value)
{
+ if (value.isNull()) {
+ removeAttributeInternal(index);
+ return;
+ }
+
Attribute* old = index != notFound ? m_attributeMap->attributeItem(index) : 0;
-#if ENABLE(INSPECTOR)
if (!isSynchronizingStyleAttribute())
InspectorInstrumentation::willModifyDOMAttr(document(), this);
-#endif
document()->incDOMTreeVersion();
willModifyAttribute(name, old ? old->value() : nullAtom, value);
- if (old && value.isNull())
- m_attributeMap->removeAttribute(index);
- else if (!old && !value.isNull())
- m_attributeMap->addAttribute(createAttribute(name, value));
- else if (old) {
+ if (!old) {
+ RefPtr<Attribute> attribute = createAttribute(name, value);
+ m_attributeMap->addAttribute(attribute);
+ attributeChanged(attribute.get());
+ } else {
if (Attr* attrNode = old->attr())
attrNode->setValue(value);
else
@@ -664,10 +689,10 @@
attributeChanged(old);
}
-#if ENABLE(INSPECTOR)
- if (!isSynchronizingStyleAttribute())
+ if (!isSynchronizingStyleAttribute()) {
InspectorInstrumentation::didModifyDOMAttr(document(), this, name.localName(), value);
-#endif
+ dispatchSubtreeModifiedEvent();
+ }
}
PassRefPtr<Attribute> Element::createAttribute(const QualifiedName& name, const AtomicString& value)
@@ -1469,20 +1494,7 @@
return;
String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
- size_t index = m_attributeMap->getAttributeItemIndex(localName, false);
- if (index == notFound)
- return;
-
- Attribute* attribute = m_attributeMap->attributeItem(index);
-
- InspectorInstrumentation::willModifyDOMAttr(document(), this);
-
- if (!attribute->isNull())
- willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
-
- m_attributeMap->removeAttribute(index);
-
- InspectorInstrumentation::didRemoveDOMAttr(document(), this, name);
+ removeAttributeInternal(m_attributeMap->getAttributeItemIndex(localName, false));
}
void Element::removeAttributeNS(const String& namespaceURI, const String& localName)
Modified: trunk/Source/WebCore/dom/Element.h (103451 => 103452)
--- trunk/Source/WebCore/dom/Element.h 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/dom/Element.h 2011-12-21 22:34:29 UTC (rev 103452)
@@ -396,6 +396,7 @@
virtual bool childTypeAllowed(NodeType) const;
void setAttributeInternal(size_t index, const QualifiedName&, const AtomicString& value);
+ void removeAttributeInternal(size_t index);
virtual PassRefPtr<Attribute> createAttribute(const QualifiedName&, const AtomicString& value);
#ifndef NDEBUG
Modified: trunk/Source/WebCore/dom/NamedNodeMap.cpp (103451 => 103452)
--- trunk/Source/WebCore/dom/NamedNodeMap.cpp 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/dom/NamedNodeMap.cpp 2011-12-21 22:34:29 UTC (rev 103452)
@@ -122,13 +122,18 @@
m_element->willModifyAttribute(attribute->name(), oldAttribute ? oldAttribute->value() : nullAtom, attribute->value());
- // ### slightly inefficient - resizes attribute array twice.
RefPtr<Attr> oldAttr;
if (oldAttribute) {
oldAttr = oldAttribute->createAttrIfNeeded(m_element);
replaceAttribute(index, attribute);
} else
addAttribute(attribute);
+
+ m_element->attributeChanged(attribute);
+
+ if (attribute->name() != styleAttr)
+ m_element->dispatchSubtreeModifiedEvent();
+
return oldAttr.release();
}
@@ -137,9 +142,6 @@
return setNamedItem(node, ec);
}
-// The DOM2 spec doesn't say that removeAttribute[NS] throws NOT_FOUND_ERR
-// if the attribute is not found, but at this level we have to throw NOT_FOUND_ERR
-// because of removeNamedItem, removeNamedItemNS, and removeAttributeNode.
PassRefPtr<Node> NamedNodeMap::removeNamedItem(const QualifiedName& name, ExceptionCode& ec)
{
ASSERT(m_element);
@@ -150,12 +152,24 @@
return 0;
}
- Attribute* attribute = attributeItem(index);
+ RefPtr<Attribute> attribute = attributeItem(index);
RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element);
- m_element->willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
+ if (!attribute->isNull())
+ m_element->willModifyAttribute(attribute->name(), attribute->value(), nullAtom);
removeAttribute(index);
+
+ if (!attribute->isNull()) {
+ AtomicString savedValue = attribute->value();
+ attribute->setValue(nullAtom);
+ m_element->attributeChanged(attribute.get());
+ attribute->setValue(savedValue);
+ }
+
+ if (attribute->name() != styleAttr)
+ m_element->dispatchSubtreeModifiedEvent();
+
return attr.release();
}
@@ -240,44 +254,21 @@
m_element->attributeChanged(m_attributes[i].get(), true);
}
-void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
+void NamedNodeMap::addAttribute(PassRefPtr<Attribute> attribute)
{
- RefPtr<Attribute> attribute = prpAttribute;
-
- // Add the attribute to the list
m_attributes.append(attribute);
-
- if (Attr* attr = attribute->attr())
+ if (Attr* attr = m_attributes.last()->attr())
attr->m_element = m_element;
-
- // Notify the element that the attribute has been added, and dispatch appropriate mutation events
- // Note that element may be null here if we are called from insertAttribute() during parsing
- if (m_element) {
- m_element->attributeChanged(attribute.get());
- // Because of our updateStyleAttribute() style modification events are never sent at the right time, so don't bother sending them.
- if (attribute->name() != styleAttr)
- m_element->dispatchSubtreeModifiedEvent();
- }
}
void NamedNodeMap::removeAttribute(size_t index)
{
ASSERT(index < length());
- RefPtr<Attribute> attribute = m_attributes[index];
- if (Attr* attr = attribute->attr())
+ if (Attr* attr = m_attributes[index]->attr())
attr->m_element = 0;
m_attributes.remove(index);
-
- if (m_element && !attribute->m_value.isNull()) {
- AtomicString value = attribute->m_value;
- attribute->m_value = nullAtom;
- m_element->attributeChanged(attribute.get());
- attribute->m_value = value;
- }
- if (m_element)
- m_element->dispatchSubtreeModifiedEvent();
}
void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)
@@ -289,12 +280,6 @@
m_attributes[index] = attribute;
if (Attr* attr = m_attributes[index]->attr())
attr->m_element = m_element;
-
- if (m_element) {
- m_element->attributeChanged(m_attributes[index].get());
- if (m_attributes[index]->name() != styleAttr)
- m_element->dispatchSubtreeModifiedEvent();
- }
}
void NamedNodeMap::setClass(const String& classStr)
Modified: trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h (103451 => 103452)
--- trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h 2011-12-21 22:31:48 UTC (rev 103451)
+++ trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h 2011-12-21 22:34:29 UTC (rev 103452)
@@ -33,14 +33,17 @@
struct SVGAnimatedPropertySynchronizer<true> {
static void synchronize(SVGElement* ownerElement, const QualifiedName& attrName, const AtomicString& value)
{
- NamedNodeMap* namedAttrMap = ownerElement->attributes(false);
+ // If the attribute already exists on the element, we change the
+ // Attribute directly rather than going through Element::setAttribute to
+ // avoid a call to Element::attributeChanged that could cause the
+ // SVGElement to erroneously reset its properties.
+ // svg/dom/SVGStringList-basics.xhtml exercises this behavior.
+ NamedNodeMap* namedAttrMap = ownerElement->attributes(false);
Attribute* old = namedAttrMap->getAttributeItem(attrName);
- if (old && value.isNull())
- namedAttrMap->removeAttribute(old->name());
- else if (!old && !value.isNull())
- namedAttrMap->addAttribute(ownerElement->createAttribute(attrName, value));
- else if (old && !value.isNull())
- old->setValue(value);
+ if (old)
+ old->setValue(value);
+ else
+ ownerElement->setAttribute(attrName, value);
}
};
