Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

Issue 788923002: Touch Events - changedTouches list includes non-changed touch points on Android (Closed)

Created:
6 years ago by USE s.singapati at gmail.com
Modified:
5 years, 10 months ago
Reviewers:
jdduke (slow)
CC:
chromium-reviews, darin-cc_chromium.org, jdduke+watch_chromium.org, jam, Zeeshan Qureshi
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Ensure touchmove changedTouches list only includes moving touches On Android, WebTouchPoint.state of all touches is set to "StateMoved" thus included in changedTouches when any touch moves. This patch compares current touch points position to previous position in TouchMove event and updates state. This comparision is done after all filtering and potential coalescing in TouchEventQueue. BUG=416236 Committed: https://crrev.com/750a80d9cce15010794afcd80f514e67421e2c01 Cr-Commit-Position: refs/heads/master@{#314315}

Patch Set 1 #

Total comments: 6

Patch Set 2 : stores touchevent without latency info and other review comment fixes #

Total comments: 6

Patch Set 3 : Added unit test and fixed review comments #

Total comments: 13

Patch Set 4 : Added CHECK for last sent event and minor fixes. #

Total comments: 4

Patch Set 5 : Relaxed touches state condition for TouchMove events for now #

Total comments: 4

Patch Set 6 : minor changes in comments ONLY. #

Total comments: 2

Patch Set 7 : Fixed trybots errors. #

Total comments: 3

Patch Set 8 : minor change to comments #

Total comments: 2

Patch Set 9 : comparing all properties of touch points to update the state. #

Total comments: 6

Patch Set 10 : Added unit test for checking Pointer states when other than position changed. #

Total comments: 10

Patch Set 11 : Removed new methods in synthetic_web_input_event_builders #

Patch Set 12 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+218 lines, -9 lines) Patch
M content/browser/renderer_host/input/touch_event_queue.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -2 lines 0 comments Download
M content/browser/renderer_host/input/touch_event_queue.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +47 lines, -5 lines 0 comments Download
M content/browser/renderer_host/input/touch_event_queue_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +151 lines, -1 line 0 comments Download
M content/common/input/synthetic_web_input_event_builders.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M content/common/input/touch_event_stream_validator.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M content/common/input/touch_event_stream_validator_unittest.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 55 (11 generated)
USE s.singapati at gmail.com
6 years ago (2014-12-09 16:43:34 UTC) #2
jdduke (slow)
https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_host/input/touch_event_queue.cc#newcode707 content/browser/renderer_host/input/touch_event_queue.cc:707: TouchEventWithLatencyInfo& touch) { This logic can probably be folded ...
6 years ago (2014-12-09 20:30:26 UTC) #3
Rick Byers
Moving Zeeshan and myself from reviewers to CC - Jared is the right reviewer here. ...
6 years ago (2014-12-09 21:19:18 UTC) #6
USE s.singapati at gmail.com
Thank you for comments. My answers below. On 2014/12/09 20:30:26, jdduke wrote: > https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_host/input/touch_event_queue.cc#newcode707 > ...
6 years ago (2014-12-12 12:12:27 UTC) #7
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_host/input/touch_event_queue.cc#newcode707 content/browser/renderer_host/input/touch_event_queue.cc:707: TouchEventWithLatencyInfo& touch) { On 2014/12/09 20:30:25, jdduke wrote: > ...
6 years ago (2014-12-13 07:46:05 UTC) #9
jdduke (slow)
This needs a test (or two) in touch_event_queue_unittest.cc. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer_host/input/touch_event_queue.cc#newcode706 content/browser/renderer_host/input/touch_event_queue.cc:706: TouchEventWithLatencyInfo& ...
6 years ago (2014-12-15 16:51:08 UTC) #10
USE s.singapati at gmail.com
Added a unit test. Any need for more testing? Also, Logic has been modified a ...
6 years ago (2014-12-17 11:28:43 UTC) #11
jdduke (slow)
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue.cc#newcode720 content/browser/renderer_host/input/touch_event_queue.cc:720: for (unsigned int i = 0; i < last_sent_touchevent_->touchesLength; ...
5 years, 11 months ago (2015-01-06 17:57:29 UTC) #12
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue_unittest.cc File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue_unittest.cc#newcode2254 content/browser/renderer_host/input/touch_event_queue_unittest.cc:2254: MoveTouchPoints(2, 3, 3, 3, 4, 4); On 2015/01/06 17:57:29, ...
5 years, 11 months ago (2015-01-07 16:08:40 UTC) #13
jdduke (slow)
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue_unittest.cc File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer_host/input/touch_event_queue_unittest.cc#newcode2254 content/browser/renderer_host/input/touch_event_queue_unittest.cc:2254: MoveTouchPoints(2, 3, 3, 3, 4, 4); On 2015/01/07 16:08:39, ...
5 years, 11 months ago (2015-01-07 16:14:31 UTC) #14
USE s.singapati at gmail.com
In current (baseline) behavior, It is possible that TouchMove event is dispatched to targets even ...
5 years, 11 months ago (2015-01-09 12:28:53 UTC) #15
jdduke (slow)
On 2015/01/09 12:28:53, s.singapati wrote: > In current (baseline) behavior, It is possible that TouchMove ...
5 years, 11 months ago (2015-01-09 17:33:05 UTC) #16
USE s.singapati at gmail.com
Touches state condition is relaxed for TouchMove in TouchEventStreamValidator for now. But this should be ...
5 years, 11 months ago (2015-01-12 19:30:26 UTC) #17
jdduke (slow)
lgtm, thanks! https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer_host/input/touch_event_queue.h File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer_host/input/touch_event_queue.h#newcode171 content/browser/renderer_host/input/touch_event_queue.h:171: // Dispatch |touch| to the client. Before ...
5 years, 11 months ago (2015-01-14 16:04:33 UTC) #18
jdduke (slow)
rbyers@: Anything else you want to see here?
5 years, 11 months ago (2015-01-14 16:07:10 UTC) #19
Rick Byers
On 2015/01/14 16:07:10, jdduke wrote: > rbyers@: Anything else you want to see here? Nope, ...
5 years, 11 months ago (2015-01-15 02:45:35 UTC) #20
USE s.singapati at gmail.com
Thanks for reviews. I think this is ready to go now. https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer_host/input/touch_event_queue.h File content/browser/renderer_host/input/touch_event_queue.h (right): ...
5 years, 11 months ago (2015-01-16 14:14:28 UTC) #21
USE s.singapati at gmail.com
@jdduke : Could you please confirm the commit approval for new patch set?
5 years, 11 months ago (2015-01-20 14:32:49 UTC) #22
jdduke (slow)
On 2015/01/20 14:32:49, s.singapati wrote: > @jdduke : Could you please confirm the commit approval ...
5 years, 11 months ago (2015-01-20 16:30:37 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/100001
5 years, 11 months ago (2015-01-20 17:34:45 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/112440) Try jobs failed on following ...
5 years, 11 months ago (2015-01-20 18:12:27 UTC) #27
jdduke (slow)
https://codereview.chromium.org/788923002/diff/100001/content/browser/renderer_host/input/touch_event_queue_unittest.cc File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/100001/content/browser/renderer_host/input/touch_event_queue_unittest.cc#newcode2283 content/browser/renderer_host/input/touch_event_queue_unittest.cc:2283: MoveTouchPoints(0, 1.1, 1, 1, 2, 20.001); Looks like you ...
5 years, 11 months ago (2015-01-20 18:18:09 UTC) #28
USE s.singapati at gmail.com
Needed to have a workaround in TouchEventStreamValidator - InvalidPointStates unit test to fix failing try ...
5 years, 11 months ago (2015-01-26 10:26:08 UTC) #29
USE s.singapati at gmail.com
@jdduke: Could you please check the latest patch set? Can we proceed with commit process? ...
5 years, 11 months ago (2015-01-26 15:55:33 UTC) #30
jdduke (slow)
https://codereview.chromium.org/788923002/diff/120001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/120001/content/browser/renderer_host/input/touch_event_queue.cc#newcode720 content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) So, now I'm wondering if ...
5 years, 11 months ago (2015-01-26 16:18:14 UTC) #31
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/120001/content/common/input/touch_event_stream_validator_unittest.cc File content/common/input/touch_event_stream_validator_unittest.cc (right): https://codereview.chromium.org/788923002/diff/120001/content/common/input/touch_event_stream_validator_unittest.cc#newcode163 content/common/input/touch_event_stream_validator_unittest.cc:163: // TODO(): Remove this StateStationary workaround check for On ...
5 years, 11 months ago (2015-01-27 14:00:04 UTC) #32
jdduke (slow)
https://codereview.chromium.org/788923002/diff/140001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/140001/content/browser/renderer_host/input/touch_event_queue.cc#newcode720 content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) Yeah, for correctness, I think ...
5 years, 11 months ago (2015-01-27 17:29:54 UTC) #33
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/140001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/140001/content/browser/renderer_host/input/touch_event_queue.cc#newcode720 content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) On 2015/01/27 17:29:54, jdduke wrote: ...
5 years, 10 months ago (2015-01-28 16:09:52 UTC) #34
jdduke (slow)
Thanks, just a couple things. https://codereview.chromium.org/788923002/diff/160001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/renderer_host/input/touch_event_queue.cc#newcode737 content/browser/renderer_host/input/touch_event_queue.cc:737: const WebTouchPoint& current_point) const ...
5 years, 10 months ago (2015-01-28 16:18:39 UTC) #35
USE s.singapati at gmail.com
On 2015/01/28 16:18:39, jdduke wrote: > Thanks, just a couple things. > > https://codereview.chromium.org/788923002/diff/160001/content/browser/renderer_host/input/touch_event_queue.cc > ...
5 years, 10 months ago (2015-01-28 16:58:02 UTC) #36
USE s.singapati at gmail.com
In current patch I added one method for changing both RotationAngle and force. May be ...
5 years, 10 months ago (2015-01-29 16:38:20 UTC) #37
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/160001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/renderer_host/input/touch_event_queue.cc#newcode737 content/browser/renderer_host/input/touch_event_queue.cc:737: const WebTouchPoint& current_point) const { On 2015/01/28 16:18:38, jdduke ...
5 years, 10 months ago (2015-01-29 16:40:31 UTC) #38
jdduke (slow)
Almost there, thanks! https://codereview.chromium.org/788923002/diff/180001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/renderer_host/input/touch_event_queue.cc#newcode67 content/browser/renderer_host/input/touch_event_queue.cc:67: last_point.screenPosition != current_point.screenPosition) { Nit: Let's ...
5 years, 10 months ago (2015-01-29 16:53:19 UTC) #39
USE s.singapati at gmail.com
https://codereview.chromium.org/788923002/diff/180001/content/browser/renderer_host/input/touch_event_queue.cc File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/renderer_host/input/touch_event_queue.cc#newcode67 content/browser/renderer_host/input/touch_event_queue.cc:67: last_point.screenPosition != current_point.screenPosition) { On 2015/01/29 16:53:19, jdduke wrote: ...
5 years, 10 months ago (2015-01-30 12:19:28 UTC) #40
USE s.singapati at gmail.com
@jdduke: Any comments on latest patch set?
5 years, 10 months ago (2015-02-02 14:36:11 UTC) #41
jdduke (slow)
lgtm, thanks for your patience.
5 years, 10 months ago (2015-02-02 16:05:59 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/200001
5 years, 10 months ago (2015-02-02 16:14:05 UTC) #44
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/54458) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng/builds/6400) ios_rel_device_ninja_ng ...
5 years, 10 months ago (2015-02-02 16:16:48 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/200001
5 years, 10 months ago (2015-02-02 19:12:09 UTC) #48
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/54508) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng/builds/6450) ios_rel_device_ninja_ng ...
5 years, 10 months ago (2015-02-02 19:15:37 UTC) #50
jdduke (slow)
On 2015/02/02 19:15:37, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
5 years, 10 months ago (2015-02-02 19:17:42 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/220001
5 years, 10 months ago (2015-02-03 11:22:01 UTC) #53
commit-bot: I haz the power
Committed patchset #12 (id:220001)
5 years, 10 months ago (2015-02-03 12:18:53 UTC) #54
commit-bot: I haz the power
5 years, 10 months ago (2015-02-03 12:19:58 UTC) #55
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/750a80d9cce15010794afcd80f514e67421e2c01
Cr-Commit-Position: refs/heads/master@{#314315}

Powered by Google App Engine
This is Rietveld 408576698