492627 - Remove Placeholder Continuations

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[fantasai]

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

Comment 1

[fantasai]

Attachment: Patch Part I: Use Overflow Containers

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

Comment 2

[fantasai]

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.

Comment 3

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

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.

Comment 4

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

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

Comment 5

[fantasai]

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

Comment 6

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

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

Comment 7

[fantasai]

Attachment: fun resizing floats in columns test

Comment 8

[fantasai]

Note to self: Don't regress bug 153785

Comment 9

[fantasai]

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

Patch Part I reassigned to bug 503183

Comment 10

[fantasai]

Patch Part II assigned to bug 499377

Comment 11

[fantasai]

Attachment: Patch Part III: float-unsplit

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.

Comment 12

[fantasai]

Attachment: Patch Part IV: float-unplaceholder

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

Comment 13

[fantasai]

Attachment: Patch Part V: float-continuations

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.

Comment 14

[fantasai]

Attachment: Patch Part VI: float-br-clear

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 15

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

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

This looks fine

Comment 16

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

+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 17

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

Comment on attachment 388649 [details] [diff] [review]
Patch Part VI: float-br-clear

Seems like we need tests here.

Overall, this is really impressive!!!

Comment 18

[fantasai]

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

Comment 19

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

(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 20

[fantasai]

Attachment: Patch Part V: float-continuations

Comment 21

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

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.

Comment 22

[fantasai]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

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

nsFrameList::AppendFrames already has a nullcheck ...

but alright, leave your if condition in for now.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

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

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.

Comment 27

[fantasai]

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.

Comment 28

[fantasai]

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.

Comment 29

[fantasai]

Attachment: Patch Part V: float-continuations

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

Comment 30

[fantasai]

Attachment: tests for <br clear>

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

Comment 31

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

Why don't you use reftest-print instead of columns?

I don't think we're ready to reenable float breaking yet.

Comment 32

[fantasai]

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?

Comment 33

[fantasai]

Attachment: tests for <br clear>

Comment 34

[fantasai]

Attachment: Patch Part V mod 1: float-cont-stealframe

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

Comment 35

[fantasai]

Attachment: Patch Part V mod 2: float-shuffle

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

Comment 36

[fantasai]

Attachment: Patch Part VII: oc-cont-delete

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 37

[fantasai]

Comment on attachment 394920 [details] [diff] [review]
Patch Part V mod 1: float-cont-stealframe

Forgot to flag review on this one

Comment 38

[fantasai]

Attachment: Patch Part V mod 3: nsfloatcache-unleak

Fixes an nsFloatCache leak introduced by Part V.

Comment 39

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

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

Comment 40

[fantasai]

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

Comment 41

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

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

Comment 42

[fantasai]

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

Comment 43

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

OK, let's go with comment #40 then

Comment 44

[fantasai]

Attachment: Patch Part V mod 3: nsfloatcache-unleak

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

Comment 45

[fantasai]

Comment on attachment 394920 [details] [diff] [review]
Patch Part V mod 1: float-cont-stealframe

Set the review flag wrong.

Comment 46

[fantasai]

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

Comment 47

[Daniel Holbert [:dholbert]]

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.