Do not increase the pointer id when type is eraser

Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be
considered as a pen type.

BUG=685252
Review-Url: https://codereview.chromium.org/2867093003
Cr-Commit-Position: refs/heads/master@{#471391}
Committed: https://chromium.googlesource.com/chromium/src/+/0474c2a5ba9d963f73bc2cdecc6d16e575a4a4be

[lanwei, 2017-05-08 19:26:07]

Description was changed from

==========
eraser id

BUG=
==========

to

==========
Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be considered
as a pen type.

BUG=685252
==========

[lanwei, 2017-05-08 19:26:18]

Description was changed from

==========
Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be considered
as a pen type.

BUG=685252
==========

to

==========
Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be
considered as a pen type.

BUG=685252
==========

[lanwei, 2017-05-08 19:26:43]

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

[lanwei, 2017-05-08 19:26:50]

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

[commit-bot: I haz the power, 2017-05-08 19:28:20]

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-05-08 20:42:58]

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

[commit-bot: I haz the power, 2017-05-08 20:42:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lanwei, 2017-05-09 02:10:26]

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

[commit-bot: I haz the power, 2017-05-09 02:11: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-05-09 03:51:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-09 04:57:37]

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

[commit-bot: I haz the power, 2017-05-09 04:57:38]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-05-09 13:24:45]

https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:487: if
(p.GetPointerType() == WebPointerProperties::PointerType::kEraser) {
Sorry this seems wrong to me. In that if we get an kEraser pointer type first
and then release it then you will end up incrementing the pointer ID. In that
the eraser pointer id != pen pointer id.

Can we just treat the creation of IncomingId if the type is kEraser as kPen?

[Navid Zolghadr, 2017-05-09 15:00:38]

https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:193: const
IncomingId incoming_id(pointer_type, pointer_properties.id);
How about this here?
const IncomingId incoming_id(pointer_type == kEraser ? kPen : pointer_type,
pointer_properties.id)

[lanwei, 2017-05-09 15:02:04]

On 2017/05/09 13:24:45, dtapuska wrote:
>
https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> 
>
https://codereview.chromium.org/2867093003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/events/PointerEventFactory.cpp:487: if
> (p.GetPointerType() == WebPointerProperties::PointerType::kEraser) {
> Sorry this seems wrong to me. In that if we get an kEraser pointer type first
> and then release it then you will end up incrementing the pointer ID. In that
> the eraser pointer id != pen pointer id.
> 
> Can we just treat the creation of IncomingId if the type is kEraser as kPen?

Even if we treat the type of kEraser same as kPen, if its pointer id is
different than the pen pointer id, we will increase the pointer ID, and since
there are more than one pens, the eraser will be treated as a non-primary pen. I
think it is better to threat it as a eraser in this case. What do you think?

[lanwei, 2017-05-09 17:00:06]

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

[commit-bot: I haz the power, 2017-05-09 17:01:05]

Dry run: CQ is trying da patch.

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

[dtapuska, 2017-05-09 20:53:08]

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:203: if
(pointer_type == WebPointerProperties::PointerType::kEraser)
Can this be rolled into the other pointer_type == kEraser check... So they are
together? (we'd need to adjust the buttons != 0 conditional moving it inside.

[Navid Zolghadr, 2017-05-09 20:58:46]

lgtm % nits.

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:26: case
WebPointerProperties::PointerType::kEraser:
nit: With the way you changed the caller of this function we can now remove this
kEraser case to hit the DCHECK at the end of this function.

[commit-bot: I haz the power, 2017-05-09 23:07:50]

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

[commit-bot: I haz the power, 2017-05-09 23:07:51]

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

[lanwei, 2017-05-10 15:09:26]

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

[lanwei, 2017-05-10 15:09:37]

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

[commit-bot: I haz the power, 2017-05-10 15:10:06]

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-05-10 15:34:53]

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

[commit-bot: I haz the power, 2017-05-10 15:34:54]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lanwei, 2017-05-10 15:46:56]

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

[lanwei, 2017-05-10 15:47:22]

Patchset #3 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-10 15:48:00]

Dry run: CQ is trying da patch.

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

[lanwei, 2017-05-10 17:50:42]

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:26: case
WebPointerProperties::PointerType::kEraser:
On 2017/05/09 20:58:46, Navid Zolghadr wrote:
> nit: With the way you changed the caller of this function we can now remove
this
> kEraser case to hit the DCHECK at the end of this function.

Done.

https://codereview.chromium.org/2867093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:203: if
(pointer_type == WebPointerProperties::PointerType::kEraser)
On 2017/05/09 20:53:08, dtapuska wrote:
> Can this be rolled into the other pointer_type == kEraser check... So they are
> together? (we'd need to adjust the buttons != 0 conditional moving it inside.

Done.

[commit-bot: I haz the power, 2017-05-10 17:56:27]

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

[commit-bot: I haz the power, 2017-05-10 17:56:28]

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

[lanwei, 2017-05-11 18:15:00]

lanwei@chromium.org changed reviewers:
- rbyers@chromium.org

[lanwei, 2017-05-11 18:15:28]

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

[WRONG-USE-chromium, 2017-05-11 18:30:59]

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

[WRONG-USE-chromium, 2017-05-11 18:30:59]

lgtm

[dtapuska, 2017-05-11 19:06:55]

On 2017/05/11 18:30:59, WRONG-USE-chromium wrote:
> lgtm

lgtm

[Rick Byers, 2017-05-11 19:07:22]

On 2017/05/11 18:30:59, WRONG-USE-chromium wrote:
> lgtm

Darn multi-login never working correctly :-(

LGTM

[lanwei, 2017-05-11 19:19:06]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2017-05-11 19:19:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
nzolghadr@chromium.org
Link to the patchset: https://codereview.chromium.org/2867093003/#ps80001
(title: "eraser id")

[commit-bot: I haz the power, 2017-05-11 19:20:42]

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-05-11 21:46:04]

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

[commit-bot: I haz the power, 2017-05-11 21:46:06]

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

[lanwei, 2017-05-12 16:51:28]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2017-05-12 16:52:14]

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-05-12 18:52:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494607888882540,
"parent_rev": "1720aab9a99b13135c33bb593beefa7cb21b603d", "commit_rev":
"0474c2a5ba9d963f73bc2cdecc6d16e575a4a4be"}

[commit-bot: I haz the power, 2017-05-12 18:52:23]

Description was changed from

==========
Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be
considered as a pen type.

BUG=685252
==========

to

==========
Do not increase the pointer id when type is eraser

We are setting the pointer type to eraser when using an eraser button. We
should not increase the pointer id in this case, because it should be
considered as a pen type.

BUG=685252

Review-Url: https://codereview.chromium.org/2867093003
Cr-Commit-Position: refs/heads/master@{#471391}
Committed:
https://chromium.googlesource.com/chromium/src/+/0474c2a5ba9d963f73bc2cdecc6d...
==========

[commit-bot: I haz the power, 2017-05-12 18:52:24]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0474c2a5ba9d963f73bc2cdecc6d...