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

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: lpy)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=dholbert][lang=c++])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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).
(Reporter)

Comment 1

5 years ago
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.
Whiteboard: [mentor=dholbert][lang=c++]
(Assignee)

Updated

5 years ago
Assignee: nobody → pylaurent1314
(Reporter)

Comment 2

5 years ago
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
Blocks: 113424
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.
(Assignee)

Comment 4

5 years ago
Created attachment 8360963 [details] [diff] [review]
bug959874.patch

Thank you. This patch can be compiled correctly and pass the tests.
Attachment #8360963 - Flags: review?(dholbert)
(Reporter)

Comment 5

5 years ago
(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.)
(Reporter)

Comment 6

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

Looks great! Just one nit:

># HG changeset patch
># Parent 1f835fe670d7c83dd5235a67dcfcd77c1b58d24f
># User Peiyong Lin <pylaurent1314@gmail.com>
>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.
Attachment #8360963 - Flags: review?(dholbert) → review+
(Reporter)

Comment 7

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

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 8

5 years ago
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!
Flags: in-testsuite-
(Assignee)

Comment 9

5 years ago
Thank you for changing the checkin comment for me. :)
https://hg.mozilla.org/mozilla-central/rev/0f0b3d2a7af5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.