671280 - Rename KIND_MAPPED to KIND_NONHEAP in nsIMemoryReporter

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

See bug 664659 comment 32: I think KIND_NONHEAP is a better name than KIND_MAPPED.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: njn

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 3

[Justin Lebar (not reading bugmail)]

In addition to looking through all the files which contained "KIND_MAPPED", I also looked through the results of

 $ hg locate | xargs grep -l 'nsIMemoryReporter' | xargs grep -i mapped

to see if I'd missed passing references to "mapped".  I didn't see any.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 545677 [details] [diff] [review]
Patch v1

Thinking some more, I don't want this change, because it can break add-ons.  For example, the MozMill Endurance tests have been broken by every nsIMemoryReporter change that has happened in the last couple of months.  At least the previous changes introduced new functionality.

Also, not changing the names will also save Sheppy from having to update https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter again.

I'm sorry that I didn't think of these issues until after you wrote the patch.

Comment 5

[Justin Lebar (not reading bugmail)]

I'm very happy to update the wiki, if that's all it takes.

I'm not familiar with how addons work, but presumably it would break someone who did

  MAPPED = Ci.nsIMemoryReporter.KIND_MAPPED;

?  That'd be unfortunate.

I'd be happy if I could keep "nonheap" in my patch for bug 664659 -- it's inconsistent to use "nonheap", but seems incorrect to use "mapped" -- and modify the comment in nsIMemoryReporter for KIND_MAPPED to say that it's memory in the process which isn't on the heap.

What would you think of that?

Comment 6

[Justin Lebar (not reading bugmail)]

> What would you think of that?

I'll post a patch with the comment change to bug 664659.  WONTFIX'ing this.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 545677 [details] [diff] [review]
Patch v1

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

Thinking some more:  if you keep KIND_MAPPED around for backwards compatibility (along with a comment explaining why it's there) I'm ok with this patch.  It'll still need the 'dev-doc-needed' keyword.

Comment 8

[Justin Lebar (not reading bugmail)]

(In reply to comment #7)
> Thinking some more:  if you keep KIND_MAPPED around for backwards
> compatibility (along with a comment explaining why it's there) I'm ok with
> this patch.  It'll still need the 'dev-doc-needed' keyword.

Would we ever be able to get rid of KIND_MAPPED?  The next time someone modifies nsIMemoryReporter?  After N versions?  If we leave it, I'd like it to have an explicitly finite lifetime.

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to comment #8)
> Would we ever be able to get rid of KIND_MAPPED?  The next time someone
> modifies nsIMemoryReporter?  After N versions?  If we leave it, I'd like it
> to have an explicitly finite lifetime.

Nick, I presume you're watching this component so you don't need to be cc'ed on the bug to see this?  But cc'ing you just in case.

Comment 10

[Nicholas Nethercote [inactive]]

I don't think removing KIND_MAPPED is important, but if you want to do it, after 3 versions is probably fine.  I've CC'd the people who I know have written extensions that would be affected.

Comment 11

[Justin Lebar (not reading bugmail)]

inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/5ea415e7f9a6

Comment 12

[Justin Lebar (not reading bugmail)]

inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/5ea415e7f9a6

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5ea415e7f9a6