Closed Bug 1342034 Opened 8 years ago Closed 8 years ago

Wrong documentation comment for 'getLoadedModules' in nsITelemetry.idl

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch Patch (obsolete) (deleted) — Splinter Review
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?
Attachment #8840412 - Flags: review?(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 ..."?
Attached patch Patch (obsolete) (deleted) — Splinter Review
(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!
Attachment #8840412 - Attachment is obsolete: true
Attachment #8840412 - Flags: review?(gfritzsche)
Attachment #8840415 - Flags: review?(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.
Attachment #8840415 - Flags: review?(gfritzsche)
Priority: -- → P3
Attached patch Patch (obsolete) (deleted) — Splinter Review
(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.
Attachment #8840415 - Attachment is obsolete: true
Attachment #8844407 - Flags: review?(gfritzsche)
Attachment #8844407 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
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.
Attachment #8844407 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Attached patch Patch (deleted) — Splinter Review
Attachment #8844407 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25b6db4e4810 Fix documentation of getLoadedModules in nsITelemetry.idl. r=Dexter
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: