959874 - nsContainerFrame::ReflowChild and FinishReflowChild have their aReflowState and aDesiredSize args in swapped positions

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Noticed while reviewing bug 101019:

We have what appears to be an unnecessary inconsistency in the nsContainerFrame ReflowChild vs. FinishReflowChild function-signatures.

ReflowChild's function-signature is:
201   nsresult ReflowChild(nsIFrame*                      aKidFrame,
202                        nsPresContext*                 aPresContext,
203                        nsHTMLReflowMetrics&           aDesiredSize,
204                        const nsHTMLReflowState&       aReflowState,
205                        nscoord                        aX,
206                        nscoord                        aY,
207                        uint32_t                       aFlags,
208                        nsReflowStatus&                aStatus,
209                        nsOverflowContinuationTracker* aTracker = nullptr);

FinishReflowChild's function-signature is:
228   static nsresult FinishReflowChild(nsIFrame*                  aKidFrame,
229                                     nsPresContext*             aPresContext,
230                                     const nsHTMLReflowState*   aReflowState,
231                                     const nsHTMLReflowMetrics& aDesiredSize,
232                                     nscoord                    aX,
233                                     nscoord                    aY,
234                                     uint32_t                   aFlags);

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.h

Note that FinishReflowChild's parameters exactly match the first 7 args of ReflowChild (ignoring constness), with one exception: aReflowState and aDesiredSize are in swapped order.

I can't imagine there's any reason for this; it must've just been an oversight. We should make these consistent.

We probably should modify FinishReflowChild -- putting its nsHTMLReflowMetrics parameter before the nsHTMLReflowState parameter -- for consistency with the ordering in nsIFrame::Reflow (which matches ReflowChild).

Comment 1

[Daniel Holbert [:dholbert]]

This should be relatively straightforward as a mentored bug.

Someone should be able to fix this by searching the codebase for all instances of "FinishReflowChild(" and swapping the order of the 3rd and 4th parameter.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for taking the bug, Peiyong! Let me know if you need any help. Hopefully, this should be the type of patch where if it compiles, it's probably correct. :)

(Also, for the record -- I did some CVS archeology and confirmed that the ordering-reversal here does seem to have just been accidental. Bug 113424 is where we first added the nsHTMLReflowState parameter to FinishReflowChild(), and it's been reversed with respect to ReflowChild ever since that patch landed.[1] That bug doesn't have any discussion of parameter ordering, or any discussion of trying to match (or avoid matching) ReflowChild, so I think the author just didn't notice that he was making the function signatures almost-the-same-with-two-params-reversed.)

[1] http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsContainerFrame.h&rev=3.46&mark=135-136,161-162#133

Comment 3

[Mats Palmgren (inactive)]

Another thing that annoys me is the inconsistent naming.  A brief analysis
using 'grep' shows 238 aDesiredSize, 62 aMetrics, and 31 "other".  This makes
it hard to move code between methods that use different names, or just to
write code that touches many methods since you constantly have to lookup
what the name is in the current method.

Comment 4

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug959874.patch

Thank you. This patch can be compiled correctly and pass the tests.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> Another thing that annoys me is the inconsistent naming.  A brief analysis
> using 'grep' shows 238 aDesiredSize, 62 aMetrics, and 31 "other".

(Agreed. though I believe we're still eventually intending to rename the structures themselves, along the lines of reflowstate --> reflowinputs & reflowmetrics --> reflowresults, or something like that. Maybe that would be a good time to do the renaming, too, if we haven't gotten around to it by then.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8360963 [details] [diff] [review]
bug959874.patch

Looks great! Just one nit:

># HG changeset patch
># Parent 1f835fe670d7c83dd5235a67dcfcd77c1b58d24f
># User Peiyong Lin <[e-mail]>
>Bug 959874 - nsContainerFrame::ReflowChild and FinishReflowChild have their aReflowState and aDesiredSize args in swapped positions. r=dholbert

The commit message (checkin comment) should describe the change, not the problem. For reference, see:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

So, this should be something more like:
"Bug 959874 - Swap two arguments in nsContainerFrame::FinishReflowChild to make it consistent with ReflowChild. r=dholbert"

r=me with that fixed.

Comment 7

[Daniel Holbert [:dholbert]]

I verified that this builds correctly, on my local machine. Sanity-check Try run (builds only):
  https://tbpl.mozilla.org/?tree=Try&rev=120b849f391f

Comment 8

[Daniel Holbert [:dholbert]]

Try run looks good, so I went ahead and landed this (with a tweaked commit message, per comment 6):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0b3d2a7af5

Thanks for the patch!

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Thank you for changing the checkin comment for me. :)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f0b3d2a7af5