Closed
Bug 370952
Opened 18 years ago
Closed 18 years ago
Fix the reflow functions in nsPresShell
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: sharparrow1, Assigned: sharparrow1)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
The current system of having four different ways to enter frame reflow (including three which as almost identical) is completely ridiculous.
There should be one path to reflow frames and insert content. That path should deal with all special cases.
InitialReflow is bad because it assumes there will be a root content node when it is called, and breaks if there isn't (for example, bug 370812). All the things it needs to do should be moved elsewhere. (We might need some sort of to replace some calls to InitialReflow().)
Proposed changes:
In general, all reflows should go through ProcessReflowCommands.
The following are the changes needed to make all reflows go through ProcessReflowCommands.
Fixing InitialReflow:
1. The root frame (the viewport) is constructed upon initialization of the PresShell.
2. The frame constructor should deal with any special frame construction handling needed for inserting/removing the root content of the document.
3. The frame constructor should notify the presshell when it creates a root scroll frame so that the presshell can register the appropriate listeners.
4. Content notifications shouldn't be suppressed before the initial reflow.
5. Reflow requests shouldn't be suppressed before the initial reflow.
6. The frame constructor should notify the presshell the first time it costructs a non-root frame so that the presshell can start the paint suppression timer.
OR
6. The content sink should notify the presshell at some point during the document load.
7. nsDocumentViewer should unconditionally SetVisibleArea.
8. If there is already a root content node in nsPresShell::Init, force frame construction to occur.
9. Replace InitialReflow with a method to start the paint suppression timer.
Essentially, the idea is that the initial reflow shouldn't be a special case, since there's nothing that makes it fundamentally different from any other reflow. This part's a bit complicated, but I think it's significantly better than what we have now.
Fixing ResizeReflow:
ResizeReflow() simply sets the size of the viewport in the prescontext, sets a resize reflow notification on the root frame, and possibly flushes reflow.
Fixing StyleChangeReflow:
StyleChangeReflow() simply sets a style change notification on the root frame, and possibly flushes reflow.
No patch yet, just thoughts.
|
||
Comment 1•18 years ago
|
||
Sounds like a good idea. These were a bit more different before the reflow branch, I think.
However, I think we do want to suppress stuff before we start displaying things -- generally the incremental codepaths are slower than doing everything in one pass, so suppressing things (frame construction, reflow) is desirable if we're not displaying anything yet. Fixing that to be reasonable may have some interactions with bz's patch to not block the parser for loading style sheets, since there is (presumably) some suppression mechanism in that patch for when style sheets haven't loaded yet.
|
Assignee | |
Comment 2•18 years ago
|
||
Hmm, if we really want to be putting off frame construction in a systematic way, then I suppose we should have methods to do that. Maybe have a flag to Init to delay frame construction, and a StartFrameConstruction method. That would make it easy to delay as long as we wanted.
Currently, for HTML, we start frame construction immediately after we get the <body> tag. Bug 84582 changes changes that to immediately after we get the <body> unless there's a stylesheet loading. Another posibility is doing frame construction once the parser runs out of data (unless there's a stylesheet loading), which would be faster if incremental frame construction is slower. It wouldn't be too hard to write a PostStartFrameConstructionEvent method.
Is there a bug about batching frame construction like we do reflow?
|
||
Comment 3•18 years ago
|
||
I think sicking's been considering changing the sinks to "batch" frame construction by simply inserting content in batches....
And yes, right now the InitialReflow() stuff for HTML happens before we have any real content (all the <head> stuff is display:none), so it's not really much of an optimization there....
|
||
Comment 4•18 years ago
|
||
(In reply to comment #0)
> Fixing StyleChangeReflow:
> StyleChangeReflow() simply sets a style change notification on the root frame,
> and possibly flushes reflow.
FWIW, this would save me a lot of trouble in bug 365130.
|
|
Assignee | |
Comment 5•18 years ago
|
||
This patch covers fixing ResizeReflow and StyleChangeReflow. (I figure this will be easier to review if I do it in parts.)
Attachment #255878 -
Flags: review?(dbaron)
|
|
||
Comment 6•18 years ago
|
||
With this change, ResizeReflow always reflows everything (because the DIRTY bit gets propagated down the tree), right? Is that really what we want? e.g., there's no real need to reflow fixed-width stuff if the viewport width changes... Or do we already do that with the old code?
|
|
Assignee | |
Comment 7•18 years ago
|
||
Sprry about that; small mistake in the first version.
This seems to completely fix the "wasDirty lied" assertion. Probably has something to do with always doing reflows in the order they are requested. (It seems like that could be O(n^2) in exreme cases, but that's a separate issue).
Attachment #255878 -
Attachment is obsolete: true
Attachment #255886 -
Flags: review?(dbaron)
Attachment #255878 -
Flags: review?(dbaron)
|
|
Assignee | |
Comment 8•18 years ago
|
||
(In reply to comment #6)
> With this change, ResizeReflow always reflows everything (because the DIRTY bit
> gets propagated down the tree), right? Is that really what we want? e.g.,
> there's no real need to reflow fixed-width stuff if the viewport width
> changes... Or do we already do that with the old code?
The dirty bit doesn't propagate down. A dirty frame always reflows itself, and does whatever other reflows are necessary based on its children. If a block get a dirty bit set but its width ends up the same, it doesn't need to reflow its kids if they aren't dirty.
The old ResizeReflow code also reflowed everything.
|
|
||
Comment 9•18 years ago
|
||
> The dirty bit doesn't propagate down.
Actually, it does. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.264&mark=151-156#150
|
|
Assignee | |
Comment 10•18 years ago
|
||
(In reply to comment #9)
> > The dirty bit doesn't propagate down.
>
> Actually, it does. See
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.264&mark=151-156#150
>
Okay, I'm not really sure about this, so don't believe everthing I say.
That said, that code should only get executed when the child gets reflowed. In cases we can optimize, we shouldn't even be trying to reflow the children. For example, see nsAbsoluteContainingBlock::Reflow.
|
|
Assignee | |
Comment 11•18 years ago
|
||
Nevermind, I see your point... I'll have to think about it a bit.
|
|
||
Comment 12•18 years ago
|
||
So maybe I'm missing something, but for the root frame, calling FrameNeedsReflow() with a reason of resize is equivalent to doing nothing, no? We'll skip the |if (aIntrinsicDirty != eResize)| and |if (aIntrinsicDirty == eStyleChange)| blocks, then the |if| inside the for loop will test true (no parent), but wasDirty is true, so we do nothing. And then we break out.
It seems like we could replace:
+ rootFrame->AddStateBits(NS_FRAME_IS_DIRTY);
+ FrameNeedsReflow(rootFrame, eResize);
with
if (!(rootFrame->GetStateBits() &
(NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) {
rootFrame->AddStateBits(NS_FRAME_HAS_DIRTY_CHILDREN);
mDirtyRoots.AppendElement(rootFrame;
}
It's a bit of a fib with the bits, but it'll have the desired effect, I think...
I wonder whether we could also get rid of the unconditional flush then.
|
|
||
Comment 13•18 years ago
|
||
Comment on attachment 255886 [details] [diff] [review]
Patch Part 1 v2
When a frame is NS_FRAME_IS_DIRTY, that means it and all its descendants need reflow. I suppose that should be clearer in the comments in nsIFrame.h.
Attachment #255886 -
Flags: review?(dbaron) → review-
|
|
Assignee | |
Comment 14•18 years ago
|
||
Okay, I think this is correct now. I made resize reflows add the root directly to mDirtyRoots, and made ProcessReflowCommands reflow the root even when it wasn't marked dirty.
I still haven't figured out the reason why the reflow flush is needed for the specific case of reducing the size of a window by dragging its edge; I think it has something to do with events.
Attachment #255886 -
Attachment is obsolete: true
Attachment #255925 -
Flags: review?(dbaron)
|
|
Assignee | |
Comment 15•18 years ago
|
||
Comment on attachment 255925 [details] [diff] [review]
Patch Part 1 v3
I really need to figure out WTF is going on with resize reflows... it's really confusing me, and I can't seem to get it quite right.
Attachment #255925 -
Flags: review?(dbaron)
|
|
Assignee | |
Comment 16•18 years ago
|
||
Attachment #255925 -
Attachment is obsolete: true
Attachment #256109 -
Flags: review?(dbaron)
|
|
||
Comment 17•18 years ago
|
||
Should StyleChangeReflow stay synchronous to be compatible with its previous behavior, or are you sure that callers don't depend on that? (i.e., should it call ProcessReflowCommands like ResizeReflow does?)
While you're here, how about removing this bit at the end of ProcessReflowCommands and just always posting the event:
// If we're in the middle of a drag, process it right away (needed for mac,
// might as well do it on all platforms just to keep the code paths the same).
// XXXbz but how does this actually "process it right away"?
// Isn't this more like "never process it"?
if ( !IsDragInProgress() )
Do you understand the business with the pres shell's visible area? Sometimes it's the aWidth/aHeight parameter to InitialReflow/ResizeReflow, and sometimes it's the resulting size of the root frame. But during reflow it could be either, depending on the type. Should the SetVisibleArea calls at the end of reflow instead be assertions that the root frame's size matches the pres context's visible area, or is it actually changing?
I notice you're not changing InitialReflow. Planning to do that later? (Fine with me, probably easier to review.)
The reason we need to fix the computed height for reflow roots is that the point of reflow roots is that what's inside of them doesn't affect what's outside. Therefore they must stay the same size during reflow. But we don't necessarily have all the information to initialize their height correctly, since the normal codepath depends on some of the information to do that being computed during recursive reflow. (We need to fix it up afterwards since passing in a height to the reflow state tells it that it's paginated, which we don't want.)
More later...
|
|
Assignee | |
Comment 18•18 years ago
|
||
(In reply to comment #17)
> Should StyleChangeReflow stay synchronous to be compatible with its previous
> behavior, or are you sure that callers don't depend on that? (i.e., should it
> call ProcessReflowCommands like ResizeReflow does?)
The only caller of StyleChangeReflow is in nsPresContext; it only triggers off of preference changes, so I don't think it's an issue.
> While you're here, how about removing this bit at the end of
> ProcessReflowCommands and just always posting the event:
> // If we're in the middle of a drag, process it right away (needed for mac,
> // might as well do it on all platforms just to keep the code paths the
> same).
> // XXXbz but how does this actually "process it right away"?
> // Isn't this more like "never process it"?
> if ( !IsDragInProgress() )
Okay, although I can't test on a mac.
> Do you understand the business with the pres shell's visible area? Sometimes
> it's the aWidth/aHeight parameter to InitialReflow/ResizeReflow, and sometimes
> it's the resulting size of the root frame. But during reflow it could be
> either, depending on the type. Should the SetVisibleArea calls at the end of
> reflow instead be assertions that the root frame's size matches the pres
> context's visible area, or is it actually changing?
The way it works is a little confusing. The visible area shouldn't change for an initial reflow. The size to content case makes resize reflows different. For sizing to content, the visible area gets set to nsRect(0, 0, preferredWidth, NS_UNCONSTRAINEDSIZE), and then reset to the correct value after reflow.
> I notice you're not changing InitialReflow. Planning to do that later? (Fine
> with me, probably easier to review.)
Yeah, one step at a time. (It's also dependent on fixing how we the root frames.)
> The reason we need to fix the computed height for reflow roots is that the
> point of reflow roots is that what's inside of them doesn't affect what's
> outside. Therefore they must stay the same size during reflow. But we don't
> necessarily have all the information to initialize their height correctly,
> since the normal codepath depends on some of the information to do that being
> computed during recursive reflow. (We need to fix it up afterwards since
> passing in a height to the reflow state tells it that it's paginated, which we
> don't want.)
That's a bit confusing, but I guess it makes sense.
|
|
||
Comment 19•18 years ago
|
||
> The only caller of StyleChangeReflow is in nsPresContext;
Technically, nsIPresShell is our "public" API to the presentation, so various binary components could be calling these methods... Not sure we care.
|
|
Assignee | |
Comment 20•18 years ago
|
||
(In reply to comment #19)
> > The only caller of StyleChangeReflow is in nsPresContext;
>
> Technically, nsIPresShell is our "public" API to the presentation, so various
> binary components could be calling these methods... Not sure we care.
Yes, nsIPresShell is "public", but it's so far from being frozen that anyone who actually used it would have to recompile... plus, I can't imagine where anything would depend on sync behavior, and I can't imagine any other use for StyleChangeReflow.
|
|
Assignee | |
Comment 21•18 years ago
|
||
Version with comments so far addressed.
Attachment #256109 -
Attachment is obsolete: true
Attachment #259775 -
Flags: review?(dbaron)
Attachment #256109 -
Flags: review?(dbaron)
|
|
Assignee | |
Updated•18 years ago
|
Attachment #259775 -
Attachment description: Patch v4 → Patch v5
|
|
||
Comment 22•18 years ago
|
||
I'm still not entirely comfortable with the FrameNeedsReflow and ProcessReflowCommands changes you're making so that you can handle resize reflows using the same mechanism as other reflows (which do mark dirty). I'm also worried that getting that change right will disable some other optimizations, e.g., suppression of reflows inside reflow roots after a toplevel reflow (although that currently only happens when they're processed in the right order). I need to look at this more closely -- it's not an easy review.
|
|
||
Comment 23•18 years ago
|
||
Comment on attachment 259775 [details] [diff] [review]
Patch v5
I've been thinking about this a bit, and I think what we want to do here is leave ResizeReflow separate from ProcessReflowCommands. There's still probably a bit of code that could be merged (the stuff inside the loop in ProcessReflowCommands, following the continue). (And InitialReflow and StyleChangeReflow probably should use ProcessReflowCommands.)
In that merging, I'd note that you should probably set reflowSize and size in the same if/else, and spell "exception" correctly.
Thanks for working on this, and sorry I misled you at first and took so long to get to reviewing this.
Attachment #259775 -
Flags: review?(dbaron) → review-
|
|
Assignee | |
Comment 24•18 years ago
|
||
So, more like this?
Attachment #259775 -
Attachment is obsolete: true
Attachment #262335 -
Flags: review?(dbaron)
|
|
||
Comment 25•18 years ago
|
||
Comment on attachment 262335 [details] [diff] [review]
Patch v6
Did you mean to call DoVerifyReflow from ResizeReflow? I'm not sure if
anyone cares about the VerifyReflow code anymore, though. (Maybe we
should, though.)
+void
+PresShell::DoReflow(nsIFrame* target) {
Please put the opening brace for functions on its own line.
+ NS_ENSURE_SUCCESS(rv, );
I don't think that's portable. I think you need at least
NS_ENSURE_SUCCESS(rv, /**/);
but I'm not even sure if that's portable. I suggest just writing out
if (NS_FAILED(rv)) {
NS_NOTREACHED("CreateRenderingContext failure");
return;
}
In StyleChangeReflow, could you put the unusual case (!rootFrame) as an
early return and make the main flow of the function not indented?
I suppose you know better than I would that UpdateViewProperties isn't
needed anymore, but it makes sense since it was only that one codepath
(StyleChangeReflow).
But what about the calls to PositionFrameView for the root frame? I'm a
little curious about the latter -- I thought doing that before reflow
was important for view positioning to be efficient.
// All the stuff we are creating that needs releasing
- nsPresContext* cx;
- nsIViewManager* vm;
- nsIPresShell* sh;
+ nsCOMPtr<nsPresContext> cx;
+ nsCOMPtr<nsIViewManager> vm;
+ nsCOMPtr<nsIPresShell> sh;
You don't need that comment anymore, and it's better to declare the
variables at or right before first use.
- // XXXldb Set mIsReflowing (and unset it later)?
This still applies (in ResizeReflow).
+ // XXX Kinda evil, but there really isn't any other way to deal with
+ // reflow requests targeting the root frame
+ PRBool wasDirty = f->GetParent() || mDirtyRoots.IndexOf(f) != -1;
I think the better way to make this change is to have the caller put the
root frame in mDirtyRoots *before* marking it dirty, when it's not
already dirty/has-dirty-children. (Your way fires the "wasDirty lied"
warning a lot, I'd think.)
r+sr=dbaron with those issues fixed
Attachment #262335 -
Flags: superreview+
Attachment #262335 -
Flags: review?(dbaron)
Attachment #262335 -
Flags: review+
|
|
Assignee | |
Comment 26•18 years ago
|
||
(In reply to comment #25)
> (From update of attachment 262335 [details] [diff] [review])
> Did you mean to call DoVerifyReflow from ResizeReflow? I'm not sure if
> anyone cares about the VerifyReflow code anymore, though. (Maybe we
> should, though.)
No; I could, but it's not really worth it at the moment.
I'm absolutely certain nobody uses VerifyReflow currently because without this patch, enabling it makes the browser crash on startup. I've revived it to the point where the browser mostly works with it on, but I'm getting way too many errors for it to be really useful.
We probably should care, though, because it could find some rather hard-to-spot bugs, especially if we combined it with fuzz testing. Shall I file a bug?
> +void
> +PresShell::DoReflow(nsIFrame* target) {
>
> Please put the opening brace for functions on its own line.
Okay.
> + NS_ENSURE_SUCCESS(rv, );
>
> I don't think that's portable. I think you need at least
>
> NS_ENSURE_SUCCESS(rv, /**/);
>
> but I'm not even sure if that's portable. I suggest just writing out
> if (NS_FAILED(rv)) {
> NS_NOTREACHED("CreateRenderingContext failure");
> return;
> }
Fine.
> In StyleChangeReflow, could you put the unusual case (!rootFrame) as an
> early return and make the main flow of the function not indented?
Okay.
> I suppose you know better than I would that UpdateViewProperties isn't
> needed anymore, but it makes sense since it was only that one codepath
> (StyleChangeReflow).
I don't think any preference changes can affect visibility, z-index, or position, so it shouldn't be an issue.
> But what about the calls to PositionFrameView for the root frame? I'm a
> little curious about the latter -- I thought doing that before reflow
> was important for view positioning to be efficient.
The start of PositionFrameView():
381 nsIFrame* parentFrame = aKidFrame->GetParent();
382 if (!aKidFrame->HasView() || !parentFrame)
383 return;
So calling PositionFrameView on the root frame isn't all that useful... it's always positioned at (0,0) anyway.
>
> // All the stuff we are creating that needs releasing
> - nsPresContext* cx;
> - nsIViewManager* vm;
> - nsIPresShell* sh;
> + nsCOMPtr<nsPresContext> cx;
> + nsCOMPtr<nsIViewManager> vm;
> + nsCOMPtr<nsIPresShell> sh;
>
> You don't need that comment anymore, and it's better to declare the
> variables at or right before first use.
Okay.
> - // XXXldb Set mIsReflowing (and unset it later)?
>
> This still applies (in ResizeReflow).
Okay, I'll restore the comment. (I'm not really sure what to do about it, though; we could move setting mIsReflowing into DoReflow, but I'm not really sure when it's useful.)
> + // XXX Kinda evil, but there really isn't any other way to deal with
> + // reflow requests targeting the root frame
> + PRBool wasDirty = f->GetParent() || mDirtyRoots.IndexOf(f) != -1;
>
> I think the better way to make this change is to have the caller put the
> root frame in mDirtyRoots *before* marking it dirty, when it's not
> already dirty/has-dirty-children. (Your way fires the "wasDirty lied"
> warning a lot, I'd think.)
Your solution would cause wasDirty lied assertions, not fix them. That assertion fires when wasDirty is false and the frame is in the mDirtyRoots array.
> r+sr=dbaron with those issues fixed
Okay; I'll wait for a response before I check in.
|
|
Assignee | |
Comment 27•18 years ago
|
||
Version updated to comments.
Attachment #262335 -
Attachment is obsolete: true
|
|
||
Comment 28•18 years ago
|
||
(In reply to comment #26)
> Your solution would cause wasDirty lied assertions, not fix them. That
> assertion fires when wasDirty is false and the frame is in the mDirtyRoots
> array.
OK, yours will work (I'd forgotten about the bit after the ||), but I still don't like the cost of scanning the mDirtyRoots array. What's wrong with mine?
|
|
Assignee | |
Comment 29•18 years ago
|
||
(In reply to comment #28)
> (In reply to comment #26)
> > Your solution would cause wasDirty lied assertions, not fix them. That
> > assertion fires when wasDirty is false and the frame is in the mDirtyRoots
> > array.
>
> OK, yours will work (I'd forgotten about the bit after the ||), but I still
> don't like the cost of scanning the mDirtyRoots array. What's wrong with mine?
Oh, I didn't quite follow your solution the first time I read it; I'll change it.
|
|
Assignee | |
Comment 30•18 years ago
|
||
I'll check this in once the tree reopens, whenever that is.
Attachment #262488 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 31•18 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 32•18 years ago
|
||
And backed out. Could someone test this on a mac to see what the issue is?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
|
|
Assignee | |
Comment 33•18 years ago
|
||
(In reply to comment #32)
> And backed out. Could someone test this on a mac to see what the issue is?
Relanded with a fix to always pass the root frame to CreateRenderingContext (I'm not sure why that made it crash on the mac only.)
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
|
||
Updated•18 years ago
|
Flags: in-testsuite-
|
|
Assignee | |
Comment 34•18 years ago
|
||
Surprisingly, this had a large impact on tdhtml (in a good way). At an extreme:
Before:
layers1: 3959 3775,3959,4055,4057,3947
After:
layers1: 3051 2994,3127,3045,3059,3031
I'm pretty sure the cause of this improvement is the removal of the call to IsDragInProgress, since that's the only part of this patch that's on a critical path for DHTML. If so, that's a pretty impressive improvement for such a tiny change. Maybe we should be looking for other improvements along that critical path.
|
|
||
Comment 35•18 years ago
|
||
Odd. I'd have noticed if that were popping up in profiles, and I don't _think_ it had been. In fact, I just rechecked and it's not.
I wonder whether we were in fact hitting one of the other reflow methods in there due to a bug in the test suite itself (when it starts timing or something).... But I just tried profiling the time we actually measure in that test, and that doesn't seem to be it either... So it's really odd that this had any effect.
On a separate note, can't IsDragInProgress be removed altogether now? It seems to be unused.
|
|
||
Comment 36•18 years ago
|
||
It actually seriously worries me that this had that much effect on Tdhtml... it really should not have. Which makes me suspect something is wrong somewhere.
|
|
Assignee | |
Comment 37•18 years ago
|
||
I have a feeling that the bug in ResizeReflow is the cause of the decrease in tdhtml, although I'm not sure. Oops :(
Attachment #262791 -
Flags: review?(dbaron)
|
|
Assignee | |
Updated•18 years ago
|
Attachment #262791 -
Attachment is patch: true
|
|
||
Comment 38•18 years ago
|
||
Ah, yes. That bug, if hit, would cause us to not do any incremental reflow at all, I think. ;)
|
|
||
Comment 39•18 years ago
|
||
Comment on attachment 262791 [details] [diff] [review]
Followup patch
r+sr=dbaron
Attachment #262791 -
Flags: superreview+
Attachment #262791 -
Flags: review?(dbaron)
Attachment #262791 -
Flags: review+
|
|
Assignee | |
Comment 40•18 years ago
|
||
Followup checked in.
You need to log in
before you can comment on or make changes to this bug.
Description
•