Remove nsPlaceholderFrame continuations. This should make nsBlockFrame splitting less fragile.

Use overflow containers instead of shoving placeholders up and down the ancestor chain.

So I've run into a problem here. nsLineBox is used to store nsFloatCaches of all the floats with placeholders on the line. nsFloatCache stores two things: a pointer to the placeholder, and the region affected by the float in the space manager. This second piece of information is not stored anywhere else, and can't really be computed except during reflow because it includes margin-collapsing results. The patch I posted up to use overflow containers instead of moving continuations up and down the ancestor chain is still valid. But I'm starting to wonder if decoupling float reflow from nsLineBox &co is really a good idea.

Can we just get rid of the float cache? The floats themselves are in the block's float list and know their placeholders. We can attach the affected region as a frame property of the placeholder or the float itself.

Or better still, we could create a subclass of nsPlaceholderFrame for floats and store the affected region directly in the placeholder.

> Can we just get rid of the float cache? Problem with that is that RecoverFloats then has to dig through all the descendants of a line to find the placeholders. We could bubble up through the placeholder to find the ancestor that's a direct child of the block, but I'm not sure how that helps RecoverFloats... > create a subclass of nsPlaceholderFrame for floats and store the > affected region directly in the placeholder. Subclassing nsPlaceholderFrame to store the region won't help much if we get rid of placeholders for the float's continuations. :) What's going to store the region for the continuations?

(In reply to comment #5) > > Can we just get rid of the float cache? > > Problem with that is that RecoverFloats then has to dig through all the > descendants of a line to find the placeholders. We could bubble up through the > placeholder to find the ancestor that's a direct child of the block, but I'm > not sure how that helps RecoverFloats... What if we just store a bit in the linebox to indicate whether there are any float placeholders in the list? If that bit is set, then we crawl the line's frames looking for float placeholders. That sounds like it would work pretty well, to me. > > create a subclass of nsPlaceholderFrame for floats and store the > > affected region directly in the placeholder. > > Subclassing nsPlaceholderFrame to store the region won't help much if we get > rid of placeholders for the float's continuations. :) What's going to store the > region for the continuations? OK, store them as a property on the out-of-flow frame itself.

Note to self: Don't regress bug 153785

Comment on attachment 378410 [details] [diff] [review] Patch Part I: Use Overflow Containers Patch Part I reassigned to bug 503183

Patch Part II assigned to bug 499377

This patch removes most of the code for splitting floats. It's not a total cleanup, since I've left in a few hooks to remind me where to reattach float splitting.

This patch removes the dependencies on use of placeholders in various functions, switching them to accepting the out-of-flow frame directly.

This patch re-implements float splitting without splitting placeholders. Float continuations are stored at the front of the mFloats list; they are distinguished from non-continuations via GetPrevInFlow() rather than keeping them on a separate list.

This patch implements BR clearance across columns/pages. BR clearance is a pain compared to normal clearance, because it's applied to the line *before* the clearance rather than after... so we have to be careful not to lose track of it across continuation breaks.

Comment on attachment 388646 [details] [diff] [review] Patch Part III: float-unsplit This looks fine

+GK_ATOM(floatContinuationProperty, "FloatContinuationProperty") // nsFrameList* Why do we need this? We used to need to be able to get an arbitrary block's temporary overflow placeholder list, when we were pushing continuation placeholders up to ancestor blocks. But you've removed that already (yay!!) so now we don't need a property here. All we need is mFloatContinuations in nsBlockReflowState, and only code that already has access to the nsBlockReflowState needs access to it. + if (floatContinuations.NotEmpty()) + aState.mFloatContinuations.AppendFrames(nsnull, floatContinuations); This "if" isn't useful since AppendFrames will be fast enough with an empty floatContinuations, so let's not do this optimization. + if (floatContinuations.NotEmpty()) + mFloats.InsertFrames(nsnull, nsnull, floatContinuations); Same here + if (!prevBlock || prevBlock->mFloats.IsEmpty()) + return; The prevBlock->mFloats.IsEmpty() check is also unnecessary since the following loop will terminate instantly if the list is empty. + if (!nif) continue; Put the "continue" on its own line please + NS_ASSERTION(0, "Destroying float without removing from a child list."); aFloat->Destroy(); Use NS_ERROR and remove the following aFloat->Destroy call. + } + else { One line please + if (!aLineLayout || (mBelowCurrentLineFloats.IsEmpty() && + (aLineLayout->LineIsEmpty() || mBlock->ComputeFloatWidth(*this, + floatAvailableSpace, fc->mFloat) <= aAvailableWidth))) { Reformat please, something like if (!aLineLayout || (mBelowcurrentLineFloats.IsEmpty() && (aLineLayout->LineIsEmpty() || mBlock->ComputeFloatWidth(...) <= aAvailableWidth))) { + PRBool forceFit = !aLineLayout || + (IsAdjacentWithTop() && !aLineLayout->LineIsBreakable()); Hmmm. What happens if the first float continuation takes up all the available space? Should we be setting forceFit here only for the first float continuation? I guess not, since we'd be in big trouble if we moved the float continuation forward to the block next-in-flow (violating the invariant that in the steady state frame->GetParent()->GetNextInFlow() == frame->GetNextInFlow()->GetParent() ). Although I don't know how to fix them, I think we have some bugs here. By the way, could you configure things so that your diffs have -p and more lines of context, say 8?

Comment on attachment 388649 [details] [diff] [review] Patch Part VI: float-br-clear Seems like we need tests here. Overall, this is really impressive!!!

> Why do we need this? I didn't completely understand the "defense" fix in bug 359371, so I renamed instead of removing the continuation-tracking property. I'll remove it if you think we're ok. > One line please Did we change our preferred indentation style? The prevailing style so far seems to be two lines for } else {. > Should we be setting forceFit here only for the first float continuation? No, because continuations should always start at the top of the page. If we have problems fitting them its either because we've implemented variable-width pages/columns, or because it's auto-width and we need the shrink-to-fit algorithm to span pages. > Seems like we need tests here. Yeah, I'll definitely add some <br clear> tests before closing out this bug.

(In reply to comment #18) > > Why do we need this? > > I didn't completely understand the "defense" fix in bug 359371, so I renamed > instead of removing the continuation-tracking property. I'll remove it if you > think we're ok. Yes, please do. > > One line please > > Did we change our preferred indentation style? The prevailing style so far > seems to be two lines for } else {. Really? OK then never mind. > > Should we be setting forceFit here only for the first float continuation? > > No, because continuations should always start at the top of the page. If we > have problems fitting them its either because we've implemented variable-width > pages/columns, or because it's auto-width and we need the shrink-to-fit > algorithm to span pages. Hmm, OK.

Comment on attachment 390964 [details] [diff] [review] Patch Part V: float-continuations + if (state.mFloatContinuations.NotEmpty()) + mFloats.AppendFrames(nsnull, state.mFloatContinuations); Remove the "if" guard --- it's a not-needed optimization.

roc, the code asserts if I try to append an empty nsFrameList. What do you want me to do: remove the assert or keep the if-checks?

For what it's worth, I'd kept the asserts at least partly for symmetry with the nsIFrame* versions of the methods... I'm probably fine removing the asserts, though we'll have to then put null-checks in the framelist methods...

nsFrameList::AppendFrames already has a nullcheck ... but alright, leave your if condition in for now.

I'm pretty happy adding the null-checks. Once we get rid of the nsIFrame* setters, it'll be a hassle to keep track of when frame lists are empty or not, I think.

I think we should probably not land these patches until after we branch for 1.9.2. For 1.9.2 these patches have no real benefit, only risk.

Depends on if you want to switch nsBlockFrame to nsFrameList APIs within the 1.9.2 cycle. (bz and I were thinking we could have mFrames just point to the first frame in mLines so we don't need to rewrite nsBlockFrame to do it.) I suspect that will be easier to do if this patch lands first because it gets rid of the placeholder-continuations-must-stay-in-front dance.

I think I'm going to opt for leaving the if-checks in my code and not in nsFrameList. It's not a simple change because nsFrameList::Append/Insert returns Slices now, and we probably also want to audit all callers when we make that change. So I'll file a separate bug if we want to do that, but I won't make it part of this fix.

Same as the previous, with the if-checks added back in.

They're marked as failing because float breaking isn't enabled currently. Maybe we should re-enable it when this patch lands?

Why don't you use reftest-print instead of columns? I don't think we're ready to reenable float breaking yet.

Because it's harder. :) Okay, I'll make reftest-print tests. What else do you think needs to happen to reenable float breaking in columns?

> I didn't completely understand the "defense" fix in bug 359371, so I renamed > instead of removing the continuation-tracking property. I'll remove it if you > think we're ok. We are so not ok. StealFrame can get called when descendant placeholders are reflowed, and it can't find the frame if it's on the mFloatContinuations list. We do need to store these on the frame temporarily so that they can be found. (Crashtest for this already in tree.)

Need to call DrainOverflowLines (which can add floats to our mFloats list) before DrainFloatContinuations so that the latter can weed out float continuations that belong to our next-in-flow (and beyond) and order the mFloats list properly. (Crashtest already in the tree.)

This patch does two things: 1. It cleans up our code for RemoveFrame on floats a bit. 2. More importantly, it makes sure all overflow containers get removed when their prev-in-flow is removed. (Previously this triggered an assertion in our crashtests, but with the float-continuations patch it also triggers a crash.)

Comment on attachment 394920 [details] [diff] [review] Patch Part V mod 1: float-cont-stealframe Forgot to flag review on this one

Fixes an nsFloatCache leak introduced by Part V.

How about we add a float list method that allocates a cache object for a frame and appends it to the list?

Is something like mCurrentLineFloats.Append(mFloatCacheFreeList.Alloc(aFloat)); good enough? I'm hesitant to change the Append methods here, they're a little confusing.

I was thinking more like mCurrentLineFloats.Append(aFloat);

How do I get ahold of mFloatCacheFreeList, then? (I *really* don't want to rewrite nsFloatCache memory handling here, just because we don't like it.)

OK, let's go with comment #40 then

Revised to change Alloc(). Please review both this and V mod 1.

Comment on attachment 394920 [details] [diff] [review] Patch Part V mod 1: float-cont-stealframe Set the review flag wrong.

Fix checked in: http://hg.mozilla.org/mozilla-central/rev/e9130436ada8 http://hg.mozilla.org/mozilla-central/rev/9d788a1b2364 http://hg.mozilla.org/mozilla-central/rev/28008648ca04 http://hg.mozilla.org/mozilla-central/rev/ccbef5922a0f http://hg.mozilla.org/mozilla-central/rev/ac59a41e7815 http://hg.mozilla.org/mozilla-central/rev/1b724a06a345

It looks like this bug's fix is pretty sizeable, but is there any chance it'd be appropriate for backporting to 1.9.2 branch for a Firefox 3.6.x minor release? I ask because this checkin appears to fix a printing dataloss regression which remains on 1.9.2 -- see bug 550866 comment 1.