1065258 - Refactor nsMemoryInfoDumper.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

I have a stack of small patches.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Inline TempDirMemoryFinishCallback()

I moved the code, nothing else.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Rename some variables

"mr" is short for "memory reports", but that's not obvious. Also, some field
members had "mr" prefixes which goes against the usual "mFoo" naming.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove TempDirMemoryFinishCallback::mIdentifier, which is dead

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Move FinishReportingCallback earlier

This is necessary for one of the subsequent patches.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Make TempDirMemoryFinishCallback a nsIFinishDumpingCallback object

This one's a little trickier.

- FinishReportCallback::Callback got its |writer| via |aData|; I changed it to
  get it via a class member, which is how TempDirMemoryFinishCallback does it.

- FinishReportingCallback::Callback and TempDirMemoryFinishCallback::Callback
  have some overlap. So I made the latter call the former, which then calls on
  to do the rest of the TempDir finish-up stuff.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Factor out code shared by DumpMemoryReportsToNamedFile() and DumpMemoryInfoToTempDir()

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Move DumpMemoryReportsToNamedFile earlier

This is just for aesthetics -- it makes sense to be next to DumpMemoryInfoToTempDir().

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Rename TempDirMemoryFinishCallback

Because the "Memory" part isn't helpful.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8487013 [details] [diff] [review]
(part 2) - Rename some variables

Review of attachment 8487013 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.  I remember thinking the mrFoo variables were a little confusing when I first saw them.

Comment 10

[Nicholas Nethercote [inactive]]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=7f612080cd6d

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40e1a85a139e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2019276e57d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2f3e9c3106
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5e7447df864
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43ce1b77bee
https://hg.mozilla.org/integration/mozilla-inbound/rev/68ea49d31316
https://hg.mozilla.org/integration/mozilla-inbound/rev/468af76c47a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef537d9428fd

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40e1a85a139e
https://hg.mozilla.org/mozilla-central/rev/2019276e57d6
https://hg.mozilla.org/mozilla-central/rev/dd2f3e9c3106
https://hg.mozilla.org/mozilla-central/rev/d5e7447df864
https://hg.mozilla.org/mozilla-central/rev/f43ce1b77bee
https://hg.mozilla.org/mozilla-central/rev/68ea49d31316
https://hg.mozilla.org/mozilla-central/rev/468af76c47a7
https://hg.mozilla.org/mozilla-central/rev/ef537d9428fd