Bug 74953 – Avoid unnecessary work when removing attributes from an element

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Avoid unnecessary work when removing attributes from an element

[Adam Klein]

Created attachment 120076 [details]
Patch

[Adam Klein]

I'm seeing inspector test failures which I'm now investigating...

[Adam Klein]

Created attachment 120077 [details]
More changelog detail

[Adam Klein]

(In reply to comment #2)
> I'm seeing inspector test failures which I'm now investigating...

Never mind, just the usual debug timeouts. Ready for review.

[Early Warning System Bot]

Comment on attachment 120077 [details]
More changelog detail

Attachment 120077 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993335

[Adam Klein]

Created attachment 120088 [details]
Fix Qt build

[Ryosuke Niwa]

Comment on attachment 120077 [details]
More changelog detail

View in context: https://bugs.webkit.org/attachment.cgi?id=120077&action=review

> Source/WebCore/dom/Element.cpp:202
> +    willModifyAttribute(name, m_attributeMap->attributeItem(index)->value(), nullAtom);
> +
> +    m_attributeMap->removeAttribute(index);

It's not great that some of these functions call willModifyAttribute themselves but not attributeChanged. We should try to make them symmetric as possible to make the interface less obscure. Asymmetric interfaces like this one makes the correctness harder to verify if it doesn't introduce more bugs. e.g. why is it okay not to call InspectorInstrumentation::willModifyDOMAttr here?

> Source/WebCore/dom/Element.cpp:207
>  void Element::setBooleanAttribute(const QualifiedName& name, bool b)
>  {
>      if (b)

Could you rename b? e.g. value.

> Source/WebCore/dom/NamedNodeMap.cpp:154
>      RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element);

Do we need to create attr here or can we get away by creating on return?

> Source/WebCore/dom/NamedNodeMap.cpp:263
> -void NamedNodeMap::removeAttribute(const QualifiedName& name)
> +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)

Could you define replaceAttribute after removeAttribute so that the diff would look saner?

> Source/WebCore/dom/NamedNodeMap.cpp:271
> +    if (Attr* attr = m_attributes[index]->attr())
> +        attr->m_element = m_element;

It seems like we should define attr outside of the if condition to avoid having to duplicate "m_attributes[index]->attr()"

[Adam Klein]

Created attachment 120093 [details]
Update setAttribute to use indices as well

[Ryosuke Niwa]

Comment on attachment 120093 [details]
Update setAttribute to use indices as well

View in context: https://bugs.webkit.org/attachment.cgi?id=120093&action=review

> Source/WebCore/dom/NamedNodeMap.cpp:263
> -void NamedNodeMap::removeAttribute(const QualifiedName& name)
> +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)

Please re-odering these two functions if that improves the readability of diff.

[Adam Klein]

Comment on attachment 120077 [details]
More changelog detail

View in context: https://bugs.webkit.org/attachment.cgi?id=120077&action=review

>> Source/WebCore/dom/Element.cpp:202
>> +    m_attributeMap->removeAttribute(index);
> 
> It's not great that some of these functions call willModifyAttribute themselves but not attributeChanged. We should try to make them symmetric as possible to make the interface less obscure. Asymmetric interfaces like this one makes the correctness harder to verify if it doesn't introduce more bugs. e.g. why is it okay not to call InspectorInstrumentation::willModifyDOMAttr here?

Agreed that it's not ideal.  Once I've pruned NamedNodeMap down to the simplest possible interface I aim to fix up who calls attributeChanged and willModifyAttribute appropriately.

As for InspectorInstrumentation, I didn't want to change any behavior in this change: it's basically totally broken and needs fixing.  I guess I should file a bug with the inspector folks.

>> Source/WebCore/dom/Element.cpp:207
>>      if (b)
> 
> Could you rename b? e.g. value.

Done.

>> Source/WebCore/dom/NamedNodeMap.cpp:154
>>      RefPtr<Attr> attr = attribute->createAttrIfNeeded(m_element);
> 
> Do we need to create attr here or can we get away by creating on return?

The Attribute* would be invalid by the time we get to the return.  We're not doing any refcount churn here (since we return attr.release()), so I'm not sure I see a problem with this construction.

>> Source/WebCore/dom/NamedNodeMap.cpp:263
>> +void NamedNodeMap::replaceAttribute(size_t index, PassRefPtr<Attribute> attribute)
> 
> Could you define replaceAttribute after removeAttribute so that the diff would look saner?

Moved, we'll see if it helps.

>> Source/WebCore/dom/NamedNodeMap.cpp:271
>> +        attr->m_element = m_element;
> 
> It seems like we should define attr outside of the if condition to avoid having to duplicate "m_attributes[index]->attr()"

These are different m_attributes[index]->attr()s: the first one is the one we're about to remove, and the second is the new one.

[Adam Klein]

Created attachment 120096 [details]
Patch for landing

[Early Warning System Bot]

Comment on attachment 120096 [details]
Patch for landing

Attachment 120096 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993359

[Adam Klein]

Created attachment 120106 [details]
Patch for landing

[Adam Klein]

Committed r103373: <http://trac.webkit.org/changeset/103373>