Closed Bug 723181 Opened 12 years ago Closed 12 years ago

add some comments to TelemetryTimestamps

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(1 file)

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!
Attached patch patchSplinter Review
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
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.