743402 - multi-column column frames should not run out of computed height

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[fantasai]

Right now we're using mComputedHeight to constrain the height of the anonymous nsBlockFrames that make up the columns in the multi-col element. This causes those frames to use up computed height, and any content beyond that computed is then treated as overflow within the multi-col element. The computed height should only apply to the nsColumnSetFrame. See testcase in attachment 612052 [details]: the columns start returning an NS_FRAME_OVERFLOW_INCOMPLETE status when print-previewed.

Note: This would only have a visible effect if the column frame is styled.

Comment 1

[fantasai]

Attachment: patch for reftests

Comment 2

[Scott Johnson (:jwir3)]

Attachment: b743702 (WIP)

Work in progress patch. Includes fantasai's reftests, in both w3c format as well as moz-reftest format (basically the same, but I just copied them to another directory).

Comment 3

[fantasai]

Not sure why you think we should have two copies of the same test, there should only be one copy... and it should be in w3c-css. Either way, though, don't forget to hg add everything when you post your final patch. ;)

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b743702

We'll probably also need to get an sr from roc, but I'll do that in a bit.

Comment 5

[Scott Johnson (:jwir3)]

(In reply to fantasai from comment #3)
> Not sure why you think we should have two copies of the same test, there
> should only be one copy... and it should be in w3c-css. Either way, though,
> don't forget to hg add everything when you post your final patch. ;)

What? All of our current multicol tests are in layout/reftests/columns. That includes ones that we're not submitting to the w3c. If we have reftests in w3c-tests, then we're either going to have some tests in two places (what I have right now), as the w3c tests aren't going to be run by our tbpl framework, or we're going to have to make tbpl run the tests in w3c directory, and then we have to worry about maintaining tests in two different locations for the same feature(s).

My understanding was that the w3c tests wouldn't run on the master build machine, so we only run them locally. This test doesn't fall into that case - we want it to run both locally as well as in aggregate when we run the entire suite automatically.

Comment 6

[fantasai]

+    if (aReflowState.ComputedHeight() != NS_INTRINSICSIZE) {
+      contentSize.height =
+        NS_MIN(aReflowState.availableHeight, aConfig.mComputedHeight);
+    } else {
+      contentSize.height = aReflowState.ApplyMinMaxHeight(contentSize.height);

So, I think this bit is wrong. Like the computed height in the first clause, the min/max constraints need to be the "effective" heights.

Maybe what we need to do is, instead of having GetEffectiveHeight on the container, have a "GetPreviouslyConsumedHeight" method on nsReflowState that computes the sum of the prev-in-flows' heights and caches it on the nsReflowState object, so that we can use that when applying computed/min/max constraints?

Comment 7

[Scott Johnson (:jwir3)]

Attachment: b743402 (v2)

This is the version of the patch that has the changes we discussed, fantasai. Just wanted you to take a look at it before I submit to roc for review as well.

Thanks!

Comment 8

[fantasai]

Almost the entire GetEffectiveComputedHeight function is written out twice. Don't do that. Either fold the two together by using a default argument, or require the second argument always.

GetAvailableContentHeight seems to only be used in nsColumnSetFrame, so it should probably say there.

  nsMargin bp = aReflowState.mComputedBorderPadding;
  ApplySkipSides(bp);
  return NS_MIN(aComputedHeight,
                NS_MAX(0, aReflowState.availableHeight - bp.top - bp.bottom));

Hm, so, this is actually a bit tricky. ApplySkipSides is intended for use after reflow is complete. So in order to check whether we need the bottom border, it checks for a next-in-flow. While we're actually *in* reflow, this may or may not reflect the state we'll be in after reflow. We could, for example, have a next-in-flow from the previous reflow, but now we have enough room to fit all our content, so that next-in-flow will be deleted by our parent as soon as we return from Reflow(). For this to give consistent results, we need to either always subtract the bottom border or never subtract it. I think, but I'm not sure, that we should always subtract the bottom border here.

ComputeFinalHeight should take aConsumed as a parameter to avoid recomputing it, no? Also the function doesn't appear to be used more than once, perhaps this code should stay where it is?

  // The computed height should be equal to the height of the element, i.e.
  // the computed height itself, plus the consumed height.

This comment seems to be using "computed height" to mean two different things.

(to be continued... almost done!)

Comment 9

[fantasai]

+    // We add the "consumed height" back in, so that we're applying
+    // constraints to the correct height value, then subtract it again after
+    // we've finished with the min/max calculation. This prevents us from
+    // having a last continuation that is smaller than the min height be, but
+    // which has prev-in-flows trigger a larger height than actually required.
+    contentSize.height += aConfig.mConsumedHeight;
+    contentSize.height = aReflowState.ApplyMinMaxHeight(contentSize.height);
+    contentSize.height -= aConfig.mConsumedHeight;

Didn't you just add a nifty aConsumed argument to ApplyMinMaxHeight to take handle this?

+  aDesiredSize.height = borderPadding.top +
+                        contentSize.height +
+                        borderPadding.bottom;
+  aDesiredSize.width = contentSize.width +
+                       borderPadding.left +
+                       borderPadding.right;

This needs to handle ApplySkipSides, probably up here:
  // get our border and padding
  const nsMargin &borderPadding = aReflowState.mComputedBorderPadding;

Maybe ApplySkipSides needs to take an optional nsReflowStatus argument, so it can use that in place of a next-in-flow check while we're reflowing.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b743402 (v3)

Thanks for the in-depth review, Elika. I have updated the patch to account for a few of the things you requested, but I also have some questions:

(In reply to fantasai from comment #8)
> Almost the entire GetEffectiveComputedHeight function is written out twice.
> Don't do that. Either fold the two together by using a default argument, or
> require the second argument always.

Fixed.

> GetAvailableContentHeight seems to only be used in nsColumnSetFrame, so it
> should probably say there.

Hmmm... yeah, I'm not sure why we pulled this out. I have toyed with the idea 
of leaving it there multiple times, but I can't remember why I decided not to...
I'll move it back, unless I can come up with a good reason... ;)

>   nsMargin bp = aReflowState.mComputedBorderPadding;
>   ApplySkipSides(bp);
>   return NS_MIN(aComputedHeight,
>                 NS_MAX(0, aReflowState.availableHeight - bp.top -
> bp.bottom));
> 
> Hm, so, this is actually a bit tricky. ApplySkipSides is intended for use
> after reflow is complete. So in order to check whether we need the bottom
> border, it checks for a next-in-flow. While we're actually *in* reflow, this
> may or may not reflect the state we'll be in after reflow. We could, for
> example, have a next-in-flow from the previous reflow, but now we have
> enough room to fit all our content, so that next-in-flow will be deleted by
> our parent as soon as we return from Reflow(). For this to give consistent
> results, we need to either always subtract the bottom border or never
> subtract it. I think, but I'm not sure, that we should always subtract the
> bottom border here.

If I understand correctly, you want me to change this:
>   nsMargin bp = aReflowState.mComputedBorderPadding;
>   ApplySkipSides(bp);
>   return NS_MIN(aComputedHeight,
>                 NS_MAX(0, aReflowState.availableHeight - bp.top -
> bp.bottom));

to be this:
>   nsMargin bp = aReflowState.mComputedBorderPadding;
>   ApplySkipSides(bp);
>   return NS_MIN(aComputedHeight - bp.bottom,
>                 NS_MAX(0, aReflowState.availableHeight - bp.top -
> bp.bottom));
>

Is that a correct interpretation of what you are saying?
 
> ComputeFinalHeight should take aConsumed as a parameter to avoid recomputing
> it, no? Also the function doesn't appear to be used more than once, perhaps
> this code should stay where it is?

I disagree with leaving it where it is, because we decided we'd need it for the patch for
bug 724978, although we could leave this refactoring until that bug if need be.

And, you're right, it should take aConsumed as a parameter. I've fixed that.

>   // The computed height should be equal to the height of the element, i.e.
>   // the computed height itself, plus the consumed height.
> 
> This comment seems to be using "computed height" to mean two different
> things.

Yeah, I remember writing this comment and thinking it was pretty awkward. I've adjusted
it a bit.


(In reply to fantasai from comment #9)
> +    // We add the "consumed height" back in, so that we're applying
> +    // constraints to the correct height value, then subtract it again after
> +    // we've finished with the min/max calculation. This prevents us from
> +    // having a last continuation that is smaller than the min height be,
> but
> +    // which has prev-in-flows trigger a larger height than actually
> required.
> +    contentSize.height += aConfig.mConsumedHeight;
> +    contentSize.height = aReflowState.ApplyMinMaxHeight(contentSize.height);
> +    contentSize.height -= aConfig.mConsumedHeight;
> 
> Didn't you just add a nifty aConsumed argument to ApplyMinMaxHeight to take
> handle this?

*sigh*. Long couple of weeks, I guess. Either that, or I'm getting old. ;)
I've made this change.

> 
> +  aDesiredSize.height = borderPadding.top +
> +                        contentSize.height +
> +                        borderPadding.bottom;
> +  aDesiredSize.width = contentSize.width +
> +                       borderPadding.left +
> +                       borderPadding.right;
> 
> This needs to handle ApplySkipSides, probably up here:
>   // get our border and padding
>   const nsMargin &borderPadding = aReflowState.mComputedBorderPadding;
> 
> Maybe ApplySkipSides needs to take an optional nsReflowStatus argument, so
> it can use that in place of a next-in-flow check while we're reflowing.

Just so I'm clear on this - you're saying that ApplySkipSides, since it might skip the
top/bottom or left/right borders, should set the borderPadding to zero for the cases that 
it skips? (In other words, if GetSkipSides() were to return that the left border is skipped,
then this:

> +  aDesiredSize.width = contentSize.width +
> +                       borderPadding.left +
> +                       borderPadding.right;

should actually be this:

> +  aDesiredSize.width = contentSize.width +
> +                       borderPadding.right;
?) Or is that not what you're saying? I guess I'm not exactly sure why you're saying that ApplySkipSides could take a reflow state
to account for this, since the reflow state is going to be constant... it wouldn't be able to set the border width to 0 in these
cases... Or perhaps I'm missing something else here that you're saying.

Comment 11

[fantasai]

(In reply to Scott Johnson (:jwir3) from comment #10)
>
> If I understand correctly, you want me to change this:
> >   nsMargin bp = aReflowState.mComputedBorderPadding;
> >   ApplySkipSides(bp);
> >   return NS_MIN(aComputedHeight,
> >                 NS_MAX(0, aReflowState.availableHeight - bp.top -
> > bp.bottom));
> 
> to be this:
> >   nsMargin bp = aReflowState.mComputedBorderPadding;
> >   ApplySkipSides(bp);
> >   return NS_MIN(aComputedHeight - bp.bottom,
> >                 NS_MAX(0, aReflowState.availableHeight - bp.top -
> > bp.bottom));
> >
> 
> Is that a correct interpretation of what you are saying?

Yes. Although I'm not 100% sure of that. :/ Actually I think that's wrong, now that I think about it. We do have a problem with ApplySkipSides being inconsistent, though, as its results will depend on whether we've reflowed once before and what the result of that was. see below.

> > +  aDesiredSize.height = borderPadding.top +
> > +                        contentSize.height +
> > +                        borderPadding.bottom;
> > 
> > This needs to handle ApplySkipSides, probably up here:
> >   // get our border and padding
> >   const nsMargin &borderPadding = aReflowState.mComputedBorderPadding;
> > 
> 
> Just so I'm clear on this - you're saying that ApplySkipSides, since it might skip the
> top/bottom or left/right borders, should set the borderPadding to zero for the cases that 
> it skips?

What I'm saying is, the borderPadding variable here needs to be piped through ApplySkipSides, so that it's set to zero in the appropriate places when there's a continuation.

> > Maybe ApplySkipSides needs to take an optional nsReflowStatus argument, so
> > it can use that in place of a next-in-flow check while we're reflowing.
>
> I guess I'm not exactly sure why
> you're saying that ApplySkipSides could take a reflow state
> to account for this, since the reflow state is going to be constant... it
> wouldn't be able to set the border width to 0 in these
> cases... Or perhaps I'm missing something else here that you're saying.

Sorry, I guess I didn't explain it very well. ApplySkipSides looks at the prev-in-flow and the next-in-flow to determine whether a borderpadding needs to be zeroed or not: if there's a prev-in-flow, the top gets zeroed, and if there's a next-in-flow the bottom gets zeroed. (This is because we don't paint the bottom border until the box is complete.)

The problem is, the next-in-flow does not necessarily exist yet when we're in this function. If we are reflowing for the first time, and we're incomplete, we /will have/ a next-in-flow, but it hasn't been created yet. ApplySkipSides doesn't know that, so it won't zero the bottom borderpadding. (Alternately, we might be done, but have a next-in-flow that hasn't been deleted yet. In that case ApplySkipSides will zero out the bottom border when it shouldn't.) Basically, when reflow is done, ApplySkipSides works fine, but while we're in reflow, it doesn't. It needs more information, and what it needs is the nsReflowState for the frame: that's what will determine whether a next-in-flow gets created if newly needed, or an existing next-in-flow gets deleted if no longer needed.

The complication here is that whether or not we apply the bottom border depends on whether the frame we're currently reflowing is the last-in-flow or not. If it's not, then the height of the columns will stretch to the bottom of the page. If it is, however, we need to shorten the column heights enough that the bottom border will fit. I'm not sure how that should fit into the column height logic, though; it's capable of causing a loop, since adding in the border could be what triggers spilling onto another page.

Probably the safest thing is to always include the bottom border. Which, from a code perspective, means grabbing the bottom border before calling ApplySkipSides and then restoring it after. We might leave a little too much space at the bottom, but borders are usually small so in the common cases, it won't matter, and we can figure out the right answer later.

roc, does that seem right?

Sorry about the super-late response, btw. This completely fell off my radar. I don't have a review queue, so next time you don't get a response within 2 days, feel free to smack me...

Comment 12

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

reply to fantasai from comment #11)
> Probably the safest thing is to always include the bottom border. Which,
> from a code perspective, means grabbing the bottom border before calling
> ApplySkipSides and then restoring it after. We might leave a little too much
> space at the bottom, but borders are usually small so in the common cases,
> it won't matter, and we can figure out the right answer later.
> 
> roc, does that seem right?

Yes, I think when calculating whether content fits in the available height, you should always including the block's bottom border and padding, since for the content to fit the block's bottom border and padding must fit with it. We don't allow the border or padding to be alone in the next continuation.

Comment 13

[fantasai]

Comment on attachment 647663 [details] [diff] [review]
b743402 (v3)

(If we know we're creating a next continuation, though, we shouldn't be leaving room for the bottom borderpadding as we lay things out. Imagine if that bottom borderpadding were 50% of the height of the page. We'd try to lay things out assuming it's there, realize we need a next continuation, create one, and then move to the next page, leaving the bottom half of the page blank. But, I think we should tackle that problem in a separate bug.)

So, afaict for here...

+  nsMargin bp = aReflowState.mComputedBorderPadding;
+  ApplySkipSides(bp);
+  return NS_MIN(aComputedHeight,
+                NS_MAX(0, aReflowState.availableHeight - bp.top - bp.bottom));

This probably needs to be something like
  nsMargin bp = aReflowState.mComputedBorderPadding;
  ApplySkipSides(bp);
  bp.bottom = aReflowState.mComputedBorderPadding.bottom;
  return NS_MIN(aComputedHeight,
                NS_MAX(0, aReflowState.availableHeight - bp.top - bp.bottom));
which will apply the bottom border unconditionally, and the use of borderPadding in nsColumnSetFrame::ReflowChildren will need to do much the same thing, e.g.
  nsMargin borderPadding = aReflowState.mComputedBorderPadding;
  ApplySkipSides(borderPadding);
  borderPadding.bottom = aReflowState.mComputedBorderPadding.bottom;

So I'd say fix that, so that our layout calculations don't depend on the state of the last reflow and whether it created a next-in-flow or not, fix the computed height comment, which still makes no sense, and then pass it over to roc for final review.

Comment 14

[Scott Johnson (:jwir3)]

Quick Update on progress for this bug:

It seems that the column-fill stuff messed up this patch, so I'm in the process of merging it and debugging it. I hope to have it finished early next week so that I can post another revision for roc to review.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Does the dependency chain makes this bug worth pushing on?

Comment 16

[Markus Popp]

(In reply to David Bolter [:davidb] from comment #15)
> Does the dependency chain makes this bug worth pushing on?

Looks like this bug is now the only one blocking multi-column properties from being un-prefixed: bug #698783

Comment 17

[Scott Johnson (:jwir3)]

(In reply to Markus Popp from comment #16)
> (In reply to David Bolter [:davidb] from comment #15)
> > Does the dependency chain makes this bug worth pushing on?
> 
> Looks like this bug is now the only one blocking multi-column properties
> from being un-prefixed: bug #698783

Well, this isn't entirely true. We also need to fix a few other items before unprefixing in bug 698783, and figure out the testing situation. I will add dependencies as soon as I can so the chain is accurate. But, we are pushing forward on this, yes.

Comment 18

[Scott Johnson (:jwir3)]

Attachment: b743402-new

Ok, here's a new WIP version of this bug. There is still a small problem in that the final size of the column set frame is getting the bottom borderpadding added in twice, causing the print preview to place the bottom border on a third page, instead of at the end of the second page, as it should be (there is room for it). This also seems to be causing some unit test failures. I'm looking into this, but I've yet to determine where this is getting added in.

I'm just requesting feedback to make sure that I've addressed all of your previous concerns with the other patches, fantasai, and as a sanity check, since it's been a while since this bug has been touched. ;)

Comment 19

[Scott Johnson (:jwir3)]

Attachment: b743402-part1

I've split the original patch into a few parts, to make it easier to review. I believe I've addressed all of fantasai's original comments on the patch, and I've fixed all the unit tests locally (although I haven't run this new patch set through try yet, so we'll see). 

This is the first of the patches.

Comment 20

[Scott Johnson (:jwir3)]

Attachment: b743402-part2

Comment 21

[Scott Johnson (:jwir3)]

Attachment: b743402-part3

Comment 22

[Scott Johnson (:jwir3)]

Attachment: b743402-part4

Comment 23

[Scott Johnson (:jwir3)]

Attachment: b743402-part5

Comment 24

[Scott Johnson (:jwir3)]

Attachment: b743402-tests

Comment 25

[Scott Johnson (:jwir3)]

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=964bfe856555

Comment 26

[Scott Johnson (:jwir3)]

Attachment: b743402-part5 (v2)

Fixed some issues that cropped up as part of the try server run. Mainly, the infinite looping problem we saw in bug 724978 was failing again, since the computed height could be 0. I added some logic to detect situations where we're not making progress, likely because the computed height is being entirely consumed by prev-in-flows.

Comment 27

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

Comment on attachment 776342 [details] [diff] [review]
b743402-part1

Review of attachment 776342 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.h
@@ +500,1 @@
>      return std::max(aHeight, mComputedMinHeight);

Shouldn't we subtract aConsumed before returning?

It seems to me ApplyMinMaxHeight should return aHeight if there are no min/max constraints, but it doesn't.

::: layout/generic/nsSplittableFrame.cpp
@@ +210,5 @@
> +  if (GetPrevInFlow()) {
> +    // Reduce the height by the computed height of prev-in-flows.
> +    for (nsIFrame* prev = GetPrevInFlow(); prev; prev = prev->GetPrevInFlow()) {
> +      height += prev->GetRect().height;
> +    }

The "if" check is superfluous. Remove it.

Comment 28

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

Comment on attachment 776344 [details] [diff] [review]
b743402-part2

Review of attachment 776344 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSplittableFrame.h
@@ +87,5 @@
>  
> +  /**
> +   * Retrieve the effective computed height of this frame, which is the computed
> +   * height, minus the height consumed by any previous in-flows, or the available
> +   * height of the page, whichever is smaller.

Your implementation doesn't look at the available height of the page. I'm not sure why we'd want to include that here anyway.

Comment 29

[Scott Johnson (:jwir3)]

Attachment: b743402-part1 (v2)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 776342 [details] [diff] [review]
> b743402-part1
> 
> Review of attachment 776342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsHTMLReflowState.h
> @@ +500,1 @@
> >      return std::max(aHeight, mComputedMinHeight);
> 
> Shouldn't we subtract aConsumed before returning?

Yep - Good catch. I meant to add that in (I saw it right before I posted the patch for review, but then apparently forgot). 

> It seems to me ApplyMinMaxHeight should return aHeight if there are no
> min/max constraints, but it doesn't.

I've fixed this in the latest version of the patch.

> ::: layout/generic/nsSplittableFrame.cpp
> @@ +210,5 @@
> > +  if (GetPrevInFlow()) {
> > +    // Reduce the height by the computed height of prev-in-flows.
> > +    for (nsIFrame* prev = GetPrevInFlow(); prev; prev = prev->GetPrevInFlow()) {
> > +      height += prev->GetRect().height;
> > +    }
> 
> The "if" check is superfluous. Remove it.

Done.

Comment 30

[Scott Johnson (:jwir3)]

Attachment: b743402-part2 (v2)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Comment on attachment 776344 [details] [diff] [review]
> b743402-part2
> 
> Review of attachment 776344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsSplittableFrame.h
> @@ +87,5 @@
> >  
> > +  /**
> > +   * Retrieve the effective computed height of this frame, which is the computed
> > +   * height, minus the height consumed by any previous in-flows, or the available
> > +   * height of the page, whichever is smaller.
> 
> Your implementation doesn't look at the available height of the page. I'm
> not sure why we'd want to include that here anyway.

Agreed. I've removed that part of the comment.

Comment 31

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

Comment on attachment 779232 [details] [diff] [review]
b743402-part1 (v2)

Review of attachment 779232 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsHTMLReflowState.h
@@ +492,3 @@
>     */
> +  nscoord ApplyMinMaxHeight(nscoord aHeight, nscoord aConsumed = 0) const {
> +    aHeight = aHeight + aConsumed;

aHeight += aConsumed

Comment 32

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

Comment on attachment 779234 [details] [diff] [review]
b743402-part2 (v2)

Review of attachment 779234 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment fixed

::: layout/generic/nsSplittableFrame.h
@@ +101,5 @@
> +   *        current available (page) height.
> +   * @param aContentHeight The height of content, precomputed outside of this
> +   *        function. The final height that is used in aMetrics will be set to
> +   *        either this or the available height, whichever is larger, in the
> +   *        case where our available height is constrained.

It's not just "where our available height is constrained", but "where our available height is constrained AND we overflow it". Right?

Comment 33

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

Comment on attachment 779234 [details] [diff] [review]
b743402-part2 (v2)

Review of attachment 779234 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment fixed

::: layout/generic/nsSplittableFrame.h
@@ +97,5 @@
> +   *
> +   * @param aReflowState Data structure passed from parent during reflow.
> +   * @param aReflowStatus The reflow status for when we're finished doing reflow.
> +   *        this will get set appropriately if the height causes exceeds the
> +   *        current available (page) height.

Actually, make aReflowStatus a pointer parameter so it's really clear it can be modified.

@@ +101,5 @@
> +   *        current available (page) height.
> +   * @param aContentHeight The height of content, precomputed outside of this
> +   *        function. The final height that is used in aMetrics will be set to
> +   *        either this or the available height, whichever is larger, in the
> +   *        case where our available height is constrained.

It's not just "where our available height is constrained", but "where our available height is constrained AND we overflow it". Right?

Comment 34

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

Comment on attachment 776349 [details] [diff] [review]
b743402-part4

Review of attachment 776349 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSplittableFrame.cpp
@@ +318,5 @@
> +    // created a nif yet. If our content height is going to exceed our available
> +    // height, though, then we're going to need a next-in-flow, it just hasn't
> +    // been created yet.
> +
> +    nscoord effectiveCH = this->GetEffectiveComputedHeight(*aReflowState);

Shouldn't you check aReflowState->availableHeight to see if it's constrained, before we do this potentially O(N)-expensive call?

Comment 35

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

Comment on attachment 777156 [details] [diff] [review]
b743402-part5 (v2)

Review of attachment 777156 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsColumnSetFrame.cpp
@@ +161,5 @@
> +  nsMargin bp = aReflowState.mComputedBorderPadding;
> +  ApplySkipSides(bp, &aReflowState);
> +  bp.bottom = aReflowState.mComputedBorderPadding.bottom;
> +  return std::min(aComputedHeight,
> +                  std::max(0, aReflowState.availableHeight - bp.top - bp.bottom));

bp.TopBottom()

@@ +779,5 @@
> +                        contentSize.height +
> +                        borderPadding.bottom;
> +  aDesiredSize.width = contentSize.width +
> +                       borderPadding.left +
> +                       borderPadding.right;

use borderPadding.LeftRight() and borderPadding.TopBottom()

Comment 36

[Scott Johnson (:jwir3)]

Attachment: b743402-part4 (v2)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> aHeight += aConsumed

Fixed.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 779234 [details] [diff] [review]
> Actually, make aReflowStatus a pointer parameter so it's really clear it can
> be modified.

Done.

> It's not just "where our available height is constrained", but "where our
> available height is constrained AND we overflow it". Right?

Yes. I've fixed the comment.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)

> Shouldn't you check aReflowState->availableHeight to see if it's
> constrained, before we do this potentially O(N)-expensive call?

Ah, thanks for catching that. You're completely right.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> bp.TopBottom()

Fixed.

> use borderPadding.LeftRight() and borderPadding.TopBottom()

Fixed.

Comment 37

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2edbbf6440c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7907c54187f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada93e976dca
https://hg.mozilla.org/integration/mozilla-inbound/rev/3098fea37f2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/049168537ae0
https://hg.mozilla.org/integration/mozilla-inbound/rev/19848fff857e

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/91356879fbfd

The failures are visible in the reftest logs.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=19848fff857e

Comment 39

[Scott Johnson (:jwir3)]

Ok, I forgot to add in mats' newer code from bug 883676. New try server run with this merged in correctly:

https://tbpl.mozilla.org/?tree=Try&rev=1180430e18dc

Comment 40

[Scott Johnson (:jwir3)]

Re-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbced4cc70f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/088bf371078a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bd0ed5fb4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/352501d3f390
https://hg.mozilla.org/integration/mozilla-inbound/rev/993dc8e10a42
https://hg.mozilla.org/integration/mozilla-inbound/rev/579445e5d533

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbced4cc70f4
https://hg.mozilla.org/mozilla-central/rev/088bf371078a
https://hg.mozilla.org/mozilla-central/rev/f1bd0ed5fb4e
https://hg.mozilla.org/mozilla-central/rev/352501d3f390
https://hg.mozilla.org/mozilla-central/rev/993dc8e10a42
https://hg.mozilla.org/mozilla-central/rev/579445e5d533