Closed Bug 492627 Opened 10 years ago Closed 10 years ago

Remove Placeholder Continuations

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: fantasai.bugs, Assigned: fantasai.bugs)

References

Details

Attachments

(10 files, 5 obsolete files)

1.48 KB, text/html
Details
31.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
23.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.17 KB, patch
roc
: review+
Details | Diff | Splinter Review
35.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.99 KB, patch
Details | Diff | Splinter Review
5.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
Remove nsPlaceholderFrame continuations. This should make nsBlockFrame splitting less fragile.
Blocks: 447660
Use overflow containers instead of shoving placeholders up and down the ancestor chain.
Attachment #378410 - Flags: review?(roc)
Attachment #378410 - Flags: review?(roc)
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.
Depends on: 499377
Note to self: Don't regress bug 153785
Depends on: 503183
Comment on attachment 378410 [details] [diff] [review]
Patch Part I: Use Overflow Containers

Patch Part I reassigned to bug 503183
Attachment #378410 - Attachment is obsolete: true
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.
Attachment #388646 - Flags: review?(roc)
This patch removes the dependencies on use of placeholders in various functions, switching them to accepting the out-of-flow frame directly.
Attachment #388647 - Flags: review?(roc)
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.
Attachment #388648 - Flags: review?(roc)
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.
Attachment #388649 - Flags: review?(roc)
Comment on attachment 388646 [details] [diff] [review]
Patch Part III: float-unsplit

This looks fine
Attachment #388646 - Flags: review?(roc) → review+
+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!!!
Attachment #388649 - Flags: review?(roc) → review+
> 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.
Flags: in-testsuite?
(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.
Attachment #388648 - Attachment is obsolete: true
Attachment #390964 - Flags: review?(roc)
Attachment #388648 - Flags: review?(roc)
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.
Attachment #390964 - Flags: review?(roc) → review+
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.
Attachment #390964 - Attachment is obsolete: true
Attachment #391231 - Flags: review?(roc)
Attached patch tests for <br clear> (obsolete) — Splinter Review
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?
Attachment #391756 - Attachment is obsolete: true
> 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.)
Attachment #394927 - Flags: review?(roc)
Attachment #394920 - Attachment is patch: true
Attachment #394920 - Attachment mime type: application/octet-stream → text/plain
Attachment #394927 - Attachment is patch: true
Attachment #394927 - Attachment mime type: application/octet-stream → text/plain
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.)
Attachment #394929 - Flags: review?(roc)
Comment on attachment 394920 [details] [diff] [review]
Patch Part V mod 1: float-cont-stealframe

Forgot to flag review on this one
Attachment #394920 - Flags: review+
Fixes an nsFloatCache leak introduced by Part V.
Attachment #397127 - Flags: review?(roc)
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.)
Revised to change Alloc(). Please review both this and V mod 1.
Attachment #397127 - Attachment is obsolete: true
Attachment #397306 - Flags: review?(roc)
Attachment #397127 - Flags: review?(roc)
Comment on attachment 394920 [details] [diff] [review]
Patch Part V mod 1: float-cont-stealframe

Set the review flag wrong.
Attachment #394920 - Flags: review+ → review?(roc)
Depends on: 514689
Blocks: 550866
No longer blocks: 550866
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.
Depends on: 564968
Depends on: 570160
Depends on: 574958
Blocks: 508473
You need to log in before you can comment on or make changes to this bug.