1630457 - Lift some responsibilities from nsFlexContainerFrame::ComputeFinalSize (and maybe rename it)

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

File per this review comments:

(Side note: on top of populating the ReflowOutput, this function also calls FinishReflowWithAbsoluteFrames and NS_FRAME_SET_TRUNCATION -- but maybe we should bump those out of this function and back up to the caller, for better separation-of-concerns? And then we could give this function a clearer name, like e.g. PopulateReflowOutput perhaps... That adjustment may want to happen in a separate patch from this one, though.)

We should do the above cleanup after bug 1627125 lands.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

By taking a closer look at FinishReflowWithAbsoluteFrames, it takes a writable reference of ReflowOutput, and it can modify ReflowOutput::mOverflowAreas further down in the stack in ReflowAbsoluteFrames. I feel it's maybe fine to leave it as is.

Also, ComputeFinalSize also computes nsReflowStatus, so it might be too strict to rename it to PopulateReflowOutput. Though we can make at its documentation clearer by saying:

   * This method computes the metrics to be reported via the flex container's
   * ReflowOutput & nsReflowStatus output parameters in Reflow().

Daniel, do you feel what you suggested in the side note is still worth fixing?

Comment 2

[Daniel Holbert [:dholbert]]

So right now, we kind of have a convention that NS_FRAME_SET_TRUNCATION is the very last thing (or nearly last thing) to happen in every ::Reflow implementation, and it seems worthwhile to maintain that convention. (I think this is universal or near-universal across all of our Reflow method implementations. It seems to be, from a quick searchfox skim.)

And also: for the frames whose reflow methods call "FinishReflowWithAbsoluteFrames", that seems to always happen as the nearly last thing in Reflow, usually right before the NS_FRAME_SET_TRUNCATION call. This seems like a good spot for that, too, because "FinishReflow" seems like something that should explicitly happen at the very end of a Reflow implementation, based on its naming.

So: yeah, I'd still like to move both of these calls out up to the ::Reflow() top-level function, if that doesn't complexify things.

(You're correct that this would mean we're making additional modifications to the ReflowOutput outside of ComputeFinalSize. That's fine, I think.)

Also, ComputeFinalSize also computes nsReflowStatus, so it might be too strict to rename it to PopulateReflowOutput. Though we can make at its documentation clearer by saying:

   * This method computes the metrics to be reported via the flex container's
   * ReflowOutput & nsReflowStatus output parameters in Reflow().

That documentation snippet sounds like a good addition. And I still prefer PopulateReflowOutput() over ComputeFinalSize() -- it's still more accurate, even if it doesn't entirely encompass the scope of what the function does. :)

Daniel, do you feel what you suggested in the side note is still worth fixing?

Yes please. Thanks! :)

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thanks for the clarification! I'll take this.

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1630457 - Rename ComputeFinalSize to PopulateReflowOutput along with some minor improvements. r=dholbert

1. Move FinishReflowWithAbsoluteFrames and NS_FRAME_SET_TRUNCATION to Reflow().
2. Improve documentation for PopulateReflowOutput.

Depends on D71613

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28da49962f2a
Rename ComputeFinalSize to PopulateReflowOutput along with some minor improvements. r=dholbert

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/28da49962f2a