824119 - Refactor about:telemetry to expose stack printing utilities.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

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

Attachment: patch

These will be used to display late writes too.

Comment 1

[Vladan Djeric (:vladan)]

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.

Comment 2

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

(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

[Vladan Djeric (:vladan)]

(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.

Comment 4

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

> > 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

[Vladan Djeric (:vladan)]

(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

Comment 6

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

> 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

[Vladan Djeric (:vladan)]

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).

Comment 8

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

Attachment: patch v2

I don't agree with it, but here it is.

Comment 9

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

ok, I will commit this once my vm finishes building it.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/938a2b2d9782

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/938a2b2d9782