Bug 74953 - Avoid unnecessary work when removing attributes from an element
: Avoid unnecessary work when removing attributes from an element
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Adam Klein
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-20 13:56 PST by Adam Klein
Modified: 2011-12-20 17:33 PST (History)
7 users (show)

See Also:


Attachments
Patch (24.85 KB, patch)
2011-12-20 14:01 PST, Adam Klein
no flags Details | Formatted Diff | Diff
More changelog detail (24.94 KB, patch)
2011-12-20 14:04 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Fix Qt build (26.37 KB, patch)
2011-12-20 14:30 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Update setAttribute to use indices as well (28.85 KB, patch)
2011-12-20 15:02 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (29.04 KB, patch)
2011-12-20 15:10 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (29.35 KB, patch)
2011-12-20 16:02 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-12-20 13:56:51 PST
Avoid unnecessary work when removing attributes from an element
Comment 1 Adam Klein 2011-12-20 14:01:43 PST
Created attachment 120076 [details]
Patch
Comment 2 Adam Klein 2011-12-20 14:02:16 PST
I'm seeing inspector test failures which I'm now investigating...
Comment 3 Adam Klein 2011-12-20 14:04:37 PST
Created attachment 120077 [details]
More changelog detail
Comment 4 Adam Klein 2011-12-20 14:15:41 PST
(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.
Comment 5 Early Warning System Bot 2011-12-20 14:23:21 PST
Comment on attachment 120077 [details]
More changelog detail

Attachment 120077 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993335
Comment 6 Adam Klein 2011-12-20 14:30:37 PST
Created attachment 120088 [details]
Fix Qt build
Comment 7 Ryosuke Niwa 2011-12-20 14:58:12 PST
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()"
Comment 8 Adam Klein 2011-12-20 15:02:27 PST
Created attachment 120093 [details]
Update setAttribute to use indices as well
Comment 9 Ryosuke Niwa 2011-12-20 15:04:24 PST
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.
Comment 10 Adam Klein 2011-12-20 15:08:53 PST
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.
Comment 11 Adam Klein 2011-12-20 15:10:16 PST
Created attachment 120096 [details]
Patch for landing
Comment 12 Early Warning System Bot 2011-12-20 15:44:50 PST
Comment on attachment 120096 [details]
Patch for landing

Attachment 120096 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993359
Comment 13 Adam Klein 2011-12-20 16:02:49 PST
Created attachment 120106 [details]
Patch for landing
Comment 14 Adam Klein 2011-12-20 17:33:04 PST
Committed r103373: <http://trac.webkit.org/changeset/103373>