Bug 74987 – Make calls to willModifyAttribute and attributeChanged symmetrical

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

Make calls to willModifyAttribute and attributeChanged symmetrical

[Adam Klein]

Created attachment 120133 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 120133 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:199
> +inline void Element::removeAttributeInternal(size_t index)

We should move this function over to NamedNodeMap so that all calls to willModifyAttribute, etc... will be done inside one class.

> Source/WebCore/dom/Element.cpp:218
> +        AtomicString savedValue = attribute->value();
> +        attribute->setValue(nullAtom);
> +        attributeChanged(attribute.get());
> +        attribute->setValue(savedValue);

Why do we need to do this?

> Source/WebCore/dom/Element.cpp:684
> +        Attribute* attributePtr = attribute.get();
> +        m_attributeMap->addAttribute(attribute.release());
> +        attributeChanged(attributePtr);

!? Why are you obtaining a raw pointer here? That's not safe is it? We should use attribute in both function calls.

> Source/WebCore/dom/Element.cpp:1497
>      String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;

We should just call getAttributeItemIndex with shouldIgnoreAttributeCase=true here and improve the implementation of getAttributeItemIndex to avoid doing useless for-loop when shouldIgnoreAttributeCase=true.

> Source/WebCore/dom/NamedNodeMap.cpp:171
> +    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();

This code is repeated over and over. Can we add a helper function in Attribute or NamedNodeMap so that we don't have to repeat these over and over?

[Adam Klein]

Comment on attachment 120133 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:199
>> +inline void Element::removeAttributeInternal(size_t index)
> 
> We should move this function over to NamedNodeMap so that all calls to willModifyAttribute, etc... will be done inside one class.

I was planning to do that next, would you like that in this change?

>> Source/WebCore/dom/Element.cpp:218
>> +        attribute->setValue(savedValue);
> 
> Why do we need to do this?

attributeChanged() expects to be called with the new value of the Attribute.  Since we just removed it, the new value is nullAtom.  We can avoid an allocation by mutating the just-removed Attribute instead of cloning it.  If you're asking why we set it back to the previous value, I suspect it's due to the refcounting: we can't be sure someone isn't holding a reference (though that would be a very strange thing to do).

>> Source/WebCore/dom/Element.cpp:684
>> +        attributeChanged(attributePtr);
> 
> !? Why are you obtaining a raw pointer here? That's not safe is it? We should use attribute in both function calls.

I'm minimizing the refcount churn at the expense of slightly uglier code: the reference is passed to m_attributeMap, leaving attributePtr still valid.  I can simplify this by not calling release() when passing to addAttribute at the cost of refcount churn, but I'd rather not take that hit if I don't have to (since half the point of this series of changes is to minimize the cost of these otherwise very simple methods).

>> Source/WebCore/dom/Element.cpp:1497
>>      String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
> 
> We should just call getAttributeItemIndex with shouldIgnoreAttributeCase=true here and improve the implementation of getAttributeItemIndex to avoid doing useless for-loop when shouldIgnoreAttributeCase=true.

Which "useless" for loop did you have in mind? I believe the idea here is to optimize for an HTML document where all attributes are always referred to by lowercase names.  I'd agree that the Element <-> NamedNodeMap interface could still use some work in this regard, but optimizing for this case makes sense to me, so I'm not sure what could you wanted to get rid of (rather than just reorganizing, say, by putting the .lower() call inside getAttributeItemIndex()).

>> Source/WebCore/dom/NamedNodeMap.cpp:171
>> +        m_element->dispatchSubtreeModifiedEvent();
> 
> This code is repeated over and over. Can we add a helper function in Attribute or NamedNodeMap so that we don't have to repeat these over and over?

Which do you mean? dispatchSubtreeModifiedEvent is called twice here and twice in Element; that duplication should be eliminated once we move setAttributeInternal/removeAttributeInternal into NamedNodeMap (see question above about whether you want to see that in this patch).  Once we're down to two calls it's not clear to me it's worth a helper function.

[Ryosuke Niwa]

Comment on attachment 120133 [details]
Patch

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

>>> Source/WebCore/dom/Element.cpp:1497
>>>      String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
>> 
>> We should just call getAttributeItemIndex with shouldIgnoreAttributeCase=true here and improve the implementation of getAttributeItemIndex to avoid doing useless for-loop when shouldIgnoreAttributeCase=true.
> 
> Which "useless" for loop did you have in mind? I believe the idea here is to optimize for an HTML document where all attributes are always referred to by lowercase names.  I'd agree that the Element <-> NamedNodeMap interface could still use some work in this regard, but optimizing for this case makes sense to me, so I'm not sure what could you wanted to get rid of (rather than just reorganizing, say, by putting the .lower() call inside getAttributeItemIndex()).

Ah, you're right. I think we should just get rid of the call to lower() here. If using lower() is faster, then we should modify getAttributeItemIndex to always lower first. If not, then we're slowing things down.

[Adam Klein]

Comment on attachment 120133 [details]
Patch

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

>>> Source/WebCore/dom/Element.cpp:199
>>> +inline void Element::removeAttributeInternal(size_t index)
>> 
>> We should move this function over to NamedNodeMap so that all calls to willModifyAttribute, etc... will be done inside one class.
> 
> I was planning to do that next, would you like that in this change?

Per discussion, will do a followup to move this code into NamedNodeMap.

>>> Source/WebCore/dom/Element.cpp:218

>> 
>> Why do we need to do this?
> 
> attributeChanged() expects to be called with the new value of the Attribute.  Since we just removed it, the new value is nullAtom.  We can avoid an allocation by mutating the just-removed Attribute instead of cloning it.  If you're asking why we set it back to the previous value, I suspect it's due to the refcounting: we can't be sure someone isn't holding a reference (though that would be a very strange thing to do).

I'll look into removing this resetting behavior later, but for now I want to avoid changing behavior if at all possible.

>>> Source/WebCore/dom/Element.cpp:684
>>> +        attributeChanged(attributePtr);
>> 
>> !? Why are you obtaining a raw pointer here? That's not safe is it? We should use attribute in both function calls.
> 
> I'm minimizing the refcount churn at the expense of slightly uglier code: the reference is passed to m_attributeMap, leaving attributePtr still valid.  I can simplify this by not calling release() when passing to addAttribute at the cost of refcount churn, but I'd rather not take that hit if I don't have to (since half the point of this series of changes is to minimize the cost of these otherwise very simple methods).

Removed the refcounting tricks.

>>>> Source/WebCore/dom/Element.cpp:1497
>>>>      String localName = shouldIgnoreAttributeCase(this) ? name.lower() : name;
>>> 
>>> We should just call getAttributeItemIndex with shouldIgnoreAttributeCase=true here and improve the implementation of getAttributeItemIndex to avoid doing useless for-loop when shouldIgnoreAttributeCase=true.
>> 
>> Which "useless" for loop did you have in mind? I believe the idea here is to optimize for an HTML document where all attributes are always referred to by lowercase names.  I'd agree that the Element <-> NamedNodeMap interface could still use some work in this regard, but optimizing for this case makes sense to me, so I'm not sure what could you wanted to get rid of (rather than just reorganizing, say, by putting the .lower() call inside getAttributeItemIndex()).
> 
> Ah, you're right. I think we should just get rid of the call to lower() here. If using lower() is faster, then we should modify getAttributeItemIndex to always lower first. If not, then we're slowing things down.

Sadly, the lower() call can't move into getAttributeItem, since in setAttribute may create a new QualifiedName and we depend on the fact that all QualifiedNames we create in an HTMLDocument have lowercased localNames.  So we could move it in, but then we'd be double-lowering in some cases.

>>> Source/WebCore/dom/NamedNodeMap.cpp:171
>>> +        m_element->dispatchSubtreeModifiedEvent();
>> 
>> This code is repeated over and over. Can we add a helper function in Attribute or NamedNodeMap so that we don't have to repeat these over and over?
> 
> Which do you mean? dispatchSubtreeModifiedEvent is called twice here and twice in Element; that duplication should be eliminated once we move setAttributeInternal/removeAttributeInternal into NamedNodeMap (see question above about whether you want to see that in this patch).  Once we're down to two calls it's not clear to me it's worth a helper function.

Will factor this out appropriately in the next patch.

[Adam Klein]

Created attachment 120206 [details]
Patch

[Adam Klein]

Committed r103452: <http://trac.webkit.org/changeset/103452>