ensure scrolling_touch_action_ has value when used |
||||||
Issue descriptionWe could end up having no value in the case when all the gesture event are queued up and flushed after touch sequence end (finger lifted up).
,
Jun 15 2018
I investigated a bit about the non-iframe timeout case, and that should be fine. This is because in the PassthroughTouchEventQueue::FlushQueue, we the ack state to INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS and that will set touch action to Auto.
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40d56da5038844308b4d3921ae8aa0900d592650 commit 40d56da5038844308b4d3921ae8aa0900d592650 Author: Xida Chen <xidachen@chromium.org> Date: Wed Jul 25 13:31:26 2018 Handle iframe touch ack time out When there is a touch ack time out, all the gesture events will be flushed to the touch action filter. This case is already taken care of in the non-iframe case, and this CL handles the iframe case. When we see that a gesture event is flushed due to time out, we set the effective touch action to auto which will allow all the gesture event. A browser test is added to ensure the correctness. This CL is break into few patches: PS#22: The essential fix in line 1190 in the render_widget_host_input_event_router.cc is commented out. When running the newly added browser test, it will hit the DCHECK in touch_action_filter.cc at line 55. PS#23: Uncomment the essential fix in the render_widget_host_input_event_router.cc and remove the DCHECK in touch_action_filter.cc at line 55. Uncomment the temporary fix in touch_action_filter.cc for bug 851664. TBR=creis@chromium.org Bug: 851644 Change-Id: Ic381dd17fa531c1e9f7d649b48dc9f0f0d7d9c21 Reviewed-on: https://chromium-review.googlesource.com/1102729 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#577858} [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/input/touch_action_filter_unittest.cc [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_host_input_event_router.h [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_targeter.cc [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/renderer_host/render_widget_targeter.h [modify] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/browser/site_per_process_hit_test_browsertest.cc [add] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/test/data/frame_tree/page_with_janky_frame.html [add] https://crrev.com/40d56da5038844308b4d3921ae8aa0900d592650/content/test/data/page_with_touch_start_janking_main_thread.html
,
Jul 25
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a48983eff56446dba26a010834780667362eaafd commit a48983eff56446dba26a010834780667362eaafd Author: Xida Chen <xidachen@chromium.org> Date: Wed Jul 25 20:03:27 2018 Revert "Handle iframe touch ack time out" This reverts commit 40d56da5038844308b4d3921ae8aa0900d592650. Reason for revert: <INSERT REASONING HERE> The test seems to be flaky, it failed sometimes: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/47384 https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/47378 Original change's description: > Handle iframe touch ack time out > > When there is a touch ack time out, all the gesture events will be flushed > to the touch action filter. This case is already taken care of in the > non-iframe case, and this CL handles the iframe case. > > When we see that a gesture event is flushed due to time out, we set the > effective touch action to auto which will allow all the gesture event. > A browser test is added to ensure the correctness. > > This CL is break into few patches: > PS#22: The essential fix in line 1190 in the > render_widget_host_input_event_router.cc is commented out. When running the > newly added browser test, it will hit the DCHECK in touch_action_filter.cc > at line 55. > > PS#23: Uncomment the essential fix in the > render_widget_host_input_event_router.cc and remove the DCHECK in > touch_action_filter.cc at line 55. Uncomment the temporary fix in > touch_action_filter.cc for bug 851664. > > TBR=creis@chromium.org > > Bug: 851644 > Change-Id: Ic381dd17fa531c1e9f7d649b48dc9f0f0d7d9c21 > Reviewed-on: https://chromium-review.googlesource.com/1102729 > Commit-Queue: Xida Chen <xidachen@chromium.org> > Reviewed-by: Timothy Dresser <tdresser@chromium.org> > Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> > Cr-Commit-Position: refs/heads/master@{#577858} TBR=tdresser@chromium.org,xidachen@chromium.org,nzolghadr@chromium.org Change-Id: I266946883f56f95fa2fdf0ee56d9b678221a7d60 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 851644 Reviewed-on: https://chromium-review.googlesource.com/1150583 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#578028} [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/input/touch_action_filter_unittest.cc [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_host_input_event_router.h [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_targeter.cc [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/renderer_host/render_widget_targeter.h [modify] https://crrev.com/a48983eff56446dba26a010834780667362eaafd/content/browser/site_per_process_hit_test_browsertest.cc [delete] https://crrev.com/a2c9d0a944b7aab058845d061e877104702534cf/content/test/data/frame_tree/page_with_janky_frame.html [delete] https://crrev.com/a2c9d0a944b7aab058845d061e877104702534cf/content/test/data/page_with_touch_start_janking_main_thread.html
,
Jul 25
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38642faa718b019b824e178b73ae72d60d017892 commit 38642faa718b019b824e178b73ae72d60d017892 Author: Xida Chen <xidachen@chromium.org> Date: Mon Jul 30 17:11:43 2018 Reland of: Handle iframe touch ack time out The previous CL: https://chromium-review.googlesource.com/c/chromium/src/+/1102729 was reverted. The reason is that in the test we have two gesture scroll, and the second gesture starts before the first TapDown finishes. This CL solve the problem. PS#1 is exactly the same as the previous CL which was reverted, so that it is easier for review. TBR=creis@chromium.org Bug: 851644 Change-Id: Ife9bf8f9a37892380e6587b80bcc03366cc0e80d Reviewed-on: https://chromium-review.googlesource.com/1151094 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Xida Chen <xidachen@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Cr-Commit-Position: refs/heads/master@{#579074} [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/input/touch_action_filter_unittest.cc [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_host_input_event_router.h [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_targeter.cc [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/renderer_host/render_widget_targeter.h [modify] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/browser/site_per_process_hit_test_browsertest.cc [add] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/test/data/frame_tree/page_with_janky_frame.html [add] https://crrev.com/38642faa718b019b824e178b73ae72d60d017892/content/test/data/page_with_touch_start_janking_main_thread.html
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3 commit 205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3 Author: Xida Chen <xidachen@chromium.org> Date: Tue Jul 31 20:04:57 2018 Revert "Reland of: Handle iframe touch ack time out" This reverts commit 38642faa718b019b824e178b73ae72d60d017892. Reason for revert: <INSERT REASONING HERE> Crashes on Win canary Original change's description: > Reland of: Handle iframe touch ack time out > > The previous CL: > https://chromium-review.googlesource.com/c/chromium/src/+/1102729 > was reverted. The reason is that in the test we have two gesture scroll, > and the second gesture starts before the first TapDown finishes. > > This CL solve the problem. PS#1 is exactly the same as the previous CL > which was reverted, so that it is easier for review. > > TBR=creis@chromium.org > > Bug: 851644 > Change-Id: Ife9bf8f9a37892380e6587b80bcc03366cc0e80d > Reviewed-on: https://chromium-review.googlesource.com/1151094 > Commit-Queue: Xida Chen <xidachen@chromium.org> > Reviewed-by: Xida Chen <xidachen@chromium.org> > Reviewed-by: Timothy Dresser <tdresser@chromium.org> > Cr-Commit-Position: refs/heads/master@{#579074} TBR=creis@chromium.org,tdresser@chromium.org,xidachen@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 851644 Change-Id: I8d1c4753963f22e55f1b338bf2b264376b683660 Reviewed-on: https://chromium-review.googlesource.com/1157036 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#579540} [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/input/touch_action_filter_unittest.cc [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_host_input_event_router.h [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_targeter.cc [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/renderer_host/render_widget_targeter.h [modify] https://crrev.com/205abd5f65e6d6b2c98d4172b8e144cc3fc7e8b3/content/browser/site_per_process_hit_test_browsertest.cc [delete] https://crrev.com/687b3af44494680e7f4d0bacb45d09d6ec6dc084/content/test/data/frame_tree/page_with_janky_frame.html [delete] https://crrev.com/687b3af44494680e7f4d0bacb45d09d6ec6dc084/content/test/data/page_with_touch_start_janking_main_thread.html
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee78df3802eebbbea595e48e714a4348a06ee43e commit ee78df3802eebbbea595e48e714a4348a06ee43e Author: Xida Chen <xidachen@chromium.org> Date: Fri Aug 03 17:40:40 2018 Reland of: Handle iframe touch ack time out The previous CL: https://chromium-review.googlesource.com/c/chromium/src/+/1151094 was reverted. The reason is that there were some crashes. This CL solve the problem. PS#1 is exactly the same as the previous CL which was reverted, so that it is easier for review. TBR=creis@chromium.org Bug: 851644 Change-Id: I80a8c3b7dfd94e7abace4e3281b959f9fc1dcd2a Reviewed-on: https://chromium-review.googlesource.com/1157698 Reviewed-by: Xida Chen <xidachen@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#580589} [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_host_input_event_router.h [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_targeter.cc [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/renderer_host/render_widget_targeter.h [modify] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/browser/site_per_process_hit_test_browsertest.cc [add] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/test/data/frame_tree/page_with_janky_frame.html [add] https://crrev.com/ee78df3802eebbbea595e48e714a4348a06ee43e/content/test/data/page_with_touch_start_janking_main_thread.html
,
Aug 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b945abbef17031ed77ba22808289ff39a88735d commit 3b945abbef17031ed77ba22808289ff39a88735d Author: Xida Chen <xidachen@chromium.org> Date: Tue Aug 14 23:55:51 2018 Add detailed debugging in TouchActionFilter Previously we added some debugging instrumentation in TouchActionFilter, when the allowed_touch_action_ is set or reset to non-Auto. The results that we see is not enough to diagose the crash. This CL adds more detailed debugging. In particular, whenever it is set or reset, we add a string. Bug: 851644 Change-Id: I2d19055c1008659365f9c3f54fcbdb30760b2166 Reviewed-on: https://chromium-review.googlesource.com/1174696 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#583087} [modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/3b945abbef17031ed77ba22808289ff39a88735d/content/browser/renderer_host/input/touch_action_filter.h
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/097ccfa873338b4d96dcd060782b0f3aecbcb971 commit 097ccfa873338b4d96dcd060782b0f3aecbcb971 Author: Xida Chen <xidachen@chromium.org> Date: Thu Aug 16 15:18:53 2018 Add some more debugging in TouchActionFilter This is a follow up for https://chromium-review.googlesource.com/c/chromium/src/+/1174696 TBR=dtapuska@chromium.org Bug: 851644 Change-Id: I702bd9afc80a82d0b7db02cb8f06a1765c69aaaf Reviewed-on: https://chromium-review.googlesource.com/1177631 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#583655} [modify] https://crrev.com/097ccfa873338b4d96dcd060782b0f3aecbcb971/content/browser/renderer_host/input/touch_action_filter.cc
,
Aug 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/67e36d282d8a39011b62688c915b9aa79601ea24 commit 67e36d282d8a39011b62688c915b9aa79601ea24 Author: Xida Chen <xidachen@chromium.org> Date: Fri Aug 17 20:08:44 2018 Fix a crash when pinch is sent to root while targeting the iframe When there is a pinch zoom gesture, it will always be sent to the root frame even if the fingers are targeting the child frame. In this case, the TouchActionFilter::allowed_touch_action_ of the root frame may not have a value and results in a crash. This CL fixes it by force the allowed_touch_action_ of the root frame to auto in this case. A new browser test is added. TBR=creis@chromium.org Bug: 851644 Change-Id: I1c9d87a1b5fecd7140fdc0ef3bfcfc4b0ea930ea Reviewed-on: https://chromium-review.googlesource.com/1178634 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Cr-Commit-Position: refs/heads/master@{#584165} [modify] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/browser/renderer_host/render_widget_host_input_event_router.cc [modify] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/browser/site_per_process_hit_test_browsertest.cc [add] https://crrev.com/67e36d282d8a39011b62688c915b9aa79601ea24/content/test/data/div_with_touch_action_auto.html
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34 commit 7a7759f7643b3c052c87a30ce1dcc99d7a85fe34 Author: Xida Chen <xidachen@chromium.org> Date: Wed Aug 22 16:42:31 2018 Mark gesture starts on GestureTapDown Right now in TouchActionFilter, the member gesture_sequence_in_progress_ turns to true at GestureScrollBegin and false at GestureScrollEnd. When this is true, we do not reset the |scrolling_touch_action_| because being true means there are more incoming gesture events. However, when the renderer is busy, it could happen that we have GestureTapDown, and then we receive the OnHasTouchEventHandlers(true) message and reset the |scrolling_touch_action_|. In this case, processing the incoming gestures such as GSB can cause a crash because the |scrolling_touch_action_| has no value. Bug: 851644 Change-Id: Idd9de11fc59ef87d7fa8f2d9e3d951fb4db5c2c5 Reviewed-on: https://chromium-review.googlesource.com/1180560 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#585072} [modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/7a7759f7643b3c052c87a30ce1dcc99d7a85fe34/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d326b48bd894605b4e41e22f6f085af7c12274b commit 0d326b48bd894605b4e41e22f6f085af7c12274b Author: Xida Chen <xidachen@chromium.org> Date: Wed Aug 22 20:39:26 2018 Reset touch action at TouchActionFilter's constructor When we start processing a gesture event, the OnHasTouchEventHandler(false) IPC message may not yet be received which means the |scrolling_touch_action_| has no value and that results a crash. This CL fixes the problem by call ResetTouchAction() in TouchActionFilter's constructor. At that moment, the |has_touch_event_handler_| is default to false which will set both |allowed_touch_action_| and |scrolling_touch_action_| to Auto. Bug: 850238 , 851644 Change-Id: I36f3edf2b9292347f184458aa059904af1a2f597 Reviewed-on: https://chromium-review.googlesource.com/1180592 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#585234} [modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/0d326b48bd894605b4e41e22f6f085af7c12274b/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/866afbedf0b038cffb6287a8e735277b2ef00d46 commit 866afbedf0b038cffb6287a8e735277b2ef00d46 Author: Xida Chen <xidachen@chromium.org> Date: Fri Aug 24 12:16:28 2018 Remove temp fix for a crash in TouchActionFilter Previously in order to prevent a crash, we add some temporary fix in touch_action_filter.cc. With some of the recent committed changes, there should be no more crashing. This CL removes the temporary code for suppressing the crash but keeps DumpWithoutCrashing so that we can get debugging info for the crash. Bug: 851644 Change-Id: I614ae6d20a24b402141563451c51b36d52302d70 Reviewed-on: https://chromium-review.googlesource.com/1185627 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#585803} [modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/866afbedf0b038cffb6287a8e735277b2ef00d46/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 24
,
Aug 24
re-open, more clean up is needed.
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a648ed8345adc49563301bde7071ae0f16b128e0 commit a648ed8345adc49563301bde7071ae0f16b128e0 Author: Xida Chen <xidachen@chromium.org> Date: Mon Aug 27 18:47:16 2018 [Debugging] Added debugging instrumentation in TouchActionFilter Right now there's crash at TouchActionFilter when filtering a gesture scroll update generated by fling controller. This CL adds debugging instrumentation to record the gesture sequence. TBR=dtapuska@chromium.org Bug: 851644 Change-Id: Ifb9b246eab063e1df9563501b7c31d69ae45c16d Reviewed-on: https://chromium-review.googlesource.com/1191024 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#586338} [modify] https://crrev.com/a648ed8345adc49563301bde7071ae0f16b128e0/content/browser/renderer_host/input/touch_action_filter.cc
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9001acb31e5c937b6b66ffaeffeaa23157d83ea commit b9001acb31e5c937b6b66ffaeffeaa23157d83ea Author: Xida Chen <xidachen@chromium.org> Date: Mon Aug 27 18:57:50 2018 Mark gesture sequence in progress true for GestureTapCancel Right now in TouchActionFilter, we set |gesture_sequence_in_progress_| false when processing GestureTap and GestureTapCancel. This is wrong with considering the following gesture sequence: 1. A simple tap: GestureTapDown-->GestureTapUnconfirmed-->GestureTap 2. Double tap: GestureTapDown-->GestureTapUnconfirmed-->GestureTapCancel -->GestureTapDown-->GestureTapCancel-->GestureDoubleTap 3. A gesture scroll: GestureTapDown-->GestureTapCancel -->GestureScrollBegin-->GestureScrollUpdate-->GestureScrollEnd 4. A fling: GestureTapDown-->GestureTapCancel-->GestureScrollBegin -->GestureScrollUpdate-->GestureScrollEnd All these sequence indicates that the gesture_sequence_in_progress_ should be true when filtering a GestureTapCancel and false when GestureTap. This CL does that. In additional to that, this CL also mark gesture_sequence_in_progress_ false when it is GestureLongTap or GestureTwoFingerTap. Unit tests are added for both cases. TBR=dtapuska@chromium.org Bug: 851644 Change-Id: I40e70cb95a93c20b13952c571d1e1108405c6051 Reviewed-on: https://chromium-review.googlesource.com/1190423 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Sahel Sharify <sahel@chromium.org> Cr-Commit-Position: refs/heads/master@{#586351} [modify] https://crrev.com/b9001acb31e5c937b6b66ffaeffeaa23157d83ea/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/b9001acb31e5c937b6b66ffaeffeaa23157d83ea/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45630997c15b80dec5d37add5a2f685e91548327 commit 45630997c15b80dec5d37add5a2f685e91548327 Author: Xida Chen <xidachen@chromium.org> Date: Wed Aug 29 19:54:20 2018 Set touch action to Auto in GSB if there is no GestureTapDown In chrome VR, we could get GestureScrollBegin without receiving a GestureTapDown. In this case we should set touch action to Auto. As a result of this, we need to add a TapDown in many unit tests because they send GSB without TapDown. TBR=dtapuska@chromium.org Bug: 851644 Change-Id: Ifa04259b68524e96b0a4faa2f8e1d0bd9b35c81c Reviewed-on: https://chromium-review.googlesource.com/1195649 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#587259} [modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/input_router_impl_unittest.cc [modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/45630997c15b80dec5d37add5a2f685e91548327/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3811c0a0b69929b3013172732e6b2beb73cfe5c7 commit 3811c0a0b69929b3013172732e6b2beb73cfe5c7 Author: Xida Chen <xidachen@chromium.org> Date: Wed Aug 29 20:01:50 2018 Don't reset touch action when there is a active touch sequence Right now when the browser receives the OnHasTouchEventHandlers IPC message, it resets the |scrolling_touch_action_| as long as both of these are true: 1. it is not in the middle of a gesture sequence. 2. the IPC message indicates that there is a touch event handler. Consider the following case: we have received the touch start ACK from the renderer, which assgins a value to |allowed_touch_action_|, then before GestureTapDown comes, JS removes the touch event listener and re-added it. In this case, it is not in the middle of a gesture sequence beause GestureTapDown has not arrived yet, so our code resets both the |allowed_touch_action_| and the |scrolling_touch_action_|. Later on when GestureScrollBegin comes, the browser crashes. This CL fixes the issue by adding a |active_touch_in_progress_| member variable. It is true when we receive a touch sequence started, and false at touch end. When we receive the OnHasTouchEventHandler(true), we should reset the touch actions only when there is no active touch and no gesture sequence. TBR=dtapuska@chromium.org Bug: 851644 Change-Id: I269d77dff0ae0e577a55734550cc639363267e50 Reviewed-on: https://chromium-review.googlesource.com/1195146 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#587262} [modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/3811c0a0b69929b3013172732e6b2beb73cfe5c7/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Aug 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/578be39046de0eb8ad2c103b3a98dda76b5651b2 commit 578be39046de0eb8ad2c103b3a98dda76b5651b2 Author: Xida Chen <xidachen@chromium.org> Date: Wed Aug 29 20:07:04 2018 Make InputRouterImpl::AllowedTouchAction return the actual touch action Right now this function either returns the actual allowed touch action when it has value, or return Auto when it has no value. It is written this way to temporary fix a crash. Now that the crash is properly fixed, we can return the actual allowed touch action. TBR=dtapuska@chromium.org Bug: 851644 Change-Id: I6ae91f5f26c327377fbb0d0cbbfede925ac59680 Reviewed-on: https://chromium-review.googlesource.com/1189136 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Sahel Sharify <sahel@chromium.org> Cr-Commit-Position: refs/heads/master@{#587266} [modify] https://crrev.com/578be39046de0eb8ad2c103b3a98dda76b5651b2/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/578be39046de0eb8ad2c103b3a98dda76b5651b2/content/browser/site_per_process_hit_test_browsertest.cc
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ee2335137207b20e2dd3878adff3b1c8aac484c commit 0ee2335137207b20e2dd3878adff3b1c8aac484c Author: Xida Chen <xidachen@chromium.org> Date: Thu Aug 30 22:31:40 2018 Count number of active touches in TouchActionFilter Right now we have a active_touch_in_progress_ to indicate whether there is an active touch or not. It is true when we receive ACK for touch start and false at touch end. From the current crash report, we are seeing multiple consecutive ack for touch end, without any ack for touch start in between. This CL changes the active_touch_in_progress_ to num_of_active_touches_. It is increased by one when we receive ack for touch start and decrease by one at touch end. At touch end, we only reset touch action when this number is zero. In theory, touch start and associated touch end should arrive in pair. This CL does a DumpWithoutCrashing if this number is ever larger than one. Bug: 851644 Change-Id: I584f1aad56bf5bc5c08e20e3ae4ca36d5f9e3586 Reviewed-on: https://chromium-review.googlesource.com/1196696 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#587813} [modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/0ee2335137207b20e2dd3878adff3b1c8aac484c/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b commit 284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Sep 05 07:51:48 2018 Revert "Count number of active touches in TouchActionFilter" This reverts commit 0ee2335137207b20e2dd3878adff3b1c8aac484c. Reason for revert: Top crash in canary and blocks dev release per bug 880021. Original change's description: > Count number of active touches in TouchActionFilter > > Right now we have a active_touch_in_progress_ to indicate whether there > is an active touch or not. It is true when we receive ACK for touch > start and false at touch end. > > From the current crash report, we are seeing multiple consecutive ack > for touch end, without any ack for touch start in between. > > This CL changes the active_touch_in_progress_ to num_of_active_touches_. > It is increased by one when we receive ack for touch start and decrease > by one at touch end. At touch end, we only reset touch action when this > number is zero. > > In theory, touch start and associated touch end should arrive in pair. > This CL does a DumpWithoutCrashing if this number is ever larger than > one. > > Bug: 851644 > Change-Id: I584f1aad56bf5bc5c08e20e3ae4ca36d5f9e3586 > Reviewed-on: https://chromium-review.googlesource.com/1196696 > Reviewed-by: Dave Tapuska <dtapuska@chromium.org> > Commit-Queue: Xida Chen <xidachen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#587813} TBR=dtapuska@chromium.org,xidachen@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 851644 , 880021 Change-Id: I265b573cef100ff6210683e7f5ec01dfcb28ea9f Reviewed-on: https://chromium-review.googlesource.com/1205923 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#588791} [modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/284cfbd5863b68dd8b075910b2b8bca8b9b0ad2b/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/52121d9dedda38ffda872776a58cf0a705b4e1c5 commit 52121d9dedda38ffda872776a58cf0a705b4e1c5 Author: Xida Chen <xidachen@chromium.org> Date: Fri Sep 07 16:04:15 2018 Reland: Count number of active touches in TouchActionFilter The original CL was reverted because there were many DumpWithoutCrashing, and the reason is that the num_of_active_touches_ goes below 0. The problem is that in InputRouterImpl::TouchEventHandled, we increment this num only when it is ack from the main thread. In other words, we missed other cases such as when there is a passive event listerner. This CL fixes the problem. PS#1 is exactly the same as the original CL, so that it is easier for review. PS#2 fixes the problem and added some more unit tests. It also removes the temporary fix to prevent crashes. TBR=tdresser@chromium.org Bug: 851644 Change-Id: I4d668ab322c94ea2704e3e220e2afd1a44833dca Reviewed-on: https://chromium-review.googlesource.com/1209545 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#589533} [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl.h [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/input_router_impl_unittest.cc [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter.h [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/input/touch_action_filter_unittest.cc [modify] https://crrev.com/52121d9dedda38ffda872776a58cf0a705b4e1c5/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
,
Sep 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8eea72eee6acae9a39be6775ed7332e8b837e6a commit e8eea72eee6acae9a39be6775ed7332e8b837e6a Author: Xida Chen <xidachen@chromium.org> Date: Tue Sep 11 18:34:53 2018 [Debugging] Add InputEventAckState to the debug string for TouchAction Right now in InputRouterImpl::OnTouchEventAck, we append a "T" to the debug string in TouchActionFilter when the touch event is a touch sequence start and that the input event ack state is NO_CONSUMER_EXISTS. This CL make changes to append the input event ack state to the debug string. The reason is that all the current crashes seems to be the case that is not acked from the main thread, but we do not know the ack state yet. Bug: 851644 Change-Id: Ia57de7aa2c63e104008893dc8809a5eac679460d Reviewed-on: https://chromium-review.googlesource.com/1219781 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#590415} [modify] https://crrev.com/e8eea72eee6acae9a39be6775ed7332e8b837e6a/content/browser/renderer_host/input/input_router_impl.cc
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06614c433f82c637514f25f44f42ce461bb14661 commit 06614c433f82c637514f25f44f42ce461bb14661 Author: Xida Chen <xidachen@chromium.org> Date: Thu Sep 13 21:17:51 2018 Fix a crash at TouchActionFilter::FilterGestureEvent All crash reports can be put in two category: 1. The InputEventAckState can be NON_BLOCKING at InputRouterImpl::OnTouchEventAck, in this case, we should just set the allowed touch action to Auto. 2. TouchActionFilter can start filtering gesture event without an ack for a touch sequence start. In both cases, we set the allowed touch action to Auto. Bug: 851644 Change-Id: I630a5d7e8804078c6f39a9102476bb484127ae61 Reviewed-on: https://chromium-review.googlesource.com/1223247 Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#591158} [modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/input_router_impl_unittest.cc [modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/touch_action_filter.cc [modify] https://crrev.com/06614c433f82c637514f25f44f42ce461bb14661/content/browser/renderer_host/input/touch_action_filter_unittest.cc
,
Sep 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55 commit cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55 Author: Xida Chen <xidachen@chromium.org> Date: Fri Sep 14 20:24:04 2018 [Debugging] Append unique_touch_event_id to debug string in TouchActionFilter In this CL, we append the unique_touch_event_id to a debug string, at the time when we get ACK for touch sequence start and touch sequence end, this way we'd know their correspondences. Bug: 851644 Change-Id: I2ee486bb2a2348a14e74a17e32f495aceaaab374 Reviewed-on: https://chromium-review.googlesource.com/1226820 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#591454} [modify] https://crrev.com/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/cdaa2db2c8a8996880ea51a43abeaa69aa8ecf55/content/browser/renderer_host/input/touch_action_filter.cc
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f05759040106ea0aa09a35074431c35b84f4cbc commit 1f05759040106ea0aa09a35074431c35b84f4cbc Author: Xida Chen <xidachen@chromium.org> Date: Tue Sep 18 22:46:12 2018 Set touch action Auto in corner cases WidgetInputHandlerManager::DispatchEvent is the function where we run the callback that sets the touch action. There are some corner cases where the touch action is set to base::nullopt. This is fine previously when the TouchActionFilter::allowed_touch_action_ has a default value of kTouchActionAuto. However, now that we changed the |allowed_touch_action_| to Optional instead of having a default value (Auto), and rely on the fact that it will be set by the renderer when ack from the main thread, we have to set |allowed_touch_action_| to Auto for these corner cases. This CL sets it at receiver end, which is in InputRouterImpl::OnTouchEventAck. When receiving the ack for touch sequence start, if the |allowed_touch_action_| is not set, then we maybe hitting these corner cases. Bug: 851644 Change-Id: If1bb393580619eb7b7ba60ac2b01099c83c71f26 Reviewed-on: https://chromium-review.googlesource.com/1228295 Commit-Queue: Xida Chen <xidachen@chromium.org> Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> Cr-Commit-Position: refs/heads/master@{#592235} [modify] https://crrev.com/1f05759040106ea0aa09a35074431c35b84f4cbc/content/browser/renderer_host/input/input_router_impl.cc [modify] https://crrev.com/1f05759040106ea0aa09a35074431c35b84f4cbc/content/browser/renderer_host/input/input_router_impl_unittest.cc
,
Sep 20
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by xidac...@chromium.org
, Jun 13 2018