Closed Bug 671280 Opened 9 years ago Closed 9 years ago

Rename KIND_MAPPED to KIND_NONHEAP in nsIMemoryReporter

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

See bug 664659 comment 32: I think KIND_NONHEAP is a better name than KIND_MAPPED.
Blocks: 664659
Attached patch njn (obsolete) — Splinter Review
Attached patch Patch v1Splinter Review
Attachment #545677 - Flags: review?(nnethercote)
Attachment #545676 - Attachment is obsolete: true
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 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.
Attachment #545677 - Flags: review?(nnethercote) → review-
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?
> What would you think of that?

I'll post a patch with the comment change to bug 664659.  WONTFIX'ing this.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
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.
Attachment #545677 - Flags: review- → review+
(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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
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.
http://hg.mozilla.org/mozilla-central/rev/5ea415e7f9a6
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.