951855 - nsICycleCollectorListener's documentation is confusing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jim Blandy :jimb]

Attachment: Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

The comments in xpcom/base/nsICycleCollectorListener.idl don't make clear the relationship between nsICycleCollectorListener and nsICycleCollectorHandler, and its rationale. bz dared me to improve the docs on IRC, so here's my attempt. It's probably too long-winded, but hopefully it's an improvement.

Comment 1

[Jim Blandy :jimb]

Attachment: Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

Fixed some typos.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349667 [details] [diff] [review]
Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

Could you document disableLog = true and wantAfterProcessing too.
JS probably wants
disableLog = true; // disables the default print-log-to-file
wantAfterProcessing = true; // enables graph recording.


And yes, we could have two different implementations of 
nsICycleCollectorHandler, but so far the setup has worked just fine.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349667 [details] [diff] [review]
Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

Could you document disableLog = true and wantAfterProcessing too.
JS probably wants
disableLog = true; // disables the default print-log-to-file
wantAfterProcessing = true; // enables graph recording.


And yes, we could have two different implementations of 
nsICycleCollectorHandler, but so far the setup has worked just fine.

Comment 4

[Jim Blandy :jimb]

(In reply to Olli Pettay [:smaug] from comment #2)
> And yes, we could have two different implementations of 
> nsICycleCollectorHandler, but so far the setup has worked just fine.

Hah, didn't I make some remark to you long ago about using the same underlying C++ class to implement all the different flavors of message managers? nsICycleCollectorListener is the same strategy: have one class that implements two different behaviors, under the guidance of some booleans.

Comment 5

[Jim Blandy :jimb]

Attachment: Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

Address review comments: document disableLog and wantAfterProcessing, and describe the general flow of using from JS.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Message manager is very different kind of beast, and there it just makes the implementation easier to
keep everything in one place, and it doesn't affect to the API.

Here keeping everything in one places makes the API look a bit bad.

Comment 7

[Jim Blandy :jimb]

Attachment: Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.

Document allTraces, too.

Comment 8

[Jim Blandy :jimb]

(In reply to Olli Pettay [:smaug] from comment #6)
> Here keeping everything in one places makes the API look a bit bad.

I filed bug 952257 with what I imagine you had in mind.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8350157 [details] [diff] [review]
Improve documentation for nsICycleCollectionListener and nsICycleCollectionHandler.


> [scriptable, builtinclass, uuid(7096e77d-7834-4996-b52c-50564144d4be)]
> interface nsICycleCollectorListener : nsISupports
> {
>+    // Return a listener that directs the cycle collector to traverse objects
>+    // that it knows won't be collectable. If your purpose is to build a picture
>+    // of the heap, and not simply find garbage, then you should use the
>+    // listener this returns.
"of the heap" isn't quite right, since CC doesn't know about the whole heap.
It knows about the possible garbage
(and allTraces() disables optimizations which try to remove certainly alive objects from that set of possible garbage objects)


>-    attribute boolean wantAfterProcessing;
> 
>     // This string will appear somewhere in the log's filename.
>     attribute AString filenameIdentifier;
> 
>+    // If true, record all method calls in memory, to be retrieved later
>+    // using |processNext|. Initially false.
>+    attribute boolean wantAfterProcessing;
Since you move this attribute, you need to also update uuid of this interface.

Comment 10

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6dd74ed6c1

Comment 11

[Jim Blandy :jimb]

(In reply to Olli Pettay [:smaug] from comment #9)
> >+    // Return a listener that directs the cycle collector to traverse objects
> >+    // that it knows won't be collectable. If your purpose is to build a picture
> >+    // of the heap, and not simply find garbage, then you should use the
> >+    // listener this returns.
> "of the heap" isn't quite right, since CC doesn't know about the whole heap.
> It knows about the possible garbage
> (and allTraces() disables optimizations which try to remove certainly alive
> objects from that set of possible garbage objects)

I missed this comment. Amended in this push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbc1d0a9dbb


> Since you move this attribute, you need to also update uuid of this
> interface.

I did update the UUID in my first push.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a6dd74ed6c1
https://hg.mozilla.org/mozilla-central/rev/2bbc1d0a9dbb