Closed Bug 1107782 Opened 5 years ago Closed 4 years ago
State Managers event filter for "user-interaction-active"
Currently we count NS_MOUSE_MOZHITTEST for "user-interaction-active", which is wrong.
blacklist might be just fine here too, but sure NS_MOUSE_MOZHITTEST isn't a user input. Want to write a patch? I promise a fast review :)
I will put a patch up, was just too tired earlier. Whitelisting sounds more robust in case of new event types being added (most likely they are not tracked activity).
We should be aware that fixing this bug could have some implications, including possibly lower number of FHR activeTicks count, more opportunities for maintenance tasks (GC/CC etc) etc.
We don't use user-interaction-* for GC/CC scheduling anymore.
This fixes the mouse filtering. Looking through the events we have, we were definitely missing gamepad events, i added those. What i'm not sure about yet is whether we account for touch events properly (surface devices & other touchscreens). Not asking for review at this point because our important activeTicks metric is based on this and we have to figure out whether we can break it.
Summary: EventStateManager should whitelist mouse events for "user-interaction-active" → Fix EventStateManagers event filter for "user-interaction-active"
bsmedberg, i believe there are talks ongoing on whether we can change this, which implies changing the activeTicks metrics significantly. Were there any results on that yet?
Personally I'd prefer we add a "user-interaction-active2" event and report it in a new field. It looks like it wouldn't be too hard to do based on this patch. I defer the decision to Benjamin, he knows better how the Firefox usage metric is actually used
(In reply to Vladan Djeric (:vladan) from comment #7) > Personally I'd prefer we add a "user-interaction-active2" event Please no. That would just cause confusion. We don't really use user-interaction-* in Gecko atm. The notification is used only by services/datareporting/sessions.jsm so I'm sure it could be tweaked to deal with the possible change in notification triggering. (There have be several changes to how we trigger the user-interaction-* notifications in the past.)
(In reply to Olli Pettay [:smaug] from comment #8) > The notification is used only by services/datareporting/sessions.jsm so I'm > sure it could be tweaked to deal with the possible change in notification > triggering. If it suddenly just triggers differently, i don't think we can keep the metric based on it stable. I think it really comes down whether we can live with changing the context for the metric while we're fixing things.
Just a reminder since I don't see it mentioned explicitly: if you change the behavior of the event, you should bump the FHR measurement version. That way server-side analysis will be able to easily differentiate between the 2 population sets.
The issue we need a decision on is with losing the historic basis for comparison on this metric.
So we are currently ok with changes to this metric, provided they go with a bump to the FHR provider version.
Comment on attachment 8532727 [details] [diff] [review] Fix event filter for "user-interaction-active"/"-inactive" smaug, does this good? Do you think this is complete regarding events that represent user activity? What about touch events or other things?
Attachment #8532727 - Flags: review?(bugs)
Comment on attachment 8532727 [details] [diff] [review] Fix event filter for "user-interaction-active"/"-inactive" For touch, just add aEvent->mClass == eTouchEventClass
Attachment #8532727 - Flags: review?(bugs) → review+
Updated to review comment.
Please hold off on landing this until I hear back form the FHR people. I think code ugliness (two user-interaction-active notifications & two activeTicks counts) for a release or two is bearable if it helps maintain continuity of an important metric like usage hours
I don't actually have time for this right now with the FHR/Telemetry unification work.
Assignee: gfritzsche → nobody
On Windows we're sending artificial hittests every 200ms if there's a mouse just hanging idle over one of our windows. bug 506815 is where I'm looking to stop that. Even without :gfritzsche's patch that'll make quite the change to the existing measurement. There's coordination needed here.
From discussion on fhr-dev ( https://mail.mozilla.org/pipermail/fhr-dev/2015-November/000688.html ) we're good to get this patch in. Here's the trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83d9a37db63 Since the first patch, touch events were added and events' messages became an enum mMessage. Otherwise, the patch is mostly untouched.
Comment on attachment 8691009 [details] [diff] [review] 0001-bug-1107782-Only-accept-certain-mouse-gamepad-events.patch Great to see this finally landing. Moving the review to smaug who actually knows about this code.
Attachment #8691009 - Flags: review?(gfritzsche) → review?(bugs)
Comment on attachment 8691009 [details] [diff] [review] 0001-bug-1107782-Only-accept-certain-mouse-gamepad-events.patch >+static bool >+IsMessageMouseUserActivity(EventMessage m) Nit, argument names should be in form aName, so aMessage here >+static bool >+IsMessageGamepadUserActivity(EventMessage m) ditto
Attachment #8691009 - Flags: review?(bugs) → review+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.