Closed Bug 1037177 Opened 5 years ago Closed 5 years ago

Minor flexbox refactoring, in support of bug 1015474

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 3 obsolete files)

4.79 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.91 KB, patch
mats
: review+
Details | Diff | Splinter Review
7.99 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.15 KB, patch
mats
: review+
Details | Diff | Splinter Review
2.84 KB, patch
Details | Diff | Splinter Review
Bug 1015474 is making "min-[width|height]: auto" and "flex-basis:auto" a bit more complicated to resolve.

I'm filing this bug on doing some minor refactoring (without affecting behavior), in support of that bug, so that the patches over there can be cleaner/simpler.
This first patch is for conciseness & better optimization.

This makes us pass an (existing) reflow state into the FlexItem() constructor, which...
 (1) removes the need for a few other args in the constructor (which are taken from the reflow state)

 (2) makes for faster access to style structs in FlexItem(), since per bug 864129 comment 3, frame->StyleFoo() is slower than reflowState->mStyleFoo. (We currently use the former several times in the constructor; this patch makes us do the latter.)
Attachment #8454041 - Flags: review?(mats)
(same patch, now with more lines of context)
Attachment #8454041 - Attachment is obsolete: true
Attachment #8454041 - Flags: review?(mats)
Attachment #8454092 - Flags: review?(mats)
This patch just renames a function.

Currently (as of bug 984711), "ResolveFlexItemMaxContentSizing" measures the auto-height, and applies it to either flex-basis:auto or min-height:auto. This resolving process will be becoming more complex in bug 1015474 (and will be handling "min-width:auto" as well), so this more generic name ("ResolveAutoFlexBasisAndMinSize") is more appropriate.
Attachment #8454094 - Flags: review?(mats)
(one more patch coming in a bit)
This patch splits out the "measuring reflow" part of ResolveAutoFlexBasisAndMinSize() (the renamed function from part 2) into its own function.

Right now, this looks a little silly because the measuring reflow is the bulk of what ResolveAutoFlexBasisAndMinSize() does right now. But bug 1015474 will add more to that function, making this refactoring more valuable.
Attachment #8454119 - Flags: review?(mats)
Attachment #8454119 - Attachment description: part 3: → part 3: Split out auto-height measuring into its own dedicated function.
(previous version of part 3 accidentally changed logic in a way I didn't intend -- I'd swapped out !isMainSizeAuto for !isMainMinSizeAuto. Fixed that here, so that the logic stays the same.)
Attachment #8454119 - Attachment is obsolete: true
Attachment #8454119 - Flags: review?(mats)
Attachment #8454127 - Flags: review?(mats)
Attachment #8454092 - Flags: review?(mats) → review+
Attachment #8454094 - Flags: review?(mats) → review+
Comment on attachment 8454127 [details] [diff] [review]
part 3 v2: Split out auto-height measuring into its own dedicated function.

r=mats
Attachment #8454127 - Flags: review?(mats) → review+
One last (basically trivial) patch here: just bumping the "ResolveAutoFlexBasisAndMinSize()" call from happening just-after GenerateFlexItemForChild() to now happening right-at-the-end of GenerateFlexItemForChild().

(The reason for this change is: in bug 1015474, I'm going to make ResolveAutoFlexBasisAndMinSize() make use of GenerateFlexItemForChild()'s local-var "childRS".)
Attachment #8456694 - Flags: review?(mats)
Attachment #8456694 - Attachment description: part 4: → part 4: Call ResolveAutoFlexBasisAndMinSize at the end of GenerateFlexItemForChild, instead of just after it.
Comment on attachment 8456694 [details] [diff] [review]
part 4: Call ResolveAutoFlexBasisAndMinSize at the end of GenerateFlexItemForChild, instead of just after it.

It seems like GenerateFlexItemForChild now does a lot more than the name
indicates (even more so after bug 1015474 I suspect), so it might be
motivated to rename it and/or at least updating its doc comment.
Feel free to punt that to bug 1015474 though.
Attachment #8456694 - Flags: review?(mats) → review+
Good idea. This patch updates the documentation. Not bothering w/ review, since this is comment-only, but feedback is welcome.
Comment on attachment 8456860 [details] [diff] [review]
part 5: update GenerateFlexItemForChild documentation

Thanks, a minor nit:

/* Returns ...

should be

/** 
 * Returns ...

to follow the standard doc comment format.
Thanks! Fixed locally.
(When posting patches on bug 1015474, I realized that MeasureFlexItemContentHeight & ResolveAutoFlexBasisAndMinSize could use some documentation, too, so I added some in this updated version of part 5.)
Attachment #8456860 - Attachment is obsolete: true
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.