Closed Bug 1148298 Opened 5 years ago Closed 5 years ago

Reduce explicit calls to "IsAxisHorizontal(AxisOrientationType)" in nsFlexContainerFrame.cpp, outside of FlexboxAxisTracker

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

tl;dr: I want to replace checks of the form...
> IsAxisHorizontal(aAxisTracker.GetMainAxis())
...with this:
> if (aAxisTracker.IsMainAxisHorizontal())

MOTIVATION:

Right now, FlexboxAxisTracker stores the physical axis-orientation for the main-axis and cross-axis. This makes it easy to convert flex-relative coordinates to *physical* coordinates, but it's harder to convert them to logical coordinates. We need to address this. My current plan is for FlexboxAxisTracker to now store (1) the writing mode, and (2) a representation of how our main/cross axes map to the logical axes. Not exactly sure what that representation will be right now, but we'll eventually want to drop the purely-physical AxisOrientationType member-variables.

So, to move in this direction, I want to reduce usages of AxisOrientationType. Most usages are in "IsAxisHorizontal()" checks (which takes a AxisOrientationType arg).  I'd like to replace these checks with FlexboxAxisTracker methods "IsMainAxisHorizontal" and "IsCrossAxisHorizontal", so that I can change the underlying FlexboxAxisTracker representation (and rip out AxisOrientationType) without breaking the callers.

(Even after moving to a more logical representation, we'll likely still need these "IsMainAxisHorizontal()/IsCrossAxisHorizontal()" methods, and FlexboxAxisTracker will still be able to implement them, by querying how we our main/cross axes map onto the logical axes, & checking how the logical axes map onto physical axes using the writing mode.)
This patch adds methods IsMainAxisHorizontal() and IsCrossAxisHorizontal() to FlexboxAxisTracker, and converts nearly all "IsAxisHorizontal" callers to use them.

(There are a few more IsAxisHorizontal callers that require some refactoring to get a FlexboxAxisTracker in-scope for us to use; those will be fixed in subsequent patches.)

(review note: you may want to review this patch using a merge tool like "meld" which highlights just the changed part of each line. Essentially all of the changes in this patch look like this:
-    if (IsAxisHorizontal(aAxisTracker.GetCrossAxis())) {
+    if (aAxisTracker.IsCrossAxisHorizontal()) {
and they're easy to quickly sanity-check if you can have the computer highlight the changed parts of the line for you.)
Attachment #8584304 - Flags: review?(mats)
This patch refactors code a bit to remove one more "IsAxisHorizontal" call.

This call is in a static helper-function GetSizePropertyForAxis(). I'm refactoring it to be a method on FlexboxAxisTracker -- and I'm making it cross-axis specific while we're at it, since that's the only axis it's used for.  So we can rip out IsAxisHorizontal() and replace it with a call to IsCrossAxisHorizontal(), ultimately.

(If you're curious why we only need this method in the cross-axis, it's because "align-self:stretch" is specced as *only* stretching flex items with "auto" in their cross-axis size property.  So, we need this special check of the cross-size property (width or height), to satisfy that requirement.)
Attachment #8584306 - Flags: review?(mats)
This patch makes just changes a function-signature to take a FlexboxAxisTracker, instead of a AxisOrientationType for the cross-axis, so that we can replace one last IsAxisHorizontal() method with our new FlexboxAxisTracker IsCrossAxisHorizontal method.
Attachment #8584307 - Flags: review?(mats)
(I'm updating part 3 to remove some now-stale documentation about the modified function needing to be told the cross axis. Now it takes a FlexboxAxisTracker instead, and the comment isn't that helpful anyway, so I'm just removing it.)
Attachment #8584307 - Attachment is obsolete: true
Attachment #8584307 - Flags: review?(mats)
Attachment #8584313 - Flags: review?(mats)
(er, on further reflection, the comment was actually a bit helpful, to document what the passed-in edge represents. So, I'm now just tweaking it instead of removing it.

I also qreffed with more lines of context, so that the rest of the function's documentation is visible. Not that it matters too much, since this is just a targeted refactoring that should preserve behavior.

Sorry for the churn.)
Attachment #8584313 - Attachment is obsolete: true
Attachment #8584313 - Flags: review?(mats)
Attachment #8584317 - Flags: review?(mats)
This final patch just moves IsAxisHorizontal() to be a static private method on FlexboxAxisTracker, to ensure that I caught all of the usages, and to (mostly) prevent new usages from being inadvertantly added.
Attachment #8584320 - Flags: review?(mats)
Blocks: 1148294
Attachment #8584304 - Flags: review?(mats) → review+
Comment on attachment 8584306 [details] [diff] [review]
part 2: Convert GetSizePropertyForAxis() into a FlexboxAxisTracker method, to remove another IsAxisHorizontal() check

Since you're changing the name anyway: perhaps CrossSizeStyle would
be more accurate?  "Property" sounds a bit like it could return some
symbol representing 'width' or 'height'.
Attachment #8584306 - Flags: review?(mats) → review+
Comment on attachment 8584317 [details] [diff] [review]
part 3 v3:  Change a function-signature to take a FlexboxAxisTracker intead of an AxisOrientationType, to remove one last IsHorizontal() check

Looks like the indentation got messed up in the last hunk:

>       nscoord itemBaselineOffset =
>-        aItem.GetBaselineOffsetFromOuterCrossEdge(mAxis,
>+        aItem.GetBaselineOffsetFromOuterCrossEdge(
>           aAxisTracker.AreAxesInternallyReversed() ?
>-          eAxisEdge_End : eAxisEdge_Start);
>+          eAxisEdge_End : eAxisEdge_Start,
>+       aAxisTracker);

I think aAxisTracker should line up with (the start of) the first arg,
and since that arg spans two lines I would indent it to make it easier
to see that.  Something like:

      nscoord itemBaselineOffset =
        aItem.GetBaselineOffsetFromOuterCrossEdge(
          aAxisTracker.AreAxesInternallyReversed() ?
            eAxisEdge_End : eAxisEdge_Start,
          aAxisTracker);
Attachment #8584317 - Flags: review?(mats) → review+
Comment on attachment 8584320 [details] [diff] [review]
part 4: Move "IsAxisHorizontal" to be a static private method, so that new callers can't be added

I guess you're going to remove this function at some point,
but just in case you'll keep it: the return type should go
on the same line to follow the local convention here, AFAICT:

  static inline bool IsAxisHorizontal(AxisOrientationType aAxis) {
Attachment #8584320 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #8)
> Since you're changing the name anyway: perhaps CrossSizeStyle would
> be more accurate?  "Property" sounds a bit like it could return some
> symbol representing 'width' or 'height'.

Hmm, good point. I think "Style" is similarly ambiguous, so I changed this to "ComputedCrossSize", which I think is a bit clearer.

(ComputedCrossSize[Property|Style] would perhaps be ideal, except it makes the line too long in the one call-site & requires awkward wrapping)
(In reply to Mats Palmgren (:mats) from comment #9)
> Looks like the indentation got messed up in the last hunk:
[...]
> I think aAxisTracker should line up with (the start of) the first arg,
> and since that arg spans two lines I would indent it to make it easier
> to see that.

"tab" in emacs auto-indents it the way I had it, but I agree it's pretty unreadable.

I ended up just splitting out the eAxisEdge ternary-expression into a temporary local variable, to make it clearer.
(In reply to Mats Palmgren (:mats) from comment #10)
> I guess you're going to remove this function at some point,
> but just in case you'll keep it: the return type should go
> on the same line to follow the local convention here

Yup, you're right -- thanks for catching that.

Thanks for the reviews!
You need to log in before you can comment on or make changes to this bug.