Closed Bug 1046950 Opened 5 years ago Closed 5 years ago

convert nsIFrame::ComputeSize to take logical-coordinate parameters

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(4 files)

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.
Just some more-convenient API.
Attachment #8469192 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
So here's my first attempt at converting ComputeSize to logical-coord parameters/result. Does this seem a reasonable approach to the API?
Attachment #8469194 - Flags: review?(smontagu)
Second part - propagate the logical values down to ComputeAutoSize as well.
Attachment #8469196 - Flags: review?(smontagu)
Attachment #8469192 - Flags: review?(smontagu) → review+
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?
Attachment #8469194 - Flags: review?(smontagu) → review+
Attachment #8469196 - Flags: review?(smontagu) → review+
(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?
Flags: needinfo?(smontagu)
OK, fair enough
Flags: needinfo?(smontagu)
Tryserver run including parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=3230529a9d98
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
Attachment #8474267 - Flags: review?(smontagu) → review+
(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
Target Milestone: --- → mozilla34
Depends on: 1059167
Depends on: 1059633
Depends on: 1059845
You need to log in before you can comment on or make changes to this bug.