378975 - [FIX]Gut InitialReflow

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Eli Friedman]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Eli Friedman]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er, I meant mDocumentOnloadBlocked

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

This includes the patch for bug 379093 for now.  I'll post an update once that lands.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263269 [details] [diff] [review]
Proposed patch

r+sr=dbaron, but Eli should probably review this as well

Comment 10

[Eli Friedman]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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...  

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's try again, with all the pieces this time

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in.  This actually seemed to help Tp/Tp2 a bit across the board.

Comment 14

[Bill Gianopoulos [:WG9s]]

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.