Closed Bug 1107782 Opened 5 years ago Closed 4 years ago

Fix EventStateManagers event filter for "user-interaction-active"

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: gfritzsche, Assigned: chutten)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 1107779
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?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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.
Attachment #8532727 - Attachment is obsolete: true
Attachment #8536654 - Flags: review+
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
Blocks: 1069869
Blocks: 1105864
No longer blocks: 1069869
I don't actually have time for this right now with the FHR/Telemetry unification work.
Assignee: gfritzsche → nobody
Blocks: e10s-rc
Blocks: 1198650
No longer blocks: e10s-rc
Assignee: nobody → chutten
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.
Attachment #8536654 - Attachment is obsolete: true
Attachment #8691009 - Flags: review?(gfritzsche)
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)
Status: NEW → ASSIGNED
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7a5e293d017
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1228368
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.