Closed
Bug 922410
Opened 11 years ago
Closed 11 years ago
Consolidate loops in MainAxisPositionTracker constructor (in nsFlexContainerFrame.cpp)
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file)
1.95 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Haven't tested this yet, but I think this should do it.
Assignee | ||
Comment 2•11 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•11 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 4•11 years ago
|
||
Comment on attachment 812353 [details] [diff] [review] fix v1 r=mats
Attachment #812353 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/111be6d857e1
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite-
Comment 6•11 years ago
|
||
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.
Description
•