Diff
Modified: trunk/LayoutTests/ChangeLog (103610 => 103611)
--- trunk/LayoutTests/ChangeLog 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/LayoutTests/ChangeLog 2011-12-23 09:12:28 UTC (rev 103611)
@@ -1,3 +1,14 @@
+2011-12-23 Adam Klein <[email protected]>
+
+ Minimize callsites and duplication of before/after advice for attribute mutations
+ https://bugs.webkit.org/show_bug.cgi?id=75054
+
+ Reviewed by Ryosuke Niwa.
+
+ * inspector/debugger/dom-breakpoints.html: Add tests for breakpoints
+ due to mutation of Attr nodes/NamedNodeLists.
+ * platform/chromium/inspector/debugger/dom-breakpoints-expected.txt:
+
2011-12-23 Pierre Rossi <[email protected]>
[Qt] REGRESSION(r103467): It broke fast/images/animated-gif-restored-from-bfcache.html
Modified: trunk/LayoutTests/inspector/debugger/dom-breakpoints.html (103610 => 103611)
--- trunk/LayoutTests/inspector/debugger/dom-breakpoints.html 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/LayoutTests/inspector/debugger/dom-breakpoints.html 2011-12-23 09:12:28 UTC (rev 103611)
@@ -18,6 +18,20 @@
element.setAttribute(name, value);
}
+function modifyAttrNode(elementId, name, value)
+{
+ var element = document.getElementById(elementId);
+ element.attributes[name].value = value;
+}
+
+function setAttrNode(elementId, name, value)
+{
+ var element = document.getElementById(elementId);
+ var attr = document.createAttribute(name);
+ attr.value = value;
+ element.attributes.setNamedItem(attr);
+}
+
function modifyStyleAttribute(elementId, name, value)
{
var element = document.getElementById(elementId);
@@ -80,10 +94,10 @@
InspectorTest.addResult("Test that 'Attribute Modified' breakpoint is hit when modifying attribute.");
pane._setBreakpoint(rootElement, pane._breakpointTypes.AttributeModified, true);
InspectorTest.addResult("Set 'Attribute Modified' DOM breakpoint on rootElement.");
- InspectorTest.evaluateInPageWithTimeout("modifyAttribute('rootElement', 'className', 'foo')");
- InspectorTest.addResult("Modify rootElement className.");
+ InspectorTest.evaluateInPageWithTimeout("modifyAttribute('rootElement', 'data-test', 'foo')");
+ InspectorTest.addResult("Modify rootElement data-test attribute.");
waitUntilPausedAndDumpStack(step2);
-
+
function step2(callFrames)
{
pane._removeBreakpoint(rootElement, pane._breakpointTypes.AttributeModified);
@@ -91,6 +105,38 @@
}
},
+ function testModifyAttrNode(next)
+ {
+ InspectorTest.addResult("Test that 'Attribute Modified' breakpoint is hit when modifying Attr node.");
+ pane._setBreakpoint(rootElement, pane._breakpointTypes.AttributeModified, true);
+ InspectorTest.addResult("Set 'Attribute Modified' DOM breakpoint on rootElement.");
+ InspectorTest.evaluateInPageWithTimeout("modifyAttrNode('rootElement', 'data-test', 'bar')");
+ InspectorTest.addResult("Modify rootElement data-test attribute.");
+ waitUntilPausedAndDumpStack(step2);
+
+ function step2(callFrames)
+ {
+ pane._removeBreakpoint(rootElement, pane._breakpointTypes.AttributeModified);
+ next();
+ }
+ },
+
+ function testSetAttrNode(next)
+ {
+ InspectorTest.addResult("Test that 'Attribute Modified' breakpoint is hit when adding a new Attr node.");
+ pane._setBreakpoint(rootElement, pane._breakpointTypes.AttributeModified, true);
+ InspectorTest.addResult("Set 'Attribute Modified' DOM breakpoint on rootElement.");
+ InspectorTest.evaluateInPageWithTimeout("setAttrNode('rootElement', 'data-foo', 'bar')");
+ InspectorTest.addResult("Modify rootElement data-foo attribute.");
+ waitUntilPausedAndDumpStack(step2);
+
+ function step2(callFrames)
+ {
+ pane._removeBreakpoint(rootElement, pane._breakpointTypes.AttributeModified);
+ next();
+ }
+ },
+
function testModifyStyleAttribute(next)
{
InspectorTest.addResult("Test that 'Attribute Modified' breakpoint is hit when modifying style attribute.");
Modified: trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt (103610 => 103611)
--- trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/LayoutTests/platform/chromium/inspector/debugger/dom-breakpoints-expected.txt 2011-12-23 09:12:28 UTC (rev 103611)
@@ -28,7 +28,7 @@
Remove grandchildElement.
Script execution paused.
Call stack:
- 0) removeElement (dom-breakpoints.html:30)
+ 0) removeElement (dom-breakpoints.html:44)
1) (:1)
Paused on a "Subtree Modified" breakpoint set on div#rootElement, because its descendant div#grandchildElement was removed.
Script execution resumed.
@@ -36,7 +36,7 @@
Running: testModifyAttribute
Test that 'Attribute Modified' breakpoint is hit when modifying attribute.
Set 'Attribute Modified' DOM breakpoint on rootElement.
-Modify rootElement className.
+Modify rootElement data-test attribute.
Script execution paused.
Call stack:
0) modifyAttribute (dom-breakpoints.html:18)
@@ -44,13 +44,35 @@
Paused on a "Attribute Modified" breakpoint set on div#rootElement.
Script execution resumed.
+Running: testModifyAttrNode
+Test that 'Attribute Modified' breakpoint is hit when modifying Attr node.
+Set 'Attribute Modified' DOM breakpoint on rootElement.
+Modify rootElement data-test attribute.
+Script execution paused.
+Call stack:
+ 0) modifyAttrNode (dom-breakpoints.html:24)
+ 1) (:1)
+Paused on a "Attribute Modified" breakpoint set on div#rootElement.
+Script execution resumed.
+
+Running: testSetAttrNode
+Test that 'Attribute Modified' breakpoint is hit when adding a new Attr node.
+Set 'Attribute Modified' DOM breakpoint on rootElement.
+Modify rootElement data-foo attribute.
+Script execution paused.
+Call stack:
+ 0) setAttrNode (dom-breakpoints.html:32)
+ 1) (:1)
+Paused on a "Attribute Modified" breakpoint set on div#rootElement.
+Script execution resumed.
+
Running: testModifyStyleAttribute
Test that 'Attribute Modified' breakpoint is hit when modifying style attribute.
Set 'Attribute Modified' DOM breakpoint on rootElement.
Modify rootElement style.color attribute.
Script execution paused.
Call stack:
- 0) modifyStyleAttribute (dom-breakpoints.html:24)
+ 0) modifyStyleAttribute (dom-breakpoints.html:38)
1) (:1)
Paused on a "Attribute Modified" breakpoint set on div#rootElement.
Script execution resumed.
@@ -61,7 +83,7 @@
Remove elementToRemove.
Script execution paused.
Call stack:
- 0) removeElement (dom-breakpoints.html:30)
+ 0) removeElement (dom-breakpoints.html:44)
1) (:1)
Paused on a "Node Removed" breakpoint set on div#elementToRemove.
Script execution resumed.
Modified: trunk/Source/WebCore/ChangeLog (103610 => 103611)
--- trunk/Source/WebCore/ChangeLog 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/ChangeLog 2011-12-23 09:12:28 UTC (rev 103611)
@@ -1,3 +1,56 @@
+2011-12-23 Adam Klein <[email protected]>
+
+ Minimize callsites and duplication of before/after advice for attribute mutations
+ https://bugs.webkit.org/show_bug.cgi?id=75054
+
+ Reviewed by Ryosuke Niwa.
+
+ r103452 helpfully made before and after advice regarding attribute
+ changes symmetrical. This change finishes that work, by pulling
+ together all the before/after work, not just the crumbs previously
+ covered. This includes incrementing Document::domTreeVersion()
+ when an attribute is about to be changed, Inspector instrumentation,
+ and MutationEvent dispatch. This is in addition to the previous code,
+ which handled enqueueing MutationRecords for MutationObservers and
+ updating the Document's list of IDs.
+
+ The only change in behavior should be in InspectorInstrumentation,
+ which causes DOM breakpoints to occur for more cases of Attribute
+ mutation. This seems like more correct behavior, and a test has
+ been included to exercise it.
+
+ Hopefully the last Attribute-related refactor for awhile.
+
+ * dom/Attr.cpp:
+ (WebCore::Attr::setValue): Update to call didModifyAttribute instead
+ of attributeChanged.
+ * dom/Element.cpp:
+ (WebCore::Element::removeAttribute): Got rid of
+ removeAttributeInternal as most of that logic moved back into
+ NamedNodeMap::removeAttribute.
+ (WebCore::Element::setAttributeInternal): Reorganized to read better
+ now that only some cases result in calls to will/didModifyAttribute.
+ (WebCore::Element::willModifyAttribute): Un-inlined and added
+ incDOMTreeVersion and InspectorInstrumentation calls.
+ (WebCore::Element::didModifyAttribute): New method which encapsulates
+ calling attributeChanged, InspectorInstrumentation, and MutationEvents.
+ (WebCore::Element::didRemoveAttribute): New method which encapsulates
+ calling attributeChanged, InspectorInstrumentation, and MutationEvents.
+ Separate from didModifyAttribute because it has special handling of
+ the removed Attribute's value.
+ * dom/Element.h:
+ (WebCore::Element::willRemoveAttribute): New method which delegates to
+ willModifyAttribute as appropriate.
+ * dom/NamedNodeMap.cpp:
+ (WebCore::NamedNodeMap::setNamedItem): Simplified.
+ (WebCore::NamedNodeMap::removeNamedItem): Simplified.
+ (WebCore::NamedNodeMap::addAttribute): Added calls to will/didModifyAttribute.
+ (WebCore::NamedNodeMap::removeAttribute): ditto.
+ (WebCore::NamedNodeMap::replaceAttribute): ditto.
+ * svg/properties/SVGAnimatedPropertySynchronizer.h: Reverted changes
+ made in r103452 now that addAttribute/removeAttribute once again
+ call attributeChanged appropriately.
+
2011-12-22 Matt Falkenhagen <[email protected]>
Map 'lang' and xml:lang attributes to '-webkit-locale' CSS property for use with font fallback and text-transform
Modified: trunk/Source/WebCore/dom/Attr.cpp (103610 => 103611)
--- trunk/Source/WebCore/dom/Attr.cpp 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/dom/Attr.cpp 2011-12-23 09:12:28 UTC (rev 103611)
@@ -140,7 +140,7 @@
setValue(value);
if (m_element)
- m_element->attributeChanged(m_attribute.get());
+ m_element->didModifyAttribute(m_attribute.get());
}
void Attr::setNodeValue(const String& v, ExceptionCode& ec)
Modified: trunk/Source/WebCore/dom/Element.cpp (103610 => 103611)
--- trunk/Source/WebCore/dom/Element.cpp 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/dom/Element.cpp 2011-12-23 09:12:28 UTC (rev 103611)
@@ -178,52 +178,14 @@
{
}
-#if ENABLE(MUTATION_OBSERVERS)
-void Element::enqueueAttributesMutationRecordIfRequested(const QualifiedName& attributeName, const AtomicString& oldValue)
-{
- if (isSynchronizingStyleAttribute())
- return;
- if (OwnPtr<MutationObserverInterestGroup> mutationRecipients = MutationObserverInterestGroup::createForAttributesMutation(this, attributeName))
- mutationRecipients->enqueueMutationRecord(MutationRecord::createAttributes(this, attributeName, oldValue));
-}
-#endif
-
void Element::removeAttribute(const QualifiedName& name)
{
if (!m_attributeMap)
return;
- removeAttributeInternal(m_attributeMap->getAttributeItemIndex(name));
+ m_attributeMap->removeAttribute(name);
}
-inline void Element::removeAttributeInternal(size_t index)
-{
- ASSERT(m_attributeMap);
- if (index == notFound)
- return;
-
- 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)
{
if (value)
@@ -663,36 +625,26 @@
inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value)
{
+ Attribute* old = index != notFound ? m_attributeMap->attributeItem(index) : 0;
if (value.isNull()) {
- removeAttributeInternal(index);
+ if (old)
+ m_attributeMap->removeAttribute(index);
return;
}
- Attribute* old = index != notFound ? m_attributeMap->attributeItem(index) : 0;
+ if (!old) {
+ m_attributeMap->addAttribute(createAttribute(name, value));
+ return;
+ }
- if (!isSynchronizingStyleAttribute())
- InspectorInstrumentation::willModifyDOMAttr(document(), this);
-
- document()->incDOMTreeVersion();
-
willModifyAttribute(name, old ? old->value() : nullAtom, value);
- 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
- old->setValue(value);
- attributeChanged(old);
- }
+ if (Attr* attrNode = old->attr())
+ attrNode->setValue(value);
+ else
+ old->setValue(value);
- if (!isSynchronizingStyleAttribute()) {
- InspectorInstrumentation::didModifyDOMAttr(document(), this, name.localName(), value);
- dispatchSubtreeModifiedEvent();
- }
+ didModifyAttribute(old);
}
PassRefPtr<Attribute> Element::createAttribute(const QualifiedName& name, const AtomicString& value)
@@ -1494,7 +1446,11 @@
return;
String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
- removeAttributeInternal(m_attributeMap->getAttributeItemIndex(localName, false));
+ size_t index = m_attributeMap->getAttributeItemIndex(localName, false);
+ if (index == notFound)
+ return;
+
+ m_attributeMap->removeAttribute(index);
}
void Element::removeAttributeNS(const String& namespaceURI, const String& localName)
@@ -1992,4 +1948,51 @@
}
#endif
+void Element::willModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
+{
+ document()->incDOMTreeVersion();
+
+ if (isIdAttributeName(name))
+ updateId(oldValue, newValue);
+
+#if ENABLE(MUTATION_OBSERVERS)
+ if (!isSynchronizingStyleAttribute()) {
+ if (OwnPtr<MutationObserverInterestGroup> recipients = MutationObserverInterestGroup::createForAttributesMutation(this, name))
+ recipients->enqueueMutationRecord(MutationRecord::createAttributes(this, name, oldValue));
+ }
+#endif
+
+#if ENABLE(INSPECTOR)
+ if (!isSynchronizingStyleAttribute())
+ InspectorInstrumentation::willModifyDOMAttr(document(), this);
+#endif
+}
+
+void Element::didModifyAttribute(Attribute* attr)
+{
+ attributeChanged(attr);
+
+ if (!isSynchronizingStyleAttribute()) {
+ InspectorInstrumentation::didModifyDOMAttr(document(), this, attr->name().localName(), attr->value());
+ dispatchSubtreeModifiedEvent();
+ }
+}
+
+void Element::didRemoveAttribute(Attribute* attr)
+{
+ if (attr->isNull())
+ return;
+
+ AtomicString savedValue = attr->value();
+ attr->setValue(nullAtom);
+ attributeChanged(attr);
+ attr->setValue(savedValue);
+
+ if (!isSynchronizingStyleAttribute()) {
+ InspectorInstrumentation::didRemoveDOMAttr(document(), this, attr->name().localName());
+ dispatchSubtreeModifiedEvent();
+ }
+}
+
+
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/Element.h (103610 => 103611)
--- trunk/Source/WebCore/dom/Element.h 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/dom/Element.h 2011-12-23 09:12:28 UTC (rev 103611)
@@ -265,7 +265,11 @@
virtual String title() const;
void updateId(const AtomicString& oldId, const AtomicString& newId);
+
void willModifyAttribute(const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue);
+ void willRemoveAttribute(const QualifiedName&, const AtomicString& value);
+ void didModifyAttribute(Attribute*);
+ void didRemoveAttribute(Attribute*);
LayoutSize minimumSizeForResizing() const;
void setMinimumSizeForResizing(const LayoutSize&);
@@ -392,7 +396,6 @@
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
@@ -429,10 +432,6 @@
SpellcheckAttributeState spellcheckAttributeState() const;
-#if ENABLE(MUTATION_OBSERVERS)
- void enqueueAttributesMutationRecordIfRequested(const QualifiedName&, const AtomicString& oldValue);
-#endif
-
private:
mutable RefPtr<NamedNodeMap> m_attributeMap;
};
@@ -514,16 +513,10 @@
scope->addElementById(newId, this);
}
-inline void Element::willModifyAttribute(const QualifiedName& name, const AtomicString& oldValue, const AtomicString& newValue)
+inline void Element::willRemoveAttribute(const QualifiedName& name, const AtomicString& value)
{
- if (isIdAttributeName(name))
- updateId(oldValue, newValue);
-
- // FIXME: Should probably call InspectorInstrumentation::willModifyDOMAttr here.
-
-#if ENABLE(MUTATION_OBSERVERS)
- enqueueAttributesMutationRecordIfRequested(name, oldValue);
-#endif
+ if (!value.isNull())
+ willModifyAttribute(name, value, nullAtom);
}
inline bool Element::fastHasAttribute(const QualifiedName& name) const
Modified: trunk/Source/WebCore/dom/NamedNodeMap.cpp (103610 => 103611)
--- trunk/Source/WebCore/dom/NamedNodeMap.cpp 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/dom/NamedNodeMap.cpp 2011-12-23 09:12:28 UTC (rev 103611)
@@ -120,8 +120,6 @@
return 0;
}
- m_element->willModifyAttribute(attribute->name(), oldAttribute ? oldAttribute->value() : nullAtom, attribute->value());
-
RefPtr<Attr> oldAttr;
if (oldAttribute) {
oldAttr = oldAttribute->createAttrIfNeeded(m_element);
@@ -129,11 +127,6 @@
} else
addAttribute(attribute);
- m_element->attributeChanged(attribute);
-
- if (attribute->name() != styleAttr)
- m_element->dispatchSubtreeModifiedEvent();
-
return oldAttr.release();
}
@@ -152,24 +145,10 @@
return 0;
}
- RefPtr<Attribute> attribute = attributeItem(index);
- RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element);
+ RefPtr<Attr> attr = m_attributes[index]->createAttrIfNeeded(m_element);
- 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();
}
@@ -254,32 +233,56 @@
m_element->attributeChanged(m_attributes[i].get(), true);
}
-void NamedNodeMap::addAttribute(PassRefPtr<Attribute> attribute)
+void NamedNodeMap::addAttribute(PassRefPtr<Attribute> prpAttribute)
{
+ RefPtr<Attribute> attribute = prpAttribute;
+
+ if (m_element)
+ m_element->willModifyAttribute(attribute->name(), nullAtom, attribute->value());
+
m_attributes.append(attribute);
- if (Attr* attr = m_attributes.last()->attr())
+ if (Attr* attr = attribute->attr())
attr->m_element = m_element;
+
+ if (m_element)
+ m_element->didModifyAttribute(attribute.get());
}
void NamedNodeMap::removeAttribute(size_t index)
{
ASSERT(index < length());
- if (Attr* attr = m_attributes[index]->attr())
+ RefPtr<Attribute> attribute = m_attributes[index];
+
+ if (m_element)
+ m_element->willRemoveAttribute(attribute->name(), attribute->value());
+
+ if (Attr* attr = attribute->attr())
attr->m_element = 0;
+ m_attributes.remove(index);
- m_attributes.remove(index);
+ if (m_element)
+ m_element->didRemoveAttribute(attribute.get());
}
-void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)
+void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> prpAttribute)
{
ASSERT(index < length());
- if (Attr* attr = m_attributes[index]->attr())
+ RefPtr<Attribute> attribute = prpAttribute;
+ Attribute* old = m_attributes[index].get();
+
+ if (m_element)
+ m_element->willModifyAttribute(attribute->name(), old->value(), attribute->value());
+
+ if (Attr* attr = old->attr())
attr->m_element = 0;
m_attributes[index] = attribute;
- if (Attr* attr = m_attributes[index]->attr())
+ if (Attr* attr = attribute->attr())
attr->m_element = m_element;
+
+ if (m_element)
+ m_element->didModifyAttribute(attribute.get());
}
void NamedNodeMap::setClass(const String& classStr)
Modified: trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h (103610 => 103611)
--- trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h 2011-12-23 08:32:05 UTC (rev 103610)
+++ trunk/Source/WebCore/svg/properties/SVGAnimatedPropertySynchronizer.h 2011-12-23 09:12:28 UTC (rev 103611)
@@ -34,16 +34,18 @@
static void synchronize(SVGElement* ownerElement, const QualifiedName& attrName, const AtomicString& value)
{
// 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.
+ // Attribute directly 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)
+ 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);
- else
- ownerElement->setAttribute(attrName, value);
+
}
};
