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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

57 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
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.
Attachment #8963469 - Flags: review?(mats) → review+

Comment 9

a year ago
mozreview-review
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
Attachment #8963470 - Flags: review?(mats) → review+
(Assignee)

Comment 10

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
Pushed by dholbert@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70c80f12ce8e
https://hg.mozilla.org/mozilla-central/rev/99e609ea0c47
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.