831623 - Move handleSymbolResponse and fetchSymbol to a new class so it can be used for late write stacks too

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

Comment 1

[Vladan Djeric (:vladan)]

Comment on attachment 703137 [details] [diff] [review]
patch

>+        <a id="hang-fetch-symbols" href="javascript:">&aboutTelemetry.fetchSymbols;</a>
>+        <a id="hang-hide-symbols" class="hidden" href="javascript:">&aboutTelemetry.hideSymbols;</a>

Nit: fetch-hang-symbols & fetch-hide-symbols.. otherwise it reads like we're hanging something :)

---

I do understand your argument against unnecessary use of singletons but I think this is one case where an object-oriented approach (not a singleton) would make the code more readable. The code as you've refactored it right now is idiosyncratic: there's a global function handleSymbolResponse that references this.fetchElement and calls this.renderHeader. Late-writes and chrome-hangs will be duplicating code for creating an XMLHttpRequest object and setting its arguments (e.g. request type, Content-length, etc.) + you're essentially declaring a class inside ChromeHangs_fetchSymbols with "let request = {};"

Can you create a top level SymbolicationRequest class using "function/.prototype" and have it encapsulate the functionality of sending a request, receiving the response and calling the "render" method on the appropriate singleton? Then LateWrites and ChromeHangs singletons can just do:

let request = new SymbolicationRequest(requestJSON, ChromeHangs);
request.send();

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

I have tested that symbolication works on the mac. It is building on windows and I will test it tomorrow.

Comment 3

[Vladan Djeric (:vladan)]

Patch looks good, although I want to take another look in the morning. 

What do you think of checking symbolRequest.readyState and symbolRequest.status in SymbolicationRequest.handleSymbolResponse? 

Then handleSymbolResponse would either 1) return immediately (if readyState != 4), 2) call StackRenderer.renderSymbolicatedStacks (if status = 200), or 3) call StackRenderer.showErrorMessage (if status != 200).

That way all XMLHttpRequest logic is in SymbolicationRequest and StackRenderer just renders stuff it's told to render.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

The hang code was missing an 's' (hang instead of hangs). That is the only difference is this patch.

There is only one call to renderSymbolicatedStacks, so we can inline as much of it as you want without duplicating code, but we should probably do it in a followup bug.

Comment 5

[Vladan Djeric (:vladan)]

Filed bug 832693 for the SymoblicationRequest change

Comment 6

[Vladan Djeric (:vladan)]

Rafael is on vacation, so I landed this patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/077caacae057

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/077caacae057

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Vladan Djeric (:vladan) from comment #6)
> Rafael is on vacation, so I landed this patch:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/077caacae057

Thanks!