add some comments to TelemetryTimestamps

RESOLVED FIXED in Firefox 13

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 13
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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!
Created attachment 593508 [details] [diff] [review]
patch

Also made a slight tweak to the code, making "timeStamps" private to the module rather than a property on the exposed object.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #593508 - Flags: review?(mak77)
OS: Mac OS X → All
Hardware: x86 → All
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?
Attachment #593508 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d547447987
Flags: in-testsuite-
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/62d547447987
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.