Bug 129476 – StyledElement::attributeChanged shouldn't do any work when the attribute value didn't change

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

[Ryosuke Niwa]

StyledElement::attributeChanged shouldn't do any work when the attribute value didn't change

[Ryosuke Niwa]

Created attachment 225446 [details]
Fixes the bug

[Alexey Proskuryakov]

Comment on attachment 225446 [details]
Fixes the bug

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

> Source/WebCore/ChangeLog:8
> +        Exit early in attributeChanged when the attribute value didn't change like r164856.

This description doesn't quite match the code change, there is no early exit.

[Andreas Kling]

Comment on attachment 225446 [details]
Fixes the bug

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

r=me

>> Source/WebCore/ChangeLog:8
>> +        Exit early in attributeChanged when the attribute value didn't change like r164856.
> 
> This description doesn't quite match the code change, there is no early exit.

Yeah, could be worded better.

[Ryosuke Niwa]

Comment on attachment 225446 [details]
Fixes the bug

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

>>> Source/WebCore/ChangeLog:8
>>> +        Exit early in attributeChanged when the attribute value didn't change like r164856.
>> 
>> This description doesn't quite match the code change, there is no early exit.
> 
> Yeah, could be worded better.

Oops, yeah. We still need to call Element::attributeChanged.

[Ryosuke Niwa]

Created attachment 293700 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 293700 [details]
Patch for landing

Wait for EWS first.

[Antti Koivisto]

Comment on attachment 293700 [details]
Patch for landing

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

> Source/WebCore/dom/StyledElement.cpp:158
> +    if (oldValue != newValue) {

Can't we just not call attributeChanged() when nothing changed?

[Ryosuke Niwa]

(In reply to comment #7)
> Comment on attachment 293700 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293700&action=review
> 
> > Source/WebCore/dom/StyledElement.cpp:158
> > +    if (oldValue != newValue) {
> 
> Can't we just not call attributeChanged() when nothing changed?

No, unfortunately, parseAttribute needs to be called even when the attribute value didn't change because some attributes on some elements can have a side effect.

And Element::attributeChanged also creates a mutation record for mutation observer, which is pretty important.

[Ryosuke Niwa]

Created attachment 294279 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294279 [details]
Patch for landing

Clearing flags on attachment: 294279

Committed r208485: <http://trac.webkit.org/changeset/208485>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.