723181 - add some comments to TelemetryTimestamps

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Marco rightfully pointed out on IRC that it's not obvious what this module is for, or how it should be used. Let's add some comments!

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Also made a slight tweak to the code, making "timeStamps" private to the module rather than a property on the exposed object.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 593508 [details] [diff] [review]
patch

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

::: browser/modules/TelemetryTimestamps.jsm
@@ +17,1 @@
>  let TelemetryTimestamps = {

nit: newline before and after timestamps var definition may help readability.

@@ +22,5 @@
> +   * "name" must be a unique, generally "camelCase" descriptor of what the
> +   *        timestamp represents. e.g.:
> +   *          sessionRestoreRestoring, delayedStartupStarted
> +   * "value" is a timeStamp in milliseconds since the epoch. If omitted,
> +   *         defaults to Date.now().

may you use javadoc @param style here?

@@ +42,5 @@
> +
> +  /**
> +   * Get a JS object containing all of the timeStamps as properties (can be
> +   * easily serialized to JSON). Used by TelemetryPing to retrieve the data
> +   * to attach to the telemetry submission.

may get a @return javadoc part.

@@ +48,2 @@
>    get: function TT_get() {
> +    return JSON.parse(JSON.stringify(timeStamps));

may you please add a brief comment on why we actually have to stringify and parse the object? Is this for normalizing names or such?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62d547447987

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/62d547447987