All users were logged out of Bugzilla on October 13th, 2018

many menuitem clicks are not logged

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
The approach introduced in bug 338591 doesn't work very well in practice, since a lot of click targets (menuitems, etc) are missing id's.  The intent of this change was to differentiate between click and key events, and we can do that more reliably like this:

- revert to the pre-338591 behavior of logging the id of the command event target
- if the sourceEvent's target is a <key>, log that element's id in a separate "keyid" attribute on the event.  Key elements generally have ids.

With this change, any <uielement> event with a keyid would correspond to a keyboard shortcut, and otherwise would correspond to a click (or rarely, to a script-invoked command).
(Assignee)

Comment 1

12 years ago
Created attachment 250769 [details] [diff] [review]
patch

This patch adds the key ids as described above.  While I was adding this, I realized that HandleEvent() had gotten really long, so I broke out most things except for the actual logging into helper functions.  The logic should be the same, but I think it's easier to follow.

I added a unit test that exercises 2 of the helper functions.  The test itself is pretty straightforward, but I had to do some wacky stuff with module registration so that I could call non-interface methods.  Using the static component loader ensures that the copy of the metrics code linked into the binary is the same copy that XPCOM knows about.

I also bumped the data version to 2.  That will make it easy to distinguish between older metrics extensions that are logging the sourcEvent id/anonid, and newer ones which will revert to the Firefox 1.5 behavior for those properties.
Attachment #250769 - Flags: first-review?(marria)

Comment 2

12 years ago
Comment on attachment 250769 [details] [diff] [review]
patch

>+  // Given a command event, determines the appropriate id and anonid values
>+  // to log.
>+  nsresult GetEventTargets(nsIDOMEvent *event,
>+                           nsString &targetId, nsString &targetAnonId) const;

Might want to add to this comment to indicate when the function returns failure

Thanks for the reorganization and the unittest, looks great.
- Marria
Attachment #250769 - Flags: first-review?(marria) → first-review+
(Assignee)

Comment 3

12 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

8 years ago
Component: Data Collection/Metrics → Data Collection/Metrics
Flags: first-review+
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.