942335 - Collect click counts on the pre-Australis Firefox button and its children

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

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

We'd like to:

* Count how many times Firefox button is clicked
* Count how many times a command is kicked off from the Firefox menu
* Create a list of built-in, default menu items and their IDs or classes, and count how many times each one of those are clicked.
  * For items not in that list, have that group fall into "add-on menu item" and still count the click

Comment 1

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

Attachment: WIP Patch 1

Comment 2

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

Attachment: Patch v1

This seems to do the job. What do you think of my approach, Blair?

Comment 3

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

Attachment: Patch v1.1

Switches us from using a Set to a vanilla array, since bug 943606 required an array, and the inconsistency was annoying.

Comment 4

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

Attachment: Patch v1.2

Replaced single with double quotes, as per Dao's suggestion in bug 943606.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8340014 [details] [diff] [review]
Patch v1.2

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

This looks OK, but I think getParentId went missing and there's a few IDs in the list that drew my attention, so f+ for now. :-)

::: browser/modules/BrowserUITelemetry.jsm
@@ +22,5 @@
> +// bookmarks, history and feed menu popups where the user might
> +// click on an id-less node, we record this as a click on the
> +// menupopup id with a :child suffix.
> +const kAppmenuWhitelist = [
> +  "appmenu_newTab:child",

So... I just did the following (yay scratchpad/console):


let ary = document.querySelectorAll("[id*='appmenu']")
Array.map(ary, (el) => el.id).join('\n');

I don't see this appmenu_newTab:child of which you speak. What is it? Was this meant to be autogenerated for add-ons that add stuff to it? Same with the other :child bits below (which I also don't see in my list). There doesn't seem to be code in this patch that does that...

@@ +50,5 @@
> +    "appmenu_print_popup",
> +    "appmenu_printPreview",
> +    "appmenu_printSetup",
> +
> +  "appmenu_webDeveloper:child",

My privacy geek wonders if we really care about all these items or if we are OK just knowing people clicked on something in this menu. Collecting less is always better.

@@ +73,5 @@
> +
> +  "chardet.off",
> +  "chardet.ja_parallel_state_machine",
> +  "chardet.ruprob",
> +  "chardet.ukprob",

These IDs changed recently. Bug 943524.

@@ +106,5 @@
> +
> +  "appmenu_customize:child",
> +    "appmenu_customizeMenu:child",
> +      "appmenu_preferences",
> +      "appmenu_toggleTabsOnTop",

I also don't see this item on my windows machine. Is it provided by an add-on?

@@ +180,5 @@
> +
> +  _getAppmenuItemId: function(aEvent) {
> +    let candidate =
> +      aEvent.originalTarget.id ? aEvent.originalTarget.id
> +                               : getParentId(aEvent.originalTarget);

getParentId seems to not be defined?

@@ +183,5 @@
> +      aEvent.originalTarget.id ? aEvent.originalTarget.id
> +                               : getParentId(aEvent.originalTarget);
> +
> +    // We only accept items that are in our whitelist OR start with
> +    // "charset."

Because of the ID change, this will need to change too

Comment 6

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

Attachment: Patch v1.3

Comment 7

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

Attachment: Patch v1.4

Comment 8

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

Comment on attachment 8340119 [details] [diff] [review]
Patch v1.4

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8340014 [details] [diff] [review]
> Patch v1.2
> 
> Review of attachment 8340014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks OK, but I think getParentId went missing and there's a few IDs in
> the list that drew my attention, so f+ for now. :-)
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +22,5 @@
> > +// bookmarks, history and feed menu popups where the user might
> > +// click on an id-less node, we record this as a click on the
> > +// menupopup id with a :child suffix.
> > +const kAppmenuWhitelist = [
> > +  "appmenu_newTab:child",
> 
> So... I just did the following (yay scratchpad/console):
> 
> 
> let ary = document.querySelectorAll("[id*='appmenu']")
> Array.map(ary, (el) => el.id).join('\n');
> 
> I don't see this appmenu_newTab:child of which you speak. What is it? Was
> this meant to be autogenerated for add-ons that add stuff to it? Same with
> the other :child bits below (which I also don't see in my list). There
> doesn't seem to be code in this patch that does that...
> 

(Already said in IRC, but included here for posterity) So I tried to make this clearer in the documentation above the whitelist - basically, splitmenu items have a left and right side, the right side spawns the menu popup, and the left side is some command. When we click on the left side, we're actually clicking on a child of the splitmenu element which has no ID. That's why we accept appmenu_newTab:child - that indicates that we're clicking on the left half of the new tab splitmenu button.


> @@ +50,5 @@
> > +    "appmenu_print_popup",
> > +    "appmenu_printPreview",
> > +    "appmenu_printSetup",
> > +
> > +  "appmenu_webDeveloper:child",
> 
> My privacy geek wonders if we really care about all these items or if we are
> OK just knowing people clicked on something in this menu. Collecting less is
> always better.
> 

I'm communicating with the folks in #privacy about this, but so far I haven't heard any worry.

> @@ +73,5 @@
> > +
> > +  "chardet.off",
> > +  "chardet.ja_parallel_state_machine",
> > +  "chardet.ruprob",
> > +  "chardet.ukprob",
> 
> These IDs changed recently. Bug 943524.
> 

Thanks! Rebased and modified to work with that patch.

> @@ +106,5 @@
> > +
> > +  "appmenu_customize:child",
> > +    "appmenu_customizeMenu:child",
> > +      "appmenu_preferences",
> > +      "appmenu_toggleTabsOnTop",
> 
> I also don't see this item on my windows machine. Is it provided by an
> add-on?
> 

Nope, it's in browser-appmenu.inc, but it's very much hidden. Strangely, it seems to get removed from the DOM somehow, but mxr isn't helping me find out how that happens. Pretty strange. Anyhow, I've removed it from the whitelist.


> @@ +180,5 @@
> > +
> > +  _getAppmenuItemId: function(aEvent) {
> > +    let candidate =
> > +      aEvent.originalTarget.id ? aEvent.originalTarget.id
> > +                               : getParentId(aEvent.originalTarget);
> 
> getParentId seems to not be defined?
> 

How embarrassing. I've re-included it.

> @@ +183,5 @@
> > +      aEvent.originalTarget.id ? aEvent.originalTarget.id
> > +                               : getParentId(aEvent.originalTarget);
> > +
> > +    // We only accept items that are in our whitelist OR start with
> > +    // "charset."
> 
> Because of the ID change, this will need to change too

Good call - fixed.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8340119 [details] [diff] [review]
Patch v1.4

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

With 2 nits, r=me. :-)

::: browser/modules/BrowserUITelemetry.jsm
@@ +133,5 @@
> +];
> +
> +const APPMENU_PREFIX_WHITELIST = [
> +  'appmenu_developer_charset',
> +  'appmenu_charset',

But you only need appmenu_charset for appmenu_charsetMenu, AFAICT. Might as well include it explicitly in the list above.

@@ +242,5 @@
> + * Returns the id of the first ancestor of aNode that has an id, with
> + * ":child" appended to it. If aNode has no parent, or no ancestor has an
> + * id, returns null.
> + */
> +function getIDBasedOnFirstIDedAncestor(aNode) {

In this function, just s/parent/aNode/ everywhere, and delete the first 4 lines after this comment. No need to unroll the loop. :-)

Comment 10

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

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8340119 [details] [diff] [review]
> Patch v1.4
> 
> Review of attachment 8340119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With 2 nits, r=me. :-)
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +133,5 @@
> > +];
> > +
> > +const APPMENU_PREFIX_WHITELIST = [
> > +  'appmenu_developer_charset',
> > +  'appmenu_charset',
> 
> But you only need appmenu_charset for appmenu_charsetMenu, AFAICT. Might as
> well include it explicitly in the list above.
> 

I need appmenu_charset for each of the character encoding choices under appmenu_charsetMenu.

> @@ +242,5 @@
> > + * Returns the id of the first ancestor of aNode that has an id, with
> > + * ":child" appended to it. If aNode has no parent, or no ancestor has an
> > + * id, returns null.
> > + */
> > +function getIDBasedOnFirstIDedAncestor(aNode) {
> 
> In this function, just s/parent/aNode/ everywhere, and delete the first 4
> lines after this comment. No need to unroll the loop. :-)

No problem!

Comment 11

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

Landed on holly as https://hg.mozilla.org/projects/holly/rev/8ae5dbb8a36a

I'll request Aurora uplift when it turns green.

Comment 12

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Comment on attachment 8340119 [details] [diff] [review]
> > Patch v1.4
> > 
> > Review of attachment 8340119 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > With 2 nits, r=me. :-)
> > 
> > ::: browser/modules/BrowserUITelemetry.jsm
> > @@ +133,5 @@
> > > +];
> > > +
> > > +const APPMENU_PREFIX_WHITELIST = [
> > > +  'appmenu_developer_charset',
> > > +  'appmenu_charset',
> > 
> > But you only need appmenu_charset for appmenu_charsetMenu, AFAICT. Might as
> > well include it explicitly in the list above.
> > 
> 
> I need appmenu_charset for each of the character encoding choices under
> appmenu_charsetMenu.

Do you actually see such menuitems? When I checked, I only saw appmenu_developer_charset... menuitems. Not near my Windows machine now, so can't recheck, but that's why I asked.

Comment 13

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

Comment on attachment 8340119 [details] [diff] [review]
Patch v1.4

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

None. This just gives BrowserUITelemetry the capability of counting click events on (whitelisted) children of the Firefox button.


User impact if declined: 

None.


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

Lots of manual testing on Holly, which has since merged to Aurora.


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

None.


String or IDL/UUID changes made by this patch:

None.

Comment 14

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

Removing Australis:P1 whiteboard tag because these already block an Australis:P1 bug, and being redundant isn't helpful.

Comment 15

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

Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/caebdf206440