186953 - remove maxElementSize::height calculation

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now the maxElementSize member variable of nsHTMLReflowMetrics is an
nsSize, with a width and a height.  We fill in the height using roughly the same
algorithm as the width.  The width has a useful purpose:  it's the smallest the
width of an element can go without overflow, or at least it's supposed to be. 
The height has no such useful meaning: it doesn't even take into account such
basic characteristics of flow layouts, such as forced line breaks.

Fortunately, nobody uses the height.  Well, almost.  nsListBoxFrame does, but I
could replace its use with a 6-line loop.

I'm attaching a large patch to convert the |nsSize* maxElementSize| to
|PRPackedBool mComputeMEW| (in the old code you requested computation of the MES
by making the pointer non-null) and |nscoord mMaxElementWidth|.

The only substantive changes are in nsListControlFrame and nsBoxToBlockAdaptor.
 I'm not sure yet that the nsBoxToBlockAdaptor changes are correct (and I'd also
like to look at the nsBoxFrame, nsBoxLayoutState, and ns{Image/Leaf}BoxFrame
changes a little more closely.

(This is one tiny step towards pulling the min/max width computation out of
|Reflow|.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 110252 [details] [diff] [review]
patch

I ran the regression tests and got only the usual false positives.

I decided I'm happy with the nsBoxToBlockAdaptor changes since the
mCachedMaxElementHeight variable should never be used in the current code,
since a reflow should always produce a height equal to or larger than the
maxelementheight.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110252 [details] [diff] [review]
patch

Shouldn't nsHTMLReflowMetrics& operator=(const nsHTMLReflowMetrics& aOther)
copy the value of mComputeMEW?

layout/html/base/src/nsBRFrame.cpp

>       // Update max-element-size to keep us honest

Fix that comment?

layout/html/base/src/nsBlockBandData.cpp

> MaxElementSizePropertyDtor

Maybe this should be renamed?

> +                                      nscoord aMaxElementSize)

(in signature of nsBlockBandData::StoreMaxElementWidth).  also, rename.

layout/html/base/src/nsBlockBandData.h

The comments here probably need some updating.

layout/html/base/src/nsBlockFrame.cpp

> +  { "max-element-size", &nsBlockFrame::gNoisyMaxElementWidth },

Shouldn't that become "max-element-width"?

>       printf ("nsBlockFrame::CFS: %p returning MES %d\n", 

MEW, right?

>  // Compute the line's max-element-size by adding into the raw value

update that comment more?

> +    printf(": maxFloaterSize=%d\n", maxWidth);

maxFloaterWidth?

> +    printf ("nsBlockFrame::ComputeLineMaxElementSize: %p returning MEW %d\n", 

You renamed the function....

>       printf ("nsBlockFrame::PostPlaceLine: %p setting line %p MES %d\n", 

MEW

layout/html/base/src/nsBlockReflowContext.cpp

>        nsFrame::ListTag(stdout, mFrame);
>       printf(" didn't set max-element-size!\n");

update the string?

> +        printf(": maxElementSize=%d wh=%d,%d\n",
> +               mMetrics.mMaxElementWidth,

maxElementWidth

layout/html/base/src/nsBlockReflowState.cpp

> +    printf("BRS: setting compute-MES to %d\n", aMetrics.mComputeMEW);

"setting compute-MEW"

layout/html/base/src/nsContainerFrame.cpp

>     printf(" didn't set max-element-size!\n");

max-element-width

layout/html/base/src/nsHRFrame.cpp

> +    NS_ASSERTION(aDesiredSize.mMaxElementWidth <= aDesiredSize.width, "bad max-element-size width");

"bad max-element-width"

layout/html/base/src/nsLineLayout.cpp

>        printf(" didn't set max-element-size!\n");

max-element-width

>     // Compute max-element-size if necessary

same

layout/html/document/src/nsFrameFrame.cpp

>   // For unknown reasons, the maxElementSize for the InnerFrame is used, but the

fix this comment?  (first and second lines)

layout/html/forms/src/nsFieldSetFrame.cpp

>     printf("FIELDSET:  w=%d, maxWidth=%d, MES=%d\n",

MEW, not MES.

layout/html/table/src/nsTableCellFrame.cpp

> +  nsHTMLReflowMetrics kidSize(NS_UNCONSTRAINEDSIZE == aReflowState.availableWidth ||
> +                                aDesiredSize.mComputeMEW,

odd indentation....

layout/xul/base/src/nsBoxToBlockAdaptor.cpp

> +    PRBool canSetMaxElementSize = CanSetMaxElementWidth(aState, reason, &path);

Rename the var?

fix those nits, and r+sr=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Shouldn't nsHTMLReflowMetrics& operator=(const nsHTMLReflowMetrics& aOther)
copy the value of mComputeMEW?

That was the one substantive comment I made.... and it seems to have gotten lost
in the flood of nits ;)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I was trying to keep things equivalent to the old code, which didn't.  And
I hope nobody uses that operator=...

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Anyway, fix checked in to trunk, 2003-01-09 06:25/26 PDT.