Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 11 months ago by jdduke (slow) Modified:
6 years, 10 months ago Reviewers:
Avi (use Gerrit), Sami, mkosiba (inactive), aelias_OOO_until_Jul13, Rick Byers, Ted C, tdresser CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, James Su, miu+watch_chromium.org, aelias_OOO_until_Jul13, Rick Byers, klobag.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Perform eager gesture recognition on MotionEvents
ContentViewGestureHandler conditionally forwards MotionEvent's to the Android
gesture detection pipeline, depending on the current touch stream handling
disposition. However, this approach is fragile, as the detection pipeline
expects a consistent touch stream for each sequence. Instead, forward all
MotionEvent's immediately for gesture detection, conditionally dispatching the
generated gestures depending on the touch stream ack dispositions.
BUG=295075, 334040, 327444, 240550, 234516
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248066
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : Cleanup #Patch Set 4 : Remove LongPressDetector entirely (happiness) #
Total comments: 27
Patch Set 5 : GestureEventQueue + GestureEventPacket #Patch Set 6 : More cleanup #
Total comments: 18
Patch Set 7 : Code review, fix popup zoomer and double tap UMA #Patch Set 8 : Code cleanup #
Total comments: 12
Patch Set 9 : Code review #Patch Set 10 : Compilation fixes and code cleanup #Patch Set 11 : Fix AndroidScrollIntegrationTest #Patch Set 12 : Fix clang compile #Patch Set 13 : Findbugs fix #
Total comments: 16
Patch Set 14 : Fix gesture listening types #Patch Set 15 : Fix test, code review comments #Patch Set 16 : More cleanup #Patch Set 17 : Avoid touching GestureDetector #Patch Set 18 : More cleanup #
Total comments: 32
Patch Set 19 : Remove ZoomManager #
Total comments: 4
Patch Set 20 : Code review and test cleanup #
Total comments: 10
Patch Set 21 : Remove synthetic gesture packets #Patch Set 22 : Allow scrolling after longpress #
Total comments: 2
Patch Set 23 : Findbugs update #Patch Set 24 : Fix tests #Patch Set 25 : Cleanup of interfaces #Patch Set 26 : Satisfy findbugs #Patch Set 27 : Rebase #Patch Set 28 : Undo some changes #
Total comments: 16
Patch Set 29 : Code review #Patch Set 30 : Fix webview #Patch Set 31 : Fix clang #Patch Set 32 : Test #Patch Set 33 : More testing #
Created: 6 years, 10 months ago
Messages
Total messages: 57 (0 generated)
mkosiba@: My chief worry with this change is how it might affect WebView's hooks for listening to the gesture stream. I *think* I've rewired the pipeline properly so that WebView will not regress, but I'd appreciate it if you could look at the key changes to android_webview/ and ContentViewCore. This patch is very much a rough draft, but any feedback would be great. tdresser@: Did you have any thoughts on the basic structure of TouchToGestureQueue? I'll do another cleanup pass later, but if you had any questions or concerns I'm all ears. The current Web-facing change is that a gesture sequence can be preventDefault'ed by a TouchMove within the slop region. Also, double-tap related events will not be prevented if only the first tap in the sequence is pd'ed, but I think that's less of an issue.
On 2014/01/13 22:36:58, jdduke wrote: > mkosiba@: My chief worry with this change is how it might affect WebView's hooks > for listening to the gesture stream. I *think* I've rewired the pipeline > properly so that WebView will not regress, but I'd appreciate it if you could > look at the key changes to android_webview/ and ContentViewCore. This patch is > very much a rough draft, but any feedback would be great. > > tdresser@: Did you have any thoughts on the basic structure of > TouchToGestureQueue? I'll do another cleanup pass later, but if you had any > questions or concerns I'm all ears. > > The current Web-facing change is that a gesture sequence can be > preventDefault'ed by a TouchMove within the slop region. Also, double-tap > related events will not be prevented if only the first tap in the sequence is > pd'ed, but I think that's less of an issue. One note: I have yet to modify the ContentViewGestureHandlerTest accordingly (this should mostly be code removal), and the TouchToGestureQueue tests need some more fleshing out as well.
https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:38: struct TouchToGesture { These data structures aren't very optimized. I'll likely have a fixed size array for gestures per touch, and reserve a certain size upon creation of the |TouchToGesture.sequence| array.
Awesome! https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1812: //if (!hasWindowFocus) { what's this about?
https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1812: //if (!hasWindowFocus) { On 2014/01/14 00:23:16, Rick Byers wrote: > what's this about? Ah, yeah, good catch. I disabled this during testing, but I'll need to come up with an appropriate solution for the current approach, perhaps synthesizing a cancellation event.
Mainly nits and some naming suggestions. The biggest thing I'm not sure of is whether or not it makes sense to dispatch touch events before passing them to the TTGQ, or not. I'm leaning towards dispatching touch events beforehand, so you don't need to bother keeping track of them in the TTGQ. https://codereview.chromium.org/120513005/diff/520001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/android... content/browser/android/content_view_core_impl.h:338: // TouchToGestureQueue implementation. TouchToGestureQueue -> TouchToGestureQueueClient https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.cc (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:60: bool IsTimeout(const WebGestureEvent& event) { Perhaps IsTimeout -> IsTriggeredByTimeout? https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:72: InputEventAckState Intersection(InputEventAckState ack_old, The word "Intersection" applied to an enum could be interpreted a couple different ways. Perhaps |LeastConsumed| would be clearer? Any other ideas? https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:169: void TouchToGestureQueue::ForwardGestureEvent(const WebGestureEvent& event) { I don't think this is any more readable than a bigger switch statement which explicitly tests all the event types. Having all those three line functions feels a little unnecessary. I'm fine with this approach too though. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void ForwardTouchEvent(const blink::WebTouchEvent&) = 0; I think it may be cleaner to do the touch event dispatch outside of the TouchToGestureQueue. If you dispatch the touch events before passing the touch events to the TTGQ, you don't need to store the touch events in the TTGQ at all. I guess there's also an advantage in unifying the flow of touch and gesture events, so I'm open to either solution. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class CONTENT_EXPORT TouchToGestureQueue { The names TouchToGesture, TouchToGestureSequence, and TouchToGestureSequence, are a little confusing. I think they might be a little clearer as TouchToGestures, TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't fantastic names. If we decide to stop storing touch events, this naming might get a bit simpler too. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:28: // TODO(jdduke): Refactor to take a touch event with a vector of gestures. Is it reasonable to perform this refactor before this lands? It would make interaction with the TouchToGestureQueue a lot clearer. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc:234: // An unconsumed touch's gesture should not be sent. I think these comments are incorrect? https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1365: private boolean filterGesture(int type, int x, int y) { Can we find a better name for this? It isn't clear what the return value indicates, and most of what we're doing is updating state, not filtering. I haven't thought of a name I really like, but perhaps something along the lines of |processGesture|, |handleGesture|, or |updateForGesture|? https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:186: Looks like we normally omit this space (122 occurrences without vs 23 occurrences with).
> mkosiba@: My chief worry with this change is how it might affect WebView's hooks > for listening to the gesture stream. I *think* I've rewired the pipeline > properly so that WebView will not regress, but I'd appreciate it if you could > look at the key changes to android_webview/ and ContentViewCore. This patch is > very much a rough draft, but any feedback would be great. nothing obvious stands out. I tried compiling this but couldn't so can't do a manual check. Once you have a more polished version of the patch ping me and I'll test it.
https://codereview.chromium.org/120513005/diff/520001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/android... content/browser/android/content_view_core_impl.h:338: // TouchToGestureQueue implementation. On 2014/01/14 16:25:30, tdresser wrote: > TouchToGestureQueue -> TouchToGestureQueueClient Done. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.cc (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:60: bool IsTimeout(const WebGestureEvent& event) { On 2014/01/14 16:25:30, tdresser wrote: > Perhaps IsTimeout -> IsTriggeredByTimeout? Done. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:72: InputEventAckState Intersection(InputEventAckState ack_old, On 2014/01/14 16:25:30, tdresser wrote: > The word "Intersection" applied to an enum could be interpreted a couple > different ways. Perhaps |LeastConsumed| would be clearer? Any other ideas? Hmm, I'll noodle on this. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.cc:169: void TouchToGestureQueue::ForwardGestureEvent(const WebGestureEvent& event) { On 2014/01/14 16:25:30, tdresser wrote: > I don't think this is any more readable than a bigger switch statement which > explicitly tests all the event types. Having all those three line functions > feels a little unnecessary. I'm fine with this approach too though. You're totally right, this originally started with just the tap queries, but a switch is better. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void ForwardTouchEvent(const blink::WebTouchEvent&) = 0; On 2014/01/14 16:25:30, tdresser wrote: > I think it may be cleaner to do the touch event dispatch outside of the > TouchToGestureQueue. If you dispatch the touch events before passing the touch > events to the TTGQ, you don't need to store the touch events in the TTGQ at all. > > I guess there's also an advantage in unifying the flow of touch and gesture > events, so I'm open to either solution. The main reason I did it this way was because of the |OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending the touch until we've assembled all of its gestures (as the touch ack may be synchronous). If we can all agree that PD'ed touchmove's within the touch slop region will prevent both tapping and scrolling, then I think we can go ahead with forwarding the touches immediately. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class CONTENT_EXPORT TouchToGestureQueue { On 2014/01/14 16:25:30, tdresser wrote: > The names TouchToGesture, TouchToGestureSequence, and TouchToGestureSequence, > are a little confusing. > > I think they might be a little clearer as TouchToGestures, > TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't fantastic > names. > > If we decide to stop storing touch events, this naming might get a bit simpler > too. > > Let's just call it GestureEventQueue? To match TouchEventQueue? https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:28: // TODO(jdduke): Refactor to take a touch event with a vector of gestures. On 2014/01/14 16:25:30, tdresser wrote: > Is it reasonable to perform this refactor before this lands? It would make > interaction with the TouchToGestureQueue a lot clearer. We could. With the way gestures are piped here (via callbacks), we'll need to do the gesture "collecting" somewhere, but that could be offloaded to a wrapper or even just the ContentViewCoreImpl for now, no need to clutter the interface for something the caller should be doing. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue_unittest.cc:234: // An unconsumed touch's gesture should not be sent. On 2014/01/14 16:25:30, tdresser wrote: > I think these comments are incorrect? Done. https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:186: On 2014/01/14 16:25:30, tdresser wrote: > Looks like we normally omit this space (122 occurrences without vs 23 > occurrences with). Oops, yeah accidental addition, thanks.
https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void ForwardTouchEvent(const blink::WebTouchEvent&) = 0; On 2014/01/14 23:24:03, jdduke wrote: > On 2014/01/14 16:25:30, tdresser wrote: > > I think it may be cleaner to do the touch event dispatch outside of the > > TouchToGestureQueue. If you dispatch the touch events before passing the touch > > events to the TTGQ, you don't need to store the touch events in the TTGQ at > all. > > > > I guess there's also an advantage in unifying the flow of touch and gesture > > events, so I'm open to either solution. > > The main reason I did it this way was because of the > |OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending the > touch until we've assembled all of its gestures (as the touch ack may be > synchronous). > > If we can all agree that PD'ed touchmove's within the touch slop region will > prevent both tapping and scrolling, then I think we can go ahead with forwarding > the touches immediately. I was under the impression that this solution would already only work if PD'ed touchmoves within the touch slop region would prevent tapping and scrolling. Is that incorrect? https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:23: class CONTENT_EXPORT TouchToGestureQueue { On 2014/01/14 23:24:03, jdduke wrote: > On 2014/01/14 16:25:30, tdresser wrote: > > The names TouchToGesture, TouchToGestureSequence, and TouchToGestureSequence, > > are a little confusing. > > > > I think they might be a little clearer as TouchToGestures, > > TouchToGesturesSequence, and TouchToGesturesQueue, though those aren't > fantastic > > names. > > > > If we decide to stop storing touch events, this naming might get a bit simpler > > too. > > > > > > Let's just call it GestureEventQueue? To match TouchEventQueue? I like that name. I do think that TouchToGesture -> TouchToGestures would also clarify matters. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:28: // TODO(jdduke): Refactor to take a touch event with a vector of gestures. On 2014/01/14 23:24:03, jdduke wrote: > On 2014/01/14 16:25:30, tdresser wrote: > > Is it reasonable to perform this refactor before this lands? It would make > > interaction with the TouchToGestureQueue a lot clearer. > > We could. With the way gestures are piped here (via callbacks), we'll need to > do the gesture "collecting" somewhere, but that could be offloaded to a wrapper > or even just the ContentViewCoreImpl for now, no need to clutter the interface > for something the caller should be doing. Aggregating the gestures in CVCI sounds like a good plan to me.
OK, did some massive cleanup and refactoring. This should compile, though I have yet to update the tests to reflect the latest changes. That will come next. Sorry if you had any compilation problems. https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_to_gesture_queue.h (right): https://codereview.chromium.org/120513005/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_to_gesture_queue.h:19: virtual void ForwardTouchEvent(const blink::WebTouchEvent&) = 0; On 2014/01/15 13:43:08, tdresser wrote: > On 2014/01/14 23:24:03, jdduke wrote: > > On 2014/01/14 16:25:30, tdresser wrote: > > > I think it may be cleaner to do the touch event dispatch outside of the > > > TouchToGestureQueue. If you dispatch the touch events before passing the > touch > > > events to the TTGQ, you don't need to store the touch events in the TTGQ at > > all. > > > > > > I guess there's also an advantage in unifying the flow of touch and gesture > > > events, so I'm open to either solution. > > > > The main reason I did it this way was because of the > > |OnTouchEventHandling{Begin,End]| methods, requiring that we delay sending the > > touch until we've assembled all of its gestures (as the touch ack may be > > synchronous). > > > > If we can all agree that PD'ed touchmove's within the touch slop region will > > prevent both tapping and scrolling, then I think we can go ahead with > forwarding > > the touches immediately. > > I was under the impression that this solution would already only work if PD'ed > touchmoves within the touch slop region would prevent tapping and scrolling. Is > that incorrect? In it's current form, that's very much correct. We'd have to make some (non-trivial) changes to make it support touch slop suppression (in short, we'd have to hold touch events until the initial TouchStart ack is received, getting us into the business of touch queueing which I'd prefer to avoid). However, per our discussion, I'm inclined not to support that at all, unless we find in the future that we absolutely need it. https://codereview.chromium.org/120513005/diff/520001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/520001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1365: private boolean filterGesture(int type, int x, int y) { On 2014/01/14 16:25:30, tdresser wrote: > Can we find a better name for this? It isn't clear what the return value > indicates, and most of what we're doing is updating state, not filtering. > > I haven't thought of a name I really like, but perhaps something along the lines > of |processGesture|, |handleGesture|, or |updateForGesture|? Sure. I guess the common pattern on Android is |onFooEvent()|, where the boolean return value indicates that it was handled. I suggest we do the same, |onGestureEvent()|, or perhaps |onForwardingGestureEvent()|.
https://codereview.chromium.org/120513005/diff/820001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/android... content/browser/android/content_view_core_impl.h:401: scoped_ptr<GestureEventQueue> gesture_event_queue_; Why not just allocate this on the stack? https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:18: bool IsTouchSequenceBegin(const WebTouchEvent& event) { nit: "IsTouchSequenceStart" probably makes more sense. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:49: // TODO(jdduke): Use GestureTapDown as a new sequence marker? Agreed, that sounds simpler. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:73: DCHECK_LT(gesture_count_, kMaxGesturesPerTouch); Is it reasonable to make this a CHECK_LT? I'm pretty likely to add a new gesture that bumps us over 5 gestures per touch, test on device in release mode, and get extremely confused. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:19: InputEventAckState TransitionToMostRestrictiveState( TransitionToMostRestrictiveState implies that this mutates something. No "transition" is taking place. You could actually mutate ack_state_ in here, in which case the name would make sense. Otherwise I think naming it GetMostRestrictiveState or MostRestrictiveState would be reasonable. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:71: case GestureEventPacket::SYNTHETIC: Couldn't this violate some ordering constraints? https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:81: // until the ack in the event that the sequence receives a timeout event. This is the only place where you're transitioning to the next touch sequence, isn't it? I'm confused by the comment. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:163: // Timeout-derived gestures should have beeen forwarded or dropped beeen -> been
did some quick testing and one weird thing I observed is that when I add touch handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still able to perform one fling/scroll on the red inner div. This repros both with android_webview_shell and content_shell_apk
On 2014/01/16 15:08:06, mkosiba wrote: > did some quick testing and one weird thing I observed is that when I add touch > handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still able > to perform one fling/scroll on the red inner div. This repros both with > android_webview_shell and content_shell_apk OK, thanks for the repro, I'll dig into it. My main concern was simply that WebView was getting the right event bits/updates for the types that it was interested in. I'll do some more rigorous integration/unit testing over the next several days.
https://codereview.chromium.org/120513005/diff/820001/content/browser/android... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/android... content/browser/android/content_view_core_impl.h:401: scoped_ptr<GestureEventQueue> gesture_event_queue_; On 2014/01/16 14:31:45, tdresser wrote: > Why not just allocate this on the stack? I had this behind a flag before, so there's no longer any need... though it will never be on the stack proper, as CVCI is always heap allocated. I'll change. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:18: bool IsTouchSequenceBegin(const WebTouchEvent& event) { On 2014/01/16 14:31:45, tdresser wrote: > nit: "IsTouchSequenceStart" probably makes more sense. Done. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:73: DCHECK_LT(gesture_count_, kMaxGesturesPerTouch); On 2014/01/16 14:31:45, tdresser wrote: > Is it reasonable to make this a CHECK_LT? I'm pretty likely to add a new gesture > that bumps us over 5 gestures per touch, test on device in release mode, and get > extremely confused. Yeah. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:19: InputEventAckState TransitionToMostRestrictiveState( On 2014/01/16 14:31:45, tdresser wrote: > TransitionToMostRestrictiveState implies that this mutates something. No > "transition" is taking place. > You could actually mutate ack_state_ in here, in which case the name would make > sense. > > Otherwise I think naming it GetMostRestrictiveState or MostRestrictiveState > would be reasonable. Yeah, except the order matters (i.e., we never go from a restrictive state to a less restrictive state). I agree this isn't the best wording, but I'd like the name to indicate that arg order is important. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:71: case GestureEventPacket::SYNTHETIC: On 2014/01/16 14:31:45, tdresser wrote: > Couldn't this violate some ordering constraints? Yeah, though on Android that constraint is already violated (e.g., synthetic gestures sent down while the CVGH is awaiting a touch ack will already be processed out-of-order). One thought is to simply intersperse synthetic packets with an existing sequence; they'd be processed in-order, but they'd simply ignore the disposition of the current sequence and always be sent. wdyt? https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:81: // until the ack in the event that the sequence receives a timeout event. On 2014/01/16 14:31:45, tdresser wrote: > This is the only place where you're transitioning to the next touch sequence, > isn't it? I'm confused by the comment. It's a terrible comment. I was thinking of adding a TOUCH_END marker so that we'd pop a sequence then, though in-practice the result should be the same. Regardless, it's a bad comment. https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:163: // Timeout-derived gestures should have beeen forwarded or dropped On 2014/01/16 14:31:45, tdresser wrote: > beeen -> been Done. https://codereview.chromium.org/120513005/diff/820001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/820001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911: private void reportDoubleTap() { I'm inclined to remove all double tap reporting in this patch, otherwise it will have to be completely reworked... rbyers@: Do we still need/want this?
https://codereview.chromium.org/120513005/diff/820001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/820001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911: private void reportDoubleTap() { On 2014/01/16 17:17:53, jdduke wrote: > I'm inclined to remove all double tap reporting in this patch, otherwise it will > have to be completely reworked... rbyers@: Do we still need/want this? Hmm, I'd prefer to keep it until we know that we've got the data we need (it was added in M33, and M33 isn't yet available to users). Hopefully we'll get the data we need from M33, but we might find we need to tweak our collection. How hard is it to rework it to fit with eager GR?
On 2014/01/16 17:46:18, Rick Byers wrote: > https://codereview.chromium.org/120513005/diff/820001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java > (right): > > https://codereview.chromium.org/120513005/diff/820001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:911: > private void reportDoubleTap() { > On 2014/01/16 17:17:53, jdduke wrote: > > I'm inclined to remove all double tap reporting in this patch, otherwise it > will > > have to be completely reworked... rbyers@: Do we still need/want this? > > Hmm, I'd prefer to keep it until we know that we've got the data we need (it was > added in M33, and M33 isn't yet available to users). Hopefully we'll get the > data we need from M33, but we might find we need to tweak our collection. How > hard is it to rework it to fit with eager GR? Probably not too difficult, I think I'd just move all the code to ContentViewCore.
https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/820001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:71: case GestureEventPacket::SYNTHETIC: On 2014/01/16 17:17:53, jdduke wrote: > On 2014/01/16 14:31:45, tdresser wrote: > > Couldn't this violate some ordering constraints? > > Yeah, though on Android that constraint is already violated (e.g., synthetic > gestures sent down while the CVGH is awaiting a touch ack will already be > processed out-of-order). > > One thought is to simply intersperse synthetic packets with an existing > sequence; they'd be processed in-order, but they'd simply ignore the disposition > of the current sequence and always be sent. wdyt? SGTM.
On 2014/01/16 15:08:06, mkosiba wrote: > did some quick testing and one weird thing I observed is that when I add touch > handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still able > to perform one fling/scroll on the red inner div. This repros both with > android_webview_shell and content_shell_apk Hmm, are you able to repro in a regular ToT build? I did a bunch of debugging, and was only able to repro if I tried to scroll before the page had finished loading. In that case, I'm seeing a NO_CONSUMER ack for the TouchStart, so I wonder if this may be a touch handler registration issue?
I fleshed out the unit tests a bit more, and did a fun little hack job on ContentViewGestureHandlerTest. Some of the removed tests I'd like to port to either TouchEventQueueTest or GestureEventQueueTest, as I think they may hit cases not currently covered by existing tests.
On 2014/01/16 20:02:15, jdduke wrote: > On 2014/01/16 15:08:06, mkosiba wrote: > > did some quick testing and one weird thing I observed is that when I add touch > > handlers that prevent everything ( http://jsbin.com/uGoqoBi/5/ ) I'm still > able > > to perform one fling/scroll on the red inner div. This repros both with > > android_webview_shell and content_shell_apk > > Hmm, are you able to repro in a regular ToT build? I did a bunch of debugging, > and was only able to repro if I tried to scroll before the page had finished > loading. In that case, I'm seeing a NO_CONSUMER ack for the TouchStart, so I > wonder if this may be a touch handler registration issue? yeath, this was a regular ToT build. I thought it was a registration issue but I could repro this after waiting ~10 seconds after the content had loaded.
https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:44: typedef base::Callback<void(const GestureEventPacket& packet)> PacketSender; I'm not super clear why you're adding a layer of abstraction here. Can you comment this? https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:198: // Event if the subsequent touch had no consumer, continue dropping gestures. You mention touches having no consumer fairly frequently in comments, but never actually pass INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS. Do you just mean that the touch wasn't consumed? https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:222: SendTouchEventACK(INPUT_EVENT_ACK_STATE_NOT_CONSUMED); Based on the comment, shouldn't this be INPUT_EVENT_ACK_STATE_CONSUMED? https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:243: // The first gesture sequence should not be allowed. "The first gesture sequence should not be allowed." -> "The second gesture sequence should be allowed." https://codereview.chromium.org/120513005/diff/1000001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/1000001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817: /** Remove accidental space.
Thanks for review tdresser@. I've fleshed out the GestureEventQueue unit tests a bit more. Next I'll be tackling cleanup for ContentViewGestureHandlerTest. https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:44: typedef base::Callback<void(const GestureEventPacket& packet)> PacketSender; On 2014/01/17 15:24:17, tdresser wrote: > I'm not super clear why you're adding a layer of abstraction here. Can you > comment this? Well, I'd like the GestureSequence to perform dispatch of packets, as there may be multiple per touch ack, e.g., for queued synthetic or timeout packets. There are several ways to do this, my initial one had GEQ implement a simple forwarding interface. I could also have the GestureSequence return a vector of packets, but I'd prefer not to dynamically allocate anything. https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:198: // Event if the subsequent touch had no consumer, continue dropping gestures. On 2014/01/17 15:24:17, tdresser wrote: > You mention touches having no consumer fairly frequently in comments, but never > actually pass INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS. Do you just mean that > the touch wasn't consumed? Yeah, I'll tighten up the verbiage here (though I also should add some NO_CONSUMER_EXISTS coverage). https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:222: SendTouchEventACK(INPUT_EVENT_ACK_STATE_NOT_CONSUMED); On 2014/01/17 15:24:17, tdresser wrote: > Based on the comment, shouldn't this be INPUT_EVENT_ACK_STATE_CONSUMED? Done. https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:243: // The first gesture sequence should not be allowed. On 2014/01/17 15:24:17, tdresser wrote: > "The first gesture sequence should not be allowed." -> > "The second gesture sequence should be allowed." Done. https://codereview.chromium.org/120513005/diff/1000001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/1000001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817: /** On 2014/01/17 15:24:17, tdresser wrote: > Remove accidental space. I'm not seeing the space :(
https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1000001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:44: typedef base::Callback<void(const GestureEventPacket& packet)> PacketSender; On 2014/01/17 19:38:04, jdduke wrote: > On 2014/01/17 15:24:17, tdresser wrote: > > I'm not super clear why you're adding a layer of abstraction here. Can you > > comment this? > > Well, I'd like the GestureSequence to perform dispatch of packets, as there may > be multiple per touch ack, e.g., for queued synthetic or timeout packets. There > are several ways to do this, my initial one had GEQ implement a simple > forwarding interface. I could also have the GestureSequence return a vector of > packets, but I'd prefer not to dynamically allocate anything. Sounds reasonable. https://codereview.chromium.org/120513005/diff/1000001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/1000001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:817: /** On 2014/01/17 19:38:04, jdduke wrote: > On 2014/01/17 15:24:17, tdresser wrote: > > Remove accidental space. > > I'm not seeing the space :( Whoops, I was confused by looking at "delta from patch set" diffs which included changes other than yours...
https://codereview.chromium.org/120513005/diff/1420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/1420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1376: private boolean onForwardingGestureEvent(int type, int x, int y) { TODO: Fix |type|, as it's completely bogus. WebInputEvent::Type does NOT align with ContentViewGestureHandler.Type, so we need a way to map between the two. https://codereview.chromium.org/120513005/diff/1420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2010: if (type != ContentViewGestureHandler.GESTURE_SINGLE_TAP_CONFIRMED TODO(jdduke): These types are all wrong. We need to convert back from WebInputEvent::Type, though I fear that's lossy for SINGLE_TAP_CONFIRMED from GestureTap.
A few higher-level comments https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:50: case GestureEventPacket::TOUCH_TIMEOUT: This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat redundant with the logic in OnTouchEventAck. Can we unify these somehow to simplify things? Eg. are you just trying to ensure these get sent without needing a touch ack? Maybe I just don't fully understand why these cases need to be handled in a fundamentally different fashion. Overall this makes what I naively expected to be relatively simple logic around gesture event dispatch rather more complicated. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the synthetic packet immediately if no other packets pending. Can you talk a bit about why synthetic gestures are handled differently? Are these for gestures created without any corresponding touch events? Where do we still do that (Dominik's synthetic gesture framework seems superior in most cases IMHO)? https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ = GetMostRestrictiveState(old_ack_state, new_ack_state); Couldn't this just be a single bit "is gesture allowed", rather than trying to track some (imho awkward) notion of 'most restrictive ack state'? https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:28: class CONTENT_EXPORT GestureEventQueue { I think we need either more discussion of the design here, or a link to the design doc (not sure if you guys are still maintaining the one Tim started with). I may be slow (or short on caffine right now), but a bunch of the details/motivation here are non-obvious to me (and eager GR was my idea to begin with <grin>). I'll add a few comments throughout. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:67: InputEventAckState ack_state_; maybe call this 'aggregate_ack_state' or something? To me it's not really an individual "ack state" at all anymore, but some representation of all the acks that have been received for this sequence. Eg. I believe I'll need to modify this to track a bit more state for some stuff I'm doing.
Thanks for review! https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:50: case GestureEventPacket::TOUCH_TIMEOUT: On 2014/01/21 20:10:10, Rick Byers wrote: > This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat redundant > with the logic in OnTouchEventAck. Can we unify these somehow to simplify > things? Eg. are you just trying to ensure these get sent without needing a > touch ack? Maybe I just don't fully understand why these cases need to be > handled in a fundamentally different fashion. Overall this makes what I naively > expected to be relatively simple logic around gesture event dispatch rather more > complicated. It's not redundant, unfortunately. Synthetic packets will always be sent, but they will be dispatched in the order they were received with respect to other packets. Timeout packets will only be sent if the current sequence is "allowed". I should make that clear in the comments, thanks. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the synthetic packet immediately if no other packets pending. On 2014/01/21 20:10:10, Rick Byers wrote: > Can you talk a bit about why synthetic gestures are handled differently? Are > these for gestures created without any corresponding touch events? Where do we > still do that (Dominik's synthetic gesture framework seems superior in most > cases IMHO)? This is done for things like WebView's zoom controls (the +/- overlay). https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ = GetMostRestrictiveState(old_ack_state, new_ack_state); On 2014/01/21 20:10:10, Rick Byers wrote: > Couldn't this just be a single bit "is gesture allowed", rather than trying to > track some (imho awkward) notion of 'most restrictive ack state'? It is awkward, and I'm not opposed to using a unique state tracking bit for this. However, I think we need more than a binary state (for handling the initial transition, so a simple enum should be sufficient here. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:28: class CONTENT_EXPORT GestureEventQueue { On 2014/01/21 20:10:10, Rick Byers wrote: > I think we need either more discussion of the design here, or a link to the > design doc (not sure if you guys are still maintaining the one Tim started > with). I may be slow (or short on caffine right now), but a bunch of the > details/motivation here are non-obvious to me (and eager GR was my idea to begin > with <grin>). I'll add a few comments throughout. aelias@ and I talked about an eager GR a *long* time ago. The chief motivation then, for Android, was to prevent gesture detection issues that inevitably arise from either partial or delayed touch event streams (e.g., a misfiring ShowPress, or a disappearing TouchEnd). Added benefits include complete decoupling of detection from the final output (making it easier to experiment, and simplifying gesture detection), and a consistent way of enforcing whether a particular gesture should or should not be forwarded. I think there's a lot of room for refinement here, but this implements fully the current Android touch handling logic, and can certainly be extended for more nuanced behavior. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:67: InputEventAckState ack_state_; On 2014/01/21 20:10:10, Rick Byers wrote: > maybe call this 'aggregate_ack_state' or something? To me it's not really an > individual "ack state" at all anymore, but some representation of all the acks > that have been received for this sequence. Eg. I believe I'll need to modify > this to track a bit more state for some stuff I'm doing. As noted below, using the ack here is just bad on my part. I'll switch to a more appropriate (and localized) state variable. https://codereview.chromium.org/120513005/diff/1420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/1420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:357: if (isDistanceBetweenDownAndUpTooLong(e.getRawX(), e.getRawY())) { TODO: As a follow-up, all of this tap bookkeeping can be greatly simplified.
I'd like to start final review for this, please let me know if you have any questions or the review burden is too great (sorry!) mkosiba@: android_webview/ skyostil@: content/browser/android and content/public/android tdresser@: content/browser/renderer_host/input aelias@: content/browser/renderer_host/render_widget_host* rbyers@: UMA changes in ContentViewGestureHandler/ContentViewCore, and any additional input on content/browser/renderer_host/input Thanks!
I haven't looked at unit tests yet this pass.
I'm seeing some buggy behaviour though, with touch cancelling not happening
consistently.
To repro:
1. Visit jsbin.com/uniLISo/1?quiet=1
2. Press and move your finger quickly
- As expected, the touch count goes to 1, and then 0,
as the touch is cancelled
3. Press and move your finger slowly
- You expect the count to decrease to 0, but it stays
at 1.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.cc (right):
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.cc:8: #include
"content/common/input/web_input_event_traits.h"
Remove unused includes.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_packet.h (right):
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_packet.h:41: enum {
kMaxGesturesPerTouch = 5 };
Using an enum for a single constant isn't very common (~64 occurrences, compared
to ~8000 for const int). I'd prefer a const int, unless there's a specific
reason you've used an enum here.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:60: case
GestureEventPacket::SYNTHETIC:
If this is only for things like WebView's zoom controls, do we care about out of
order dispatch? I think it might be safe to always dispatch events like that
immediately. (Sorry I've been switching back and forth on this). Are there any
synthetic events which would cause issues if they were dispatched out of order?
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:74: void
GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) {
A DCHECK(!sequences.empty()) might be good here.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:80:
A DCHECK(!sequences.empty()) might be good here too.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:148:
Perhaps a comment separating GestureEventQueue methods from
GestureEventQueue::GestureSequence methods?
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER|
should only be effective when the *first* touch is ack'ed.
If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future acks
in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED?
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.h (right):
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:27: // generating
touch sequence. Synthetic gestures are dispatched immediately.
Synthetic gestures are currently not always dispatched immediately (but see
comments on gesture_event_queue.cc).
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:61:
std::deque<GestureEventPacket> sequence_;
This should just be an std::queue, shouldn't it? Using the more restrictive type
clarifies the purpose of the object.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.h:80:
std::deque<GestureSequence> sequences_;
std::queue?
> rbyers@: UMA changes in ContentViewGestureHandler/ContentViewCore, This LGTM > and any additional input on content/browser/renderer_host/input See comments, mostly nits at this stage. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:50: case GestureEventPacket::TOUCH_TIMEOUT: On 2014/01/21 20:52:38, jdduke wrote: > On 2014/01/21 20:10:10, Rick Byers wrote: > > This special handling for TOUCH_TIMEOUT and SYNTHETIC seems somewhat redundant > > with the logic in OnTouchEventAck. Can we unify these somehow to simplify > > things? Eg. are you just trying to ensure these get sent without needing a > > touch ack? Maybe I just don't fully understand why these cases need to be > > handled in a fundamentally different fashion. Overall this makes what I > naively > > expected to be relatively simple logic around gesture event dispatch rather > more > > complicated. > > It's not redundant, unfortunately. Synthetic packets will always be sent, but > they will be dispatched in the order they were received with respect to other > packets. Timeout packets will only be sent if the current sequence is > "allowed". I should make that clear in the comments, thanks. Sorry, what I meant was that we've got two separate places that need to know about handling synthetic and timeout packets specially in this regard. One in OnGestureEventPacket when they're received, and one in GestureSequence::OnTouchEventAck when they happen to be the next thing in the queue. Can we put some of this logic in a common place and call it from both locations. Some sort of MaybeHandlePacketNow(GestureEventPacket) function or something? It would return true if the packet was special in some way and so handled immediately. Then I think we might be able to have a single switch on gesture_source() statement in this file instead of two different ones. It doesn't look like there's perfect overlap though and I don't fully understand some of the cases here, so perhaps this would actually be more awkward than leaving it as is. Your call. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:61: // Handle the synthetic packet immediately if no other packets pending. On 2014/01/21 20:52:38, jdduke wrote: > On 2014/01/21 20:10:10, Rick Byers wrote: > > Can you talk a bit about why synthetic gestures are handled differently? Are > > these for gestures created without any corresponding touch events? Where do > we > > still do that (Dominik's synthetic gesture framework seems superior in most > > cases IMHO)? > > This is done for things like WebView's zoom controls (the +/- overlay). Ah, thanks! I had forgotten we used synthetic gestures in production like that. https://codereview.chromium.org/120513005/diff/1420001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:189: ack_state_ = GetMostRestrictiveState(old_ack_state, new_ack_state); On 2014/01/21 20:52:38, jdduke wrote: > On 2014/01/21 20:10:10, Rick Byers wrote: > > Couldn't this just be a single bit "is gesture allowed", rather than trying to > > track some (imho awkward) notion of 'most restrictive ack state'? > > It is awkward, and I'm not opposed to using a unique state tracking bit for > this. However, I think we need more than a binary state (for handling the > initial transition, so a simple enum should be sufficient here. Cool, I like this better (has a nice symmetry with the new TouchEventQueue filtering state too). Thanks. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.cc:32: // TODO(jdduke): Use GestureTapDown as a new sequence marker? I think clearing using 'period when no touches are on the target window' as the precise delimitation between sequences seems right to me. This is what's important to the user, and gestures are a higher level semantic above it (eg. conceivably we could decide not to send GTD if we saw two fingers go down at once in the same input frame, or that some other gesture event could come BEFORE a GTD in a sequence). So my vote is probably for removing this TODO and leaving as-is. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:63: PENDING, Please add a brief comment describing the semantics of each of these sates. In particular, the likely distinction between ALWAYS_ALLOWED and ALLOWED isn't obvious to me (before looking at the code that implements them). https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:81: bool outstanding_tap_; Can you add a comment for these two making it clear that they're about what's outstanding on the output side of this buffer, not the input side? To me 'outstanding' by itself ambiguous in this regard. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308: EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type); The other expected gesture is a TapCancel, but that's not actually validated anywhere right? I think it's worth while to extend the test infrastructure to be able to easily EXPECT that the gestures are a particular sequence. The simplest way to do this is to have a function that just returns the list of gesture types received as a string, and then EXPECT_EQ on that. Or you could go more fancy and build an EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or something. Tim did something like this a long time ago in some new Aura GR unit tests (that ended up getting forgotten after his intership ended), and it turned out really nice.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308: EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type); On 2014/01/23 20:45:29, Rick Byers wrote: > The other expected gesture is a TapCancel, but that's not actually validated > anywhere right? > > I think it's worth while to extend the test infrastructure to be able to easily > EXPECT that the gestures are a particular sequence. The simplest way to do this > is to have a function that just returns the list of gesture types received as a > string, and then EXPECT_EQ on that. Or you could go more fancy and build an > EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or > something. Tim did something like this a long time ago in some new Aura GR unit > tests (that ended up getting forgotten after his intership ended), and it turned > out really nice. Since gmock isn't widely accepted in chromium, I think the right thing to do here is to define a custom gtest matcher. Something along the lines of this: http://stackoverflow.com/questions/10060110/how-does-gtest-compare-the-values... (though perhaps with size as an additional parameter, instead of relying on crazy templating)
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.cc:8: #include "content/common/input/web_input_event_traits.h" On 2014/01/23 16:32:43, tdresser wrote: > Remove unused includes. Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.h:41: enum { kMaxGesturesPerTouch = 5 }; On 2014/01/23 16:32:43, tdresser wrote: > Using an enum for a single constant isn't very common (~64 occurrences, compared > to ~8000 for const int). I'd prefer a const int, unless there's a specific > reason you've used an enum here. I think the enum might be a bit more clear here as it's used for the member array length. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:60: case GestureEventPacket::SYNTHETIC: On 2014/01/23 16:32:43, tdresser wrote: > If this is only for things like WebView's zoom controls, do we care about out of > order dispatch? I think it might be safe to always dispatch events like that > immediately. (Sorry I've been switching back and forth on this). Are there any > synthetic events which would cause issues if they were dispatched out of order? Honestly I have no idea. I don't think it hurts to dispatch them in-order, but there are probably scenarios under both approaches that could confuse the gesture stream. I just don't know if any of them happen in practice. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:74: void GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) { On 2014/01/23 16:32:43, tdresser wrote: > A DCHECK(!sequences.empty()) might be good here. Sure. The |Head()| call performs that check, but I can be more explicit if you like, same below, your call. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:148: On 2014/01/23 16:32:43, tdresser wrote: > Perhaps a comment separating GestureEventQueue methods from > GestureEventQueue::GestureSequence methods? Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER| should only be effective when the *first* touch is ack'ed. On 2014/01/23 16:32:43, tdresser wrote: > If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future acks > in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED? If the primary pointer press was NOT_CONSUMED, but remains stationary and the secondary pointer press is NO_CONSUMER, there's still the possibility that the primary pointer will then move and be CONSUMED. This was first reported in crbug/135102. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:27: // generating touch sequence. Synthetic gestures are dispatched immediately. On 2014/01/23 16:32:43, tdresser wrote: > Synthetic gestures are currently not always dispatched immediately (but see > comments on gesture_event_queue.cc). Good catch. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:61: std::deque<GestureEventPacket> sequence_; On 2014/01/23 16:32:43, tdresser wrote: > This should just be an std::queue, shouldn't it? Using the more restrictive type > clarifies the purpose of the object. Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:80: std::deque<GestureSequence> sequences_; On 2014/01/23 16:32:43, tdresser wrote: > std::queue? Done.
https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.cc:32: // TODO(jdduke): Use GestureTapDown as a new sequence marker? On 2014/01/23 20:45:29, Rick Byers wrote: > I think clearing using 'period when no touches are on the target window' as the > precise delimitation between sequences seems right to me. This is what's > important to the user, and gestures are a higher level semantic above it (eg. > conceivably we could decide not to send GTD if we saw two fingers go down at > once in the same input frame, or that some other gesture event could come BEFORE > a GTD in a sequence). So my vote is probably for removing this TODO and leaving > as-is. Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:80: On 2014/01/23 16:32:43, tdresser wrote: > A DCHECK(!sequences.empty()) might be good here too. Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:63: PENDING, On 2014/01/23 20:45:29, Rick Byers wrote: > Please add a brief comment describing the semantics of each of these sates. In > particular, the likely distinction between ALWAYS_ALLOWED and ALLOWED isn't > obvious to me (before looking at the code that implements them). Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.h:81: bool outstanding_tap_; On 2014/01/23 20:45:29, Rick Byers wrote: > Can you add a comment for these two making it clear that they're about what's > outstanding on the output side of this buffer, not the input side? To me > 'outstanding' by itself ambiguous in this regard. bool fantastic_fling_? ;) Done. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308: EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type); On 2014/01/23 20:45:29, Rick Byers wrote: > The other expected gesture is a TapCancel, but that's not actually validated > anywhere right? > > I think it's worth while to extend the test infrastructure to be able to easily > EXPECT that the gestures are a particular sequence. The simplest way to do this > is to have a function that just returns the list of gesture types received as a > string, and then EXPECT_EQ on that. Or you could go more fancy and build an > EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or > something. Tim did something like this a long time ago in some new Aura GR unit > tests (that ended up getting forgotten after his intership ended), and it turned > out really nice. Yeah, I should do this right. Good catch.
lgtm modulo minor comments below. https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.h:22: GESTURE_SOURCE_DEFAULT = TOUCH I'd prefer to get rid of this and the default constructor. If the default constructor is truly needed for some technical reasons, I'd prefer using an "INVALID" entry in the enum instead. https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:62: // Handle the synthetic packet immediately if no other packets pending. Looks like this was already discussed to some degree, but I'm also a bit suspicious of the different handling for synthetic input events. It seems like test stuff leaking into production code and it might affect perf characteristics. Is there some way to use GestureEventPacket::TOUCH for these and maybe change the synthetic gesture code to be more similar to a normal sequence?
I'll want to do some additional manual testing once the current issue with touches not being cancelled is sorted out. Other than that, I'm happy with this. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:74: void GestureEventQueue::OnTouchEventAck(InputEventAckState ack_state) { On 2014/01/23 22:52:34, jdduke wrote: > On 2014/01/23 16:32:43, tdresser wrote: > > A DCHECK(!sequences.empty()) might be good here. > > Sure. The |Head()| call performs that check, but I can be more explicit if you > like, same below, your call. Oh, good point. In that case I have no strong feeling either way. https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:171: // |NO_CONSUMER| should only be effective when the *first* touch is ack'ed. On 2014/01/23 22:52:34, jdduke wrote: > On 2014/01/23 16:32:43, tdresser wrote: > > If we get an INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS, shouldn't all future > acks > > in the sequence be INPUT_EVENT_ACK_STATE_NOT_CONSUMED? > > If the primary pointer press was NOT_CONSUMED, but remains stationary and the > secondary pointer press is NO_CONSUMER, there's still the possibility that the > primary pointer will then move and be CONSUMED. This was first reported in > crbug/135102. That's unfortunate. Looks good then. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:6: #include "base/logging.h" Remove unnecessary includes. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:150: void MoveTouchPoints(int index0, int x0, int y0, int index1, int x1, int y1) { Unused, and probably unnecessary. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:182: void PushGestureImpl(const WebGestureEvent& gesture) { Do we need a separate method for this? https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:247: // Event if the subsequent touch is not consumed, continue dropping gestures. Event if -> Even if? https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:253: // Event if the subsequent touch had no consumer, continue dropping gestures. As above.
https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... content/browser/renderer_host/input/gesture_event_packet.h:22: GESTURE_SOURCE_DEFAULT = TOUCH On 2014/01/24 05:35:54, aelias wrote: > I'd prefer to get rid of this and the default constructor. If the default > constructor is truly needed for some technical reasons, I'd prefer using an > "INVALID" entry in the enum instead. Done. https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/120513005/diff/1800002/content/browser/render... content/browser/renderer_host/input/gesture_event_queue.cc:62: // Handle the synthetic packet immediately if no other packets pending. On 2014/01/24 05:35:54, aelias wrote: > Looks like this was already discussed to some degree, but I'm also a bit > suspicious of the different handling for synthetic input events. It seems like > test stuff leaking into production code and it might affect perf > characteristics. Is there some way to use GestureEventPacket::TOUCH for these > and maybe change the synthetic gesture code to be more similar to a normal > sequence? Yeah, it was a mistake to include synthetic gestures here... the burden should remain on the caller to inject synthetic gestures in the proper sequence relative to a touch stream, and so shall it remain. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:6: #include "base/logging.h" On 2014/01/24 13:47:30, tdresser wrote: > Remove unnecessary includes. Done. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:150: void MoveTouchPoints(int index0, int x0, int y0, int index1, int x1, int y1) { On 2014/01/24 13:47:30, tdresser wrote: > Unused, and probably unnecessary. Done. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:182: void PushGestureImpl(const WebGestureEvent& gesture) { On 2014/01/24 13:47:30, tdresser wrote: > Do we need a separate method for this? Absolutely not. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:247: // Event if the subsequent touch is not consumed, continue dropping gestures. On 2014/01/24 13:47:30, tdresser wrote: > Event if -> Even if? Done. https://codereview.chromium.org/120513005/diff/1890001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:253: // Event if the subsequent touch had no consumer, continue dropping gestures. On 2014/01/24 13:47:30, tdresser wrote: > As above. Done.
LGTM https://codereview.chromium.org/120513005/diff/1980001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1980001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:447: } I'd like to see a test for the scroll after longpress case.
avi@: owner review for content/port/browser/render_widget_host_view_port.h? Thanks. https://codereview.chromium.org/120513005/diff/1980001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1980001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:447: } On 2014/01/24 19:30:15, tdresser wrote: > I'd like to see a test for the scroll after longpress case. Sure. I added a specific test for this case in ContentViewGestureHandlerTest, but I think it would be a good addition to have a TOUCH after TOUCH_TIMEOUT packet test.
content/port/browser/render_widget_host_view_port.h LGTM
content/browser/renderer_host/input LGTM https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/120513005/diff/1670001/content/browser/render... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:308: EXPECT_EQ(WebInputEvent::GestureScrollBegin, sent_gesture().type); On 2014/01/24 05:34:40, jdduke wrote: > On 2014/01/23 20:45:29, Rick Byers wrote: > > The other expected gesture is a TapCancel, but that's not actually validated > > anywhere right? > > > > I think it's worth while to extend the test infrastructure to be able to > easily > > EXPECT that the gestures are a particular sequence. The simplest way to do > this > > is to have a function that just returns the list of gesture types received as > a > > string, and then EXPECT_EQ on that. Or you could go more fancy and build an > > EXPECT_TYPE_LIST macro which takes a var-arg number of input event types or > > something. Tim did something like this a long time ago in some new Aura GR > unit > > tests (that ended up getting forgotten after his intership ended), and it > turned > > out really nice. > > Yeah, I should do this right. Good catch. Thanks for putting the extra time into this, I like it a LOT better now!
mkosiba@: Review for android_webview/? Thanks.
lgtm
tedchoc@: Could you take a look at ContentViewCore.java? I'd love another set of eyes on it... Thank you sir.
lgtm w/ nits...(scariest change I've seen in a long time) https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi... content/browser/android/content_view_core_impl.cc:398: env, j_obj.obj(), should only be indented 4 right? https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:486: static final int INPUT_EVENT_ACK_STATE_UNKNOWN = 0; Any chance we could convert the shared constants to use the template files to generate java constants and c++ constants instead of having to manually keep them in sync? i.e. ./content/public/android/java/src/org/chromium/content/common/ResultCodes.template using ../build/android/java_cpp_template.gypi https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1988: && type != ContentViewGestureHandler.GESTURE_LONG_TAP) return; if the conditional and statement don't all fit on one line, then braces are needed https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:149: static final int GESTURE_SHOW_PRESS = 0; same comment about sharing constants as my previous one https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:582: private boolean ignoreDetectorEvents() { why not just use getPermanentlyIgnoreDetectorEvents() above? https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:589: //if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return; Doesn't this need to be uncommented? https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:826: // TODO: Need to deal with multi-touch transition TODO(jdduke): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:97: if (type == ContentViewGestureHandler.GESTURE_SCROLL_START) since not all fit on one line, need to add braces
On 2014/01/30 02:29:47, Ted C wrote: > lgtm w/ nits...(scariest change I've seen in a long time) I'll take that as a compliment ;) (but seriously, this patch was one of those "down the rabbit hole" untanglings, and I'll be happy to wash my hands of it...) I'll address nits tomorrow, thanks for review.
https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/120513005/diff/2420001/content/browser/androi... content/browser/android/content_view_core_impl.cc:398: env, j_obj.obj(), On 2014/01/30 02:29:48, Ted C wrote: > should only be indented 4 right? Done. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:486: static final int INPUT_EVENT_ACK_STATE_UNKNOWN = 0; On 2014/01/30 02:29:48, Ted C wrote: > Any chance we could convert the shared constants to use the template files to > generate java constants and c++ constants instead of having to manually keep > them in sync? > > i.e. > ./content/public/android/java/src/org/chromium/content/common/ResultCodes.template > > using ../build/android/java_cpp_template.gypi Hmm, we may not actually need them, I've reworked those bits somewhat. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1988: && type != ContentViewGestureHandler.GESTURE_LONG_TAP) return; On 2014/01/30 02:29:48, Ted C wrote: > if the conditional and statement don't all fit on one line, then braces are > needed Done. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:149: static final int GESTURE_SHOW_PRESS = 0; On 2014/01/30 02:29:48, Ted C wrote: > same comment about sharing constants as my previous one Yeah, this is a bit more difficult as blink owns the enum. However, I suppose we could register the constants, as is done for TouchPoint.java? I'll file a bug and address that in a follow-up patch. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:582: private boolean ignoreDetectorEvents() { On 2014/01/30 02:29:48, Ted C wrote: > why not just use getPermanentlyIgnoreDetectorEvents() above? Good point, we no longer have the "temporarily ignore" value so there's no need for this. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:589: //if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return; On 2014/01/30 02:29:48, Ted C wrote: > Doesn't this need to be uncommented? Oops, yes! Good catch, I had disabled for testing... https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:826: // TODO: Need to deal with multi-touch transition On 2014/01/30 02:29:48, Ted C wrote: > TODO(jdduke): Done. https://codereview.chromium.org/120513005/diff/2420001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java (right): https://codereview.chromium.org/120513005/diff/2420001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewGestureHandlerTest.java:97: if (type == ContentViewGestureHandler.GESTURE_SCROLL_START) On 2014/01/30 02:29:48, Ted C wrote: > since not all fit on one line, need to add braces Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/120513005/2460001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/120513005/1200003
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/120513005/2490001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/120513005/2500001
Message was sent while issue was closed.
Change committed as 248066
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
