Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816613003:
Add suppresion of slop region touches in browser (Closed)
|
Created:
3 years, 8 months ago by Navid Zolghadr Modified:
3 years, 8 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd suppresion of slop region touches in browser
This change tentaively adds back the touch slop
region suppression in browser. This will cause
the touch pointerevents stop firing while the
touch point is moving within the slop region and
thus break a few tests.
But we would like to see whether it affects
our TouchToFirstScrollUpdateSwapBegin metric
or not.
BUG=704935
Review-Url: https://codereview.chromium.org/2816613003
Cr-Original-Commit-Position: refs/heads/master@{#465252}
Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5a5580cf22263
Review-Url: https://codereview.chromium.org/2816613003
Cr-Commit-Position: refs/heads/master@{#465629}
Committed: https://chromium.googlesource.com/chromium/src/+/77ac8485a0d6c1699a04d514caae94e0903864fc
Patch Set 1 #Patch Set 2 : Fix the tests #
Total comments: 8
Patch Set 3 : Fix the comment #Patch Set 4 : Add test failures to fix the revert reason #Patch Set 5 : Rebase #
Messages
Total messages: 43 (32 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, tdresser@chromium.org
Just wanted to point out that this will cause the regression of not firing touch pointerevents while the touch point moves within the slop region. In case any bugs were reported while this change gives us data in Canary and Dev.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:51: class TouchMoveSlopSuppressor { Should this move to another file? Or the anonymous namespace? https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.h (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:119: // Suppression of TouchMove's within a slop region when a sequence has not yet Suppression of -> Suppresses https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:121: std::unique_ptr<TouchMoveSlopSuppressor> touchmove_slop_suppressor_; Why use a unique_ptr here, instead of just stack allocating?
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
ptal https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:51: class TouchMoveSlopSuppressor { On 2017/04/13 18:13:35, tdresser wrote: > Should this move to another file? Or the anonymous namespace? This wasn't quite possible. At least I couldn't get it to compile. Note that I need to reference this class in the header file. I don't think creating a separate file is necessary for this particularly because I'd like to restrict the use of this class to this file and we will soon revert this change anyway. WDYT? https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.h (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:119: // Suppression of TouchMove's within a slop region when a sequence has not yet On 2017/04/13 18:13:35, tdresser wrote: > Suppression of -> Suppresses Done. https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:121: std::unique_ptr<TouchMoveSlopSuppressor> touchmove_slop_suppressor_; On 2017/04/13 18:13:35, tdresser wrote: > Why use a unique_ptr here, instead of just stack allocating? If I don't use pointers here I need to put the full definition of that class in the header file. I wanted to keep it private to this class and thus I defined the class in the cpp file and kept a pointer here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:51: class TouchMoveSlopSuppressor { On 2017/04/18 14:47:41, Navid Zolghadr wrote: > On 2017/04/13 18:13:35, tdresser wrote: > > Should this move to another file? Or the anonymous namespace? > > This wasn't quite possible. At least I couldn't get it to compile. Note that I > need to reference this class in the header file. > I don't think creating a separate file is necessary for this particularly > because I'd like to restrict the use of this class to this file and we will soon > revert this change anyway. WDYT? Acknowledged. https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.h (right): https://codereview.chromium.org/2816613003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:121: std::unique_ptr<TouchMoveSlopSuppressor> touchmove_slop_suppressor_; On 2017/04/18 14:47:41, Navid Zolghadr wrote: > On 2017/04/13 18:13:35, tdresser wrote: > > Why use a unique_ptr here, instead of just stack allocating? > > If I don't use pointers here I need to put the full definition of that class in > the header file. I wanted to keep it private to this class and thus I defined > the class in the cpp file and kept a pointer here. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492530705623070,
"parent_rev": "1c8299f990b1b6fd0c3dde87bd036708ee42ef44", "commit_rev":
"b8126037096ad1da5aa13af72af5a5580cf22263"}
Message was sent while issue was closed.
Description was changed from ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 ========== to ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2825523004/ by ojan@chromium.org. The reason for reverting is: Caused layout test failures. https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fa... https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fa....
Message was sent while issue was closed.
Description was changed from ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ========== to ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ==========
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ========== to ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region and thus break a few tests. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ==========
The CQ bit was unchecked by nzolghadr@chromium.org
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2816613003/#ps60001 (title: "Add test failures to fix the revert reason")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dtapuska@chromium.org
On 2017/04/18 20:54:10, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... you'll need to rebase.. CQ just landed a conflicting change... https://codereview.chromium.org/2826673005/
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2816613003/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492618498128040,
"parent_rev": "281d38cd426b801aa0a1323f579d3c5a0f340d85", "commit_rev":
"77ac8485a0d6c1699a04d514caae94e0903864fc"}
Message was sent while issue was closed.
Description was changed from ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region and thus break a few tests. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... ========== to ========== Add suppresion of slop region touches in browser This change tentaively adds back the touch slop region suppression in browser. This will cause the touch pointerevents stop firing while the touch point is moving within the slop region and thus break a few tests. But we would like to see whether it affects our TouchToFirstScrollUpdateSwapBegin metric or not. BUG=704935 Review-Url: https://codereview.chromium.org/2816613003 Cr-Original-Commit-Position: refs/heads/master@{#465252} Committed: https://chromium.googlesource.com/chromium/src/+/b8126037096ad1da5aa13af72af5... Review-Url: https://codereview.chromium.org/2816613003 Cr-Commit-Position: refs/heads/master@{#465629} Committed: https://chromium.googlesource.com/chromium/src/+/77ac8485a0d6c1699a04d514caae... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/77ac8485a0d6c1699a04d514caae... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
