Severity: normal → critical
Target Milestone: --- → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Assignee: karnaze → rods
Created attachment 65429 [details] [diff] [review] patch For some reason as yet undetermined, box layout changes a resize reflow back into an initial reflow so page layout for Print Preview ends up getting multiple initial reflows. This patch filters those out.
Created attachment 65432 [details] [diff] [review] patch for possible reflow optimization I found this. It seems to skip the extra resize reflow a lot both in regular layout and in PP layout. Everything still seems to work ok and look ok. Here is a diff of two runs with and without the optimization when loading the mozilla "start" page. It reduces reflow in nsBoxToBlockAdaptor and nsBoxFrame. Note the overall reflows are reduced by 36: P:\mozilla\dist\WIN32_D.OBJ\bin>diff log1.txt log2.txt 2a3 > *** Registering nsLayoutModule components (all right -- a generic module!) 75c76 < nsBoxToBlockAdaptor 0 47 6 0 0 53 --- > nsBoxToBlockAdaptor 0 36 6 0 0 42 78,79c79,80 < ViewportFrame 1 52 2 0 0 55 < nsRootBoxFrame 1 52 2 0 0 55 --- > ViewportFrame 1 47 2 0 0 50 > nsRootBoxFrame 1 47 2 0 0 50 82c83 < nsBlockFrame 32 0 4 0 0 36 --- > nsBlockFrame 22 0 4 0 0 26 84c85 < nsBoxFrame 1 52 2 0 0 55 --- > nsBoxFrame 1 47 2 0 0 50 86c87 < Grand Totals 47 203 23 0 0 273 --- > Grand Totals 37 177 23 0 0 237 *** When loading the CSS2 page it reduced the number of reflows by 381, so the longer the document the more reflows that are saved!
Created attachment 65435 [details] [diff] [review] replaces first patch (nsSimplePageSeq.cpp) I found a small issue with setting the cached value for skipping the reflow.
Attachment #65429 - Attachment is obsolete: true
This fix keeps us from crashing by filtering out the extra initial reflows. I have filed a bug on the real problem: Bug 120652
Target Milestone: mozilla0.9.9 → mozilla0.9.8
Since evaughan is no longer around, could you spend a bit of time and try to figure out why box is converting the reflow reason to eReflowReason_Initial? (hyatt may have some insight.) Hacks like this have a nasty way of living forever.
Created attachment 65611 [details] [diff] [review] full patch with optimization This is the correct full fix for the problem. When reflowing a HTML document where the content model is benig created The nsGfxScrollFrame will get an Initial reflow when the body is opned by the content sink. But there isn't enough content to really reflow very much of the document so it never needs to do layout for the scrollbars So later other reflows happen and these are Incremental reflows, and then the scrollbars get reflowed. The important point here is that when they reflowed the ReflowState inside the BoxLayoutState contains an "Incrtemental" reason and never a "Initial" reason. When it reflows for Print Preview, the content model is already full constructed and it lays out the entire document at that time. When it return back here it discovers it needs scrollbars not this is a problem because the ReflowState insode the BoxLayoutState still has a "Initial" reason and if it does a Layout it is essentially asking everything to reflow yet again with an "Initial" reason. This causes a lot of problems especially for tables. The solution for this is to change the ReflowState's reason from Initial to Resize and let all the frames do what is necessary for a resize refow. Now, we only need to do this when it is doing PrintPreview and we need only do it for HTML documents and NOT chrome. At this time it doesn't appear to be any reason for setting the state back to initial afterward. Also, we do not want to call "GetAscent" when doing an Intial in PP.
Attachment #65435 - Attachment is obsolete: true
I'm going to defer to dbaron & hyatt, since I've got one foot on a plane.
Comment on attachment 65611 [details] [diff] [review] full patch with optimization The nice mondo comment has some typos - please re-read it and clean up the grammar and spelling. Also, since you do set the state value back, you can remove the part about not needing to do that. IsInitialReflowForPrintPreview should initialize the aIsChrome arg to PR_FALSE Can you put an explanation of why you don't call GetAscent if this is the initial PP reflow for non-chrome docs? r=attinasi
Attachment #65611 - Flags: review+
I fixed the comments, and I did set the aIsChrome to false but it is really indeterminate. Also, the best explaination I give for not doing the GetAscent, is that it appears to have a vaild value from the r.height, and if called, it crashes.
email@example.com on drivers' behalf for checkin to 0.9.8. /be
No longer blocks: 115520
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Rod, can you verify? thanks...
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.