eliminate BRS_ISINLINEINCRREFLOW in favor of reflow roots

VERIFIED FIXED in mozilla1.0

Status

()

Core
Layout
P2
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Chris Waterson, Assigned: Chris Waterson)

Tracking

({perf})

Trunk
mozilla1.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: landed on the trunk)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
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.)
(Assignee)

Updated

16 years ago
Blocks: 110325, 123623
(Assignee)

Comment 2

16 years ago
Created attachment 77753 [details] [diff] [review]
proposed fix

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.
(Assignee)

Updated

16 years ago
Blocks: 73348
(Assignee)

Comment 3

16 years ago
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.
(Assignee)

Comment 5

16 years ago
Created attachment 77771 [details] [diff] [review]
revised patch

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
(Assignee)

Comment 6

16 years ago
Rolling the dice for mozilla-1.0. Reviews, anyone?
Target Milestone: mozilla1.0.1 → mozilla1.0
(Assignee)

Updated

16 years ago
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+
(Assignee)

Comment 8

16 years ago
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).

Comment 9

16 years ago
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.
(Assignee)

Comment 11

16 years ago
rjesup: could you point me to the pages where you got assertions and regressions?

Comment 12

16 years ago
I've got the patch in my (trunk) tree, and I don't see the problems rjesup
mentioned.

Comment 13

16 years ago
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 15

16 years ago
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 16

16 years ago
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 19

16 years ago
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+
(Assignee)

Comment 20

16 years ago
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

Comment 21

16 years ago
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

Comment 22

16 years ago
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).
(Assignee)

Comment 23

16 years ago
Checked in on mozilla-1.0 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Blocks: 137378
(Assignee)

Updated

16 years ago
Blocks: 58148

Comment 24

16 years ago
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
(Assignee)

Updated

16 years ago
Blocks: 133768

Comment 25

16 years ago
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.