1046950 - convert nsIFrame::ComputeSize to take logical-coordinate parameters

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

nsIFrame::ComputeSize takes a number of nsSize parameters, and returns an nsSize; these should all become LogicalSize, I think.

I think it'd make sense here for the caller to pass in a single WritingMode that applies to all of these; the frame will be responsible to convert parameters (if necessary) into its own writing mode to compute the result, and to convert the result to the caller's requested writing mode.

Various methods that may be called by ComputeSize implementation (in particular ComputeAutoSize) will also be modified to work with LogicalSize parameters and internal computations.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - additional/modified APIs on LogicalSize

Just some more-convenient API.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - convert ComputeSize to use logical-coordinate parameters

So here's my first attempt at converting ComputeSize to logical-coord parameters/result. Does this seem a reasonable approach to the API?

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - convert ComputeAutoSize to use logical-coordinate parameters

Second part - propagate the logical values down to ComputeAutoSize as well.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8469194 [details] [diff] [review]
pt 2 - convert ComputeSize to use logical-coordinate parameters

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

::: layout/generic/nsBlockReflowState.cpp
@@ +606,5 @@
>    return aFloat->ComputeSize(
>      aCBReflowState.rendContext,
> +    fosWM,
> +    LogicalSize(cbWM, aCBReflowState.ComputedISize(),
> +                      aCBReflowState.ComputedBSize()).ConvertTo(fosWM, cbWM),

we have API for this conversion -- just write |aCBReflowState.ComputedSize(fosWM)|, and then you don't need cbWM

::: layout/generic/nsFrame.cpp
@@ +4083,5 @@
>        flexDirection == NS_STYLE_FLEX_DIRECTION_ROW_REVERSE;
>  
>      // NOTE: The logic here should match the similar chunk for determining
>      // widthStyleCoord and heightStyleCoord in
>      // nsLayoutUtils::ComputeSizeWithIntrinsicDimensions().

... except it doesn't any more: I think you probably need to convert ComputeSizeWithIntrinsicDimensions in this patch set too.

@@ +8347,1 @@
>          reflowState.SetComputedHeight(

Why is this still setting physical height? Looking at the context, I think this whole block should be changed to set ISize and BSize.

::: layout/generic/nsHTMLReflowState.cpp
@@ +2142,4 @@
>                             computeSizeFlags);
>  
> +      ComputedWidth() = size.Width(wm);
> +      ComputedHeight() = size.Height(wm);

The result is the same, but I think it's better to use ComputedISize() and ComputedBSize() here and below so that it's clear that this code has been converted

::: layout/generic/nsVideoFrame.cpp
@@ +492,5 @@
>  
>    // Only video elements have an intrinsic ratio.
>    nsSize intrinsicRatio = HasVideoElement() ? size : nsSize(0, 0);
>  
> +  return LogicalSize(aWM, 

whitespace at end of line

::: layout/tables/nsTableFrame.cpp
@@ +1572,1 @@
>  

Does the same caveat as in nsFieldSetFrame ("if the caller's writing mode is orthogonal to this frame's...") not apply here?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #4)

I've updated the patch according to your comments, with the exception of this item:

> @@ +8347,1 @@
> >          reflowState.SetComputedHeight(
> 
> Why is this still setting physical height? Looking at the context, I think
> this whole block should be changed to set ISize and BSize.

ISTM this should be done along with changing BoxReflow to take logical instead of physical parameters; I didn't touch that yet, and so setting physical values here seems correct for now. Are you OK with leaving the whole logicalization of BoxReflow for a future patch?

Comment 6

[Simon Montagu :smontagu]

OK, fair enough

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - convert ComputeSizeWithIntrinsicDimensions to logical-coord parameters.

Comment 8

[Jonathan Kew [:jfkthame]]

Tryserver run including parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=3230529a9d98

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 8474267 [details] [diff] [review]
pt 4 - convert ComputeSizeWithIntrinsicDimensions to logical-coord parameters.

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

::: layout/base/nsLayoutUtils.cpp
@@ +4270,2 @@
>    } else {
> +    // Treat "min-iSize: auto" as 0.

This refers to the CSS property, doesn't it? In that case it should stay as min-width

@@ +4278,5 @@
> +
> +  if (!isAutoBSize) {
> +    bSize = nsLayoutUtils::ComputeHeightValue(aCBSize.BSize(aWM), 
> +                boxSizingAdjust.BSize(aWM), 
> +                *blockStyleCoord);

Please remove the trailing white-space while touching these lines.

@@ +4284,5 @@
> +
> +  if (!IsAutoHeight(stylePos->mMaxHeight, aCBSize.BSize(aWM)) &&
> +      !(isFlexItem && !isInlineFlexItem)) {
> +    maxBSize = nsLayoutUtils::ComputeHeightValue(aCBSize.BSize(aWM), 
> +                  boxSizingAdjust.BSize(aWM), 

and here

@@ +4292,5 @@
> +  }
> +
> +  if (!IsAutoHeight(stylePos->mMinHeight, aCBSize.BSize(aWM)) &&
> +      !(isFlexItem && !isInlineFlexItem)) {
> +    minBSize = nsLayoutUtils::ComputeHeightValue(aCBSize.BSize(aWM), 

and here

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #9)
> > +    // Treat "min-iSize: auto" as 0.
> 
> This refers to the CSS property, doesn't it? In that case it should stay as
> min-width

Oops - an over-zealous search-and-replace! Fixed, thanks.


https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3477f5614b
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ada5ad66bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04e1a0e1920
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f3a8d7e039

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee3477f5614b
https://hg.mozilla.org/mozilla-central/rev/54ada5ad66bb
https://hg.mozilla.org/mozilla-central/rev/c04e1a0e1920
https://hg.mozilla.org/mozilla-central/rev/62f3a8d7e039