Closed Bug 942335 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Menus, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- fixed
firefox29 --- wontfix

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Non-Australis-Only])

Attachments

(1 file, 5 obsolete files)

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
Whiteboard: [Australis:P1] → [Australis:P1][Non-Australis-Only]
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Attached patch Patch v1 (obsolete) — Splinter Review
This seems to do the job. What do you think of my approach, Blair?
Attachment #8338692 - Attachment is obsolete: true
Attachment #8338803 - Flags: review?(bmcbride)
Blocks: 943606
Attached patch Patch v1.1 (obsolete) — Splinter Review
Switches us from using a Set to a vanilla array, since bug 943606 required an array, and the inconsistency was annoying.
Attachment #8338803 - Attachment is obsolete: true
Attachment #8338803 - Flags: review?(bmcbride)
Attachment #8338896 - Flags: review?(bmcbride)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Replaced single with double quotes, as per Dao's suggestion in bug 943606.
Attachment #8338896 - Attachment is obsolete: true
Attachment #8338896 - Flags: review?(bmcbride)
Attachment #8340014 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8340014 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #8340014 - Attachment is obsolete: true
Attached patch Patch v1.4Splinter Review
Attachment #8340111 - Attachment is obsolete: true
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.
Attachment #8340119 - Flags: review?(gijskruitbosch+bugs)
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. :-)
Attachment #8340119 - Flags: review?(gijskruitbosch+bugs) → review+
(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!
Landed on holly as https://hg.mozilla.org/projects/holly/rev/8ae5dbb8a36a

I'll request Aurora uplift when it turns green.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
(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 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.
Attachment #8340119 - Flags: approval-mozilla-beta?
Removing Australis:P1 whiteboard tag because these already block an Australis:P1 bug, and being redundant isn't helpful.
Whiteboard: [Australis:P1][Non-Australis-Only] → [Non-Australis-Only]
Attachment #8340119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.