Bug 15394 - notifyAttributeChanged should be removed (or at least reduced in usage)
Summary: notifyAttributeChanged should be removed (or at least reduced in usage)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 16090 17166
  Show dependency treegraph
 
Reported: 2007-10-06 00:55 PDT by Eric Seidel (no email)
Modified: 2008-02-03 15:21 PST (History)
2 users (show)

See Also:


Attachments
Initial patch (239.57 KB, patch)
2008-02-03 08:59 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (237.23 KB, patch)
2008-02-03 14:54 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-10-06 00:55:22 PDT
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.
Comment 1 Nikolas Zimmermann 2008-02-03 08:43:57 PST
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.
Comment 2 Nikolas Zimmermann 2008-02-03 08:59:43 PST
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 ;-)
Comment 3 Eric Seidel (no email) 2008-02-03 14:43:40 PST
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.
Comment 4 Nikolas Zimmermann 2008-02-03 14:54:17 PST
Created attachment 18889 [details]
Updated patch

Hopefully fixed all issues Eric mentioned.
Comment 5 Eric Seidel (no email) 2008-02-03 15:08:26 PST
Comment on attachment 18889 [details]
Updated patch

r=me!
Comment 6 Nikolas Zimmermann 2008-02-03 15:21:09 PST
Landed in r29951.