Follow up to 1242777: add a clarifying comment

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Telemetry
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: azhang, Assigned: azhang)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Context (comment 24 from bug 1242777):

> Anthony, i see that the _childThreadHangs data you added here is not sent out with any Telemetry ping?
> Can you add a comment to that effect to _childThreadHangs et al in a follow-up bug?
Created attachment 8722077 [details] [diff] [review]
new-comment.patch
Assignee: nobody → azhang
Status: NEW → ASSIGNED
Attachment #8722077 - Flags: review?(gfritzsche)
Comment on attachment 8722077 [details] [diff] [review]
new-comment.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +659,5 @@
>    // where source is a weak reference to the child process,
>    // and payload is the telemetry payload from that child process.
>    _childTelemetry: [],
> +  // Thread hangs from child processes
> +  // Used for TelemetrySession.getChildThreadHangs(); not sent with Telemetry pings

I am sure that we will wonder later what this was actually used for (and not find anything in-tree).
Lets add:
// This is used e.g. by the statuser extension.

@@ +665,5 @@
>    // where source is a weak reference to the child process,
>    // and payload contains the thread hang stats from that child process.
>    _childThreadHangs: [],
>    // Array of the resolve functions of all the promises that are waiting for the child thread hang stats to arrive, used to resolve all those promises at once
> +  // Used for TelemetrySession.getChildThreadHangs(); not sent with Telemetry pings

For these helpers it seems relatively clear that they are not sent, we don't need to comment on them.
I was mostly confused about the _childThreadHangs data.
Attachment #8722077 - Flags: review?(gfritzsche) → review+
Created attachment 8722130 [details] [diff] [review]
new-comments.patch
Attachment #8722077 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8722130 [details] [diff] [review]
new-comments.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +659,5 @@
>    // where source is a weak reference to the child process,
>    // and payload is the telemetry payload from that child process.
>    _childTelemetry: [],
> +  // Thread hangs from child processes
> +  // Used for TelemetrySession.getChildThreadHangs(); not sent with Telemetry pings

Nit: Please end sentences with a period, ditto below.
Created attachment 8722530 [details] [diff] [review]
new-comments.patch
Attachment #8722130 - Attachment is obsolete: true

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/8089e684f942
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8089e684f942
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.