Closed Bug 1449838 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Version:
57 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

Bug 1449838 part 1: Add utility function nsFlexContainerFrame::IsItemInlineAxisMainAxis().
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1449838 part 2: Morph 'usingFlexBasisForISize' variable in nsFrame::ComputeSize* methods, to better reflect reality.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
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 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 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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/70c80f12ce8e
https://hg.mozilla.org/mozilla-central/rev/99e609ea0c47
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: