1288992 - Rename nsHTMLReflowMetrics variables to ReflowOutput

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

This is a followup of bug 1277129.  Need to rename variables name involving nsHTMLReflowMetrics as well

For example: 
1. http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/layout/mathml/nsMathMLContainerFrame.cpp#127
2. http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/layout/mathml/nsMathMLContainerFrame.cpp#1220

Comment 1

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

Attachment: Bug 1288992 Part 1 - Rename mSize and ReflowMetrics() in nsMathMLContainerFrame::RowChildFrameIterator.

This is to avoid the conflict that both the method name and the class
name are called ReflowOutput after mass renaming in Part 2.

Review commit: https://reviewboard.mozilla.org/r/67104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67104/

Comment 2

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

Attachment: Bug 1288992 Part 2 - Rename ReflowMetrics variables to ReflowOutput.

This patch is generated by the following scripts:

function rename() {
find layout\
     -type f\
     ! -path "./obj*"\
     ! -path "./.git"\
     ! -path "./.hg"\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "([[:alpha:]]*)([rR])eflowMetrics" "\1\2eflowOutput"

Review commit: https://reviewboard.mozilla.org/r/67106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67106/

Comment 3

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

Attachment: Bug 1288992 Part 3 - Rename local variables named metrics to reflowOutput.

This patch is edited manually.

Review commit: https://reviewboard.mozilla.org/r/67110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67110/

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> function rename() {
> find layout\
>      -type f\
>      ! -path "./obj*"\
>      ! -path "./.git"\
>      ! -path "./.hg"\

Note -- since you're restricting your script to the "layout/" subdirectory, you probably don't need to bother excluding .hg, .git, obj* subdirectories. There shouldn't be any such subdirectories inside of the layout subtree, I expect.

(Not that it matters; just mentioning so you can make your commands simpler in future similar search-and-replace, if you like.)

Comment 5

[Daniel Holbert [:dholbert]]

I want to think a bit more about "Part 1" (since Size() is extremely nsSize-sounding, and I'm wondering if we can come up with a better name), but in the meantime I'll post review feedback for parts 2 & 3.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8774662 [details]
Bug 1288992 Part 2 - Rename ReflowMetrics variables to ReflowOutput.

https://reviewboard.mozilla.org/r/67106/#review64132

r=me on part 2, with nits below addressed:

::: layout/generic/nsBlockFrame.cpp:157
(Diff revision 1)
>    { "intrinsic", &nsBlockFrame::gNoisyIntrinsic },
>    { "float-manager", &nsBlockFrame::gNoisyFloatManager },
>    { "verify-lines", &nsBlockFrame::gVerifyLines },
>    { "damage-repair", &nsBlockFrame::gNoisyDamageRepair },
>    { "lame-paint-metrics", &nsBlockFrame::gLamePaintMetrics },
> -  { "lame-reflow-metrics", &nsBlockFrame::gLameReflowMetrics },
> +  { "lame-reflow-metrics", &nsBlockFrame::gLameReflowOutput },

I don't think you want to touch gLameReflowMetrics.  As far as I can tell, it has nothing to do with nsHTMLReflowMetrics.  It's just a flag that controls whether or not we dump some metrics (about elapsed time, lines reflowed, line distances, etc.) at the end of block reflow.  For this sort of thing, the old name ("reflow metrics" i.e. reflow statistics) is actually a much more appropriate name for this flag than "reflow output".  Also, it's controlled by a debug-string named "lame-reflow-metrics" which this patch is not renaming, as shown in the above-quoted line, so it should stay aligned with the name of that debug string.

SO: Please don't rename this variable. To keep everything super-automated, if you like, you could (for example) add a second "sed" command to your list-of-commands-to-generate-this-patch, which would simply do a second-pass search-and-replace to rename gLameReflowOutput back to gLameReflowMetrics.  Or maybe there's a better way to exclude it. One way or another, please exclude this variable from your renames here.

::: layout/tables/nsTableFrame.cpp:2993
(Diff revision 1)
>                                                desiredSize.PhysicalSize()),
>               desiredSize, origTfootRect, origTfootVisualOverflow);
>  }
>  
>  // Reflow the children based on the avail size and reason in aReflowInput
> -// update aReflowMetrics a aStatus
> +// update aReflowOutput a aStatus

Instead of doing the rename here, please just drop "aReflowMetrics a" from this line.

There is no variable named "aReflowMetrics" (nor is there one named aReflowOutput, after your rename) here, so the comment is just nonsensical.

(It looks like it was always nonsensical, too -- the first revision with this comment was http://searchfox.org/mozilla-central/rev/a81a26997894997623a06abbb6f04729a32f79de/layout/tables/nsTableFrame.cpp#2812 and there was no "aReflowMetrics" at that point either.)

You could drop this line as part of this patch (with a mention in the extended commit message alongside the scripted commands), Or if you prefer you could just clean this up in a trivial "part 0" patch.  Bottom line, there's no sense in incurring churn (and making the patch larger) with a rename on a comment that already makes no sense & wants removing. :)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8774663 [details]
Bug 1288992 Part 3 - Rename local variables named metrics to reflowOutput.

https://reviewboard.mozilla.org/r/67110/#review64138

<p>Part 3 looks good; r=me</p>

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8774661 [details]
Bug 1288992 Part 1 - Rename mSize and ReflowMetrics() in nsMathMLContainerFrame::RowChildFrameIterator.

https://reviewboard.mozilla.org/r/67104/#review64150

::: layout/mathml/nsMathMLContainerFrame.cpp:1220
(Diff revision 1)
>      return *this;
>    }
>  
>    nsIFrame* Frame() const { return mChildFrame; }
>    nscoord X() const { return mX; }
> -  const ReflowOutput& ReflowMetrics() const { return mSize; }
> +  const ReflowOutput& Size() const { return mSize; }

I don't like calling this "Size()", because:
(1) It sounds too much like nsFrame::GetSize(), and/or like it would return a nsSize or nsIntSize

(2) The *one* existing caller of this method uses the result in a context that clearly expects a ReflowOutput (FinishReflowChild's ReflowOutput parameter) -- not something that expects a size.  So it looks confusing in callsite -- it looks like we're invoking a different (non-existing) FinishReflowChild overload which might take a nsSize parameter, for example.


I'd say we should just call this method "GetReflowOutput()".  (That technically violates our convention about "GetFoo()" functions being allowed to return null; but there's no serious ambiguity in this case, since it returns a reference, not a pointer.)  I can't come up with a better name at the moment; if we come up with one later, we can do a further-rename with a 2-line change.

(While you're at it, mSize should probably be renamed to mReflowOutput as well, for consistency/clarity -- since it really is a ReflowOutput, not just a size; and we do use it for more than just its size components -- e.g. there's code that looks up its blockstartascent, its boundingmetrics, etc.  With that rename, this one-line getter would make more sense, rather than "GetReflowOutput() { reutrn mSize; }" if you just rename the function without renaming the variable.)

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> <p>Part 3 looks good; r=me</p>

/me shakes fist at MozReview generating/inserting weird HTML formatting into bugzilla comments...

Comment 10

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

https://reviewboard.mozilla.org/r/67104/#review64150

> I don't like calling this "Size()", because:
> (1) It sounds too much like nsFrame::GetSize(), and/or like it would return a nsSize or nsIntSize
> 
> (2) The *one* existing caller of this method uses the result in a context that clearly expects a ReflowOutput (FinishReflowChild's ReflowOutput parameter) -- not something that expects a size.  So it looks confusing in callsite -- it looks like we're invoking a different (non-existing) FinishReflowChild overload which might take a nsSize parameter, for example.
> 
> 
> I'd say we should just call this method "GetReflowOutput()".  (That technically violates our convention about "GetFoo()" functions being allowed to return null; but there's no serious ambiguity in this case, since it returns a reference, not a pointer.)  I can't come up with a better name at the moment; if we come up with one later, we can do a further-rename with a 2-line change.
> 
> (While you're at it, mSize should probably be renamed to mReflowOutput as well, for consistency/clarity -- since it really is a ReflowOutput, not just a size; and we do use it for more than just its size components -- e.g. there's code that looks up its blockstartascent, its boundingmetrics, etc.  With that rename, this one-line getter would make more sense, rather than "GetReflowOutput() { reutrn mSize; }" if you just rename the function without renaming the variable.)

I was thinking about GetReflowOutput() at the first place, and realizing it violates the convention. Somehow I ended up choosing a worse name though ... Will change back to GetReflowOutput() as well as the rename mSize.

Comment 11

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

https://reviewboard.mozilla.org/r/67106/#review64132

> I don't think you want to touch gLameReflowMetrics.  As far as I can tell, it has nothing to do with nsHTMLReflowMetrics.  It's just a flag that controls whether or not we dump some metrics (about elapsed time, lines reflowed, line distances, etc.) at the end of block reflow.  For this sort of thing, the old name ("reflow metrics" i.e. reflow statistics) is actually a much more appropriate name for this flag than "reflow output".  Also, it's controlled by a debug-string named "lame-reflow-metrics" which this patch is not renaming, as shown in the above-quoted line, so it should stay aligned with the name of that debug string.
> 
> SO: Please don't rename this variable. To keep everything super-automated, if you like, you could (for example) add a second "sed" command to your list-of-commands-to-generate-this-patch, which would simply do a second-pass search-and-replace to rename gLameReflowOutput back to gLameReflowMetrics.  Or maybe there's a better way to exclude it. One way or another, please exclude this variable from your renames here.

Agree, and the easiest way to preserve the naming for gLameReflowMetrics is to add a second "sed".  I also update the script to drop the exclusion for excluding .hg, .git, obj subdirectories as you suggested in comment 4. Thanks!

> Instead of doing the rename here, please just drop "aReflowMetrics a" from this line.
> 
> There is no variable named "aReflowMetrics" (nor is there one named aReflowOutput, after your rename) here, so the comment is just nonsensical.
> 
> (It looks like it was always nonsensical, too -- the first revision with this comment was http://searchfox.org/mozilla-central/rev/a81a26997894997623a06abbb6f04729a32f79de/layout/tables/nsTableFrame.cpp#2812 and there was no "aReflowMetrics" at that point either.)
> 
> You could drop this line as part of this patch (with a mention in the extended commit message alongside the scripted commands), Or if you prefer you could just clean this up in a trivial "part 0" patch.  Bottom line, there's no sense in incurring churn (and making the patch larger) with a rename on a comment that already makes no sense & wants removing. :)

You're right. This comment is useless. I'll drop it in part 0.

Comment 12

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

Attachment: Bug 1288992 Part 0 - Drop the nonsensical comment for nsTableFrame::ReflowChildren.

Review commit: https://reviewboard.mozilla.org/r/67368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67368/

Comment 13

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

Comment on attachment 8774661 [details]
Bug 1288992 Part 1 - Rename mSize and ReflowMetrics() in nsMathMLContainerFrame::RowChildFrameIterator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67104/diff/1-2/

Comment 14

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

Comment on attachment 8774662 [details]
Bug 1288992 Part 2 - Rename ReflowMetrics variables to ReflowOutput.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67106/diff/1-2/

Comment 15

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

Comment on attachment 8774663 [details]
Bug 1288992 Part 3 - Rename local variables named metrics to reflowOutput.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67110/diff/1-2/

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8775021 [details]
Bug 1288992 Part 0 - Drop the nonsensical comment for nsTableFrame::ReflowChildren.

https://reviewboard.mozilla.org/r/67368/#review64398

r=me on part 0

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8774661 [details]
Bug 1288992 Part 1 - Rename mSize and ReflowMetrics() in nsMathMLContainerFrame::RowChildFrameIterator.

https://reviewboard.mozilla.org/r/67104/#review64400

r=me on updated part 1

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b48a2b6ec3a8
Part 0 - Drop the nonsensical comment for nsTableFrame::ReflowChildren. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/53d6028d2809
Part 1 - Rename mSize and ReflowMetrics() in nsMathMLContainerFrame::RowChildFrameIterator. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7e7af6bd33b8
Part 2 - Rename ReflowMetrics variables to ReflowOutput. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f06109a9a08d
Part 3 - Rename local variables named metrics to reflowOutput. r=dholbert

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b48a2b6ec3a8
https://hg.mozilla.org/mozilla-central/rev/53d6028d2809
https://hg.mozilla.org/mozilla-central/rev/7e7af6bd33b8
https://hg.mozilla.org/mozilla-central/rev/f06109a9a08d