[FIX]Gut InitialReflow

RESOLVED FIXED in mozilla1.9alpha5

Status

()

defect
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.9alpha5
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

I think we should go ahead and remove the "reflow synchronously" stuff from InitialReflow().  I don't really see a good reason to do it, and it _might_ be hurting performance.  Hard to tell.

Any obvious objections?
No objections; I was planning on doing it myself at some point.  I have a few other cleanups planned for InitialReflow, but they're independent.

You're going to have to add a FlushPendingNotifications call to nsPrintEngine because it expects InitialReflow to actually reflow everything.
Yeah, I figured I'd need that.

For what it's worth, I have this in my tree, but I'm seeing some weirdness in the ordering of onload and final reflow that I need to debug before posting it.

Of course I also need to check that this weirdness is not there without the patch...
(In reply to comment #2)
> Yeah, I figured I'd need that.
> 
> For what it's worth, I have this in my tree, but I'm seeing some weirdness in
> the ordering of onload and final reflow that I need to debug before posting it.
> 
> Of course I also need to check that this weirdness is not there without the
> patch...

Weirdness?  I don't really know much about the content side of this code, but do we explicitly flush reflow before we fire onload?  Otherwise, there isn't any good reason why we would.

(If that's the case, it could explain the tp2 regression from your stylesheet patch.)
> but do we explicitly flush reflow before we fire onload?

No, but in theory we block firing onload until after reflow commands posted during load have been processed.  See mOnloadBlocked and its management in the presshell.
Er, I meant mDocumentOnloadBlocked
OK, so the onload problem happened because we did a post-reflow callback that involved a flush partway through the load, when there were dirty roots left after the reflow.  As a result, there were no dirty reflow roots by the time DoneRemovingDirtyRoots was called, so it unblocked the onload.  But the reflow event that had gotten posted right before that was still pending, so no more reflow events got posted for subsequent content appends and more importantly onload didn't get blocked by the reflows for those content appends.

This may also explain the height differences between Tp and Tp2 in the log: onload might just be firing too early in Tp.  Of course that makes those numbers kinda useless, if that's what's happening... :(

Filed bug 379093 on resolving this in general.  Once that's done, we can try doing this and seeing what effect it has, but until that bug is fixed I just don't trust our Tp(2) numbers.
Depends on: 379093
Posted patch Possible patch (obsolete) — Splinter Review
This includes the patch for bug 379093 for now.  I'll post an update once that lands.
Posted patch Proposed patch (obsolete) — Splinter Review
Summary of changes:

1)  Make InitialReflow just post a reflow event.
2)  Change PresShell::DoReflow to not do arithmetic on NS_UNCONSTRAINEDSIZE
3)  Change the assert about size changing to allow the size of the root to
    change during DoReflow if the size coming in was unconstrained.
4)  Move the code for setting the size on the prescontext into DoReflow, so it
    happens any time DoReflow is called on an unconstrained-size root.
5)  Random comment fix.
Attachment #263095 - Attachment is obsolete: true
Attachment #263269 - Flags: superreview?(dbaron)
Attachment #263269 - Flags: review?(dbaron)
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha5
Comment on attachment 263269 [details] [diff] [review]
Proposed patch

r+sr=dbaron, but Eli should probably review this as well
Attachment #263269 - Flags: superreview?(dbaron)
Attachment #263269 - Flags: superreview+
Attachment #263269 - Flags: review?(sharparrow1)
Attachment #263269 - Flags: review?(dbaron)
Attachment #263269 - Flags: review+
Comment on attachment 263269 [details] [diff] [review]
Proposed patch

You forgot to change nsPrintEngine; that's going to break at the very least printing iframes.  Might want to test that before checking in.

A few other minor things:

1. You can get rid of the calls to WillCauseReflow/DidCauseReflow; FrameNeedsReflow will take care of posting a reflow event.

2. While you're touching this code, it's probably worth adding an NS_ENSURE_TRUE(!mDidInitialReflow), NS_ERROR_FAILURE); or something like that.
Attachment #263269 - Flags: review?(sharparrow1) → review+
> You forgot to change nsPrintEngine; 

Er... crap.  I managed to not diff all the relevant files.  :(  Complete diff coming up.

> 1. You can get rid of the calls to WillCauseReflow/DidCauseReflow;

Excellent.  Will do.

> 2. While you're touching this code, it's probably worth adding an
> NS_ENSURE_TRUE(!mDidInitialReflow), NS_ERROR_FAILURE); or something like
> that.

Actually... I seem to recall there being issues when that was tried in the past.  sicking might know better, since I think he was the one who tried it...  
Attachment #263269 - Attachment is obsolete: true
Attachment #263328 - Flags: superreview?(dbaron)
Attachment #263328 - Flags: review?(sharparrow1)
Attachment #263328 - Flags: review?(dbaron)
Attachment #263328 - Flags: review?(sharparrow1) → review+
Attachment #263328 - Flags: superreview?(dbaron)
Attachment #263328 - Flags: superreview+
Attachment #263328 - Flags: review?(dbaron)
Attachment #263328 - Flags: review+
Checked in.  This actually seemed to help Tp/Tp2 a bit across the board.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Blocks: 84582
Depends on: 380134
Depends on: 381512
This checkin appears to have broken the Linux Adobe reader plugin.  Attempting to view a PDF document with the plugin installed results in a browser hang.  Appears to be Linux only.  See bug 381512.
Depends on: 408656
You need to log in before you can comment on or make changes to this bug.