Closed Bug 831864 Opened 8 years ago Closed 8 years ago

Refactor some logic to a renderSymbolicatedStacks method

Categories

(Toolkit :: Telemetry, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
This is similar to bug 831748, but for the the symbolicated stack, not the raw one.

The main difference to bug 831748 is that right now only the hangs section uses the symbolicated stacks functionality, I am in the process of adding that to the late writes section too.

This patch also changes ChromeHangs_renderHangHeader to take only an index, which allows it to be used as a callback directly, simplifying the code a bit more.
Attachment #703423 - Flags: review?(nfroyd)
Comment on attachment 703423 [details] [diff] [review]
patch

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

::: toolkit/content/aboutTelemetry.js
@@ +353,5 @@
>      let titleElement = document.createElement("span");
>      titleElement.className = "hang-title";
>  
> +    let hangs = Telemetry.chromeHangs;
> +    let durations = hangs.durations;

Can't you use this.hangs.durations instead?
Attachment #703423 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce25418d00a8

I used  Telemetry.hangs.durations. To use 'this' I would have to call bind in the two callers just for that.
Comment on attachment 703423 [details] [diff] [review]
patch

>diff --git a/toolkit/content/aboutTelemetry.js b/toolkit/content/aboutTelemetry.js
>index 8b47821..6131719 100644
>--- a/toolkit/content/aboutTelemetry.js
>+++ b/toolkit/content/aboutTelemetry.js
>@@ -285,6 +285,42 @@ let StackRenderer = {
>       aRenderHeader(i);
>       this.renderStack(div, stack)
>     }
>+  },
>+  renderSymbolicatedStacks:
>+    function StackRenderer_renderSymbolicatedStacks(aPrefix, aRequest,
>+                                                    aRenderHeader) {
>+    if (aRequest.readyState != 4)
>+      return;

Why is StackRenderer.renderSymbolicatedStacks checking status codes of HTTP requests?
> Why is StackRenderer.renderSymbolicatedStacks checking status codes of HTTP
> requests?

Because it only needs to do anything when it gets the final answer and because it needs to display an error if the connection has one.

All this logic is sharable and we should share it. I can change the name to something more descriptive, but I can't think of anything more succinct than handleNetworkEventsAndThenDisplayErrorOrRenderStacksAsApppropriate :-)
https://hg.mozilla.org/mozilla-central/rev/ce25418d00a8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.