Consolidate loops in MainAxisPositionTracker constructor (in nsFlexContainerFrame.cpp)

RESOLVED FIXED in mozilla27

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Mats has some suggestions for consolidating loops in the MainAxisPositionTracker() constructor, in bug 921522 comment 15:

> >+  for (uint32_t i = 0; i < aItems.Length(); i++) {
> >+    nscoord itemMarginBoxMainSize =
> >+      aItems[i].GetMainSize() +
> >+      aItems[i].GetMarginBorderPaddingSizeInAxis(aAxisTracker.GetMainAxis());
> >+    mPackingSpaceRemaining -= itemMarginBoxMainSize;
> >   }
> > 
> >   if (mPackingSpaceRemaining > 0) {
> >     for (uint32_t i = 0; i < aItems.Length(); i++) {
> >       mNumAutoMarginsInMainAxis += aItems[i].GetNumAutoMarginsInAxis(mAxis);
> >     }
> >   }
> 
> I'm not sure if it's common that mPackingSpaceRemaining ends up
> being > 0, but I'll just note that it would be possible to 
> avoid the second loop if you count the auto-margins in the
> first loop instead, using a local var, and then just assign
> mNumAutoMarginsInMainAxis to the local var inside the 'if'.
> 
> In that case it might also be worth putting the item in a local:
>   const FlexItem& item = aItems[i];
> and using that instead of doing three separate aItems[i].

Filing this bug to do that minor cleanup.
(Assignee)

Comment 1

4 years ago
Created attachment 812353 [details] [diff] [review]
fix v1

Haven't tested this yet, but I think this should do it.
(Assignee)

Comment 2

4 years ago
Comment on attachment 812353 [details] [diff] [review]
fix v1

Yup, this builds and passes tests, so no change.

I'm basically implementing exactly what you suggested, except I'm optimistically modifying mNumAutoMarginsInMainAxis directly (rather than introducing a local var with approximately the same name), and then resetting it to 0 if it turns out we don't have enough space.
Attachment #812353 - Flags: review?(matspal)
(Assignee)

Comment 3

4 years ago
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Yup, this builds and passes tests, so no change.

("so no change" = "so there's no functional change, as expected")
Comment on attachment 812353 [details] [diff] [review]
fix v1

r=mats
Attachment #812353 - Flags: review?(matspal) → review+
(Assignee)

Updated

4 years ago
Flags: in-testsuite-

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/111be6d857e1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.