Closed Bug 922410 Opened 11 years ago Closed 11 years ago

Consolidate loops in MainAxisPositionTracker constructor (in nsFlexContainerFrame.cpp)

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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.
Attached patch fix v1Splinter Review
Haven't tested this yet, but I think this should do it.
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)
(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+
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/111be6d857e1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: