1449838 - Minor refactoring of flex item handling code in nsFrame::ComputeSize

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Daniel Holbert [:dholbert]]

Filing this bug for a bit of simplification/refactoring in nsFrame::ComputeSize (layered on top of bug 1436881, in support of more sharable/readable code for bug 1105111).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

This patch doesn't affect behavior - it's just refactoring / de-duplicating.

Review commit: https://reviewboard.mozilla.org/r/232404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232404/

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.

This patch doesn't affect behavior; it just adjusts a variable for clarity.

Really, this variable tracks which axis (inline vs. block) is the main axis (if
we're a flex item). So, this patch morphs the variable to more directly track
that.

The variable's old name 'usingFlexBasisForISize' was confusing, because even
when it was set to 'true', we might not *really* be using the flex-basis in
place of the ISize property. In particular: when we have 'flex-basis:auto', we
don't use 'flex-basis' in place of the ISize/BSize property -- rather, that
indicates that 'flex-basis' is *deferring* to the main-axis size property.

Hopefully the new name/type makes it clearer what we're actually tracking.

Review commit: https://reviewboard.mozilla.org/r/232406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232406/

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

https://reviewboard.mozilla.org/r/232404/#review238066

::: layout/generic/nsFlexContainerFrame.cpp:4401
(Diff revision 1)
> +  auto flexDirection = flexContainer->StylePosition()->mFlexDirection;
> +  bool flexContainerIsRowOriented =
> +    flexDirection == NS_STYLE_FLEX_DIRECTION_ROW ||
> +    flexDirection == NS_STYLE_FLEX_DIRECTION_ROW_REVERSE;
> +

I just realized this doesn't correctly handle "legacy" nsFlexContainerFrame instances, for "display:-webkit-box".

(My Try push on the 'flex-basis:content' bug clued me into this.  The assertion that this patch here is adding in the FlexItem constructor currently fails, on a -webkit-box with -webkit-box-orient:vertical, as in e.g. reftest webkit-box-align-vert-1.html).

Updated patch coming up, to address this.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232404/diff/1-2/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8963470 [details]
Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232406/diff/1-2/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232404/diff/2-3/

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8963470 [details]
Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232406/diff/2-3/

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

https://reviewboard.mozilla.org/r/232404/#review238092

::: layout/generic/nsFlexContainerFrame.cpp:4402
(Diff revision 3)
> +    // For legacy boxes, the main axis is determined by "box-orient", and we can
> +    // just directly check if that's vertical, and compare that to whether the
> +    // item's WM is also vertical:
> +    const nsStyleXUL* styleXUL = flexContainer->StyleXUL();
> +    bool boxOrientIsVertical =
> +      (styleXUL->mBoxOrient == StyleBoxOrient::Vertical);

nit: redundant parenthesis; also, I'm not sure "styleXUL" really helps readability much given it's only used once and "flexContainer->StyleXUL()" fits inline.

::: layout/generic/nsFlexContainerFrame.cpp:4403
(Diff revision 3)
> +    // just directly check if that's vertical, and compare that to whether the
> +    // item's WM is also vertical:
> +    const nsStyleXUL* styleXUL = flexContainer->StyleXUL();
> +    bool boxOrientIsVertical =
> +      (styleXUL->mBoxOrient == StyleBoxOrient::Vertical);
> +    return (flexItemWM.IsVertical() == boxOrientIsVertical);

nit: redundant parenthesis

::: layout/generic/nsFlexContainerFrame.cpp:4409
(Diff revision 3)
> +  }
> +
> +  // For modern CSS flexbox, we get our return value by asking two questions
> +  // and comparing their answers.
> +  // Question 1: does aFrame have the same inline axis as its flex container?
> +  bool frameWMSameAsParent =

super-minor nit: IsOrthogonalTo doesn't imply that the WMs are the same as your variable name suggests (they could have different text-direction for example), so I'd suggest a different name, e.g. itemInlineAxisIsParallelToParent or some such.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8963470 [details]
Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.

https://reviewboard.mozilla.org/r/232406/#review238100

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

https://reviewboard.mozilla.org/r/232404/#review238092

> nit: redundant parenthesis; also, I'm not sure "styleXUL" really helps readability much given it's only used once and "flexContainer->StyleXUL()" fits inline.

Sure, I'll drop |styleXUL|.

I'll keep the parens here, though, since it doesn't sound like you feel strongly, and personally I find
 "A = B == C"
harder to visually parse than
 "A = (B == C)"

(Granted, we do have a linebreak to help here; but it still reads better to my eye.)

> nit: redundant parenthesis

Agreed on these ones - I'll remove them.

> super-minor nit: IsOrthogonalTo doesn't imply that the WMs are the same as your variable name suggests (they could have different text-direction for example), so I'd suggest a different name, e.g. itemInlineAxisIsParallelToParent or some such.

Sure, good point. I'll use your suggested naming.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8963469 [details]
Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232404/diff/3-4/

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8963470 [details]
Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232406/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70c80f12ce8e
part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis(). r=mats
https://hg.mozilla.org/integration/autoland/rev/99e609ea0c47
part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality. r=mats

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/70c80f12ce8e
https://hg.mozilla.org/mozilla-central/rev/99e609ea0c47