Last Comment Bug 723181 - add some comments to TelemetryTimestamps
: add some comments to TelemetryTimestamps
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 10:19 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-02-04 02:42 PST (History)
1 user (show)
gavin.sharp: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.35 KB, patch)
2012-02-01 10:21 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
mak77: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-01 10:19:07 PST
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: gavin@gavinsharp.com] 2012-02-01 10:21:34 PST
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.
Comment 2 Marco Bonardo [::mak] 2012-02-01 14:10:00 PST
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: gavin@gavinsharp.com] 2012-02-03 15:36:32 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d547447987
Comment 4 Marco Bonardo [::mak] 2012-02-04 02:42:00 PST
https://hg.mozilla.org/mozilla-central/rev/62d547447987

Note You need to log in before you can comment on or make changes to this bug.