Bug 15394 – notifyAttributeChanged should be removed (or at least reduced in usage)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

notifyAttributeChanged should be removed (or at least reduced in usage)

Currently modifying any attribute on an SVGStyledElement will cause that element to relayout.  This is bad.  Very bad.

This is due to the evil "notifyAttributeChanged" function which indiscriminately calls renderer()->setNeedsLayout() w/o first checking what attribute might be changed (or allowing other parsing machinery to see if the resulting new value is actually different from the previous one.

notifyAttributeChanged (i'm told) may need to exist to support SVG "tear off" javascript bindings. I'm not 100% convinced of that.  Regardless, notifyAttributeChanged can be removed from attributeChanged and the appropriate places in the code currently depending on the nAC call cleaned up.

[Nikolas Zimmermann]

Die notifyAttributeChange, long live attributeChanged!

Using a novel approach for handling SVG DOM dynamic updates, I found a way to eliminate the notifyAttributeChange sledgehammer. SVG Animated properties know it's corresponding attribute (SVGNames::foobarAttr) already, so it's easy to call element->attributeChanged(..myCorrespondingAttribute) from the JS bindings. I don't want to go into detail how non-animated properties are handled - just want to outline the general idea.

All SVG elements need a proper svgAttributeChanged / childrenChanged implementation now - instead of implementing nAC() - which blindly updated. Gives an amazing performance boost when running layout tests or trying heavy web apps like Lively Kernel.

Uploading insanely big patch soon.

[Nikolas Zimmermann]

Created attachment 18884 [details]
Initial patch

Initial patch, no regressions, just performance boost. Contains a lot search/replace changes, like s/.localName()// in the ANIMATED_* property definitions, affecting all files in svg/*.

Have fun reviewing ;-)

[Eric Seidel (no email)]

Comment on attachment 18884 [details]
Initial patch

This is a fantastic patch.  We discussed a bunch of little stuff over IRC, and you said you were going to upload a new one.  Really really great work.

[Nikolas Zimmermann]

Created attachment 18889 [details]
Updated patch

Hopefully fixed all issues Eric mentioned.

[Eric Seidel (no email)]

Comment on attachment 18889 [details]
Updated patch

r=me!

[Nikolas Zimmermann]

Landed in r29951.