Refactor about:telemetry to expose stack printing utilities.

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla20
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 694995 [details] [diff] [review]
patch

These will be used to display late writes too.
Attachment #694995 - Attachment is patch: true
Attachment #694995 - Flags: review?(vdjeric)
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)
(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.
(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.
> > 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?
(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
> 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
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: nobody → respindola
Status: NEW → ASSIGNED
Created attachment 695521 [details] [diff] [review]
patch v2

I don't agree with it, but here it is.
Attachment #694995 - Attachment is obsolete: true
Attachment #695521 - Flags: review?(vdjeric)
Attachment #695521 - Flags: review?(vdjeric) → review+
ok, I will commit this once my vm finishes building it.
https://hg.mozilla.org/mozilla-central/rev/938a2b2d9782
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.