Closed Bug 944145 Opened 6 years ago Closed 6 years ago

Collect UITelemetry on default toolbar button usage

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Australis:P-][good first verify][fixed-in-holly])

Attachments

(2 files, 3 obsolete files)

In particular, we'd like to:

* Count the number of times the bookmarks menu button is opened
* Count the number of times a bookmark is opened from the bookmarks menu button

This applies to both pre and post-Australis.
Whiteboard: [Australis:P1]
We're interested in collecting click counts on all default toolbar buttons that might move into the panel for Australis.

This does not include buttons provided by add-ons - only the built-in buttons.
Summary: Collect UITelemetry on bookmark menu button usage → Collect UITelemetry on default toolbar button usage
Comment on attachment 8341882 [details] [diff] [review]
Patch v1

Curious to know what you think about my expando property hack, Gijs...

I'm trying to make it easier for us to detect things like clicks on the star-button without incrementing the click count on the urlbar-container - so if a child handles the event, mark the event so that we ignore it while it bubbles up. Seems probably safer than trying to prevent the event from bubbling, which might mess up add-ons or other bits of code.

Is there a better way of doing what I'm trying to do?
Attachment #8341882 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8341882 [details] [diff] [review]
Patch v1

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

I would try to add a listener to the container. If that's too complex or has downsides I've not figured out yet, this approach could work, too, but I'd like to try that first.

::: browser/modules/BrowserUITelemetry.jsm
@@ +235,5 @@
>      }
> +
> +    for (let itemID of ALL_BUILTIN_ITEMS) {
> +      // Attempt to get the item in the document, and if it's not there,
> +      // check the palette. getElementsByAttribute("id", someID)[0] will

Nit: s/someID/itemID/

@@ +239,5 @@
> +      // check the palette. getElementsByAttribute("id", someID)[0] will
> +      // be undefined if no such element exists with id someID.
> +      let item =
> +        document.getElementById(itemID) ||
> +        aWindow.gNavToolbox.palette.getElementsByAttribute("id", itemID)[0];

Nit: intermediate for the palette, then keep the assignment spread over 2 lines instead of 3.

@@ +241,5 @@
> +      let item =
> +        document.getElementById(itemID) ||
> +        aWindow.gNavToolbox.palette.getElementsByAttribute("id", itemID)[0];
> +
> +      if (item) {

So, rather than adding an event listener to each individual item, why don't you add a listener to the container toolbar/panel? You can then check the originalTarget (and if necessary, walk up from there) to figure out for which item the event fired, and you'll be sure that you catch each event exactly once.

You'd need to walk the ancestor chain until you find the toolbar or the menu panel, and then keep a list of which containers you've already added a listener to, but that seems somewhat less hacky than the expando, and will also mean the number of event listeners will be lower and the code simpler.

If you add them with the system event listener service, I think you will even be able to get these if items catch the mouseup event themselves and stopPropagate/preventDefault it.
Attachment #8341882 - Flags: feedback?(gijskruitbosch+bugs)
Attached patch Patch v2 (obsolete) — Splinter Review
Yes, putting the mouseup event listeners on the toolbars was a way, way better idea.
Attachment #8341882 - Attachment is obsolete: true
Attachment #8343100 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8343100 [details] [diff] [review]
Patch v2

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

This looks alright but I'm concerned about the direct id check vs. a loop. Can you assuage my concerns or am I right here? :-)

::: browser/modules/BrowserUITelemetry.jsm
@@ +278,5 @@
>  
>    _handleMouseUp: function(aEvent) {
> +    let targetID = aEvent.currentTarget.id;
> +
> +    switch(targetID) {

Nit: space before the (

@@ +285,5 @@
> +        break;
> +      default:
> +        // Perhaps we're seeing one of the default toolbar items
> +        // being clicked.
> +        if (ALL_BUILTIN_ITEMS.indexOf(aEvent.originalTarget.id) != -1) {

Are you sure this works? I suspect that the originalTarget might also be a descendant of the item, e.g. with the stuff inside the URL bar, or with the elements inside the search bar, etc.

I suspect you need a loop here to walk the ancestor chain up to the toolbar (at which point, if we've not got anything, we should give up). Am I just wrong? (could be!)
(In reply to :Gijs Kruitbosch from comment #6)
> Are you sure this works? I suspect that the originalTarget might also be a
> descendant of the item, e.g. with the stuff inside the URL bar, or with the
> elements inside the search bar, etc.

So right now, ALL_BUILTIN_ITEMS includes IDs for the items in the URL bar... haven't added any for the search input, since at this point UX hasn't expressed interest in it (plus, the children use classes instead of IDs... *sigh*). So I think I agree that we should do the loop up to the toolbar if our first look in ALL_BUILTIN_ITEMS turns up negative.

I'll update the patch.
Comment on attachment 8343100 [details] [diff] [review]
Patch v2

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

Clearing this for now per comment 7
Attachment #8343100 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v2.1 (obsolete) — Splinter Review
So now after we get a mouseup event on one of the toolbars, we first check the ALL_BUILTIN_ITEMS array for the item ID. If it's not in the array, we check for the first ID'd ancestor (up to the toolbar), and see if that result is in the ALL_BUILTIN_ITEMS.

If neither of those things yields results, we bail out.
Attachment #8343100 - Attachment is obsolete: true
Fixed silly typo.
Attachment #8343154 - Attachment is obsolete: true
Attachment #8343156 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8343156 [details] [diff] [review]
Patch v2.2 - for non-Australis

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

LGTM!

::: browser/modules/BrowserUITelemetry.jsm
@@ +320,5 @@
>  
>      return "unrecognized";
>    },
>  
> +  _checkForBuiltinItem: function(aEvent) {

Nit: you use aEvent.originalTarget several times, and in one case it causes the line to wrap onto the next line. If you stick it in an intermediate (say, 'item', seeing as that's the term we're using (prefixed) in the rest of the variable names), then you can save some redundancy and unwrap the wrapped line.

@@ +334,5 @@
> +    let toolbarID = aEvent.currentTarget.id;
> +    // If not, we need to check if one of the ancestors of the clicked
> +    // item is in our list of built-in items to check.
> +    let candidate = getIDBasedOnFirstIDedAncestor(aEvent.originalTarget,
> +                                                  toolbarID);

Hrm. This will work assuming there's no nested IDed things we care about. Technically, the search-container has an id and then the searchbar element has one, too, but I suspect it'll be almost impossible to hit the latter but not the former (and certainly not if you actually want to click into the focusable part of it). I don't *think* there's any others.

Note that regarding the search bar and URL bar, maybe we want to catch ctrl+l/ctrl-k as well? That can be a followup, though.
Attachment #8343156 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #11)
> Comment on attachment 8343156 [details] [diff] [review]
> Patch v2.2
> 
> Review of attachment 8343156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM!
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +320,5 @@
> >  
> >      return "unrecognized";
> >    },
> >  
> > +  _checkForBuiltinItem: function(aEvent) {
> 
> Nit: you use aEvent.originalTarget several times, and in one case it causes
> the line to wrap onto the next line. If you stick it in an intermediate
> (say, 'item', seeing as that's the term we're using (prefixed) in the rest
> of the variable names), then you can save some redundancy and unwrap the
> wrapped line.
> 

Good idea. I'll do that before I land.

> @@ +334,5 @@
> > +    let toolbarID = aEvent.currentTarget.id;
> > +    // If not, we need to check if one of the ancestors of the clicked
> > +    // item is in our list of built-in items to check.
> > +    let candidate = getIDBasedOnFirstIDedAncestor(aEvent.originalTarget,
> > +                                                  toolbarID);
> 
> Hrm. This will work assuming there's no nested IDed things we care about.
> Technically, the search-container has an id and then the searchbar element
> has one, too, but I suspect it'll be almost impossible to hit the latter but
> not the former (and certainly not if you actually want to click into the
> focusable part of it). I don't *think* there's any others.

Good point. I don't think there are any others either, but I'll keep an eye out.

> 
> Note that regarding the search bar and URL bar, maybe we want to catch
> ctrl+l/ctrl-k as well? That can be a followup, though.

I will indeed file that follow-up.
Landed on Holly as https://hg.mozilla.org/projects/holly/rev/bd9075540b41

Filed follow-up bug about Ctrl-L / Ctrl-K as bug 946977.

Leaving open since we'll put the Australis version of this measurement in here too.
Status: NEW → ASSIGNED
Comment on attachment 8343156 [details] [diff] [review]
Patch v2.2 - for non-Australis

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

None. This just gives BrowserUITelemetry the capability of counting click events on the whitelisted set of built-in toolbar items on customizable toolbars.


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 #8343156 - Flags: approval-mozilla-beta?
Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.
Whiteboard: [Australis:P1]
Attachment #8343156 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [Australis:P-] → [Australis:P-][good first verify]
Attachment #8343156 - Attachment description: Patch v2.2 → Patch v2.2 - for non-Australis
Comment on attachment 8350354 [details] [diff] [review]
Patch v1 - for Australis

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

Thoughts on this, jaws?
Attachment #8350354 - Flags: review?(jaws)
Comment on attachment 8350354 [details] [diff] [review]
Patch v1 - for Australis

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

::: browser/modules/BrowserUITelemetry.jsm
@@ +114,5 @@
> +    "copy-button",
> +    "paste-button",
> +    "zoom-out-button",
> +    "zoom-reset-button",
> +    "zoom-in-button",

Do you need to add the bookmarks-menu-button's toolbarbutton child and the dropmarker child?
Attachment #8350354 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #19)
> Comment on attachment 8350354 [details] [diff] [review]
> Patch v1 - for Australis
> 
> Review of attachment 8350354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +114,5 @@
> > +    "copy-button",
> > +    "paste-button",
> > +    "zoom-out-button",
> > +    "zoom-reset-button",
> > +    "zoom-in-button",
> 
> Do you need to add the bookmarks-menu-button's toolbarbutton child and the
> dropmarker child?

Thanks jaws! No, we don't need those because the bookmarks-menu-button is fully special-cased via _bookmarksMenuButtonMouseUp.
Whiteboard: [Australis:P-][good first verify] → [Australis:P-][good first verify][fixed-in-holly]
https://hg.mozilla.org/mozilla-central/rev/ffc142af3ca9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.