Bug 13431 – calcMinMaxWidth should be lazy.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

calcMinMaxWidth should occur as needed to answer the question of minwidth/maxwidth and not before then.

[Dave Hyatt]

Created attachment 14133 [details]
Patch to make minmaxwidth calculations happen lazily

This patch passes all layout tests and makes minmax recalc happen lazily.  This involved a substantial amount of shuffling of code that had sort of randomly glommed on to these methods to do other work (first-letter, counters, list markers, etc.).

Results should be excellent as long as you don't use a table.  Once you start using tables, you're sunk of course.

[Dave Hyatt]

Patch has no real effect on the PLT.  Too many tables I guess.  I'm going to profile just to make sure there isn't a speedup coupled to a slowdown that might be masking gains.

[Dave Hyatt]

Created attachment 14134 [details]
Snapshot #2, tighten things up a bit. Optimize more.

[Dave Hyatt]

Created attachment 14142 [details]
Passes all layout tests.

Ready to go I think.

[Darin Adler]

Comment on attachment 14142 [details]
Passes all layout tests.

It looks to me like the cssstyleselector.cpp has nothing to do with the rest of this patch.

It's too late now, I guess, but I would have suggested making the name change separate from the laziness change. It would make it a hell of a lot easier to read the patch.

I started reviewing, but didn't have the staying power to read and understand the whole thing. So not reviewed yet by me.

[Darin Adler]

Created attachment 14168 [details]
first reduced patch (still passes all layout tests)

I landed a first set of safe changes and reduced the patch to this. I can do this again and land a second set.

[Darin Adler]

Questions for Hyatt:

Why does RenderTable still have an updateFirstLetter() function? Should it be removed, or should RenderBlock's updateFirstLetter() be changed to a virtual function? What kind of test would show the difference?

Why is updateFirstLetter() called in both RenderBlock::layout() and RenderBlock::calcPrefWidths()?

[Darin Adler]

Why isn't RenderObject::containingBlock a virtual function? Instead it has code that does isTableCell() and isRenderView() at the start of the function, which is more expensive than just being a virtual function in the first place.

[Darin Adler]

Created attachment 14169 [details]
round 2 of changes to reduce the patch size

[Maciej Stachowiak]

Comment on attachment 14169 [details]
round 2 of changes to reduce the patch size

r=me

[Darin Adler]

Comment on attachment 14169 [details]
round 2 of changes to reduce the patch size

Landed now, so clearing the reviewed flag.

[Darin Adler]

Created attachment 14170 [details]
more reduced patch, passes layout tests (code 99% by Hyatt, not me)

[Darin Adler]

Comment on attachment 14170 [details]
more reduced patch, passes layout tests (code 99% by Hyatt, not me)

Comments from our review:

Need to leave updateFirstLetter virtual (so we should dump RenderBlock.h).

Also should remove the mutable.

r=me

[Dave Hyatt]

Fixed.