Closed Bug 135146 Opened 23 years ago Closed 23 years ago

eliminate BRS_ISINLINEINCRREFLOW in favor of reflow roots

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: waterson, Assigned: waterson)

References

Details

(Keywords: perf, Whiteboard: landed on the trunk)

Attachments

(1 file, 1 obsolete file)

dbaron has suggested that we ought to be able to improve typing performance and fix several typing regressions by allowing reflow to start at a frame other than the document's root frame. Currently, when typing in a text field, the reflow starts at the document's root frame, and proceeds down to target frame. The block code has some special optimizations (e.g., BRS_ISINLINEINCRREFLOW and nsLineLayout::ReflowFrame) to stifle damage propagation as reflow proceeds out of the text input frame. Instead, we ought to simply start the reflow at the text input frame.
Blocks: 129115
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
This will also allow removing a significant source of complexity in the block code. (Hopefully people won't start using 'display: inline-block' elements to take up large areas when they meant blocks once CSS3 adds that, because then we'd need that complexity back for letting incremental reflow into inlines again.)
Blocks: 110325, 123623
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
This adds a `NS_FRAME_REFLOW_ROOT' frame state bit that the nsGfxTextControlFrame2 sets on its scroll frame during SetInitialChildList. nsHTMLReflowCommand::BuildPath honors this bit by truncating the reflow path if it encounters a frame with the bit set. nsHTMLReflowCommand::Dispatch was modified so that a reflow that begins at a frame other than the root frame of the frame hierarchy uses the `reflow root' frame's width and height as the available space.
Blocks: 73348
So the above patch doesn't actually get rid of the BRS_ISINLINEINCRREFLOW crap. But it's good for flavor and fixes bug 110325 and bug 123623. I'm going to run with the above patch for a while and see if I encounter any problems.
No longer blocks: 73348
Since |GetContainingBlock| just does GetParent (it probably did something else in the past, I'd guess), I think you can remove that comment about the floater stuff as well as all the style->mFloat checks, since the two codepaths do exactly the same thing now, and also GetContainingBlock itself.
Attached patch revised patch (deleted) — Splinter Review
Fix variable hiding problem kin pointed out; remove BRS_ISINLINEINCRREFLOW stuff, remove nsHTMLReflowCommand::GetContainingBlock and related floater monkey business.
Attachment #77753 - Attachment is obsolete: true
Rolling the dice for mozilla-1.0. Reviews, anyone?
Target Milestone: mozilla1.0.1 → mozilla1.0
Keywords: review
Comment on attachment 77771 [details] [diff] [review] revised patch I've been up to my ears in this code, and this looks great. Are there any other frames other than scrollframes for which setting the reflow root bit makes sense? r=rjesup@wgate.com (if people think I'm qualified to r= on layout changes).
Attachment #77771 - Flags: review+
Well, this only sets the NS_FRAME_REFLOW_ROOT bit on the scroll frame inside of GFX text control frames (i.e., <input type="text"> and <textarea>). We could conceivably set this bit on other scroll frames (e.g., those created by <select> -- this may solve bug 119311, if bryner still cares).
adding this to the list of things we'd like to get for 1.0.
Keywords: mozilla1.0+
I tried merging this fix with the latest reflow tree branch. (Updated to tip, not yet released to CVS). It definitely improves things in terms of painting text widgets on bugzilla bug pages - the regression goes away entirely. I am getting some assertions (and this may be due to my other changes in the reflow tree branch!) from UnWind(), which could not get a parent successfully in GetBoxForFrame() when typing into a text widget. Also, another regression with typing re-appeared (every other character typed doesn't echo immediately). Again, this may have nothing to do with this patch; it may be the reflow tree code. Again, this only points out that we should have some people drop this into clean trees and try it in debug builds. I suspect the problems above are either merging mistakes or issues with the base reflow tree code. I definitely want this for 1.0 if possible.
rjesup: could you point me to the pages where you got assertions and regressions?
I've got the patch in my (trunk) tree, and I don't see the problems rjesup mentioned.
I like the patch, I'd just liek to see what happens if a text control is resized via script, so I'll apply the patch and try it out for a while before SRing.
I doubt my problems (when combined with the 129115 branch) are problems with this patch; more likely they're interactions and/or imperfect merging on my part.
Comment on attachment 77771 [details] [diff] [review] revised patch Kin showed me the testcase I was thinking of and also explained why it works correctly - I like this. sr=attinasi
Attachment #77771 - Flags: superreview+
Comment on attachment 77771 [details] [diff] [review] revised patch r=kin@netscape.com I did ask waterson to add some comments that note his findings about setting the bit on inline frames. I've been playing with these changes all morning and have yet to run across any problems that can be attributed directly to it, so this is looking real promising!
Comment on attachment 77771 [details] [diff] [review] revised patch This patch looks great to me. Any idea how it helps with typing performance? (While I was home over spring break, I was using Mozilla on my old P-233 (Linux), and I found I could (when typing quickly) type faster into Bugzilla textareas than Mozilla could keep up with me.)
It should help with typing according to waterson
Comment on attachment 77771 [details] [diff] [review] revised patch a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #77771 - Flags: approval+
Checked in on the trunk. I'll wait a day or so to make sure there are no side effects before landing on the mozilla-1.0 branch.
Whiteboard: landed on the trunk
Yee haw ... adding bug 123058 and bug 136562 to list of bugs this blocks. This bug doesn't actually fix the underlying cause of those bugs, but it definitely masks the problem and gets things working again because it causes incremental reflows to skip over the oh-so-troublesome-during-reflow-box-frame.
Blocks: 123058, 136562
Do you think that this will work OK in a dynamic way? For example, if an element has a specific width and height (like a DIV or IMG with widht and height specified) can we flick this new bit and expect the correct behavior? Then, if those attrbutes / properties are changed, unflick the bit and possibly enqueue another reflow? This could be a real winner for all kinds of situations where the size cannot change (I'm thinking of IFRAMES too).
Checked in on mozilla-1.0 branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Blocks: 137378
Blocks: 58148
adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword with verified1.0.0.
Keywords: fixed1.0.0
Blocks: 133768
Marking verified on branch OS X (2002-05-22-05)
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: