The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Menus
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Created attachment 593854 [details] [diff] [review]
Patch for bug
Attachment #593854 - Flags: review?(dietrich)
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.
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.
Attachment #593854 - Attachment is obsolete: true
Attachment #593854 - Flags: review?(dietrich)
Attachment #593860 - Flags: review?(dietrich)
const Ci = Components.itnerfaces

this is likely not working :)
ehr, sorry, commented in the wrong bug!
Attachment #593860 - Flags: review?(dietrich) → review?(ttaubert)
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.
Attachment #593860 - Flags: review?(ttaubert)
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.
Attachment #593860 - Attachment is obsolete: true
Attachment #594097 - Flags: review?(ttaubert)
(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 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)?
Attachment #594097 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/01fea1c7a1be
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/01fea1c7a1be
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.