Port Chromium OS touch noise filtering to Chromium

Port Chromium OS touch noise filtering to Chromium. The Chromium OS
touch noise filtering is at
https://chromium.googlesource.com/chromiumos/platform/touch_noise_filter/+/master

When a touch is detected as touch noise, an ET_TOUCH_CANCELLED event is sent and
all of the subsequent events for that touch are dropped. A "touch press"
initiates a new touch.

BUG=407840
TEST=TouchNoiseFinderTest.*, TouchEventConverterEvdevTouchNoiseTest.*

Committed: https://crrev.com/1df4aa125090c601b1a18c4354b8ca9323cb4c79
Cr-Commit-Position: refs/heads/master@{#322293}

[pkotwicz, 2015-03-08 00:46:57]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-03-08 00:49:49]

pkotwicz@chromium.org changed reviewers:
+ flackr@chromium.org

[pkotwicz, 2015-03-08 00:49:50]

flackr@ can you please take a preliminary look at the touch noise filtering
code? This is mostly a copy and paste from the Chromium OS code base.

[flackr, 2015-03-10 05:23:40]

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:285:
std::vector<TouchEventParams> to_dispatch;
If I understand correctly, we construct and copy changed events into this temp
vector, and then effectively reconstruct the events_ array in
touch_noise_remover from the changed events vector, and then search for canceled
touches in that copy of the touches array to remove from the to_dispatch vector,
so that we can iterate over it and dispatch.

This seems fairly inefficient. Couldn't we have touch_noise_remover look
directly at the events_ array and maintain a canceled touches bool array or even
update the event type to ET_TOUCH_CANCELLED as necessary? I recognize that this
will require a bit of refactoring of the filters to use the fields from events_
but this is called at 60Hz or more so it's probably worth making sure it's fast.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:297: events_[i].type_ =
ET_TOUCH_MOVED;
Since we change the event type to MOVED regardless of whether the touch was
canceled, it seems very important that once a touch is cancelled it remain
cancelled by the touch_noise_remover_. It's probably worth enforcing this or
verifying this with a test to prevent suddenly getting moved events for a touch
for which you never got a start.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:303:
touch_noise_remover_->RemoveNoise(&to_dispatch, delta);
Can't just remove updates for noisy touches and not dispatch events for them. In
some cases, we don't identify that a touch is noise on the very first frame in
which that finger shows up. If this happens we'll deliver a touch start, perhaps
some touch moves, and then never send a touch end. 

Instead we need to dispatch an ET_TOUCH_CANCELLED event if an existing touch is
cancelled (doesn't hurt to always send a touch cancelled which is I believe the
current behavior in X). This will cancel any touch start effects (like :active
css pseudoclass) and prevent generation of timer based events (i.e. long press).

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc
(right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc:84:
cur->canceled = true;
Since we're in chrome codebase now, we should add UMA stats to track canceled
touches (perhaps by filter) so that if we don't see many / any of a particular
kind of "noise" we can remove that filter.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/touch_noise_filter.h (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_filter.h:15: static const int
kNumSlots = 64;
Should probably use the same number of slots as in EventDeviceInfo:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/ozone/ev...

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/touch_noise_remover.cc (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_remover.cc:37: if (slot < 0 ||
slot >= num_slots)
Why do we need to check for slot < 0 || slot >= num_slots? Shouldn't all touches
have valid slots?

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_remover.cc:54: if (slot < 0 ||
slot >= num_slots || frame_.fingers[slot].canceled) {
ditto

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_remover.cc:56: it =
touches->erase(it);
Should use std::remove_if for linear complexity.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/touch_noise_remover_unittest.cc (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_remover_unittest.cc:142: }
Thanks for adding unit tests! The cancel behavior when a touch is canceled after
the PRESSED event is also important, perhaps we should have a test that verifies
cancel events will be delivered at least for in progress touches.

[pkotwicz, 2015-03-13 03:10:28]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2015-03-13 03:15:04]

Patchset #2 (id:60001) has been deleted

[pkotwicz, 2015-03-13 03:22:00]

Patchset #2 (id:80001) has been deleted

[pkotwicz, 2015-03-13 03:25:12]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2015-03-13 03:31:16]

Patchset #2 (id:120001) has been deleted

[pkotwicz, 2015-03-13 03:33:38]

Patchset #2 (id:140001) has been deleted

[pkotwicz, 2015-03-13 03:53:16]

Rob, can you please take another look? Sorry for the long delay.

The new CL makes use of |TouchEventConverterEvdev::events_| in TouchNoiseFinder
so is pretty different from the previous CL

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:285:
std::vector<TouchEventParams> to_dispatch;
I have refactored this as you requested

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:303:
touch_noise_remover_->RemoveNoise(&to_dispatch, delta);
I have changed the code to dispatch ET_TOUCH_CANCELLED when a touch is
cancelled. This is slightly different from what events_x.cc does.
GetTouchEventType() drops cancelled ET_TOUCH_PRESSes and ET_TOUCH_MOVEs. It
converts cancelled ET_TOUCH_RELEASEs to ET_TOUCH_CANCELLEDs

I have added a test as you requested:
TouchEventConverterEvdevTouchNoiseTest.TouchNoiseFiltering

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc
(right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc:84:
cur->canceled = true;
I'd rather do this in a separate CL. I have filed crbug.com/466429 My main goal
in this CL is that the code that I am adding can be checked for "the same
behavior" as the current code in ChromeOS.

For instance, we may want to remove the VLOG()s in favor of UMA stats.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/touch_noise_filter.h (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/touch_noise_filter.h:15: static const int
kNumSlots = 64;
I have made the code use the same number of slots as TouchEventConverterEvdev
EVDEV_ABS_MT_COUNT is the number of multi-touch evdev bits (e.g.
ABS_MT_TOUCH_MAJOR) not the number of slots.

[flackr, 2015-03-13 15:08:36]

Thanks for integrating this, it looks good. Just a few comments.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:285:
std::vector<TouchEventParams> to_dispatch;
On 2015/03/13 03:53:16, pkotwicz wrote:
> I have refactored this as you requested

Thanks!

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:303:
touch_noise_remover_->RemoveNoise(&to_dispatch, delta);
On 2015/03/13 03:53:16, pkotwicz wrote:
> I have changed the code to dispatch ET_TOUCH_CANCELLED when a touch is
> cancelled. This is slightly different from what events_x.cc does.
> GetTouchEventType() drops cancelled ET_TOUCH_PRESSes and ET_TOUCH_MOVEs. It
> converts cancelled ET_TOUCH_RELEASEs to ET_TOUCH_CANCELLEDs
> 
> I have added a test as you requested:
> TouchEventConverterEvdevTouchNoiseTest.TouchNoiseFiltering

Thanks. That's very strange that we used to keep the "noisy" touch as valid
until the release, and not at all the intention.

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc
(right):

https://codereview.chromium.org/991533002/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_noise/far_apart_taps_touch_noise_filter.cc:84:
cur->canceled = true;
On 2015/03/13 03:53:16, pkotwicz wrote:
> I'd rather do this in a separate CL. I have filed crbug.com/466429 My main
goal
> in this CL is that the code that I am adding can be checked for "the same
> behavior" as the current code in ChromeOS.
> 
> For instance, we may want to remove the VLOG()s in favor of UMA stats.

Of course, we may also want to track a histogram of how close to tripping the
noise filter touches are to determine if we need to adjust our thresholds.

Acknowledged

https://codereview.chromium.org/991533002/diff/160001/ui/events/event_switches.h
File ui/events/event_switches.h (right):

https://codereview.chromium.org/991533002/diff/160001/ui/events/event_switche...
ui/events/event_switches.h:26: EVENTS_BASE_EXPORT extern const char
kExtraTouchNoiseFiltering[];
nit: sort

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:299: event->type =
ET_TOUCH_CANCELLED;
I think we should only report cancelled once for the touch and then suppress all
subsequent events. Also, if the touch was never sent (i.e. no touchdown) I think
we should completely suppress it (no cancel event) with the goal of the event
stream for any touch always being touchdown touchmove* (touchend|touchcancel)

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_noise/touch_noise_finder.cc (right):

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_noise/touch_noise_finder.cc:29: if
(slots_with_noise_.test(touch.slot) && touch.type == ET_TOUCH_PRESSED)
Testing that it's true before setting it false is unnecessary, isn't it?

[pkotwicz, 2015-03-17 01:28:36]

Patchset #4 (id:200001) has been deleted

[pkotwicz, 2015-03-17 01:32:29]

Patchset #4 (id:220001) has been deleted

[pkotwicz, 2015-03-17 01:34:48]

Patchset #4 (id:240001) has been deleted

[pkotwicz, 2015-03-17 01:37:27]

Patchset #4 (id:260001) has been deleted

[pkotwicz, 2015-03-17 01:41:27]

flackr@ can you please take another look? I believe I have addressed all of your
concerns

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_noise/touch_noise_finder.cc (right):

https://codereview.chromium.org/991533002/diff/160001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_noise/touch_noise_finder.cc:29: if
(slots_with_noise_.test(touch.slot) && touch.type == ET_TOUCH_PRESSED)
Yes. I was doing this because reads are typically faster than writes, but that's
probably an overoptimization

[flackr, 2015-03-17 03:54:15]

Looks good, just a couple comments. I think we don't need to introduce the new
flag as long as you track either when calling the filter went from not noise to
noise (in which case you don't need to change the touch type before dispatching)
or just track when the touch type first changes.

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_evdev_types.h (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_evdev_types.h:28: bool cancelled;
I think this can be equivalent to type == ET_TOUCH_CANCELLED.

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:319: // state to determine
whether future touches should be cancelled.
I think this would still work with the cancelled type as long as the touch noise
finder continues to get events for cancelled events.

[pkotwicz, 2015-03-17 16:51:23]

flackr@ can you please take another look?

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:319: // state to determine
whether future touches should be cancelled.
SinglePositionTouchNoiseFilter needs to know when a touch was actually released
(If we send ET_TOUCH_CANCELLED instead of ET_TOUCH_RELEASED to
SinglePositionTouchNoiseFilter, we change the behavior)

[flackr, 2015-03-17 16:59:04]

lgtm

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:319: // state to determine
whether future touches should be cancelled.
On 2015/03/17 16:51:23, pkotwicz wrote:
> SinglePositionTouchNoiseFilter needs to know when a touch was actually
released
> (If we send ET_TOUCH_CANCELLED instead of ET_TOUCH_RELEASED to
> SinglePositionTouchNoiseFilter, we change the behavior)

Right, I was thinking that we might be able to recognize the difference from a
still active (but cancelled) touch and no touch from the tracking id in the
events structure. That being said it's not a big deal either way, getting the
original event types is a nice thing to have.

[pkotwicz, 2015-03-17 17:04:49]

pkotwicz@chromium.org changed reviewers:
+ spang@chromium.org

[pkotwicz, 2015-03-17 17:04:50]

spang@, can you please take a look at the changes in 	ui/events/ozone/evdev ?

[spang, 2015-03-18 19:20:12]

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_evdev_types.h (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_evdev_types.h:33: size_t slot;
Is it needed to duplicate the offset into the slots vector here as a field?

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:219:
events_[current_slot_].cancelled = false;
I think this block should be cut down to just:

events_[current_slot_].tracking_id = input.value;
events_[current_slot_].cancelled = false;

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:290: if
(touch_noise_finder_ && touch_noise_finder_->SlotHasNoise(event.slot)) {
Could we centralize determining the event type here? Something like:

bool contact = (event->tracking_id != -1);
EventType event_type = ET_UNKNOWN;

if (event->cancelled)
  return;

if (touch_noise_finder_ && touch_noise_finder_->SlotHasNoise(event->slot)) {
  event->cancelled = true;
  event->client_contact = false;
  event_type = ET_TOUCH_CANCELLED;
} else if (contact && event->client_contact) {
  event_type = ET_TOUCH_MOVED;
} else if (contact && !event->client_contact) {
  event_type = ET_TOUCH_PRESSED;
  event->client_contact = true;
} else if (!contact && event->client_contact) {
  event_type = ET_TOUCH_RELEASED;
  event->client_contact = false;
}

dispatcher_->DispatchTouchEvent(...);

[pkotwicz1, 2015-03-18 20:00:02]

pkotwicz@google.com changed reviewers:
+ pkotwicz@google.com

[pkotwicz1, 2015-03-18 20:00:02]

spang@ can you please take another look?

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_evdev_types.h (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_evdev_types.h:33: size_t slot;
Yes, it is a duplicate. It makes the interface of
TouchNoiseFinder::HandleTouches() much easier to document

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_evdev_types.h:34: EventType type;
It sounds like you are suggesting removing the |type| field from the struct. The
TouchNoiseFilters currently make use of the |type| field (e.g.
HorizontallyAlignedTouchNoiseFilter).

We can remove the |type| field if we add a |previous_tracking_id| field. This
would allow the TouchNoiseFilters to infer whether a touch is press or is a
move.
Alternatively, TouchNoiseFinder can keep track of the tracking ids for the
previous time that TouchNoiseFinder::HandleTouches() was called.

[spang, 2015-03-18 20:52:18]

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_evdev_types.h (right):

https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_evdev_types.h:34: EventType type;
On 2015/03/18 20:00:02, pkotwicz22 wrote:
> It sounds like you are suggesting removing the |type| field from the struct.

Yes (or rather, breaking it into its constituent parts).

> The
> TouchNoiseFilters currently make use of the |type| field (e.g.
> HorizontallyAlignedTouchNoiseFilter).
> 
> We can remove the |type| field if we add a |previous_tracking_id| field. This
> would allow the TouchNoiseFilters to infer whether a touch is press or is a
> move.

I proposed a new field "client_pressed" to track that.

Maybe it should be called "was_pressed" if it's also need for filtering, though.

> Alternatively, TouchNoiseFinder can keep track of the tracking ids for the
> previous time that TouchNoiseFinder::HandleTouches() was called.

[spang, 2015-03-18 20:55:40]

On 2015/03/18 20:52:18, spang wrote:
>
https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
> File ui/events/ozone/evdev/touch_evdev_types.h (right):
> 
>
https://codereview.chromium.org/991533002/diff/280001/ui/events/ozone/evdev/t...
> ui/events/ozone/evdev/touch_evdev_types.h:34: EventType type;
> On 2015/03/18 20:00:02, pkotwicz22 wrote:
> > It sounds like you are suggesting removing the |type| field from the struct.
> 
> Yes (or rather, breaking it into its constituent parts).
> 
> > The
> > TouchNoiseFilters currently make use of the |type| field (e.g.
> > HorizontallyAlignedTouchNoiseFilter).
> > 
> > We can remove the |type| field if we add a |previous_tracking_id| field.
This
> > would allow the TouchNoiseFilters to infer whether a touch is press or is a
> > move.
> 
> I proposed a new field "client_pressed" to track that.
> 
> Maybe it should be called "was_pressed" if it's also need for filtering,
though.

Er, I actually said "client_contact".

> 
> > Alternatively, TouchNoiseFinder can keep track of the tracking ids for the
> > previous time that TouchNoiseFinder::HandleTouches() was called.

[pkotwicz, 2015-03-19 01:03:11]

Patchset #5 (id:300001) has been deleted

[pkotwicz, 2015-03-19 01:11:10]

Patchset #5 (id:320001) has been deleted

[pkotwicz, 2015-03-19 01:18:07]

I have removed InProgressTouchEvdev::type and have added
InProgressTouchEvdev::touching and InProgressTouchEvdev::was_touching

I used InProgressTouchEvdev::touching/was_touching instead of
InProgressTouchEvdev::pressed/was_pressed because I thought it would be clearer.
(I thought that "pressed" could be easily confused for ET_TOUCH_PRESSED)

[spang, 2015-03-19 17:08:50]

On 2015/03/19 01:18:07, pkotwicz wrote:
> I have removed InProgressTouchEvdev::type and have added
> InProgressTouchEvdev::touching and InProgressTouchEvdev::was_touching
> 
> I used InProgressTouchEvdev::touching/was_touching instead of
> InProgressTouchEvdev::pressed/was_pressed because I thought it would be
clearer.
> (I thought that "pressed" could be easily confused for ET_TOUCH_PRESSED)

I find this easier to read, thanks.

lgtm

[spang, 2015-03-19 17:08:59]

https://codereview.chromium.org/991533002/diff/340001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/340001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:211:
events_[current_slot_].touching = false;
Shouldn't we update the tracking id to -1 when the contact is removed?

https://codereview.chromium.org/991533002/diff/340001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:289: if (is_type_a_)
I'm doubtful that type A device support actually works properly. We should
probably just remove it until someone shows up with a device that actually uses
this protocol.

[pkotwicz, 2015-03-23 15:07:18]

Patchset #6 (id:360001) has been deleted

[pkotwicz, 2015-03-23 16:41:19]

Sadrul can you please take a look at the changes to ui/events/event_switches.*
I ran the try bots on "Patch Set #6"

I am going to look at whether we can drop type A device support separately.
According to miletus@ the X11 driver might convert the type A protocol into type
B.

[pkotwicz, 2015-03-23 16:41:34]

https://codereview.chromium.org/991533002/diff/340001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/991533002/diff/340001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:211:
events_[current_slot_].touching = false;
On 2015/03/19 17:08:58, spang wrote:
> Shouldn't we update the tracking id to -1 when the contact is removed?

Done.

[spang, 2015-03-23 16:51:41]

On 2015/03/23 16:41:19, pkotwicz wrote:
> Sadrul can you please take a look at the changes to ui/events/event_switches.*
> I ran the try bots on "Patch Set #6"
> 
> I am going to look at whether we can drop type A device support separately.
> According to miletus@ the X11 driver might convert the type A protocol into
type
> B.

Right, I don't think what we're doing now is actually correct because contact
can change identity between reports. It probably actually really screws things
up. We don't have any devices to test with, either.

The other thing is that it doesn't look like there are many protocol A drivers
in the kernel. It looks like only one driver uses it for a USB device ("ntrig"),
the rest seem to be I2C devices that we don't ship and therefore irrelevant.

So I do think it should be removed, and only added back if someone comes forward
with a device that needs it.

[pkotwicz, 2015-03-24 00:10:45]

Sadrul, can you please take a look. I have addressed spang@'s concern about type
A devices (See https://codereview.chromium.org/1022673006/)

[pkotwicz1, 2015-03-24 21:05:24]

pkotwicz@google.com changed reviewers:
+ sadrul@chromium.org

[pkotwicz, 2015-03-24 21:06:36]

Sadrul, can you please take a look at the change to ui/events/events_switches.*

I have addressed spang@'s concern about type A devices

[sadrul, 2015-03-24 21:12:08]

Please update the CL description to say what code this is based off of, and if
possible, what kind of filtering is done.

[pkotwicz, 2015-03-25 19:29:15]

I have updated the CL description. To my knowledge the algorithms that we used
for touch noise filtering do not have names. The touch noise filtering was only
done on the ChromeOS pixel. FarApartTapsFilter only "does something" on a
ChromeOS pixel because the taps need to be very very far apart. The default
screen resolution on the Lenovo Thinkpad Yoga 11e Chromebook (glimmer) is not
big enough for any taps to be considered noise

[sadrul, 2015-03-25 19:50:55]

Thanks! lgtm

[pkotwicz, 2015-03-25 21:17:38]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-03-25 21:17:38]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
spang@chromium.org
Link to the patchset: https://codereview.chromium.org/991533002/#ps410001
(title: " ")

[commit-bot: I haz the power, 2015-03-25 21:18:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/991533002/410001

[commit-bot: I haz the power, 2015-03-26 01:50:56]

Committed patchset #8 (id:410001)

[commit-bot: I haz the power, 2015-03-26 01:51:53]

Patchset 8 (id:??) landed as
https://crrev.com/1df4aa125090c601b1a18c4354b8ca9323cb4c79
Cr-Commit-Position: refs/heads/master@{#322293}

[Mr4D (OOO till 08-26), 2015-03-26 17:41:03]

A revert of this CL (patchset #8 id:410001) has been created in
https://codereview.chromium.org/1033933004/ by skuhne@chromium.org.

The reason for reverting is: This patch breaks the PFQ for 32/64 bit due to some
string conversion problems:

0.2346.0_rc-r1:
../../../../../../../home/chrome-bot/chrome_root/src/ui/events/ozone/evdev/touch_noise/single_position_touch_noise_filter.cc:100:63:
error: format '%ld' expects argument of type 'long int', but argument 3 has type
'int64 {aka long long int}' [-Werror=format=] chromeos-chrome-43.0.2346.0_rc-r1:
touch.tracking_id, max_duration.InMilliseconds());
should I revert this - or do you want to land a patch for it?
Oh - there is also another one:
0.2346.0_rc-r1:
../../../../../../../home/chrome-bot/chrome_root/src/ui/events/ozone/evdev/touch_noise/horizontally_aligned_touch_noise_filter.cc:
In member function 'virtual void
ui::HorizontallyAlignedTouchNoiseFilter::Filter(const
std::vector<ui::InProgressTouchEvdev>&, base::TimeDelta, std::bitset<20u>*)':
chromeos-chrome-43.0.2346.0_rc-r1:
../../../../../../../home/chrome-bot/chrome_root/src/ui/events/ozone/evdev/touch_noise/horizontally_aligned_touch_noise_filter.cc:39:66:
error: format '%ld' expects argument of type 'long int', but argument 3 has type
'int64 {aka long long int}' [-Werror=format=] chromeos-chrome-43.0.2346.0_rc-r1:
other_touch.tracking_id, other_touch.x, other_touch.y);.