1342034 - Wrong documentation comment for 'getLoadedModules' in nsITelemetry.idl

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Patch

I haven't changed the documentation at the beginning ("Returns an array of the modules loaded in the process...") as that one is meant to be more of a high level description. Do you prefer I change it as well?

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Marco Castelluccio [:marco] from comment #1)
> I haven't changed the documentation at the beginning ("Returns an array of
> the modules loaded in the process...") as that one is meant to be more of a
> high level description. Do you prefer I change it as well?

Just "Asynchronously get an array of ..."?

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> > I haven't changed the documentation at the beginning ("Returns an array of
> > the modules loaded in the process...") as that one is meant to be more of a
> > high level description. Do you prefer I change it as well?
> 
> Just "Asynchronously get an array of ..."?

OK!

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8840415 [details] [diff] [review]
Patch

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

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +199,1 @@
>     * @throws NS_ERROR_FAILURE on failure.

This seems a bit inconsistent.
I see that this can:
- throw if main thread init fails or this is built without MOZ_GECKO_PROFILER
- resolve to a module array
- reject with NS_ERROR_FAILURE

We should at least document all those cases.
Better: always return a promise and resolve/reject.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Comment on attachment 8840415 [details] [diff] [review]
> Patch
> 
> Review of attachment 8840415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/nsITelemetry.idl
> @@ +199,1 @@
> >     * @throws NS_ERROR_FAILURE on failure.
> 
> This seems a bit inconsistent.
> I see that this can:
> - throw if main thread init fails or this is built without MOZ_GECKO_PROFILER
> - resolve to a module array
> - reject with NS_ERROR_FAILURE
> 
> We should at least document all those cases.

Done. I haven't documented the possible (but really unlikely) failure to create the Promise, as discussed on IRC.

> Better: always return a promise and resolve/reject.

The creation of the promise is fallible by itself, so we can't do that.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8844407 [details] [diff] [review]
Patch

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

Please change the commit message to report me as the reviewer.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b6db4e4810
Fix documentation of getLoadedModules in nsITelemetry.idl. r=Dexter

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25b6db4e4810