Closed
Bug 1342034
Opened 8 years ago
Closed 8 years ago
Wrong documentation comment for 'getLoadedModules' in nsITelemetry.idl
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
(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 ..."?
Assignee | ||
Comment 3•8 years ago
|
||
(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 4•8 years ago
|
||
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)
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8844407 -
Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment 6•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•8 years ago
|
||
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
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•