Closed Bug 1180107 Opened 7 years ago Closed 6 years ago

Refactor out helper-function "CanMainSizeInfluenceCrossSize" for use in flex layout

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Filing this bug for some patches that continue the flexbox logical-dimensions conversion work done in bug 115829.
This patch doesn't affect behavior at all -- it just refactors an existing multi-part condition into a helper-function. (and makes the logic easier to follow by considering the cases separately)

This is a step towards removing a IsCrossAxisHorizontal() call (in the condition that I'm refactoring out here).  That removal will happen in a later patch here.
Attachment #8629234 - Flags: review?(mats)
Blocks: 1186147
Comment on attachment 8629234 [details] [diff] [review]
fix: Refactor out FlexItem::CanMainSizeInfluenceCrossSize

>+  // Once the main size has been resolved, should we bother doing layout to
>+  // establish the cross size?
>+  bool CanMainSizeInfluenceCrossSize(const FlexboxAxisTracker& aAxisTracker);
...
>+// Indicates whether it's possible for main-size resolution to influence this
>+// item's cross size.
>+bool
>+FlexItem::CanMainSizeInfluenceCrossSize(const FlexboxAxisTracker& aAxisTracker)

The latter comment seems redundant.

It looks like the method can be 'const'?
Attachment #8629234 - Flags: review?(mats) → review+
Blocks: 1267462
Sorry for letting this bug go quiet!  There's more work to be done here, but I'd like to go ahead & land the attached patch, independent of the remaining logical-coordinates work.

So: I'm morphing this bug to be focused on the attached patch, and I spun off bug 1267462 for the remaining logical-coordinate conversions.
Summary: Use logical dimensions/coordinates more in flex layout → Refactor out helper-function "CanMainSizeInfluenceCrossSize" for use in flex layout
Attachment #8629234 - Attachment description: part 1: Refactor out FlexItem::CanMainSizeInfluenceCrossSize → fix: Refactor out FlexItem::CanMainSizeInfluenceCrossSize
Addressed review comments & landed. ^^
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/5e212cfe5849
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Blocks: 1030952
Comment on attachment 8629234 [details] [diff] [review]
fix: Refactor out FlexItem::CanMainSizeInfluenceCrossSize

Requesting approval to uplift this refactoring-patch.  In fact, I really want to uplift the patches in bug 1030952 -- and that bug's patches layer on top of this patch, so we need this patch first. (bug 1030952 modifies the helper-function that I'm refactoring out in this patch.)

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Won't be able to backport bug 1030952 (which will mean images & other elements with an aspect ratio may get stretched only in Firefox)

[Describe test coverage new/current, TreeHerder]: Our flexbox code has good test coverage (even better after the tests added in bug 1030952). This patch has been getting testing on Nightly for a few days now.

[Risks and why]: This is as low-risk as it gets. This patch is just refactoring out an existing check into a helper-function, so that it's easier to understand & modify later on. No functional changes.

[String/UUID change made/needed]: None.
Attachment #8629234 - Flags: approval-mozilla-aurora?
Comment on attachment 8629234 [details] [diff] [review]
fix: Refactor out FlexItem::CanMainSizeInfluenceCrossSize

Has coverage on m-c, plus we want dholbert's flexbox refactoring world domination plans to happen
Attachment #8629234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks! :D
You need to log in before you can comment on or make changes to this bug.