366088 - many menuitem clicks are not logged

Status: RESOLVED FIXED

(Toolkit Graveyard :: Data Collection/Metrics, defect)

Description

[Brian Ryner (not reading)]

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).

Comment 1

[Brian Ryner (not reading)]

Attachment: 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.

Comment 2

[Marria Nazif]

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

Comment 3

[Brian Ryner (not reading)]

checked in