Add palm suppression feature to EventConverterEvdev

Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458
Committed: https://crrev.com/0cf5d6752db81e256e5e705bc64c4bb04dc1e34a
Cr-Commit-Position: refs/heads/master@{#414860}

[denniskempin, 2016-08-19 23:23:02]

denniskempin@google.com changed reviewers:
+ spang@chromium.org

[denniskempin, 2016-08-19 23:23:02]

PTAL Michael! Let me know if you're busy and would like to defer the review to
someone else!

[spang, 2016-08-22 20:52:13]

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

https://codereview.chromium.org/2263693003/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:445: if (touch.touching &&
!touch.was_touching)
Do we need this same check so that a new slot doesn't report a cancel (which
would look spurious from client's perspective)?

Or does your check in ReportTouchEvent() handle that?

[denniskempin, 2016-08-22 22:12:16]

On 2016/08/22 20:52:13, spang wrote:
>
https://codereview.chromium.org/2263693003/diff/40001/ui/events/ozone/evdev/t...
> File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):
> 
>
https://codereview.chromium.org/2263693003/diff/40001/ui/events/ozone/evdev/t...
> ui/events/ozone/evdev/touch_event_converter_evdev.cc:445: if (touch.touching
&&
> !touch.was_touching)
> Do we need this same check so that a new slot doesn't report a cancel (which
> would look spurious from client's perspective)?
> 
> Or does your check in ReportTouchEvent() handle that?
That is a good point. I'll add a check for that. I did not get to test this
behavior yet unfortunately since the kernel driver does not handle this
situation yet, working on that.

[denniskempin, 2016-08-23 22:28:12]

I have also updated the palm detection logic to look at touch major.

PTAL, I have been running a bunch of tests and it works amazingly well :)

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

https://codereview.chromium.org/2263693003/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:445: if (touch.touching &&
!touch.was_touching)
On 2016/08/22 20:52:13, spang wrote:
> Do we need this same check so that a new slot doesn't report a cancel (which
> would look spurious from client's perspective)?
> 
> Or does your check in ReportTouchEvent() handle that?

Done.

[spang, 2016-08-23 23:32:29]

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

https://codereview.chromium.org/2263693003/diff/60001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:200: events_[i].is_palm =
(touch_major == major_max_);
Are we confident all MT drivers have the touch major axis (including USB
devices)? Otherwise this is probably (0 == 0) and will drop everything.

https://codereview.chromium.org/2263693003/diff/60001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:218: palm_filter_->Reset();
Why should losing sync cancel suppression? Dropped events seems orthogonal to
stylus use.

https://codereview.chromium.org/2263693003/diff/60001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:471: if (!palm_filter_ ||
palm_filter_->FilterTouch(params))
I'm nervous about putting filtering this late because it's fairly hard to see
that it's not filtering out cancels, lifts of slots, or generally any events
which are necessary to keep downstream code in sync.

The decision about what type of event to send to get the client into the desired
state happens a bit earlier, in GetEventTypeForTouch(). We should change that
code to decide "cancel" for any filtering we need, including  suppression of all
contacts while a stylus is in range.

I think you just need to do something like:

   bool is_noise = touch_noise_finder_ &&
touch_noise_finder_->SlotHasNoise(touch.slot);
   bool is_palm = touch.is_palm || (palm_filter_ &&
palm_filter_->FilterTouch(touch));

   if (is_noise || is_palm) {
      // Cancel this touch.
      if (touch.touching && !touch.was_touching)
        return ET_UNKNOWN;
      return ET_TOUCH_CANCELLED;
    }

[denniskempin, 2016-08-25 23:20:08]

Description was changed from

==========
Add PalmSuppressionFilter

The PalmSuppressionFilter will suppress touchscreen events when an
on-screen pen is in use.
The filter is shared between the internal touchscreen and pen device.

BUG=636458
==========

to

==========
Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458
==========

[denniskempin, 2016-08-25 23:27:46]

A lot has changed. Instead of using the PalmSuppressionFilter, we now just give
the stylus a callback to enable/disable palm suppression.

That callback will enable/disable the internal touchscreen. To make sure that
properly handles the suppression cases I adjusted the behavior while disabled a
little:

Instead of lifting all fingers and re-syncing on enabled, the code will now
cancel all fingers instead and continue to sync the internal state. This will
get rid of the ioctls to resync after re-enabling the device. 
It also allows us to make sure that touches that were once cancelled, will stay
cancelled and not magically show up after re-enabling. 

PTAL Michael!

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

https://codereview.chromium.org/2263693003/diff/60001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:200: events_[i].is_palm =
(touch_major == major_max_);
On 2016/08/23 23:32:29, spang wrote:
> Are we confident all MT drivers have the touch major axis (including USB
> devices)? Otherwise this is probably (0 == 0) and will drop everything.
Of course we shouldn't rely on this. I should have thought of that, thanks for
catching this!

https://codereview.chromium.org/2263693003/diff/60001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:218: palm_filter_->Reset();
On 2016/08/23 23:32:29, spang wrote:
> Why should losing sync cancel suppression? Dropped events seems orthogonal to
> stylus use.

Done.

[spang, 2016-08-26 00:58:34]

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:352:
converter->SetPalmSuppressionCallback(base::Callback<void(bool)>());
This call isn't conditional on settings, can you do it just once when the device
is plugged in?

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:369:
internal_pen->SetPalmSuppressionCallback(
Also this one. Unless it uses settings, it doesn't really need to run every time
settings change.

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:516:
converter->SetEnabled(!enabled);
The way to disable a device is to change the implementation of IsDeviceEnabled()
to check your condition and return "false". The policy is handled centrally, to
avoid multiple features fighting over ownership of the "enabled" status, causing
funny behavior.

If you do call SetEnabled() here directly, then as soon as settings change, or a
device is plugged or unplugged, it'll get enabled again. But you want it to stay
disabled while the stylus is active.

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

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:444: if
(touch_noise_finder_ && touch_noise_finder_->SlotHasNoise(touch.slot)) {
Can we move this earlier and just set |will_cancel| ?

Also, for consistency with |touching| and |was_touching|, let's rename
|cancelled| to |was_cancelled| and |will_cancel| to |cancelled|

I think this function then becomes something like:

if (touch.was_cancelled)
  return ET_UNKNOWN;

if (touch_noise_finder_ && touch_noise_finder_->SlotHasNoise(touch.slot)) {
  touch.cancelled = true;
}

if (touch.cancelled)
  return touch.was_touching ? ET_TOUCH_CANCELLED : ET_UNKNOWN;
if (touch.touching)
  return touch.was_touching ? ET_TOUCH_MOVED : ET_TOUCH_PRESSED;
return touch.was_touching ? ET_TOUCH_RELEASED : ET_UNKNOWN;

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:511: event->cancelled =
true;
Can we move this down next to the assignment to |was_touching|. Also I think we
can drop the conditional:

event->was_touching = event->touching;
event->cancelled = event->will_cancel;

[denniskempin, 2016-08-26 21:56:02]

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:352:
converter->SetPalmSuppressionCallback(base::Callback<void(bool)>());
On 2016/08/26 00:58:34, spang wrote:
> This call isn't conditional on settings, can you do it just once when the
device
> is plugged in?

Done.

I was a little paranoid about the theoretical possibility that we might have two
internal pen devices.
In which case they would both be using the same callback and potentially cause
issues. So I did this here where we can iterate over all converter and pick just
one pen.
Doing this right during init is much simpler though, and having multiple
internal touchscreens and pen devices would probably cause a whole bunch of
other issues anyway.

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:369:
internal_pen->SetPalmSuppressionCallback(
On 2016/08/26 00:58:34, spang wrote:
> Also this one. Unless it uses settings, it doesn't really need to run every
time
> settings change.

Done.

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/i...
ui/events/ozone/evdev/input_device_factory_evdev.cc:516:
converter->SetEnabled(!enabled);
On 2016/08/26 00:58:34, spang wrote:
> The way to disable a device is to change the implementation of
IsDeviceEnabled()
> to check your condition and return "false". The policy is handled centrally,
to
> avoid multiple features fighting over ownership of the "enabled" status,
causing
> funny behavior.
> 
> If you do call SetEnabled() here directly, then as soon as settings change, or
a
> device is plugged or unplugged, it'll get enabled again. But you want it to
stay
> disabled while the stylus is active.

Done.

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

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:444: if
(touch_noise_finder_ && touch_noise_finder_->SlotHasNoise(touch.slot)) {
On 2016/08/26 00:58:34, spang wrote:
> Can we move this earlier and just set |will_cancel| ?
Done. I have moved it to ReportTouches, since this method cannot modify touch,
which is a good distinction we should keep I think.

https://codereview.chromium.org/2263693003/diff/80001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:511: event->cancelled =
true;
On 2016/08/26 00:58:34, spang wrote:
> Can we move this down next to the assignment to |was_touching|. Also I think
we
> can drop the conditional:
> 
> event->was_touching = event->touching;
> event->cancelled = event->will_cancel;

done

[spang, 2016-08-26 22:20:03]

lgtm

https://codereview.chromium.org/2263693003/diff/120001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/2263693003/diff/120001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/input_device_factory_evdev.cc:243: converter->HasPen())
Please add braces when the body spans more than one line (even if it is one
statement).

[denniskempin, 2016-08-26 23:02:08]

all done. Go go gadget CQ

https://codereview.chromium.org/2263693003/diff/120001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/input_device_factory_evdev.cc (right):

https://codereview.chromium.org/2263693003/diff/120001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/input_device_factory_evdev.cc:243: converter->HasPen())
On 2016/08/26 22:20:03, spang wrote:
> Please add braces when the body spans more than one line (even if it is one
> statement).

Done.

[denniskempin, 2016-08-26 23:02:19]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2016-08-26 23:02:19]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/2263693003/#ps160001
(title: "fixed unit tests")

[commit-bot: I haz the power, 2016-08-26 23:03:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 23:46:55]

Description was changed from

==========
Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458
==========

to

==========
Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458
==========

[commit-bot: I haz the power, 2016-08-26 23:47:02]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-26 23:49:12]

Description was changed from

==========
Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458
==========

to

==========
Add palm suppression feature to EventConverterEvdev

The palm suppression is enabled by an internal stylus enabled
TouchEventConverterEvdev via a callback to InputDeviceFactoryEvdev.
When in palm suppression mode, all internal touchscreen devices will
be disabled.
Before a touchscreen would lift all touches when disabled, and
re-initialize when re-enabled. To prevent the unnecessary ioctls the
code has been updated to force cancel all touches while disabled, but
continue to read and sync the internal touch state. This will also
make sure that touches that went down while disabled, will not
suddenly show up after enabling and continue to stay cancelled until
lifted.

BUG=636458

Committed: https://crrev.com/0cf5d6752db81e256e5e705bc64c4bb04dc1e34a
Cr-Commit-Position: refs/heads/master@{#414860}
==========

[commit-bot: I haz the power, 2016-08-26 23:49:13]

Patchset 9 (id:??) landed as
https://crrev.com/0cf5d6752db81e256e5e705bc64c4bb04dc1e34a
Cr-Commit-Position: refs/heads/master@{#414860}