Closed Bug 947991 Opened 6 years ago Closed 6 years ago

BrowserUITelemetry should take note of which mouse button was used to click an item

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: [Non-Australis-Only][fixed-in-holly] [good first verify])

Attachments

(1 file, 2 obsolete files)

We have an event listener set on all of the toolbars to listen for mouseup events so that we can count clicks on the default toolbar items.  We are, however, not filtering by the mouse button being used to click the button.

I believe UX / UR are only interested in left-click counting for now, so we should ignore all other clicks.
Redirecting a bit - we're thinking of recording which mouse button was used to click each button.
Summary: BrowserUITelemetry should only care about left-mouse clicks for mouseup events → BrowserUITelemetry should take note of which mouse button was used to click an item
I don't think I'm going to have time to land this on Holly before the uplift, so requesting tracking.
This work was completed in bug 944114 for mozilla-central - I'll create a patch to port it over to Holly / Aurora.
Whiteboard: [Australis:P1][Non-Australis-Only]
Attached patch Patch v1.1 (obsolete) — Splinter Review
Ok, this seems to do the right thing.
Attachment #8345519 - Attachment is obsolete: true
Attachment #8345522 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8345522 [details] [diff] [review]
Patch v1.1

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

This looks fine, but see below nits

::: browser/modules/BrowserUITelemetry.jsm
@@ +287,5 @@
> +    let countObject = this._ensureObjectChain([aCategory, aAction], 0);
> +    countObject[aAction]++;
> +  },
> +
> +  _countMouseUpEvent: function(aCategory, aAction, aMouseUpEvent) {

Nit: I think I'd prefer if this passed aMouseButton (aEvent.button in the callsites)

@@ +431,5 @@
>    },
>  
>    _starButtonMouseUp: function(aEvent) {
>      let starButton = aEvent.originalTarget;
>      if (starButton.hasAttribute("starred")) {

Not sure how I missed this, but can you just make this do:

let action = starButton.hasAttribute("starred") ? "edit" : "add";
this._countMouseUpEvent("click-star-button", action, aEvent.button); // see above for why aEvent.button rather than aEvent

Rather than the if/else ? :-)
Attachment #8345522 - Flags: review?(gijskruitbosch+bugs) → review+
Sure, I can do that. And I'll push a follow-up to make the same change on mozilla-central as well.
Thanks Gijs - done (and landed a follow-up patch on mozilla-central - see https://bugzilla.mozilla.org/show_bug.cgi?id=944114#c23).

Landed on Holly as https://hg.mozilla.org/projects/holly/rev/14ad085be69f
Attachment #8345522 - Attachment is obsolete: true
Attachment #8345576 - Flags: review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][Non-Australis-Only] → [Australis:P1][Non-Australis-Only][fixed-on-holly]
Target Milestone: --- → Firefox 29
Comment on attachment 8345576 [details] [diff] [review]
Patch v1.2 (r+'d by Gijs)

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

None. This just gives BrowserUITelemetry more specificity in terms of click counting, in that it records which button was used to click an item.


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 #8345576 - Flags: approval-mozilla-beta?
Comment on attachment 8345576 [details] [diff] [review]
Patch v1.2 (r+'d by Gijs)

We'll want this on Aurora as well - the risks are the same for Beta.
Attachment #8345576 - Flags: approval-mozilla-aurora?
Removing Australis:P1 whiteboard tag because these already block an Australis:P1 and being redundant isn't helpful.
Whiteboard: [Australis:P1][Non-Australis-Only][fixed-on-holly] → [Non-Australis-Only][fixed-in-holly]
Attachment #8345576 - Flags: approval-mozilla-beta?
Attachment #8345576 - Flags: approval-mozilla-beta+
Attachment #8345576 - Flags: approval-mozilla-aurora?
Attachment #8345576 - Flags: approval-mozilla-aurora+
no need to track, go ahead and uplift.
Whiteboard: [Non-Australis-Only][fixed-in-holly] → [Non-Australis-Only][fixed-in-holly] [good first verify]
You need to log in before you can comment on or make changes to this bug.