723515 - Add telemetry probe for the opening time of the Firefox app menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Comment 1

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

Attachment: Patch for bug

Comment 2

[Dão Gottwald [:dao]]

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)]

Attachment: 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]]

const Ci = Components.itnerfaces

this is likely not working :)

Comment 5

[Marco Bonardo [:mak]]

ehr, sorry, commented in the wrong bug!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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)]

Attachment: 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] (inactive)]

(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] (inactive)]

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)]

https://hg.mozilla.org/integration/fx-team/rev/01fea1c7a1be

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/01fea1c7a1be