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

Status: RESOLVED FIXED

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

Description

[Georg Fritzsche [:gfritzsche]]

Currently we count NS_MOUSE_MOZHITTEST for "user-interaction-active", which is wrong.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

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

Comment 3

[Avi Halachmi (:avih)]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

We don't use user-interaction-* for GC/CC scheduling anymore.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix event filter for "user-interaction-active"/"-inactive"

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

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?

Comment 7

[Vladan Djeric (:vladan)]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Georg Fritzsche [:gfritzsche]]

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

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Georg Fritzsche [:gfritzsche]]

The issue we need a decision on is with losing the historic basis for comparison on this metric.

Comment 12

[Georg Fritzsche [:gfritzsche]]

So we are currently ok with changes to this metric, provided they go with a bump to the FHR provider version.

Comment 13

[Georg Fritzsche [:gfritzsche]]

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?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8532727 [details] [diff] [review]
Fix event filter for "user-interaction-active"/"-inactive"

For touch, just add 
aEvent->mClass == eTouchEventClass

Comment 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix event filter for "user-interaction-active"/"-inactive"

Updated to review comment.

Comment 16

[Vladan Djeric (:vladan)]

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

I don't actually have time for this right now with the FHR/Telemetry unification work.

Comment 18

[Chris H-C :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.

Comment 19

[Chris H-C :chutten]

Attachment: 0001-bug-1107782-Only-accept-certain-mouse-gamepad-events.patch

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 20

[Georg Fritzsche [: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.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 22

[Chris H-C :chutten]

Attachment: 0001-bug-1107782-Only-accept-certain-mouse-gamepad-events.patch

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a5e293d017

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7a5e293d017