The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 45

Status

()

Core
Event Handling
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gfritzsche, Assigned: chutten)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 3

2 years ago
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.

Updated

2 years ago
Depends on: 1107779
We don't use user-interaction-* for GC/CC scheduling anymore.
Created attachment 8532727 [details] [diff] [review]
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.
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+
Created attachment 8536654 [details] [diff] [review]
Fix event filter for "user-interaction-active"/"-inactive"

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

Updated

2 years ago
Blocks: 1198187

Updated

2 years ago
Blocks: 1198650
No longer blocks: 1198187
Assignee: nobody → chutten
(Assignee)

Comment 18

a year ago
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.
(Assignee)

Comment 19

a year ago
Created attachment 8691009 [details] [diff] [review]
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.
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+
(Assignee)

Comment 22

a year ago
Created attachment 8691363 [details] [diff] [review]
0001-bug-1107782-Only-accept-certain-mouse-gamepad-events.patch
Attachment #8691009 - Attachment is obsolete: true
Attachment #8691363 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 23

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a5e293d017
Keywords: checkin-needed

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7a5e293d017
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

a year ago
Blocks: 1228368
You need to log in before you can comment on or make changes to this bug.