944453 - Collect UITelemetry on menubar usage

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

We're interested in collecting:

1) A boolean for whether or not the menubar is displayed by default
2) A count of how many times the menubar is used
3) A count of which default menuitems are being used

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Small adjustment on the description for the sake of time - we still want 1 and 2, but not 3.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Note that I'm constraining this *just* to mouse clicks. This is not going to record keyboard access activity.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1 - for Australis

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for Australis

Tested on a machine with actual menubars. This appears to work.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8361753 [details] [diff] [review]
Patch v1 - for Australis

I swear we're almost done with these.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8361753 [details] [diff] [review]
Patch v1 - for Australis

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

Looks good, two nits below.

::: browser/modules/BrowserUITelemetry.jsm
@@ +301,5 @@
>        case "PlacesChevron":
>          this._PlacesChevronMouseUp(aEvent);
>          break;
> +      case "menubar-items":
> +        this._menubarMouseUp(aEvent);

I'm assuming this thing (y u no context, splinter?) catches mouseups in the descendants of the menu, so that mousedown->dragdown-in-menu->mouseup still gets caught?

@@ +328,5 @@
>  
> +  _menubarMouseUp: function(aEvent) {
> +    let target = aEvent.originalTarget;
> +    let tag = target.localName
> +    let result = (tag == "menu" || tag == "menuitem") ? tag : "other";

Why the distinction between menus and menuitems? Is that valuable? Or can we just log 'menu' ?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for non-Australis

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8361753 [details] [diff] [review]
> Patch v1 - for Australis
> 
> Review of attachment 8361753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, two nits below.
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +301,5 @@
> >        case "PlacesChevron":
> >          this._PlacesChevronMouseUp(aEvent);
> >          break;
> > +      case "menubar-items":
> > +        this._menubarMouseUp(aEvent);
> 
> I'm assuming this thing (y u no context, splinter?) catches mouseups in the
> descendants of the menu, so that mousedown->dragdown-in-menu->mouseup still
> gets caught?
> 

Yep, that should work.

> @@ +328,5 @@
> >  
> > +  _menubarMouseUp: function(aEvent) {
> > +    let target = aEvent.originalTarget;
> > +    let tag = target.localName
> > +    let result = (tag == "menu" || tag == "menuitem") ? tag : "other";
> 
> Why the distinction between menus and menuitems? Is that valuable? Or can we
> just log 'menu' ?

UX is interested in whether or not people are actually opening things from the menu, as well as just looking at items in the menus. An increase in "menu" usage, but not in "menuitem" usage might indicate that users are looking for (and not finding) things. I thought that might be valuable.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362550 [details] [diff] [review]
Patch v1 - for non-Australis

Pretty much a straight port.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Thanks Gijs! Australis patch landed on fx-team:

remote: https://hg.mozilla.org/integration/fx-team/rev/2bc82c3b8dcd

Comment 11

[:Gijs (he/him)]

Comment on attachment 8362550 [details] [diff] [review]
Patch v1 - for non-Australis

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

This looks almost mergable. In those cases, please feel free to carry over r+. :-)

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362550 [details] [diff] [review]
Patch v1 - for non-Australis

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

None. This gives us the ability to count clicks on menu and menuitems - things that UX and UR is interested in.


User impact if declined: 

None.


Testing completed (on m-c, etc.): 

Just local testing on Windows, OS X and Linux.


Risk to taking this patch (and alternatives if risky): 

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Landed non-Australis patch on Holly:

remote: https://hg.mozilla.org/projects/holly/rev/abe39130e6ff

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2bc82c3b8dcd

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 8362550 [details] [diff] [review]
Patch v1 - for non-Australis

just in time for our final beta's :-| to squeeze in a change like this. No end user impact and low-risk approving for uplift.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Thanks bajaj - these were the last ones. :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/164d5ef28143
https://hg.mozilla.org/releases/mozilla-beta/rev/f14cc0345552