Bug 74953 - Avoid unnecessary work when removing attributes from an element
: Avoid unnecessary work when removing attributes from an element
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-12-20 13:56 PST by
Modified: 2011-12-20 17:33 PST (History)


Attachments
Patch (24.85 KB, patch)
2011-12-20 14:01 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff
More changelog detail (24.94 KB, patch)
2011-12-20 14:04 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff
Fix Qt build (26.37 KB, patch)
2011-12-20 14:30 PST, Adam Klein
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Patch for landing (29.04 KB, patch)
2011-12-20 15:10 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (29.35 KB, patch)
2011-12-20 16:02 PST, Adam Klein
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-12-20 13:56:51 PST
Avoid unnecessary work when removing attributes from an element
------- Comment #1 From 2011-12-20 14:01:43 PST -------
Created an attachment (id=120076) [details]
Patch
------- Comment #2 From 2011-12-20 14:02:16 PST -------
I'm seeing inspector test failures which I'm now investigating...
------- Comment #3 From 2011-12-20 14:04:37 PST -------
Created an attachment (id=120077) [details]
More changelog detail
------- Comment #4 From 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 From 2011-12-20 14:23:21 PST -------
(From update of attachment 120077 [details])
Attachment 120077 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993335
------- Comment #6 From 2011-12-20 14:30:37 PST -------
Created an attachment (id=120088) [details]
Fix Qt build
------- Comment #7 From 2011-12-20 14:58:12 PST -------
(From update of attachment 120077 [details])
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 From 2011-12-20 15:02:27 PST -------
Created an attachment (id=120093) [details]
Update setAttribute to use indices as well
------- Comment #9 From 2011-12-20 15:04:24 PST -------
(From update of attachment 120093 [details])
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 From 2011-12-20 15:08:53 PST -------
(From update of attachment 120077 [details])
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 From 2011-12-20 15:10:16 PST -------
Created an attachment (id=120096) [details]
Patch for landing
------- Comment #12 From 2011-12-20 15:44:50 PST -------
(From update of attachment 120096 [details])
Attachment 120096 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10993359
------- Comment #13 From 2011-12-20 16:02:49 PST -------
Created an attachment (id=120106) [details]
Patch for landing
------- Comment #14 From 2011-12-20 17:33:04 PST -------
Committed r103373: <http://trac.webkit.org/changeset/103373>