Closed Bug 944049 Opened 6 years ago Closed 6 years ago

Collect UITelemetry on bookmarks toolbar 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])

Attachments

(2 files, 7 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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...
Flags: needinfo?(mak77)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8339527 - Attachment is obsolete: true
No longer blocks: 944145
Depends on: 944147
No longer depends on: 943606
Depends on: 944145
No longer depends on: 944147
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.
Flags: needinfo?(mak77)
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #8341888 - Attachment is obsolete: true
Attached patch Patch v1.4 (obsolete) — Splinter Review
Attachment #8343102 - Attachment is obsolete: true
Attachment #8343108 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8343108 - Flags: review?(gijskruitbosch+bugs) → review+
Oh, and it doesn't look like you've addressed comment #3... do we not care about items in the chevron?
(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!
Attached patch Patch v1.5 (obsolete) — Splinter Review
Added monitoring for the chevron and friends.
Attachment #8343108 - Attachment is obsolete: true
(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).
Ignore items that don't have the bookmark-item class.
Attachment #8343174 - Attachment is obsolete: true
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.
Attachment #8343183 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8343183 - Flags: review?(gijskruitbosch+bugs) → review+
(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 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.
Attachment #8343183 - Flags: approval-mozilla-beta?
Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.
Whiteboard: [Australis:P1]
Whiteboard: [Australis:P-]
Attachment #8343183 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [Australis:P-] → [Australis:P-][good first verify]
Attachment #8343183 - Attachment description: Patch v1.6 → Patch v1.6 - non-Australis
Comment on attachment 8348957 [details] [diff] [review]
Patch v1 - for Australis

Nope, got my signs flipped.
Attachment #8348957 - Attachment description: Patch v1 - for non-Australis → Patch v1 - for Australis
Comment on attachment 8348957 [details] [diff] [review]
Patch v1 - for Australis

This is pretty much a straight-port.
Attachment #8348957 - Flags: review?(jaws)
Fixing a whitespace issue.
Attachment #8348957 - Attachment is obsolete: true
Attachment #8348957 - Flags: review?(jaws)
Attachment #8348961 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/60c16e794a14
Status: NEW → 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.