Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12087140:
Suppress touchscreen tap immediately after a GestureFlingCancel (Closed)
|
Created:
7 years, 10 months ago by mohsen Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Yusuf Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSuppress touchscreen tap immediately after a GestureFlingCancel
Tapping on the touchscreen during a fling scroll causes a
GestureFlingCancel to be sent to stop the fling. It also sends a pair of
GestureTapDown/GestureTap events that should be suppressed if the fling
is successfully stopped (otherwise we will have undesirable side
effects; e.g. activating a link or push button in tap location). This
was working for touchpad. It is now added for touchscreen, too.
BUG=162242
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190017
Patch Set 1 #
Total comments: 35
Patch Set 2 : Applied some reviews #Patch Set 3 : Updated after TSC refactoring #
Total comments: 13
Patch Set 4 : Limited touchscreen TSC to Aura and Windows #Patch Set 5 : Allowed GFC Ack in NOTHING state + Resolved a link error on win #
Total comments: 2
Messages
Total messages: 28 (0 generated)
I have added a separate tap suppression controller for the touchscreen. It has almost the same state machine that touchpad-version has. Just a tap-cancel event is added. Maybe we can factor the state machine core into a base class? I have not added unit tests for touchscreen-version of tap suppression controller to this CL yet, to have this CL a bit lighter. I can add them after reviewing rest of the CL? Please take a look. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:143: const WebGestureEvent& gesture_event) { I have extracted GFC filtering into this method to be able to put tap suppression between this filter and tap deferral. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:273: TouchpadTapSuppressionController* GestureEventFilter::GetTouchpadTapSuppressionController() { Does this line need to be broken? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:61: Following two public methods are added to be called from TouchscreenTapSuppressionController. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:132: touchpad_tap_suppression_controller_; Should this line be broken like this? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/tap_suppression_controller.cc (left): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/tap_suppression_controller.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file is renamed to touchpad_tap_suppression_controller.cc. Other than renaming the class, nothing else is changed. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/tap_suppression_controller.h (left): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/tap_suppression_controller.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file is renamed to touchpad_tap_suppression_controller.h. Its class is also renamed to TouchpadTapSuppressionController. Nothing else is changed. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/tap_suppression_controller_aura.cc (left): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/tap_suppression_controller_aura.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file is renamed to touchpad_tap_suppression_controller_aura.cc. Other than the class name, nothing else is changed.
https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:143: const WebGestureEvent& gesture_event) { On 2013/02/04 15:09:05, mohsen wrote: > I have extracted GFC filtering into this method to be able to put tap > suppression between this filter and tap deferral. sgtm https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:148: bool GestureEventFilter::ShouldForwardForTapSuppression( say what it does? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:161: return !touchscreen_tap_suppression_controller_-> I am sad that ShouldForwardForTapSuppression inverts the general convention. We have methods that are "should forward" and "should defer". This is somewhat icky. Add a TODO to get around to fixing this. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:177: // TODO(rjkroege): separate touchpad and touchscreen events. yay! you are fixing this in this CL. Take the TODO away. :-) https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:178: bool GestureEventFilter::ShouldForwardForTapDeferral( conceptually, there is a pipeline of filters. we should make this clearer. A comment explaining this would be an excellent start. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:262: if (last_gfc_source_device_ == WebGestureEvent::Touchscreen) I suspect that this might not be robust unless the GFC filtering has ensured that there is only one GFC in flight. it would be good to test for this. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:273: TouchpadTapSuppressionController* GestureEventFilter::GetTouchpadTapSuppressionController() { On 2013/02/04 15:09:05, mohsen wrote: > Does this line need to be broken? yes https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:111: // Enqueus the event in the coalesced events queue and if it is in front of nit sp Enqueues https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:132: touchpad_tap_suppression_controller_; On 2013/02/04 15:09:05, mohsen wrote: > Should this line be broken like this? general algorithm is if len(line) > 80, split at space and indent 4 spaces. So this looks good to me. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:134: // An object tracking the state of touchscreen action on the delivery of of a touchscreen https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:140: // Whenever a GestureFlingCancel is forwarded, its source device is saved, so Do we still queue the events until acked? If so, we have this state in the GFC in the queue. Could this piece of info be stored there? My preference (not for you to implement in this CL) would be to add more state to the ACK. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/tap_suppression_controller_aura.cc (left): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/tap_suppression_controller_aura.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/04 15:09:05, mohsen wrote: > This file is renamed to touchpad_tap_suppression_controller_aura.cc. Other than > the class name, nothing else is changed. You used git mv to change its name yes? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/touchscreen_tap_suppression_controller.h (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/touchscreen_tap_suppression_controller.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_TOUCHSCREEN_TAP_SUPPRESSION_CONTROLLER_H_ there is a lot of code here in-common with touchpad_tap_suppression. I think it would be a good idea to factor out this duplication yes? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/touchscreen_tap_suppression_controller.h:71: // TODO(rjkroege): During debugging, the event times did not prove reliable. You're adding this code. You get to take the TODO over. :-) https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. you need this on windows and aura. So it is misnamed. And the gyp(i) will need adjustment. And I reiterate my comment about common code... https://codereview.chromium.org/12087140/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12087140/diff/1/content/content_browser.gypi#... content/content_browser.gypi:1128: ['exclude', '^browser/renderer_host/touchscreen_tap_suppression_controller.cc'], note that it's needed on win so you might want to adjust?
I have applied reviews. The main thing remaining is the common code in touchpad and touchscreen versions of TSC. As I mentioned in my first message, they have almost identical state machines underneath. My idea is to create a base class handling the core state machine, and derive two TSCs thereof. Any suggestions? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:148: bool GestureEventFilter::ShouldForwardForTapSuppression( On 2013/02/05 16:00:07, rjkroege wrote: > say what it does? There is a comment in .h file above this method. It runs the sub-filter for suppressing taps immediately after a GestureFlingCancel. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:161: return !touchscreen_tap_suppression_controller_-> On 2013/02/05 16:00:07, rjkroege wrote: > I am sad that ShouldForwardForTapSuppression inverts the general convention. > > We have methods that are "should forward" and "should defer". This is somewhat > icky. Add a TODO to get around to fixing this. What convention does it invert? There were some ShouldForwardForX methods for each sub-filter X. I have added this method in the same style. BTW, I agree that these ShouldForward/ShouldDiscard methods that negate each others return value are confusing. A TODO is added in the .h file. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:177: // TODO(rjkroege): separate touchpad and touchscreen events. On 2013/02/05 16:00:07, rjkroege wrote: > yay! you are fixing this in this CL. Take the TODO away. :-) Done. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:178: bool GestureEventFilter::ShouldForwardForTapDeferral( On 2013/02/05 16:00:07, rjkroege wrote: > conceptually, there is a pipeline of filters. we should make this clearer. A > comment explaining this would be an excellent start. This is somewhat clear in the body of ShouldForward() method. It calls ShouldForwardFor... sub-filters one by one. There is also a comment above that function saying that. We can add a TODO to do the refactoring? https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:262: if (last_gfc_source_device_ == WebGestureEvent::Touchscreen) On 2013/02/05 16:00:07, rjkroege wrote: > I suspect that this might not be robust unless the GFC filtering has ensured > that there is only one GFC in flight. it would be good to test for this. Since GEF fowards events one by one, i.e. waits for first event's ack and then sends the next event, we are always waiting for ack of the event in front of the queue. So, there should be no problem? (Also, note that I have replaced last_gfc_source_device_ with coalesced_gesture_events_.front().sourceDevice) https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:273: TouchpadTapSuppressionController* GestureEventFilter::GetTouchpadTapSuppressionController() { On 2013/02/05 16:00:07, rjkroege wrote: > On 2013/02/04 15:09:05, mohsen wrote: > > Does this line need to be broken? > > yes Done. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:111: // Enqueus the event in the coalesced events queue and if it is in front of On 2013/02/05 16:00:07, rjkroege wrote: > nit sp Enqueues Done. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:134: // An object tracking the state of touchscreen action on the delivery of On 2013/02/05 16:00:07, rjkroege wrote: > of a touchscreen Sentence changed a bit. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.h:140: // Whenever a GestureFlingCancel is forwarded, its source device is saved, so On 2013/02/05 16:00:07, rjkroege wrote: > Do we still queue the events until acked? If so, we have this state in the GFC > in the queue. Could this piece of info be stored there? > > My preference (not for you to implement in this CL) would be to add more state > to the ACK. Yes, you are right. The event is in front of queue, waiting for the ACK. I removed this field and used the event in front of queue, instead. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/tap_suppression_controller_aura.cc (left): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/tap_suppression_controller_aura.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/05 16:00:07, rjkroege wrote: > On 2013/02/04 15:09:05, mohsen wrote: > > This file is renamed to touchpad_tap_suppression_controller_aura.cc. Other > than > > the class name, nothing else is changed. > > You used git mv to change its name yes? No. I just renamed (also changed contents) and then used git add/rm to add the new file/remove the old file. AFAIK, git mv does the same. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/touchscreen_tap_suppression_controller.h (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/touchscreen_tap_suppression_controller.h:71: // TODO(rjkroege): During debugging, the event times did not prove reliable. On 2013/02/05 16:00:07, rjkroege wrote: > You're adding this code. You get to take the TODO over. :-) Yep, Sorry :-) Done. https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... File content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc (right): https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/02/05 16:00:07, rjkroege wrote: > you need this on windows and aura. So it is misnamed. And the gyp(i) will need > adjustment. > > And I reiterate my comment about common code... Isn't it the case for touchpad TSC, too? Don't we need touchpad_tsc in other platforms?
On 2013/02/06 16:13:06, mohsen wrote: > I have applied reviews. The main thing remaining is the common code in touchpad > and touchscreen versions of TSC. As I mentioned in my first message, they have > almost identical state machines underneath. My idea is to create a base class > handling the core state machine, and derive two TSCs thereof. Any suggestions? yes. do the refactor first in a different CL. Ideally the refactor ought not to require unit test adjustments. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/gesture_event_filter.cc (right): > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:148: bool > GestureEventFilter::ShouldForwardForTapSuppression( > On 2013/02/05 16:00:07, rjkroege wrote: > > say what it does? > > There is a comment in .h file above this method. It runs the sub-filter for > suppressing taps immediately after a GestureFlingCancel. ok > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:161: return > !touchscreen_tap_suppression_controller_-> > On 2013/02/05 16:00:07, rjkroege wrote: > > I am sad that ShouldForwardForTapSuppression inverts the general convention. > > > > We have methods that are "should forward" and "should defer". This is somewhat > > icky. Add a TODO to get around to fixing this. > > What convention does it invert? There were some ShouldForwardForX methods for > each sub-filter X. I have added this method in the same style. BTW, I agree that > these ShouldForward/ShouldDiscard methods that negate each others return value > are confusing. A TODO is added in the .h file. good. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:177: // TODO(rjkroege): > separate touchpad and touchscreen events. > On 2013/02/05 16:00:07, rjkroege wrote: > > yay! you are fixing this in this CL. Take the TODO away. :-) > > Done. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:178: bool > GestureEventFilter::ShouldForwardForTapDeferral( > On 2013/02/05 16:00:07, rjkroege wrote: > > conceptually, there is a pipeline of filters. we should make this clearer. A > > comment explaining this would be an excellent start. > > This is somewhat clear in the body of ShouldForward() method. It calls > ShouldForwardFor... sub-filters one by one. There is also a comment above that > function saying that. We can add a TODO to do the refactoring? sure. but write more now too. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:262: if > (last_gfc_source_device_ == WebGestureEvent::Touchscreen) > On 2013/02/05 16:00:07, rjkroege wrote: > > I suspect that this might not be robust unless the GFC filtering has ensured > > that there is only one GFC in flight. it would be good to test for this. > > Since GEF fowards events one by one, i.e. waits for first event's ack and then > sends the next event, we are always waiting for ack of the event in front of the > queue. So, there should be no problem? It is desirable to change the behaviour that you describe. > (Also, note that I have replaced last_gfc_source_device_ with > coalesced_gesture_events_.front().sourceDevice) > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.cc:273: > TouchpadTapSuppressionController* > GestureEventFilter::GetTouchpadTapSuppressionController() { > On 2013/02/05 16:00:07, rjkroege wrote: > > On 2013/02/04 15:09:05, mohsen wrote: > > > Does this line need to be broken? > > > > yes > > Done. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/gesture_event_filter.h (right): > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.h:111: // Enqueus the event > in the coalesced events queue and if it is in front of > On 2013/02/05 16:00:07, rjkroege wrote: > > nit sp Enqueues > > Done. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.h:134: // An object tracking > the state of touchscreen action on the delivery of > On 2013/02/05 16:00:07, rjkroege wrote: > > of a touchscreen > > Sentence changed a bit. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/gesture_event_filter.h:140: // Whenever a > GestureFlingCancel is forwarded, its source device is saved, so > On 2013/02/05 16:00:07, rjkroege wrote: > > Do we still queue the events until acked? If so, we have this state in the GFC > > in the queue. Could this piece of info be stored there? > > > > My preference (not for you to implement in this CL) would be to add more state > > to the ACK. > > Yes, you are right. The event is in front of queue, waiting for the ACK. I > removed this field and used the event in front of queue, instead. NB: acks can arrive out of order between wheels and gesture events. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/tap_suppression_controller_aura.cc (left): > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/tap_suppression_controller_aura.cc:1: // Copyright > (c) 2012 The Chromium Authors. All rights reserved. > On 2013/02/05 16:00:07, rjkroege wrote: > > On 2013/02/04 15:09:05, mohsen wrote: > > > This file is renamed to touchpad_tap_suppression_controller_aura.cc. Other > > than > > > the class name, nothing else is changed. > > > > You used git mv to change its name yes? > > No. I just renamed (also changed contents) and then used git add/rm to add the > new file/remove the old file. AFAIK, git mv does the same. ok > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > File content/browser/renderer_host/touchscreen_tap_suppression_controller.h > (right): > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/touchscreen_tap_suppression_controller.h:71: // > TODO(rjkroege): During debugging, the event times did not prove reliable. > On 2013/02/05 16:00:07, rjkroege wrote: > > You're adding this code. You get to take the TODO over. :-) > > Yep, Sorry :-) Done. > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > File > content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc > (right): > > https://codereview.chromium.org/12087140/diff/1/content/browser/renderer_host... > content/browser/renderer_host/touchscreen_tap_suppression_controller_aura.cc:1: > // Copyright (c) 2013 The Chromium Authors. All rights reserved. > On 2013/02/05 16:00:07, rjkroege wrote: > > you need this on windows and aura. So it is misnamed. And the gyp(i) will need > > adjustment. > > > > And I reiterate my comment about common code... > > Isn't it the case for touchpad TSC, too? Don't we need touchpad_tsc in other > platforms? ask girard/gideon about windows touchpad support. my understanding is that windows doesn't do touchpad events with precise deltas. But perhaps it should. so: it is possible that windows/aura shouldn't be having the TSC for touchpad. and it is definitely necessary that mac/aura shouldn't have the TSC.
This is the re-implementation of touchscreen tap suppression after TSC refactoring. Currently, GestureEventFilter and TouchscreenTapSuppressionController are compiled in all platforms. Limiting GestureEventFilter to desired platforms has some subtleties that need more thought and makes the CL complicated. Please review the CL in its current form and I will later update the CL or upload another CL to deal with limiting GEF and TSC to desired platforms, or maybe we can find a better solution! https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:178: bool GestureEventFilter::ShouldForwardForTapDeferral( ShouldForwardForTapDeferral() is divided into 3 serial filters: 1. ShouldForwardForGFCFiltering(), 2. ShouldForwardForTapDeferral() and 3. ShouldForwardForCoalescing(). 1 and 2 are separated to add tap suppression between them. 2 and 3 are separate to add more readability and also ease reuse of 3 alone in other parts of code. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:354: void GestureEventFilter::MergeOrInsertScrollAndPinchEvent( Changes in this function are just as cleanup. No functionality is changed. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/touchscreen_tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/touchscreen_tap_suppression_controller.cc:15: #if !defined(USE_AURA) && !defined(OS_WIN) These parameters should normally be read from GestureConfiguration. But, GestureConfiguraiton is only available on Aura and Windows. For other platforms we use these constant values, until we limit touchscreen TSC to these platforms or find a better way to solve this problem.
Please CC yusufo on this change. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:132: ShouldForwardForBounceReduction(gesture_event) && afaik: touchpad events shouldn't be debounced. are they? https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:262: if (type == WebInputEvent::GestureFlingCancel) { this is not how I wanted to see the code evolve. I would wanted to see touch pad handling removed from this and the handling and touchpad stuff using the touchpad_tap_suppression_controller and not pass through the gesture event filter. Going forward, it will be easier to maintain this code if touchpad and touchscreen handling code can be written about and reasoned in isolation. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_unittest.cc:1879: host_->TouchpadTapSuppressionControllerState()); a helper method on MockRenderWidgetHost might have made this a smaller change.
CC'd yusufo@ for the changes in gesture_event_filter.h/.cc.
https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:132: ShouldForwardForBounceReduction(gesture_event) && On 2013/03/07 22:56:21, rjkroege wrote: > afaik: touchpad events shouldn't be debounced. are they? I don't think so. Only when we are scrolling using GestureScrollUpdates, events are debounced. I think touchpad scrolls use MouseWheel events, so no debouncing should happen? https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:262: if (type == WebInputEvent::GestureFlingCancel) { On 2013/03/07 22:56:21, rjkroege wrote: > this is not how I wanted to see the code evolve. I would wanted to see touch pad > handling removed from this and the handling and touchpad stuff using the > touchpad_tap_suppression_controller and not pass through the gesture event > filter. > > Going forward, it will be easier to maintain this code if touchpad and > touchscreen handling code can be written about and reasoned in isolation. I can move touchpad TSC out of GEF back to RenderWidgetHostImpl. But, I think it is better to be done in another CL. Maybe it is better to add a TODO and do it after this CL lands? https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_unittest.cc:1879: host_->TouchpadTapSuppressionControllerState()); On 2013/03/07 22:56:21, rjkroege wrote: > a helper method on MockRenderWidgetHost might have made this a smaller change. These unit tests are going to be changed soon (because of the refactoring in TSC). I'll consider this point then.
On 2013/03/08 00:14:58, mohsen wrote: > https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... > File content/browser/renderer_host/gesture_event_filter.cc (right): > > https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... > content/browser/renderer_host/gesture_event_filter.cc:132: > ShouldForwardForBounceReduction(gesture_event) && > On 2013/03/07 22:56:21, rjkroege wrote: > > afaik: touchpad events shouldn't be debounced. are they? > > I don't think so. Only when we are scrolling using GestureScrollUpdates, events > are debounced. I think touchpad scrolls use MouseWheel events, so no debouncing > should happen? OK. This code is getting too complicated to remember all of the interactions. :-( > > https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... > content/browser/renderer_host/gesture_event_filter.cc:262: if (type == > WebInputEvent::GestureFlingCancel) { > On 2013/03/07 22:56:21, rjkroege wrote: > > this is not how I wanted to see the code evolve. I would wanted to see touch > pad > > handling removed from this and the handling and touchpad stuff using the > > touchpad_tap_suppression_controller and not pass through the gesture event > > filter. > > > > Going forward, it will be easier to maintain this code if touchpad and > > touchscreen handling code can be written about and reasoned in isolation. > > I can move touchpad TSC out of GEF back to RenderWidgetHostImpl. But, I think it > is better to be done in another CL. Maybe it is better to add a TODO and do it > after this CL lands? OK. Please note that (in future work) I want the GEF to (largely) fold into the GestureRecognizer once we have an "end of fling" event available. > > https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... > File content/browser/renderer_host/render_widget_host_unittest.cc (right): > > https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... > content/browser/renderer_host/render_widget_host_unittest.cc:1879: > host_->TouchpadTapSuppressionControllerState()); > On 2013/03/07 22:56:21, rjkroege wrote: > > a helper method on MockRenderWidgetHost might have made this a smaller change. > > These unit tests are going to be changed soon (because of the refactoring in > TSC). I'll consider this point then. OK.
https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:132: ShouldForwardForBounceReduction(gesture_event) && On 2013/03/08 00:14:58, mohsen wrote: > On 2013/03/07 22:56:21, rjkroege wrote: > > afaik: touchpad events shouldn't be debounced. are they? > > I don't think so. Only when we are scrolling using GestureScrollUpdates, events > are debounced. I think touchpad scrolls use MouseWheel events, so no debouncing > should happen? yes. you're right. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.h:40: // sent together. nice comment updating. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/touchscreen_tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/touchscreen_tap_suppression_controller.cc:15: #if !defined(USE_AURA) && !defined(OS_WIN) On 2013/03/07 22:31:41, mohsen wrote: > These parameters should normally be read from GestureConfiguration. But, > GestureConfiguraiton is only available on Aura and Windows. For other platforms > we use these constant values, until we limit touchscreen TSC to these platforms > or find a better way to solve this problem. Find a better way. Like not including this code in the build except on those platforms.
https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.cc:132: ShouldForwardForBounceReduction(gesture_event) && On 2013/03/08 00:14:58, mohsen wrote: > On 2013/03/07 22:56:21, rjkroege wrote: > > afaik: touchpad events shouldn't be debounced. are they? > > I don't think so. Only when we are scrolling using GestureScrollUpdates, events > are debounced. I think touchpad scrolls use MouseWheel events, so no debouncing > should happen? yes. you're right. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/gesture_event_filter.h:40: // sent together. nice comment updating. https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/touchscreen_tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/touchscreen_tap_suppression_controller.cc:15: #if !defined(USE_AURA) && !defined(OS_WIN) On 2013/03/07 22:31:41, mohsen wrote: > These parameters should normally be read from GestureConfiguration. But, > GestureConfiguraiton is only available on Aura and Windows. For other platforms > we use these constant values, until we limit touchscreen TSC to these platforms > or find a better way to solve this problem. Find a better way. Like not including this code in the build except on those platforms.
I have: 1. limited touchscreen tap suppression controller to Aura and Windows platforms, and used a stub implementation for other platforms. 2. added a TODO (in gesture_event_filter.h) to move touchpad tap suppression controller out of gesture event filter. 3. rebased the CL after recent changes in unit tests. Please take another look... https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... File content/browser/renderer_host/touchscreen_tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/38001/content/browser/renderer_... content/browser/renderer_host/touchscreen_tap_suppression_controller.cc:15: #if !defined(USE_AURA) && !defined(OS_WIN) On 2013/03/14 19:48:06, rjkroege wrote: > On 2013/03/07 22:31:41, mohsen wrote: > > These parameters should normally be read from GestureConfiguration. But, > > GestureConfiguraiton is only available on Aura and Windows. For other > platforms > > we use these constant values, until we limit touchscreen TSC to these > platforms > > or find a better way to solve this problem. > > Find a better way. Like not including this code in the build except on those > platforms. > Done. This code is now only included on Aura and Windows platforms.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/48001
Presubmit check for 12087140-48001 failed and returned exit status 1.
INFO:root:Found 7 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/content_browser.gypi
Presubmit checks took 1.5s to calculate.
jam@ for OWNERS in content/content_browser.gypi.
On 2013/03/19 22:42:38, mohsen wrote: > jam@ for OWNERS in content/content_browser.gypi. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
rjkroege@: I have changed the tap suppression state machine a bit to pass some browser unit tests. I have added a (review) comment there. Please take a look. https://codereview.chromium.org/12087140/diff/73001/content/browser/renderer_... File content/browser/renderer_host/tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/73001/content/browser/renderer_... content/browser/renderer_host/tap_suppression_controller.cc:36: case NOTHING: Here, I am allowing GFC Ack to arrive when we are in NOTHING state. That's because the Ack for a GFC can arrive after we receive the corresponding tap-down and tap-up, in which case we will be back in NOTHING state.
(still) lgtm. https://codereview.chromium.org/12087140/diff/73001/content/browser/renderer_... File content/browser/renderer_host/tap_suppression_controller.cc (right): https://codereview.chromium.org/12087140/diff/73001/content/browser/renderer_... content/browser/renderer_host/tap_suppression_controller.cc:36: case NOTHING: On 2013/03/21 14:49:32, mohsen wrote: > Here, I am allowing GFC Ack to arrive when we are in NOTHING state. That's > because the Ack for a GFC can arrive after we receive the corresponding tap-down > and tap-up, in which case we will be back in NOTHING state. I'm find with this but per the discussion about an overcomplicated code in events, this is just one more datum demonstrating the need for some serious refactoring here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/12087140/73001
Message was sent while issue was closed.
Change committed as 190017 |
