Closed Bug 841527 Opened 7 years ago Closed 7 years ago

Update the FX_IDENTITY_POPUP_OPEN_MS telemetry probe to use the TelemetryStopwatch

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: jaws, Assigned: Cykesiopka)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js])

Attachments

(1 file, 3 obsolete files)

Using TelemetryStopwatch will reduce code duplication that the FX_IDENTITY_POPUP_OPEN_MS probe handles itself.

See http://mxr.mozilla.org/mozilla-central/ident?i=TelemetryStopwatch for usages of TelemetryStopwatch.

There may be other probes that can be converted. I'll add them to this bug if I find them before the bug is closed.
Attached patch Initial patch (obsolete) — Splinter Review
Hmm, something like this...?
Attachment #715921 - Flags: review?(jAwS)
Comment on attachment 715921 [details] [diff] [review]
Initial patch

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

This patch looks really good. I think it is very close to being fixed. Please make the one change below and request review from :felipe on the next patch.

::: browser/base/content/browser.js
@@ +7141,5 @@
>    },
>  
>    onPopupShown : function(event) {
> +    if (!TelemetryStopwatch.finish("FX_IDENTITY_POPUP_OPEN_MS")) {
> +        Components.utils.reportError("Unable to report telemetry for FX_IDENTITY_POPUP_OPEN_MS.");

I think we can remove the Component.utils.reportError call now and just make the TelemetryStopwatch.finish call outside of an if-statement.
Attachment #715921 - Flags: review?(jAwS) → feedback+
Assignee: nobody → cykesiopka
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Addressed feedback comments.

Thanks for the feedback :jaws!
Attachment #715921 - Attachment is obsolete: true
Attachment #716244 - Flags: review?(felipc)
Comment on attachment 716244 [details] [diff] [review]
Patch v1

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

Hi there, thanks for the patch! There's just one thing that needs to be addressed before it's ready. With TelemetryStopwatch you can't have a timer that is started but never finishes. If that happens, the next time the timer is started again it will give an error. You have two alternatives to fix that:

- You can move the start of the timer after the return statements that might stop that function, or
- You can call TelemetryStopwatch.cancel(..) before the return statements to cancel that timer and allow a new one to be started later
Attachment #716244 - Flags: review?(felipc) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Addressed Felipe's comments.

Thanks for the feedback!
Attachment #716244 - Attachment is obsolete: true
Attachment #716769 - Flags: review?(felipc)
Comment on attachment 716769 [details] [diff] [review]
Patch v2

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

Great!
Attachment #716769 - Flags: review?(felipc) → review+
Not sure if I need to post an updated patch for check in when I'm just changing the commit message, but just in case...
Attachment #716769 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b284ccd69eb7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.