Bug 129476

Summary: StyledElement::attributeChanged shouldn't do any work when the attribute value didn't change
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cdumez, commit-queue, dbates, esprehn+autocc, ggaren, kangil.han, kling, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug
none
Patch for landing
none
Patch for landing none

Description Ryosuke Niwa 2014-02-28 01:57:23 PST 
StyledElement::attributeChanged shouldn't do any work when the attribute value didn't change
Comment 1 Ryosuke Niwa 2014-02-28 01:58:17 PST 
Created attachment 225446 [details]
Fixes the bug
Comment 2 Alexey Proskuryakov 2014-02-28 09:23:05 PST 
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.
Comment 3 Andreas Kling 2014-02-28 22:20:29 PST 
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.
Comment 4 Ryosuke Niwa 2014-02-28 22:21:15 PST 
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.
Comment 5 Ryosuke Niwa 2016-11-02 14:59:36 PDT 
Created attachment 293700 [details]
Patch for landing
Comment 6 Ryosuke Niwa 2016-11-02 15:13:18 PDT 
Comment on attachment 293700 [details]
Patch for landing

Wait for EWS first.
Comment 7 Antti Koivisto 2016-11-02 15:32:51 PDT 
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?
Comment 8 Ryosuke Niwa 2016-11-02 16:00:47 PDT 
(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.
Comment 9 Ryosuke Niwa 2016-11-09 13:43:02 PST 
Created attachment 294279 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-11-09 14:18:09 PST 
Comment on attachment 294279 [details]
Patch for landing

Clearing flags on attachment: 294279

Committed r208485: <http://trac.webkit.org/changeset/208485>
Comment 11 WebKit Commit Bot 2016-11-09 14:18:15 PST 
All reviewed patches have been landed.  Closing bug.