Closed Bug 1356210 Opened 3 years ago Closed 3 years ago

Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

We can explicitly check whether the stopwatches are already running and avoid the re-entry logic (and drop the accompanying comment). The comment on why we're starting both stopwatches can be much briefer without losing crucial information, I think. I'd also like to group this with the window.maybeRecordAbandonmentTelemetry call.
Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review132654

Thanks for cleaning this up. It looks good to me, but I think you'll likely want a review of the TelemetryStopwatch.jsm change from Dexter.
Attachment #8857912 - Flags: review?(mconley) → review+
Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review132906

Thanks for adding this new feature to TelemetryStopwatch, it looks good! Could you please also add docs for this at: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/measuring-time.rst ?

Make sure they read ok by testing them with ./mach doc :)
Let's please add test coverage for the functions to test_TelemetryStopwatch.js.
Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review133656

::: toolkit/components/telemetry/docs/collection/measuring-time.rst:53
(Diff revision 2)
>      // ... do more work.
>      TelemetryStopwatch.finish("SAMPLE_FILE_LOAD_TIME_MS");
>  
> +    // Another loading attempt? Start stopwatch again if
> +    // not already running.
> +    if (!TelemetryStopwatch.running("SAMPLE_FILE_LOAD_TIME_MS")) {

Sorry for not catching this earlier, but would you please add test coverage in [test_TelemetryStopwatch.js](http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js) too?
Comment on attachment 8857912 [details]
Bug 1356210 - Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.

https://reviewboard.mozilla.org/r/129946/#review134384

::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
(Diff revision 3)
>  
>    // Starting & finishing a keyed stopwatch for an existing histogram should work.
>    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
>    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
>    // Verify that TS.finish deleted the timers
> -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));

Please leave the test for |finishKeyed| too, just add a new do_check_false(...) for runningKeyed as well here.
Attachment #8857912 - Flags: review?(alessio.placitelli) → review+
(In reply to Alessio Placitelli [:Dexter] from comment #8)
> Comment on attachment 8857912 [details]
> Bug 1356210 - Clean up TelemetryStopwatch.start calls for
> FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.
> 
> https://reviewboard.mozilla.org/r/129946/#review134384
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
> (Diff revision 3)
> >  
> >    // Starting & finishing a keyed stopwatch for an existing histogram should work.
> >    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
> >    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> >    // Verify that TS.finish deleted the timers
> > -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));
> 
> Please leave the test for |finishKeyed| too, just add a new
> do_check_false(...) for runningKeyed as well here.

That's not useful as far as I can tell. That aspect of finishKeyed is already tested earlier in the test.
Flags: needinfo?(alessio.placitelli)
In other words, the line that I modified isn't meant to test how finishKeyed behaves when called twice; it's a test for the behavior of the previous finishKeyed call.
(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to Alessio Placitelli [:Dexter] from comment #8)
> > Comment on attachment 8857912 [details]
> > Bug 1356210 - Clean up TelemetryStopwatch.start calls for
> > FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes.
> > 
> > https://reviewboard.mozilla.org/r/129946/#review134384
> > 
> > ::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:159
> > (Diff revision 3)
> > >  
> > >    // Starting & finishing a keyed stopwatch for an existing histogram should work.
> > >    do_check_true(TelemetryStopwatch.startKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > >    do_check_true(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > >    // Verify that TS.finish deleted the timers
> > > -  do_check_false(TelemetryStopwatch.finishKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > > +  do_check_false(TelemetryStopwatch.runningKeyed(KEYED_HIST.id, KEYED_HIST.key));
> > 
> > Please leave the test for |finishKeyed| too, just add a new
> > do_check_false(...) for runningKeyed as well here.
> 
> That's not useful as far as I can tell. That aspect of finishKeyed is
> already tested earlier in the test.

Good point, I've dropped the issue on mozreview.
Flags: needinfo?(alessio.placitelli)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b4aac572731
Clean up TelemetryStopwatch.start calls for FX_TAB_CLOSE_TIME_ANIM_MS and FX_TAB_CLOSE_TIME_NO_ANIM_MS probes. r=Dexter,mconley
https://hg.mozilla.org/mozilla-central/rev/5b4aac572731
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
See Also: → 1366398
You need to log in before you can comment on or make changes to this bug.