Bug 15391 - SVGRenderContainer forces too many kids to relayout
Summary: SVGRenderContainer forces too many kids to relayout
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: 41566
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-06 00:21 PDT by Eric Seidel (no email)
Modified: 2010-07-06 00:28 PDT (History)
4 users (show)

See Also:


Attachments
Initial patch (13.31 KB, patch)
2010-07-05 08:04 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (17.12 KB, patch)
2010-07-05 09:07 PDT, Nikolas Zimmermann
krit: 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:21:31 PDT
SVGRenderContainer forces too many kids to relayout

SVGRenderContainer currently forces all kids to relayout whenever it has selfNeedsLayout() true.  This is set to true when the corresponding <g> element had its transform change OR when the nearest viewport changed bounds or a parent had a transform change.

SVGRenderContainer used to try and be smarter using SVGStyledElement::hasRelativeValues(), but when it became clear that changing the transform would also require forcing kids to relayout,  this "trick" no longer worked (since we can't differentiate currently between a "transform" inspired relayout or a "bounds" inspired relayout.

This bug is requesting that we make SVG renderers layout() methods smarter to avoid these unnecessary layouts of non-relative value elements.
Comment 1 Eric Seidel (no email) 2007-10-06 00:23:05 PDT
FYI, the "hasRelativeValues()" (broken) optimization was removed as part of fixing bug 15388.
Comment 2 Eric Seidel (no email) 2007-10-10 14:34:39 PDT
Because all renderers cache their absolute bounds, any time your bounds origin, or transform (or a parent's bounds origin or transform) changes *all* kids need to relayout.

However, any time just bounds size changes (or parents bounds size changes) only relative-sized kids need to relayout.  (The parent who's bounds size changed will invalidate the proper region for repaint.)

After fixing our localTransform handling to make the renders pull (soon to land), we now know when the local transform has changed, but that information isn't actually very useful unless we stash it away in the LayoutState so that our container kids can use it to decide what type of layout to do.
Comment 3 Nikolas Zimmermann 2010-07-03 00:56:03 PDT
Finally working on this issue! First results are very promising, saves a lot of unneeded layouts.
Comment 4 Nikolas Zimmermann 2010-07-05 08:04:29 PDT
Created attachment 60535 [details]
Initial patch

Not marked for review yet, missing a changelog so far.
Comment 5 Nikolas Zimmermann 2010-07-05 09:07:16 PDT
Created attachment 60547 [details]
Updated patch

This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.
Comment 6 Dirk Schulze 2010-07-05 09:29:59 PDT
(In reply to comment #5)
> Created an attachment (id=60547) [details]
> Updated patch
> 
> This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.

Great Patch Niko. Just a question about the LayoutTest. What causes the change? SVGRoot becomes 15px bigger, but I don't see the reason?!?
Comment 7 Nikolas Zimmermann 2010-07-05 22:44:07 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=60547) [details] [details]
> > Updated patch
> > 
> > This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.
> 
> Great Patch Niko. Just a question about the LayoutTest. What causes the change? SVGRoot becomes 15px bigger, but I don't see the reason?!?

I don't know why it had the wrong size before. If you look at the enclosing box, the new width is the right one. Not sure why it was wrong before - must be related to some premature optimizations, that calcViewport() was not called, when selfNeedsLayout was false (when not using relative lengths). Something like that, anyhow it's a progression.
Comment 8 Dirk Schulze 2010-07-06 00:11:03 PDT
Comment on attachment 60547 [details]
Updated patch

Great patch! We discussed the failing test on IRC. Please add the explaination to the changelog as well. r=me.
Comment 9 Nikolas Zimmermann 2010-07-06 00:28:58 PDT
Landed in r62531.