841527 - Update the FX_IDENTITY_POPUP_OPEN_MS telemetry probe to use the TelemetryStopwatch

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[:Cykesiopka]

Attachment: Initial patch

Hmm, something like this...?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[:Cykesiopka]

Attachment: Patch v1

Addressed feedback comments.

Thanks for the feedback :jaws!

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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

Comment 5

[:Cykesiopka]

Attachment: Patch v2

Addressed Felipe's comments.

Thanks for the feedback!

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 716769 [details] [diff] [review]
Patch v2

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

Great!

Comment 7

[:Cykesiopka]

Attachment: Patch v2 for Check in

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...

Comment 8

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b284ccd69eb7

Thanks for the patch!

Comment 9

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/b284ccd69eb7