Make pinch zoom work on chromeos by setting touch id correctly

Make pinch zoom work on chromeos by setting touch id correctly


In EventFactoryEvdev, the touch id is reset to default value 0 after setting a PointerDetails object,
which causes two touch pointer have the same id, and pinch zoom does not work.

We need to set the touch id in PointerDetails and check in all the set_pointer_details
to make sure that id is not reset to default.


BUG=695905, 696369
Review-Url: https://codereview.chromium.org/2720133002
Cr-Commit-Position: refs/heads/master@{#454378}
Committed: https://chromium.googlesource.com/chromium/src/+/de104fb57e491bef3f2883f5a9100b9524fb0e3c

[lanwei, 2017-02-27 22:04:54]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 22:05:50]

Dry run: 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, 2017-02-27 22:42:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 22:42:27]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-02-27 22:44:12]

Description was changed from

==========
pinch zoom on chromeos

BUG=695905
==========

to

==========
Make pinch zoom work on chromeos

In EventFactoryEvdev, the touch id is reset to default value 0 after setting a
PointerDetails object,
so we need to set the touch id in PointerDetails.

BUG=695905,696369
==========

[lanwei, 2017-02-27 22:46:32]

lanwei@chromium.org changed reviewers:
+ sadrul@chromium.org

[lanwei, 2017-02-27 22:56:43]

Sadrul, having id in both TouchEvent and PointerDetailas constructors seems to
be a potential problem. Should we remove it from TouchEvent?

[lanwei, 2017-02-27 22:57:08]

lanwei@chromium.org changed reviewers:
+ dtapuska@chromium.org

[spang, 2017-02-27 22:58:54]

On 2017/02/27 22:56:43, lanwei wrote:
> Sadrul, having id in both TouchEvent and PointerDetailas constructors seems to
> be a potential problem. Should we remove it from TouchEvent?
 
yes, please.. there are several other call sites that are calling the ctor and
then set_pointer_details(). We shouldn't have setters on both levels. Please at
least audit all the calls to set_pointer_details() to make sure they are not
overwriting the id.

[sadrul, 2017-02-28 16:48:21]

On 2017/02/27 22:58:54, spang wrote:
> On 2017/02/27 22:56:43, lanwei wrote:
> > Sadrul, having id in both TouchEvent and PointerDetailas constructors seems
to
> > be a potential problem. Should we remove it from TouchEvent?
>  
> yes, please.. there are several other call sites that are calling the ctor and
> then set_pointer_details(). We shouldn't have setters on both levels. Please
at
> least audit all the calls to set_pointer_details() to make sure they are not
> overwriting the id.

+1

[lanwei, 2017-02-28 20:17:02]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 20:17:33]

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

[lanwei, 2017-02-28 20:18:09]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[lanwei, 2017-02-28 20:18:19]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-28 20:18:57]

Dry run: 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, 2017-02-28 21:02:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 21:02:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lanwei, 2017-02-28 21:51:55]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 21:52:45]

Dry run: 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, 2017-02-28 22:25:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 22:25:03]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[lanwei, 2017-02-28 23:30:16]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 23:31:25]

Dry run: 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, 2017-03-01 00:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:30:48]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-01 15:35:59]

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:766: if (details.pointer_type ==
EventPointerType::POINTER_TYPE_TOUCH) {
For MouseEvent, when do we set pointer_type to TYPE_TOUCH?

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:990: pointer_details.id != PointerEvent::kMousePointerId);
When do we create a TouchEvent when pointer_type is not TYPE_TOUCH?

[lanwei, 2017-03-01 18:26:13]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 18:26:30]

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

[lanwei, 2017-03-01 18:27:40]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-01 19:35:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 19:35:50]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-03-01 20:12:22]

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:766: if (details.pointer_type ==
EventPointerType::POINTER_TYPE_TOUCH) {
On 2017/03/01 15:35:59, sadrul wrote:
> For MouseEvent, when do we set pointer_type to TYPE_TOUCH?

So far, I have not seen. I am afraid it may be set from a touch type in the
future.

https://codereview.chromium.org/2720133002/diff/60001/ui/events/event.cc#newc...
ui/events/event.cc:990: pointer_details.id != PointerEvent::kMousePointerId);
On 2017/03/01 15:35:59, sadrul wrote:
> When do we create a TouchEvent when pointer_type is not TYPE_TOUCH?

On Kevin (chromebook) device, the pointer type is pen, but it is sent as a touch
event.

[wjmaclean, 2017-03-01 22:38:47]

wjmaclean@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[wjmaclean, 2017-03-01 22:38:47]

A drive-by nit: It's not at all apparent how the title of this issue ("Make
pinch zoom work on chromeos") is related to the rest of the issue description.
It would be nice to edit one or both to provide a little more context so the
reader doesn't have to look up and read the bug description to understand the
relationship between the two.

[dtapuska, 2017-03-02 02:42:53]

On 2017/03/01 22:38:47, wjmaclean wrote:
> A drive-by nit: It's not at all apparent how the title of this issue ("Make
> pinch zoom work on chromeos") is related to the rest of the issue description.
> It would be nice to edit one or both to provide a little more context so the
> reader doesn't have to look up and read the bug description to understand the
> relationship between the two.

sadrul/lanwei@ we really need to get this fixed ASAP. What is blocking this
review? This is a P1 issue.

[dtapuska, 2017-03-02 02:44:49]

On 2017/03/02 02:42:53, dtapuska wrote:
> On 2017/03/01 22:38:47, wjmaclean wrote:
> > A drive-by nit: It's not at all apparent how the title of this issue ("Make
> > pinch zoom work on chromeos") is related to the rest of the issue
description.
> > It would be nice to edit one or both to provide a little more context so the
> > reader doesn't have to look up and read the bug description to understand
the
> > relationship between the two.
> 
> sadrul/lanwei@ we really need to get this fixed ASAP. What is blocking this
> review? This is a P1 issue.

lgtm

[lanwei, 2017-03-02 16:48:16]

Description was changed from

==========
Make pinch zoom work on chromeos

In EventFactoryEvdev, the touch id is reset to default value 0 after setting a
PointerDetails object,
so we need to set the touch id in PointerDetails.

BUG=695905,696369
==========

to

==========
Make pinch zoom work on chromeos by setting touch id correctly


In EventFactoryEvdev, the touch id is reset to default value 0 after setting a
PointerDetails object,
which causes two touch pointer have the same id, and pinch zoom does not work.

We need to set the touch id in PointerDetails and check in all the
set_pointer_details
to make sure that id is not reset to default.


BUG=695905,696369
==========

[lanwei, 2017-03-02 17:22:13]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 17:22:28]

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

[lanwei, 2017-03-02 17:37:24]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-02 17:55:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2017-03-02 17:58:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 17:58:42]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-02 18:42:44]

lgtm

https://codereview.chromium.org/2720133002/diff/80001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2720133002/diff/80001/ui/events/event.cc#newc...
ui/events/event.cc:986: DCHECK(pointer_details_.id == 0 || pointer_details.id !=
0);
DCHECK_EQ(pointer_details_.id, pointer_details.id);

I think for now, you can get rid of the rest of the dchecks

[lanwei, 2017-03-02 19:13:17]

The CQ bit was checked by lanwei@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 19:13:54]

Dry run: 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, 2017-03-02 21:01:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 21:01:44]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-03-02 21:16:32]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2017-03-02 21:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2720133002/#ps120001
(title: "pinch zoom")

[commit-bot: I haz the power, 2017-03-02 21:17:13]

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, 2017-03-02 21:22:53]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488489392128680,
"parent_rev": "74e768d1f68d8c5335abc412a898d66368c71cda", "commit_rev":
"de104fb57e491bef3f2883f5a9100b9524fb0e3c"}

[commit-bot: I haz the power, 2017-03-02 21:23:33]

Description was changed from

==========
Make pinch zoom work on chromeos by setting touch id correctly


In EventFactoryEvdev, the touch id is reset to default value 0 after setting a
PointerDetails object,
which causes two touch pointer have the same id, and pinch zoom does not work.

We need to set the touch id in PointerDetails and check in all the
set_pointer_details
to make sure that id is not reset to default.


BUG=695905,696369
==========

to

==========
Make pinch zoom work on chromeos by setting touch id correctly


In EventFactoryEvdev, the touch id is reset to default value 0 after setting a
PointerDetails object,
which causes two touch pointer have the same id, and pinch zoom does not work.

We need to set the touch id in PointerDetails and check in all the
set_pointer_details
to make sure that id is not reset to default.


BUG=695905,696369

Review-Url: https://codereview.chromium.org/2720133002
Cr-Commit-Position: refs/heads/master@{#454378}
Committed:
https://chromium.googlesource.com/chromium/src/+/de104fb57e491bef3f2883f5a910...
==========

[commit-bot: I haz the power, 2017-03-02 21:23:34]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/de104fb57e491bef3f2883f5a910...