Closed
Bug 824119
Opened 12 years ago
Closed 12 years ago
Refactor about:telemetry to expose stack printing utilities.
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: espindola, Assigned: espindola)
Details
Attachments
(1 file, 1 obsolete file)
4.63 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
These will be used to display late writes too.
Assignee | ||
Updated•12 years ago
|
Attachment #694995 -
Attachment is patch: true
Attachment #694995 -
Flags: review?(vdjeric)
Comment 1•12 years ago
|
||
Comment on attachment 694995 [details] [diff] [review] patch In keeping with the object-oriented style that ttaubert requested for this file when I first wrote it, can you make the shared functionality into another singleton class? e.g. "let StackRenderer = { .." Then ChromeHang and LateWrites can call into it with div & data arguments.
Attachment #694995 -
Flags: review?(vdjeric)
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Vladan Djeric (:vladan) from comment #1) > Comment on attachment 694995 [details] [diff] [review] > patch > > In keeping with the object-oriented style that ttaubert requested for this > file when I first wrote it, can you make the shared functionality into > another singleton class? e.g. "let StackRenderer = { .." Then ChromeHang and > LateWrites can call into it with div & data arguments. Can you explain why that is a good thing? None of these functions uses a "this" right now, so accessing the methods with StackRenderer. clearDivData(aDiv) just makes it more verbose for no added value that I can see. Their code be made to have the div as a member, but since hangsDiv and (and the writesDiv I want to add) are just local variable, the code would still look really silly: let hangsDiv = let stackRendererDiv = ... hangsDiv stackRendererDiv.clearDivData() There is no polymorphism or state hiding being done by these functions, OO doesn't look like the right hammer in here.
Comment 3•12 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #2) > (In reply to Vladan Djeric (:vladan) from comment #1) > Can you explain why that is a good thing? None of these functions uses a > "this" right now, so accessing the methods with > > StackRenderer. clearDivData(aDiv) > > just makes it more verbose for no added value that I can see. The memoryMapTitle and stackTitle resource strings would be members of the StackRenderer class instead of global variables.
Assignee | ||
Comment 4•12 years ago
|
||
> > StackRenderer. clearDivData(aDiv)
> >
> > just makes it more verbose for no added value that I can see.
>
> The memoryMapTitle and stackTitle resource strings would be members of the
> StackRenderer class instead of global variables.
sure, but that is still not visible from outside this file, so I don't see the benefit. What is the difference from those constants to, for example, TELEMETRY_DELAY in TelemetryPing.js?
Also, the function clearDivData doesn't use those constants, so should at least it remain just a function?
Comment 5•12 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #4) > > > StackRenderer. clearDivData(aDiv) > > > > > > just makes it more verbose for no added value that I can see. > > > > The memoryMapTitle and stackTitle resource strings would be members of the > > StackRenderer class instead of global variables. > > sure, but that is still not visible from outside this file, so I don't see > the benefit. What is the difference from those constants to, for example, > TELEMETRY_DELAY in TelemetryPing.js? It's more of a dislike of global variables and keeping with the singleton-per-section style in the file. TELEMETRY_DELAY is a magic-number constant, whereas those titles are resource strings belonging to individual page sections. There isn't a functional difference. > Also, the function clearDivData doesn't use those constants, so should at > least it remain just a function? Sure
Assignee | ||
Comment 6•12 years ago
|
||
> It's more of a dislike of global variables and keeping with the > singleton-per-section style in the file. TELEMETRY_DELAY is a magic-number > constant, whereas those titles are resource strings belonging to individual > page sections. There isn't a functional difference. I will update the patch in the interest of speeding things up, but I must say I still think we are just propagating a bad and confusing style. A singleton is a global constant. Anything inside it is a global constant, so the issue becomes just a namespace management issue (.i.e, a hackish namespace). Now, these names don't leak this file. So it would make sense to hide them if the names we use for stacks and memory maps were relevant for only a part of the file, but that is not more true for these names than for TelemetryPing one line above which is only used in one function. > > Also, the function clearDivData doesn't use those constants, so should at > > least it remain just a function? > > Sure
Comment 7•12 years ago
|
||
I get what you're saying and my original implementation of about:telemetry was actually without singletons. The review feedback I got was was that the functional style was hard to read and clashed with how things are structured generally in front-end code (see https://bugzilla.mozilla.org/show_bug.cgi?id=661881#c10).
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
I don't agree with it, but here it is.
Attachment #694995 -
Attachment is obsolete: true
Attachment #695521 -
Flags: review?(vdjeric)
Updated•12 years ago
|
Attachment #695521 -
Flags: review?(vdjeric) → review+
Assignee | ||
Comment 9•12 years ago
|
||
ok, I will commit this once my vm finishes building it.
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/938a2b2d9782
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/938a2b2d9782
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•