Closed Bug 831748 Opened 8 years ago Closed 8 years ago

refactor duplicated code

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 do itSplinter Review
The code for drawing raw stacks is mostly duplicated between the late writes an the hangs code. This patch fixes this. It also paves the was to adding fetch and hide links to the late write section by renaming the currently generic ones used by hangs.
Comment on attachment 703302 [details] [diff] [review]
do it

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

Nice refactoring.
Attachment #703302 - Flags: review+
Comment on attachment 703302 [details] [diff] [review]
do it

>+    function f() {}
>+    StackRenderer.renderStacks('late-writes', stacks, memoryMap, f);

This has landed already, but maybe in a follow up bug we can change this line to pass in "null" instead of a locally-defined function that does nothing
> >+    function f() {}
> >+    StackRenderer.renderStacks('late-writes', stacks, memoryMap, f);
> 
> This has landed already, but maybe in a follow up bug we can change this
> line to pass in "null" instead of a locally-defined function that does
> nothing

We could, but why would that be an improvement. Right now the callee can just call the callback without checking if it null or not.

IMHO a much better followup patch is to print a header in the late write stacks :-)
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #4)
> IMHO a much better followup patch is to print a header in the late write
> stacks :-)

That makes sense. My point was that calling back do-nothing locally-declared functions is hacky and is confusing to readers of the code. e.g. if I was reading renderStacks and saw the call to aRenderHeader(i), I'd be wondering what is the LateWrites header (there isn't one) and where it is defined
https://hg.mozilla.org/mozilla-central/rev/143e6064d88c
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.