944049 - Collect UITelemetry on bookmarks toolbar usage

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

In particular, we'd like to:

 * Collect a boolean of whether or not the bookmarks toolbar is open
 * Collect the number of times a bookmark is opened from clicking items within the bookmarks toolbar

This is the first UITelemetry probe that will be applicable in Australis-land, so this'll give us a good excuse to land bug 942244 on mozilla-central as well.

Comment 1

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

Attachment: Patch v1

Logs clicks on PersonalToolbar items that are containers and items that are not containers.

mak - are there other things that can go inside PlacesToolbarItems that I'm not considering? So far, I'm just differentiating with the container attribute...

Comment 2

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

Attachment: Patch v1.1

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

It depends if you care about measuring the chevron, it's an overflow of the toolbar, so if you care to measure the toolbar usage you should probably take it into account. That is also connected to whether you intend to count clicks on items that are inside containers on the toolbar, this code doesn't seem to differentiate at first glance (on Mac it may be more common to mousedown a menu and mouseup on an item inside it.

Comment 4

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

Attachment: Patch v1.2

Comment 5

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

Attachment: Patch v1.3

Comment 6

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

Attachment: Patch v1.4

Comment 7

[:Gijs (he/him)]

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

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

This looks alright bar the issue below. r+ if you check that you're actually on/in an item.

::: browser/modules/BrowserUITelemetry.jsm
@@ +337,5 @@
> +  _PlacesToolbarItemsMouseUp: function(aEvent) {
> +    let target = aEvent.originalTarget;
> +    if (target.hasAttribute("container")) {
> +      this._countEvent("click-bookmarks-bar", "container");
> +    } else {

Is there nothing else in here? What if I click an empty part of the bookmarks toolbar?

Comment 8

[:Gijs (he/him)]

Oh, and it doesn't look like you've addressed comment #3... do we not care about items in the chevron?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8343108 [details] [diff] [review]
> Patch v1.4
> 
> Review of attachment 8343108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks alright bar the issue below. r+ if you check that you're actually
> on/in an item.
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +337,5 @@
> > +  _PlacesToolbarItemsMouseUp: function(aEvent) {
> > +    let target = aEvent.originalTarget;
> > +    if (target.hasAttribute("container")) {
> > +      this._countEvent("click-bookmarks-bar", "container");
> > +    } else {
> 
> Is there nothing else in here? What if I click an empty part of the
> bookmarks toolbar?

I don't think we care about that case (we don't care about clicks on empty parts of the other toolbars either.

(In reply to :Gijs Kruitbosch from comment #8)
> Oh, and it doesn't look like you've addressed comment #3... do we not care
> about items in the chevron?

Ah, but we do care about that case. Thanks for reminding me!

Comment 10

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

Attachment: Patch v1.5

Added monitoring for the chevron and friends.

Comment 11

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #9)
> (In reply to :Gijs Kruitbosch from comment #7)
> > Comment on attachment 8343108 [details] [diff] [review]
> > Patch v1.4
> > 
> > Review of attachment 8343108 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks alright bar the issue below. r+ if you check that you're actually
> > on/in an item.
> > 
> > ::: browser/modules/BrowserUITelemetry.jsm
> > @@ +337,5 @@
> > > +  _PlacesToolbarItemsMouseUp: function(aEvent) {
> > > +    let target = aEvent.originalTarget;
> > > +    if (target.hasAttribute("container")) {
> > > +      this._countEvent("click-bookmarks-bar", "container");
> > > +    } else {
> > 
> > Is there nothing else in here? What if I click an empty part of the
> > bookmarks toolbar?
> 
> I don't think we care about that case (we don't care about clicks on empty
> parts of the other toolbars either.

Sure, but you'd register the mouseup, end up in this handler, and register it as clicking an item, if I understand the current code correctly (unless the PlacesToolbarItems node has a container attribute, too).

Comment 12

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

Attachment: Patch v1.6 - non-Australis

Ignore items that don't have the bookmark-item class.

Comment 13

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

Comment on attachment 8343183 [details] [diff] [review]
Patch v1.6 - non-Australis

Good call, Gijs - I was actually recording clicks on the empty space. This patch only reacts to clicks on bookmark-items in the bookmarks bar now.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8343183 [details] [diff] [review]
Patch v1.6 - non-Australis

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

r=me, vague question below (preliminary testing says 'no', but hey...)

::: browser/modules/BrowserUITelemetry.jsm
@@ +349,5 @@
> +    if (!target.classList.contains("bookmark-item")) {
> +      return;
> +    }
> +
> +    let result = target.hasAttribute("container") ? "container" : "item";

Dumb question: can a node have container="false" ? Otherwise, this seems fine.

Comment 15

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

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8343183 [details] [diff] [review]
> Patch v1.6
> 
> Review of attachment 8343183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, vague question below (preliminary testing says 'no', but hey...)
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ +349,5 @@
> > +    if (!target.classList.contains("bookmark-item")) {
> > +      return;
> > +    }
> > +
> > +    let result = target.hasAttribute("container") ? "container" : "item";
> 
> Dumb question: can a node have container="false" ? Otherwise, this seems
> fine.

According to mak, it's just "true", or not present at all, so I think we're good here. Thanks for the review!

Comment 16

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

pre-Australis patch landed on Holly: https://hg.mozilla.org/projects/holly/rev/77afab7f892f

Comment 17

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

Comment on attachment 8343183 [details] [diff] [review]
Patch v1.6 - non-Australis

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

None. This just gives BrowserUITelemetry the capability of counting click events in the Bookmarks toolbar.


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 18

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

Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.

Comment 19

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

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

Comment 20

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

Attachment: Patch v1 - for Australis

Comment 21

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

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

Nope, got my signs flipped.

Comment 22

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

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

This is pretty much a straight-port.

Comment 23

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

Attachment: Patch v1.1 - for Australis

Fixing a whitespace issue.

Comment 24

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

Thanks!

Landed on fx-team as https://hg.mozilla.org/integration/fx-team/rev/60c16e794a14

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/60c16e794a14