Closed Bug 831623 Opened 11 years ago Closed 11 years ago

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


(Toolkit :: Telemetry, defect)

Not set





(Reporter: espindola, Assigned: espindola)




(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
      No description provided.
Attachment #703137 - Flags: review?(vdjeric)
Comment on attachment 703137 [details] [diff] [review]

>+        <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);
Attachment #703137 - Flags: review?(vdjeric) → review-
Summary: Generalize handleSymbolResponse so it can be used for late write stacks too → Move handleSymbolResponse and fetchSymbol to a new class so it can be used for late write stacks too
Attached patch patch (obsolete) — Splinter Review
I have tested that symbolication works on the mac. It is building on windows and I will test it tomorrow.
Attachment #703761 - Flags: review?(vdjeric)
Attachment #703137 - Attachment is obsolete: true
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.
Attached patch patchSplinter Review
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.
Attachment #703761 - Attachment is obsolete: true
Attachment #703761 - Flags: review?(vdjeric)
Attachment #703871 - Flags: review?(vdjeric)
Blocks: 832693
Attachment #703871 - Flags: review?(vdjeric) → review+
Filed bug 832693 for the SymoblicationRequest change
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(In reply to Vladan Djeric (:vladan) from comment #6)
> Rafael is on vacation, so I landed this patch:

You need to log in before you can comment on or make changes to this bug.