563584 - ###!!! ASSERTION: float manager state should match saved state

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[Bernd]

Attachment: test case

print preview the attached test case

please notice that the assert also leads to a missing div in the printout, you should see A B C D but you will see only A B D.

Comment 1

[Daniel Holbert [:dholbert]]

CC'ing dbaron, since he wrote the assertion (and the float manager code, more generally).

Comment 2

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

The problem is more likely around nsBlockFrame::DoReflowInlineFrames, though.  See the comment:

        // We should never hit this case if we've placed floats on the
        // line; if we have, then the GetFloatAvailableSpace call is wrong
        // and needs to happen after the caller pops the space manager
        // state.
        aState.mFloatManager->AssertStateMatches(aFloatStateBeforeLine);

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mmm, inline reflow. dbaron, do you want it or shall I take it?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron agreed to take this.

Comment 5

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

The assertion is failing because we've added a float to the float manager:  the saved state has 3 floats but the float manager now has 4.  However, the new mFloats[3] is the same float as mFloats[2], though placed at a different Y coordinate.

Comment 6

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

... which is in turn happening because a placeholder frame that has in fact placed a float in the float manager (although a second time!) is returning break-*before* status.  Returning break-before status when it's placed a float seems broken.

Comment 7

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

I currently have three sets of patches in my queue related to this bug:

set 1:
restore-float-manager-state

This patch is the straightforward fix for this assertion, but it doesn't fix the massive layout problems on the testcase here, and doesn't fix bug 563009.

set 2:
remove-force-fit
remove-most-of-can-place-float
reflow-float-with-vertical-position
columnset-top-of-page
set-is-top-of-page
add-notreached-unconstrained-incomplete
replace-can-place-float-guts
auto-restore-mY

This set of patches fixes the massive layout bugs on this testcase, but causes a slightly related testcase to hang.  It's also a prerequisite for the third set of patches (whereas sets 1 and 2 are independent of each other, although swapping the order requires a little merging since the diffs are very close).  This set of patches is designed to fix three intertangled architectural issues with float splitting:
 * the notion of "force fit" is wrong (with errors in both directions):  whether we force a float to fit should not be related to whether we placed something on the line, but rather whether the float is against the top of the page.
 * the float placement loop in nsBlockReflowState::FlowAndPlaceFloat doesn't make sense in the presence of float splitting; for height cases, instead of moving to the next band, we should split the float
 * the order of float placement and float splitting is backwards:  we figure out at what vertical position to split the float, then decide where to put it.  (This is the cause of the massive layout bugs on this bug's testcase:  we decide to push float "C" down after deciding at what horizontal point it should be split.)

The patches within set 2 are not intended to leave things in a sensible state between patches, but I think that's necessary because of the interdependencies.

set 3:
remove-line-reflow-status-init
float-manager-record-truncated
track-float-continuations
dont-propagate-float-reflow-status
remove-reflow-status-params
dont-mark-overflow-incomplete
check-prev-siblings

The third set of patches is based on the second set (and removes the code in the first set, since it makes it unnecessary).  This changes our code to avoid propagating float break statuses in the same place as line break statuses, and also fixes a collection of serious layout bugs by allowing the entirety of a float to be pushed to a later page (rather than requiring some part to be placed on the current page).  This set is an alternative fix for this assertion and also fixes bug 563009.


This is a lot of stuff, and I'm confident it'll break something.  I still need to figure out the hang caused by set 2 before posting anything here, though.

Also, the above descriptions are pretty vague; I'll try to write better ones at some point, but I wanted to write something more down before I forgot too much.

Comment 8

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

Attachment: related test case (hang from patch set 2)

Comment 9

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

I think the solution to the hang might be removing PushTruncatedPlaceholderLine (and related code), but I think I should talk to fantasai about why that function exists in the first place.

Comment 10

[fantasai]

fantasai probably doesn't have a clue. I remember not understanding why it was there when I was modifying things near it before. :/

(Btw, I posted my float-manager patch to bug 551425, in case that's useful info.)

Comment 11

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

I'm going to attach the patch series shortly; while it could certainly use more tests, the main way I can think to write additional tests is by going through the code and writing tests for the things in it, and I really don't have the energy to write that many new tests; I still have quite a few new tests here, though.

Comment 12

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

Attachment: patch 1 (restore-float-manager-state): the straightforward fix for this assertion, but without fixing the other related bugs

For the record, here's the actual simple fix for the assertion as the bottom patch.  Later patches remove this fix, but I believe it is the correct fix for the assertion itself.  It just doesn't fix any of the other problems.

Comment 13

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

Attachment: patch 2 (remove-force-fit): Remove aForceFit/aRelaxHeightConstraint

This removes aForceFit (and the corresponding aRelaxHeightConstraint) from float handling.  The concept of forcing a float to fit in a vertical space that's too small for it should be handled adequately with nsHTMLReflowState::mFlags::mIsTopOfPage, though there are patches later in that queue to fix bugs in that handling.

This is really tightly integrated with the next patch, which removes the only caller.  (Perhaps bad patch ordering, but oh well.  I think there might have been a reason, but maybe not.)

Comment 14

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

Attachment: patch 3 (remove-most-of-can-place-float): make CanPlaceFloat very simple

This removes almost all of CanPlaceFloat, leaving only the bits related to checking that the width of the float fits.

The old complexity was largely related to the fact that we used to not enforce the rule that the top of a float can't be higher than the top of any earlier floats (we didn't enforce it in the opposite-side case), so this code existed to handle that.  It also handled some pagination issues; that handling will be replaced with much simpler code in later patches.

This reorganization is a prerequisite for the next patch, which fixes the massive layout problems on the testcases in this bug and the dependent bug.

Comment 15

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

Attachment: patch 4 (reflow-float-with-vertical-position): reflow after choosing vertical position

This is the core of the fix for the huge layout problems on the testcase in this bug.

The fundamental problem with the old code was that the algorithm in nsBlockReflowState::FlowAndPlaceFloat was:
 * reflow the float (which does splitting based on available height)
 * figure out where to place the float
This is fundamentally broken when the second part has to move the float down due to the presence of other floats.

The previous patch allows swapping the order, since we only need width information to call CanPlaceFloat.  So here we switch to the correct algorithm, where we figure out the float's vertical position and *then* reflow it (which decides where to split it).

Comment 16

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

Attachment: patch 5 (set-is-top-of-page): Set isTopOfPage correctly in reflow state for floats

This patch fixes our setting of isTopOfPage in the reflow state, to replace the needed parts of aForceFit.  The basic idea of isTopOfPage is that it should be set to true in cases where failing to place something would cause an infinite loop of pushing to the next column/page.  This means that when a float is pushed down due to the presence of other floats, it should no longer have isTopOfPage set.

Comment 17

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

Attachment: patch 6 (float-push-in-columns): in columns, we sometimes need to fail after reflow

Since we don't split floats inside columns, in the columns case, we sometimes need decide after reflow that a float doesn't fit.  This replaces the remaining part of what was removed in patch 2... at least once the patch queue is done.

Comment 18

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

Attachment: patch 7 (auto-restore-mY): use AutoRestore<nscoord>

This fixes a tricky piece of FlowAndPlaceFloat that was bugging me while writing the patches underneath, and uses AutoRestore<nscoord> instead of manually restoring mY.

Comment 19

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

Attachment: patch 8 (remove-line-reflow-status-init): remove unneeded initialization of lineReflowStatus

This removes an unneeded and confusing (when I was trying to figure out who used LINE_REFLOW_STATUS_NEXT_BAND) initialization of lineReflowStatus.  lineReflowStatus will always be initialized by the code below, and NEXT_BAND is not a particularly sensible default state, so better not to initialize it.

Comment 20

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

Attachment: patch 9 (float-manager-record-truncated): allow nsFloatManager to record that a float was pushed past a break

The second big chunk of patches in this series involve untangling float breaking from line reflow status.  This requires being able to push the first-in-flow of a float to the next page/column without moving its placeholder.

This patch adds a new concept to the float manager:  the idea that a left or right float has been pushed past a page/column break and entirely out of this float manager.  Note that this is only used when the first-in-flow is pushed (but a later patch will introduce similar flags for a split).  This is used both for clearing and for determining the lowest float top (whereas a split affects only clearing but not the lowest float top).

Comment 21

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

Attachment: patch 10 (optimize-floats-split-across-break): add similar float-splitting flags to float manager

This adds a similar pair of flags to indicate that floats have been split across a break.  This makes ClearContinues much cheaper (which is important since the previous patch made it used more).  These flags affect clearing but not lowest-top calculations.

Comment 22

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

Attachment: patch 11 (dont-clear-pushed-in-compute-final-size): ignore effects of patch 9's flags in ComputeFinalSize

There's one caller of ClearFloats that doesn't want the effects of patches 9 and 10 (saying that clear goes to nscoord_MAX):  ComputeFinalSize for blocks that establish block formatting contexts.  In that case, we want a normal size.

It just occurred to me when writing this description that a better behavior here would probably be to make the block fill all the space up to the break.  Oh well; that should be a followup bug at this point.

Comment 23

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

Attachment: patch 12 (track-float-continuations): add a state bit to floats that are pushed to the next column/page

This is yet another prerequisite for allowing first-in-flow floats to be pushed to another column/page.

We can no longer determine whether a float was pushed to the next column/page by seeing if it has a prev-in-flow, since we'll be pushing some first-in-flows.  So this adds a state bit to indicate that a float was pushed, and replaces the checks that previously checked for a prev-in-flow with checks for that bit.  (Maybe "float continuations" isn't the best name anymore?  Hmmm.)

Comment 24

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

Attachment: patch 13 (keep-pending-float-continuations-separate): keep float continuations on their own frame list

The current code keeps float continuations on their own frame list during reflow, and then tacks them on to the end of the block's float list at the end of reflow, to be pulled by the next block.  However, in cases where the block gets reflowed twice before its next-in-flow gets reflowed, this doesn't allow us to reliably distinguish between floats that need to be pulled by the next block and floats that we pulled from our previous block.  (I don't think this confusion is new to this patch series, actually, but I think these patches did make the confusion more likely to happen.  I could be wrong about this, though.)

So this makes the list separation permanent, and keeps the float continuations on a separate list (in a frame property) until they're actually pulled by the next sibling.

Comment 25

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

Attachment: patch 14 (reflow-float-conts-with-flow-and-place-float): reflow float continuations with FlowAndPlaceFloat instead of AddFloat

This is a prerequisite for one of the later patches (I think the one immediately following, actually).  It doesn't particularly depend on any of the earlier ones; this code probably should have been done this way from the beginning.

This changes the reflow of float continuations to use FlowAndPlaceFloat rather than AddFloat.  The difference (when AddFloat is called with a null aLineLayout, which is the case only for float continuations) is that AddFloat:
 * handles translation from being inside a descendant of the float's containing block (which is not relevant to float continuations since they are reflowed directly by their containing block)
 * handles the possibility that the float doesn't fit next to the text prior to its placeholder (also not relevant since no text has been placed yet when reflowing float continuations)

The next patch adds more code to AddFloat, and it simplifies things when AddFloat does not have to deal with having a null aLineLayout.

Comment 26

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

Attachment: patch 15 (steal-float-before-reflow): Steal float from its parent before reflowing it

This is another prerequisite for allowing first-in-flow floats to be pushed:  in AddFloat, we need to steal a float back from its current parent (it could be on the float list of one of our later in-flows, or on our own float continuations list).

This patch also restores the invariant that all floats on our float list have us as the parent (which was previously temporarily broken by DrainFloatContinuations).

Comment 27

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

Attachment: patch 16 (save-float-next-sibling-when-reflowing-float-continuations)

This is another prerequisite for pushing entire floats; we sometimes need to push the entire float more than once, when we have a lot of floats that fill the column or page.  Pushing the entire float changes its next sibling, so we need to be a little more careful with sibling pointers in ReflowFloatContinuations.

Comment 28

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

Attachment: patch 17 (dont-propagate-float-reflow-status): separate pushing of floats from pushing of lines

This is, in a sense, the "main patch" on this bug.  This separates the pushing of floats from the pushing of lines, and allows placing first-in-flow floats on the float continuations list.

This also removes the code from patch 1.

It leaves a bunch of residual parameters to functions that are removed in patch 20.

The changes to the handling of below-current-line floats are in the next patch, though.

Comment 29

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

Attachment: patch 18 (dont-propagate-float-reflow-status-below-current-line-floats): ditto, for below-current-line floats

This changes PlaceBelowCurrentLineFloats to handle FlowAndPlaceFloat pushing floats to the next column/page, and removes its propagation of float reflow status into line reflow status.

Comment 30

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

Attachment: patch 19 (rename-push-truncated-line): rename PushTruncatedPlaceholderLine

This renames PushTruncatedPlaceholderLine to PushTruncatedLine since the remaining reason for its existence has nothing to do with placeholders (we use it in DoReflowInlineFrames when the line needs to be pushed to the next column/page because a LINE_REFLOW_REDO_NEXT_BAND pushed it down past the end of the column/page).

Comment 31

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

Attachment: patch 20 (remove-reflow-status-params): remove residual aReflowStatus params

This removes the residual aReflowStatus parameters left after patch 17.

Comment 32

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

Attachment: patch 21 (check-prev-siblings): Add sibling checks to VerifyList

This adds sibling checks to VerifyList (DEBUG-only code), which I found useful when debugging something (I've forgotten what, though it's probably in the history of my patch queue if you really care).

Comment 33

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

Attachment: patch 22 (pass-correct-height-to-float-manager)

This passes the correct height to mFloatManager->GetFlowArea().  The old code was nonsense, and probably contributed to the horrible layout of the testcases in this bug and the dependent bug.

Comment 34

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

Attachment: patch 23 (adjust-assertions): adjust reftest/crashtest assertion counts of nscoord_MAX assertions

Changes in all sorts of math can cause tweaks to assertion counts for nscoord_MAX related assertions.  I filed bug 575011 on this mess.  This patch has the necessary manifest adjustments to deal with the changes in this patch.

Comment 35

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

Attachment: patch 24 (line-bit-for-pushed-floats): add bit to line boxes that have pushed floats

This is a prerequisite for the next patch.  It adds a bit to line boxes that contain placeholders for floats that were pushed to the next column/page.

Since the child count seemed to be veering into dangerously small territory, I increased the size of line boxes by a word.

Comment 36

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

Attachment: patch 25 (reflow-more-lines-with-floats): always reflow lines that had a pushed float

This makes us reflow any line that had a float pushed from it or has floats in it when we're in a constrained height situation, since we need to ensure that we reflow the floats in order to push/pull pieces of them or whole floats.

I think this is also needed to ensure ordering invariants in float continuations lists (since it makes us pull all the floats back, and then push them again).  This is sort of ugly.

I think this was needed even before this patch series (though the pushed-floats part was not).

Comment 37

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

Attachment: patch 26 (dont-split-when-not-paginating): don't try pushing floats given an unconstrained height

When we have an unconstrained height, we shouldn't try to push floats.  This is needed here because of the effects of my use of nscoord_MAX results from ClearFloats in patches 9 and 10 (a design decision that I'm having some second thoughts about).

Comment 38

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

Attachment: patch 27 (not-top-when-clearing): add tests that we don't do mIsTopOfPage behavior when clearing

There was originally a patch here, but it turned out to be unnecessary (although I'm not entirely sure why), so I'm adding tests to make sure it stays unnecessary.

Comment 39

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

Attachment: patch 28 (comment-is-top-of-page): add comments explaining mIsTopOfPage

This adds comments to explain my understanding of mIsTopOfPage.

Comment 40

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

Attachment: patch 29 (dont-swap-avail-height-to-unconstrained)

This is probably the ugliest hack related to the nscoord_MAX clear results from patches 9 and 10.  This ensures that we don't turn a constrained height into an unconstrained one due to nscoord_MAX clearance.  Doing that can cause us to position stuff at nscoord_MAX on the current page instead of pushing it to the next.

Comment 41

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

Attachment: patch 30 (set-incomplete-for-clearance-case)

This fixes a bad regression from bug 489477.  If we have continuations, and we're going to reflow again for clearance, we shouldn't report that we're complete, because then we'll destroy those continuations, which completely destroys the frames for anything entirely within those continuations.

(This should probably have better tests than just an assertion in a crashtest...)

Comment 42

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

Attachment: patch 31 (float-split-reftest): reftests

This adds reftests for the cases in this bug and some derived cases, testing both lack of assertions and correct layout.

It also marks the bug 507510 tests as passing... although bug 507510 is actually not fixed in the simple case.  (This bug fixes it in the multi-page case, but not the single-page case.)

Comment 43

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

Attachment: patch 32 (let-availableHeight-be-negative)

This is another patch that I think is required as a result of the nscoord_MAX stuff from patches 9 and 10, although I think this change is pretty reasonable.  This stops clamping availableHeight to a minimum of 0 and allows it to be negative.  This means that nsHTMLReflowState::SetTruncated won't claim that a zero-height element fits just fine when the available height (due to clears) was actually negative.

This fixes the testcases for bug 377204, which I believe is fixed by the changes in this bug.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // FIXME: Should give AutoRestore a getter for the value to avoid this.
+  const nscoord saveY = mY;

Would you mind just doing this?

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459219 [details] [diff] [review]
patch 7 (auto-restore-mY): use AutoRestore<nscoord>

r+ anyway I guess

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm confused about part 9. You added some tests, but it seems to me there's no behavior change since SetPushedLeft/RightFloatPastBreak never gets called in this patch?

Comment 47

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

The callers are in later patches, yes.

In theory, there shouldn't be any behavior change throughout the patch series on the issues dealt with by that patch (though I suspect there are); they just need to be handled differently now that we're not pushing lines along with the floats.  I figured it made more sense to put the tests in the patch whose code they were intending to exercise than to put them all at the end (mainly so I could track which concepts I'd written tests for).

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // FIXME: Why would the continuation of a float be associated with a line?
   if (!aFC || aFC->mFloat->GetPrevInFlow())
     return PR_TRUE;

Float continuations used to have their own placeholders. Fantasai changed that, but I guess this code still needs to be updated. Maybe you should remove that GetPrevInFlow check and assert that aFC->mBlock doesn't have NS_FRAME_IS_FLOAT_CONTINUATION?

+    if (f) {
+      nsFrameList floatContinuations = prevBlock->mFloats.RemoveFramesAfter(f);
+      mFloats.InsertFrames(nsnull, nsnull, floatContinuations);
+    } else {
+      // Move all of prevBlock->mFloats.
+      mFloats.InsertFrames(nsnull, nsnull, prevBlock->mFloats);
+    }

Why not just make RemoveFramesAfter(nsnull) remove the entire list, so we don't need two paths here or up above?

> (Maybe "float continuations" isn't the best name anymore?  Hmmm.)

Yes, it's not. We should change this, but I'm not sure what to. I guess you would want to change that as a separate patch at the end to avoid much patch updating.

Comment 49

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

Attachment: patch 22 (pass-correct-height-to-float-manager)

This passes the correct height to mFloatManager->GetFlowArea().  The old code
was nonsense, and probably contributed to the horrible layout of the testcases
in this bug and the dependent bug.

This is revised from the previous version to only change values that aren't nscoord_MAX; we shouldn't subtract from nscoord_MAX.  Doing so was causing large numbers of the "bad value" assertion in this part of nsFloatManager::GetFlowArea:
    if (bottom < top || bottom > nscoord_MAX) {
      NS_WARNING("bad value");
      bottom = nscoord_MAX;
    }
when loading articles on www.washingtonpost.com

Comment 50

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

(In reply to comment #48)
> +  // FIXME: Why would the continuation of a float be associated with a line?
>    if (!aFC || aFC->mFloat->GetPrevInFlow())
>      return PR_TRUE;
> 
> Float continuations used to have their own placeholders. Fantasai changed that,
> but I guess this code still needs to be updated. Maybe you should remove that
> GetPrevInFlow check and assert that aFC->mBlock doesn't have
> NS_FRAME_IS_FLOAT_CONTINUATION?

Sure.

> 
> +    if (f) {
> +      nsFrameList floatContinuations =
> prevBlock->mFloats.RemoveFramesAfter(f);
> +      mFloats.InsertFrames(nsnull, nsnull, floatContinuations);
> +    } else {
> +      // Move all of prevBlock->mFloats.
> +      mFloats.InsertFrames(nsnull, nsnull, prevBlock->mFloats);
> +    }
> 
> Why not just make RemoveFramesAfter(nsnull) remove the entire list, so we don't
> need two paths here or up above?

I'm a little unsure about this; I could also see cases where a caller would expect RemoveFramesAfter(nsnull) to remove nothing, so I might prefer leaving this as-is so that anybody who might pass null actually has to decide which they meant.

> > (Maybe "float continuations" isn't the best name anymore?  Hmmm.)
> 
> Yes, it's not. We should change this, but I'm not sure what to. I guess you
> would want to change that as a separate patch at the end to avoid much patch
> updating.

Maybe "pushed floats"?  I'm not crazy about it, though.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

As we discussed on IRC, having RemoveFramesAfter(nsnull) remove the entire list is consistent with InsertFrames(nsnull) inserting frames at the front of the list --- a null prevSibling means the start of the list.

"pushed floats" sounds better than anything I came up with, and I think it's better than "float continuations".

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to see a new patch #12.

Comment 53

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

(In reply to comment #50)
> I'm a little unsure about this; I could also see cases where a caller would
> expect RemoveFramesAfter(nsnull) to remove nothing, so I might prefer leaving
> this as-is so that anybody who might pass null actually has to decide which
> they meant.

roc convinced me otherwise, so revised patches coming up.

That said, this code all goes away in keep-pending-float-continuations-separate.  (Probably I should have written those patches in the reverse order...)

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459237 [details] [diff] [review]
patch 16 (save-float-next-sibling-when-reflowing-float-continuations)

This is fine, but just to clarify: once we've pushed a float, we're going to push all subsequent floats (right?), so in fact could we handle this a different way, where after the first float is pushed we push all the subsequent floats directly without going through FlowAndPlaceFloat and without changing the next-sibling pointers (except for the last one)?

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459241 [details] [diff] [review]
patch 17 (dont-propagate-float-reflow-status): separate pushing of floats from pushing of lines

This is very nice

Comment 56

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

Attachment: patch 11.5 (frame-list-remove-null): make nsFrameList::RemoveFramesAfter(null) work

This is untested, because the code that uses in in patch 12 is removed in patch 13.

Comment 57

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

Attachment: patch 12 (track-float-continuations): add a state bit to floats that are pushed to the next column/page

Revised per above comments

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459255 [details] [diff] [review]
patch 24 (line-bit-for-pushed-floats): add bit to line boxes that have pushed floats

We never clear this flag. Can we clear it at the start of ReflowLine maybe?

Comment 59

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

(In reply to comment #54)
> Comment on attachment 459237 [details] [diff] [review]
> patch 16 (save-float-next-sibling-when-reflowing-float-continuations)
> 
> This is fine, but just to clarify: once we've pushed a float, we're going to
> push all subsequent floats (right?), so in fact could we handle this a
> different way, where after the first float is pushed we push all the subsequent
> floats directly without going through FlowAndPlaceFloat and without changing
> the next-sibling pointers (except for the last one)?

Yes, we are going to push all subsequent floats (although the things that cause that are a little bit too indirect, I'll admit).  So yes, we could do it as you describe, though if we did we'd also need to teach AddFloat to skip the pushed floats in that case (or it would be pointless), but not in the case where a float was pushed in a previous reflow and might need to be pulled now (which means there would be another piece of state to track somewhere).

Comment 60

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

(In reply to comment #58)
> We never clear this flag. Can we clear it at the start of ReflowLine maybe?

I think so.  I think I meant to do something like that.

I'll revise the patch to do so, and then once I've made all the revisions, rerun all the tests and see if it breaks anything.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 459256 [details] [diff] [review]
patch 25 (reflow-more-lines-with-floats): always reflow lines that had a pushed float

I can't say I'm super-happy about always descending into blocks in a constrained-height situation where lines have moved, but OK. Maybe we could add something like NS_BLOCK_HAS_CLEAR_CHILDREN to track whether there are floats in descendants.

+    // FIXME: What about a deltaY or height change that forces us to
+    // push lines?  Why does that work?

I had a strong feeling we had code to detect this, but I can't find it!

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #59)
> Yes, we are going to push all subsequent floats (although the things that cause
> that are a little bit too indirect, I'll admit).  So yes, we could do it as you
> describe, though if we did we'd also need to teach AddFloat to skip the pushed
> floats in that case (or it would be pointless), but not in the case where a
> float was pushed in a previous reflow and might need to be pulled now (which
> means there would be another piece of state to track somewhere).

That's fine, I don't think we should change your patch, I just wanted to make sure my understanding was correct.

Comment 63

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

Attachment: patch 24 (line-bit-for-pushed-floats): add bit to line boxes that have pushed floats

revised per above comments

Comment 64

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

Attachment: patch 33 (rename-float-continuations)

Comment 65

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

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8ab7ef79b673 (all but the bottom 3 changesets there)

Still need to file a few followups...

Comment 66

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

More assertion count tweaking:
http://hg.mozilla.org/mozilla-central/rev/fb71b8680451

Comment 67

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

Patch 33 was missing a tiny bit of renaming; I just fixed that:
http://hg.mozilla.org/mozilla-central/rev/67a1e6b2a00f

Comment 68

[Thomas Ahlblom]

Patch 22, changeset 66c78df18e50 breaks the layout described in bug 658376, but I'm not sure whether this is bad page coding or a bug in Firefox.