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

RESOLVED FIXED in mozilla2.0b4

Status

()

P2
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: bernd_mozilla, Assigned: dbaron)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {testcase})

Trunk
mozilla2.0b4
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(36 attachments, 3 obsolete attachments)

377 bytes, text/html
Details
382 bytes, text/html
Details
2.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.99 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
929 bytes, patch
roc
: review+
Details | Diff | Splinter Review
10.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.97 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.81 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.85 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.06 KB, patch
Details | Diff | Splinter Review
1.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
1000 bytes, patch
roc
: review+
Details | Diff | Splinter Review
10.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.98 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.45 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
30.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
Posted file 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.
CC'ing dbaron, since he wrote the assertion (and the float manager code, more generally).
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);
Mmm, inline reflow. dbaron, do you want it or shall I take it?
dbaron agreed to take this.
Assignee: nobody → dbaron
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.
... 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.
blocking2.0: ? → final+
Priority: -- → P2
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.
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

9 years ago
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.)
Blocks: 574958
No longer blocks: 574958
Blocks: 576890
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.
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.
Attachment #459204 - Flags: review?(roc)
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.)
Attachment #459211 - Flags: review?(roc)
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.
Attachment #459212 - Flags: review?(roc)
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).
Attachment #459214 - Flags: review?(roc)
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.
Attachment #459217 - Flags: review?(roc)
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.
Attachment #459218 - Flags: review?(roc)
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.
Attachment #459219 - Flags: review?(roc)
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.
Attachment #459221 - Flags: review?(roc)
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).
Attachment #459223 - Flags: review?(roc)
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.
Attachment #459225 - Flags: review?(roc)
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.
Attachment #459226 - Flags: review?(roc)
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.)
Attachment #459228 - Flags: review?(roc)
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.
Attachment #459231 - Flags: review?(roc)
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.
Attachment #459234 - Flags: review?(roc)
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).
Attachment #459235 - Flags: review?(roc)
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.
Attachment #459237 - Flags: review?(roc)
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.
Attachment #459241 - Flags: review?(roc)
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.
Attachment #459242 - Flags: review?(roc)
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).
Attachment #459245 - Flags: review?(roc)
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).
Attachment #459248 - Flags: review?(roc)
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.
Attachment #459250 - Flags: review?(roc)
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.
Attachment #459252 - Flags: review?(roc)
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.
Attachment #459255 - Flags: review?(roc)
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).
Attachment #459256 - Flags: review?(roc)
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).
Attachment #459259 - Flags: review?(roc)
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.
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.
Attachment #459263 - Flags: review?(roc)
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...)
Attachment #459265 - Flags: review?(roc)
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.)
Attachment #459266 - Flags: review?(roc)
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.
Attachment #459267 - Flags: review?(roc)
+  // FIXME: Should give AutoRestore a getter for the value to avoid this.
+  const nscoord saveY = mY;

Would you mind just doing this?
Comment on attachment 459219 [details] [diff] [review]
patch 7 (auto-restore-mY): use AutoRestore<nscoord>

r+ anyway I guess
Attachment #459219 - Flags: review?(roc) → review+
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?
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).
+  // 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.
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
Attachment #459250 - Attachment is obsolete: true
Attachment #463320 - Flags: review?(roc)
Attachment #459250 - Flags: review?(roc)
(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.
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".
I'd like to see a new patch #12.
(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 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)?
Attachment #459237 - Flags: review?(roc) → review+
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
Attachment #459241 - Flags: review?(roc) → review+
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?
(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).
(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 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!
Attachment #459256 - Flags: review?(roc) → review+
(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.
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=8ab7ef79b673 (all but the bottom 3 changesets there)

Still need to file a few followups...
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Depends on: 585598
Depends on: 586806
Depends on: 586973
Depends on: 595758

Updated

9 years ago
Depends on: 601115
Depends on: 621424

Updated

8 years ago
Depends on: 647792

Comment 68

8 years ago
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.
Depends on: 772306
You need to log in before you can comment on or make changes to this bug.