Closed
Bug 135146
Opened 23 years ago
Closed 23 years ago
eliminate BRS_ISINLINEINCRREFLOW in favor of reflow roots
Categories
(Core :: Layout, defect, P2)
Core
Layout
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)
(deleted),
patch
|
jesup
:
review+
attinasi
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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•23 years ago
|
Comment 1•23 years ago
|
||
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•23 years ago
|
Assignee | ||
Comment 2•23 years ago
|
||
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 | ||
Comment 3•23 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
Comment 4•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Rolling the dice for mozilla-1.0. Reviews, anyone?
Target Milestone: mozilla1.0.1 → mozilla1.0
Comment 7•23 years ago
|
||
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•23 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•23 years ago
|
||
adding this to the list of things we'd like to get for 1.0.
Keywords: mozilla1.0+
Comment 10•23 years ago
|
||
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•23 years ago
|
||
rjesup: could you point me to the pages where you got assertions and regressions?
Comment 12•23 years ago
|
||
I've got the patch in my (trunk) tree, and I don't see the problems rjesup
mentioned.
Comment 13•23 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.
Comment 14•23 years ago
|
||
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•23 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•23 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 17•23 years ago
|
||
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.)
Comment 18•23 years ago
|
||
It should help with typing according to waterson
Comment 19•23 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•23 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•23 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.
Comment 22•23 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•23 years ago
|
||
Checked in on mozilla-1.0 branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 24•23 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
Comment 25•23 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.
Description
•