Last Comment Bug 723515 - Add telemetry probe for the opening time of the Firefox app menu
: Add telemetry probe for the opening time of the Firefox app menu
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on:
Blocks: 671038
  Show dependency treegraph
 
Reported: 2012-02-02 07:28 PST by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-02-04 02:38 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug (4.72 KB, patch)
2012-02-02 08:17 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Review
Patch for bug (3.51 KB, patch)
2012-02-02 08:40 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Review
Patch for bug v2 (3.14 KB, patch)
2012-02-03 02:12 PST, Jared Wein [:jaws] (please needinfo? me)
ttaubert: review+
Details | Diff | Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-02-02 07:28:03 PST

    
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2012-02-02 08:17:07 PST
Created attachment 593854 [details] [diff] [review]
Patch for bug
Comment 2 Dão Gottwald [:dao] 2012-02-02 08:28:00 PST
Comment on attachment 593854 [details] [diff] [review]
Patch for bug

>+    try {
>+      Services.telemetry.getHistogramById("FX_APP_MENU_OPEN_MS").add(duration);
>+    } catch (ex) {
>+      Components.utils.reportError("Unable to report telemetry for FX_APP_MENU_OPEN_MS.");
>+    }

This should never throw.

>+  uninit: function() {
>+    this._button.removeEventListener("click", this._onclickListenerBound, false);
>+    this._popup.removeEventListener("popupshown", this._onpopupshownListenerBound, false);
>+    this._onclickListenerBound = null;
>+    this._onpopupshownListenerBound = null;
>+  },

This shouldn't be needed, which means that you don't need this._onclickListenerBound and this._onpopupshownListenerBound either.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-02-02 08:40:23 PST
Created attachment 593860 [details] [diff] [review]
Patch for bug

Thanks for the feedback Dao. I've addressed Dao's feedback and the patch is much smaller as a result.
Comment 4 Marco Bonardo [::mak] 2012-02-02 09:27:12 PST
const Ci = Components.itnerfaces

this is likely not working :)
Comment 5 Marco Bonardo [::mak] 2012-02-02 09:27:52 PST
ehr, sorry, commented in the wrong bug!
Comment 6 Tim Taubert [:ttaubert] 2012-02-02 15:22:16 PST
Comment on attachment 593860 [details] [diff] [review]
Patch for bug

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

::: browser/base/content/browser.js
@@ +1769,5 @@
> +  let appMenuButton = document.getElementById("appmenu-button");
> +  let appMenuPopup = document.getElementById("appmenu-popup");
> +  if (appMenuButton && appMenuPopup) {
> +    let appMenuOpening = null;
> +    appMenuButton.addEventListener("click", function() {

Does the menu button consider itself clicked on "click" or maybe yet on "mouseup"? Maybe that's not really important, just came to my mind :)

What happens if I quickly click the button multiple times? What about using the right or middle mouse button?

@@ +1775,5 @@
> +    }, false);
> +    appMenuPopup.addEventListener("popupshown", function(event) {
> +      if (event.target != appMenuPopup)
> +        return;
> +      let duration = new Date - appMenuOpening;

Nit: new Date()

@@ +1816,5 @@
>      gBrowser.removeTabsProgressListener(window.TabsProgressListener);
>    } catch (ex) {
>    }
>  
> +  AppMenuTelemetry.uninit();

Is this legacy code?

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +311,5 @@
>  // #ifdef MOZ_PHOENIX
>  HISTOGRAM(FX_TAB_ANIM_OPEN_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the tab opening animation")
>  HISTOGRAM(FX_TAB_ANIM_CLOSE_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the tab closing animation")
>  HISTOGRAM_BOOLEAN(FX_CONTEXT_SEARCH_AND_TAB_SELECT, "Firefox: Background tab was selected within 5 seconds of searching from the context menu")
> +HISTOGRAM(FX_APP_MENU_OPEN_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the app-menu opening in milliseconds")

Just as a thought, do we really want to know whether the menu takes 2 or 3 seconds to load? I think it'd be sufficient to maybe have 500 or 1000ms as max value because if we reach that we anyway have a problem.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-02-03 02:12:32 PST
Created attachment 594097 [details] [diff] [review]
Patch for bug v2

(In reply to Tim Taubert [:ttaubert] from comment #6)
> > +    appMenuButton.addEventListener("click", function() {
> 
> Does the menu button consider itself clicked on "click" or maybe yet on
> "mouseup"? Maybe that's not really important, just came to my mind :)
> 
> What happens if I quickly click the button multiple times? What about using
> the right or middle mouse button?

Thank you for catching this. The menu appears on mousedown, so I have switched to using mousedown and I also now check for event.button == 0.

Clicking quickly will cause the menu to open and close. Each mousedown sets the date object. The first mousedown will open the menu, and the second mousedown will close the menu. This second mousedown will set the date object erroneously, but I'm not worried about it because the popupshown event will not be fired until a subsequent mousedown event, at which time the date object will be set to a correct time. (I hope that is clear.)

> @@ +1816,5 @@
> >      gBrowser.removeTabsProgressListener(window.TabsProgressListener);
> >    } catch (ex) {
> >    }
> >  
> > +  AppMenuTelemetry.uninit();
> 
> Is this legacy code?

Yeah... sorry about that.

> ::: toolkit/components/telemetry/TelemetryHistograms.h
> @@ +311,5 @@
> >  // #ifdef MOZ_PHOENIX
> >  HISTOGRAM(FX_TAB_ANIM_OPEN_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the tab opening animation")
> >  HISTOGRAM(FX_TAB_ANIM_CLOSE_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the tab closing animation")
> >  HISTOGRAM_BOOLEAN(FX_CONTEXT_SEARCH_AND_TAB_SELECT, "Firefox: Background tab was selected within 5 seconds of searching from the context menu")
> > +HISTOGRAM(FX_APP_MENU_OPEN_MS, 1, 3000, 10, EXPONENTIAL, "Firefox: Time taken by the app-menu opening in milliseconds")
> 
> Just as a thought, do we really want to know whether the menu takes 2 or 3
> seconds to load? I think it'd be sufficient to maybe have 500 or 1000ms as
> max value because if we reach that we anyway have a problem.

Yeah, I agree. I've changed it to 1000 now.
Comment 8 Tim Taubert [:ttaubert] 2012-02-03 02:46:36 PST
(In reply to Jared Wein [:jaws] from comment #7)
> Clicking quickly will cause the menu to open and close. Each mousedown sets
> the date object. The first mousedown will open the menu, and the second
> mousedown will close the menu. This second mousedown will set the date
> object erroneously, but I'm not worried about it because the popupshown
> event will not be fired until a subsequent mousedown event, at which time
> the date object will be set to a correct time. (I hope that is clear.)

Yes, that will work.
Comment 9 Tim Taubert [:ttaubert] 2012-02-03 02:49:14 PST
Comment on attachment 594097 [details] [diff] [review]
Patch for bug v2

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

Looks good!

::: browser/base/content/browser.js
@@ +1772,5 @@
> +    let appMenuOpening = null;
> +    appMenuButton.addEventListener("mousedown", function(event) {
> +      if (event.button != 0)
> +        return;
> +      appMenuOpening = new Date();

Nit: Can you please remove the 'return' here and change to condition to (event.button == 0)?
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-02-03 04:35:55 PST
https://hg.mozilla.org/integration/fx-team/rev/01fea1c7a1be
Comment 11 Tim Taubert [:ttaubert] 2012-02-04 02:38:38 PST
https://hg.mozilla.org/mozilla-central/rev/01fea1c7a1be

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