1302758 - Replace NOISY_FLOATMANAGER in nsBlockDebugFlags.h by nsBlockFrame::gNoisyFloatManager

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

See bug 1276180 comment #0 for the benefits.

Comment 1

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

Attachment: Bug 1302758 Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager.

To see the flag in action, export GECKO_BLOCK_DEBUG_FLAGS="float-manager" and
open a page containing float elements. For example:

$ GECKO_BLOCK_DEBUG_FLAGS="float-manager" ./mach run layout/reftests/floats/orthogonal-floats-1a.html

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

Comment 2

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

Comment on attachment 8791269 [details]
Bug 1302758 Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager.

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

Comment 3

[Daniel Holbert [:dholbert]]

So, when reviewing a patch like this, I kind of like to know the context/history around the two things that are being merged. 

e.g. maybe they were originally one thing but were split for a reason?  Or, maybe one existed but the other was added as a separate thing for some maybe-good reason (not by accident)? Or maybe we tried to replace one with the other but we just didn't finish the job, maybe also for some reason?

If any of these were the case, it'd be important to have that context when merging these two flags.

So -- I was going to ask for you to look up the history (using e.g. searchfox.org), but at this point it's more efficient for me to just look it up myself. :)  (But in the future you might want to consider doing that up-front for bugs like this, to make the reviewer's job easier.)

So, I did some archeology and found:
 * these both originally were named "Space" instead of of "Float" (changed in bug 191448) 
 * NOISY_SPACEMANAGER was added in bug 200347 (attachment 121000 [details] [diff] [review])
 * gNoisySpaceManager was added in bug 7093 ( no patch attachment, changes here: http://searchfox.org/mozilla-central/diff/6b86a5034c351b5c44360a9e2bba124be167ca7a/layout/generic/nsBlockFrame.cpp
 * http://searchfox.org/mozilla-central/diff/6b86a5034c351b5c44360a9e2bba124be167ca7a/layout/generic/nsBlockFrame.cpp )

So, it looks indeed like they were just added independently for no good reason, which means merging them should be fine.

Comment 4

[Daniel Holbert [:dholbert]]

So, a good fraction of this patch is just making unrelated fixes in contextual code (reordering #includes, s/state/input/ in a comment, s/space-manager/float-manager/ in a string, removing an unneceessary static_cast, maybe a few other things).

Could you do those fixes in their own dedicated patch (either as the first or second patch in this series, whichever you prefer), so that the main patch here is more clearly just doing the NOISY_FLOATMANAGER --> DEBUG & gNoisyFloatManager change?

Comment 5

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

Re comment 3:
> If any of these were the case, it'd be important to have that context when
> merging these two flags.

Oh yes. You're right. We should prevent the context from being lost in the rewrite like this.

> (But in the future you might want to consider doing that
> up-front for bugs like this, to make the reviewer's job easier.)

Thank you for doing the archeology, but I should do this myself next time.

Re comment 4:
> Could you do those fixes in their own dedicated patch (either as the first
> or second patch in this series, whichever you prefer), so that the main
> patch here is more clearly just doing the NOISY_FLOATMANAGER --> DEBUG &
> gNoisyFloatManager change?

Sure. I'll make these cosmetic fix in part 1 since mReflowInput.frame cannot build if I replace NOISY_FLOATMANAGER first.

Comment 6

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

Attachment: Bug 1302758 Part 1 - Minor cosmetic fixes in nsFloatManager.

These fixes are in the contextual of Part 2.

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

Comment 7

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

Comment on attachment 8791269 [details]
Bug 1302758 Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager.

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

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8791487 [details]
Bug 1302758 Part 1 - Minor cosmetic fixes in nsFloatManager.

https://reviewboard.mozilla.org/r/78926/#review77544

r=me with one request:

::: layout/generic/nsFloatManager.cpp:588
(Diff revision 1)
>    // Create a new float manager and install it in the reflow
>    // state. `Remember' the old float manager so we can restore it
>    // later.
>    mNew = new nsFloatManager(aPresContext->PresShell(),
>                              mReflowInput.GetWritingMode());
> -  if (! mNew)
> +  if (!mNew) {

Don't bother to fix up this null-check here, because it's actually an entirely unnecessary check!  "mNew = new nsFloatManager(...)" is infallible and doesn't need to be null-checked.

So let's just leave this check untouched in this patch -- and could you post one more patch here, which does the following:
 (a) removes this early-return.
 (b) simplifies this method to return "void" instead of nsresult.

(This isn't quite "cosmetic" since it's changing a function signature & removing some logic; hence, it merits a separate patch.)

(It's a good thing this check never actually fails, because it looks like the one caller doesn't actually even check its return value! :) On the upside, that means we don't have to touch the caller when making this simplification.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8791269 [details]
Bug 1302758 Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager.

https://reviewboard.mozilla.org/r/78724/#review77546

one commit message nit:
> Replace NOISY_FLOATMANAGER by nsBlockFrame::gNoisyFloatManager.

s/by/with/  ("Replace X by Y" doesn't quite make sense -- you mean "Replace X with Y")

Comment 10

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

Attachment: Bug 1302758 Part 3 - Remove nullptr check of operator new in CreateFloatManager.

The operator new is infallable. Also change the return value to void
accordingly. The only caller doesn't even check the return value.

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

Comment 11

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

Comment on attachment 8791487 [details]
Bug 1302758 Part 1 - Minor cosmetic fixes in nsFloatManager.

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

Comment 12

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

Comment on attachment 8791269 [details]
Bug 1302758 Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78724/diff/3-4/

Comment 13

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

Comment on attachment 8791487 [details]
Bug 1302758 Part 1 - Minor cosmetic fixes in nsFloatManager.

https://reviewboard.mozilla.org/r/78926/#review77544

> Don't bother to fix up this null-check here, because it's actually an entirely unnecessary check!  "mNew = new nsFloatManager(...)" is infallible and doesn't need to be null-checked.
> 
> So let's just leave this check untouched in this patch -- and could you post one more patch here, which does the following:
>  (a) removes this early-return.
>  (b) simplifies this method to return "void" instead of nsresult.
> 
> (This isn't quite "cosmetic" since it's changing a function signature & removing some logic; hence, it merits a separate patch.)
> 
> (It's a good thing this check never actually fails, because it looks like the one caller doesn't actually even check its return value! :) On the upside, that means we don't have to touch the caller when making this simplification.)

OK. I undo the cosmetic fix of mNew in Part 1, and remove the check in Part 3. 

BTW, when writing Part 3, I found more "reflow state" in comments, so I replace all the "reflow state" with "reflow input" in nsFloatManager.h and nsFloatManager.cpp, and fix them up in Part 1.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8791640 [details]
Bug 1302758 Part 3 - Remove nullptr check of operator new in CreateFloatManager.

https://reviewboard.mozilla.org/r/79012/#review77592

r=me. Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98f95cc61ab3
Part 1 - Minor cosmetic fixes in nsFloatManager. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3b8d624b89da
Part 2 - Replace NOISY_FLOATMANAGER with nsBlockFrame::gNoisyFloatManager. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1eb767946c2e
Part 3 - Remove nullptr check of operator new in CreateFloatManager. r=dholbert

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98f95cc61ab3
https://hg.mozilla.org/mozilla-central/rev/3b8d624b89da
https://hg.mozilla.org/mozilla-central/rev/1eb767946c2e