Defer GestureTapDown events briefly.

Defer GestureTapDown events briefly.

GestureTapDown events are always handled by the WK thread. They are
frequently followed by a GestureScrollStart which can be handled on the
compositor thread. In this case, the GTD event is unnecessary and its only
side effect is to make the compositor thread block on the WK thread. This
makes touchscreen scrolling sluggish if the WK thread is busy.

This change defers sending the GTD briefly so that it can be suppressed
if the GTD event is part of a scroll gesture. This increases the likelyhood
that a scroll gesture can be successfully handled on the compositor thread.

BUG=137555
TESTS= by hand, by unit test.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152230

[rjkroege, 2012-08-16 15:07:15]

Please take a look. Feel free to ignore the nits that I'm already fixing.

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
File content/browser/renderer_host/gesture_event_filter.cc (right):

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
content/browser/renderer_host/gesture_event_filter.cc:7: #include
"base/debug/stack_trace.h"
am removing.

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
content/browser/renderer_host/gesture_event_filter.cc:106: // TODO(rjkroege):
Reset the tap suppression controller.
isn't currently getting reset. This CL does not alter this fact.

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
content/browser/renderer_host/gesture_event_filter.cc:145:
gesture_event.timeStampSeconds >=
indenting looks wrongish. I'll fix it up.

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:13: #include
"base/debug/stack_trace.h"
i'll remove this too

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/10855200/diff/1/content/browser/render...
content/browser/renderer_host/render_widget_host_unittest.cc:903: EXPECT_EQ(2U,
host_->GestureEventLastQueueEventSize());
i keep the event being handled by the renderer in the queue instead of separate
from it.

[sadrul, 2012-08-16 16:08:15]

LGTM with some comments.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
File content/browser/renderer_host/gesture_event_filter.cc (right):

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:59: case
WebInputEvent::GestureFlingCancel:
indent!

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:71:
deferred_tap_gesture_event_ = gesture_event;
Let's call it deferred_tap_down_event_

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:82:
deferred_tap_gesture_event_);
Should the tap-event be also forwarded immediately?

and braces!

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:98: DLOG(FATAL) <<
"Unhandled case.\n";
You can use NOTREACHED here.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:130: if
(ShouldHandleEventNow())
braces!

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:138: if
(coalesced_gesture_events_.size() > 1 &&
Could it be == 1 as well?

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:149: << "Bad gesture type
after coalescing?\n";
Perhaps just DCHECK at the beginning that gesture_event.type ==
GestureScrollUpdate

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:133:
select_range_pending_(false),
unintentional?

[rjkroege, 2012-08-16 17:12:26]

sky@ Owners please?

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
File content/browser/renderer_host/gesture_event_filter.cc (right):

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:59: case
WebInputEvent::GestureFlingCancel:
On 2012/08/16 16:08:15, sadrul wrote:
> indent!

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:71:
deferred_tap_gesture_event_ = gesture_event;
On 2012/08/16 16:08:15, sadrul wrote:
> Let's call it deferred_tap_down_event_

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:82:
deferred_tap_gesture_event_);
On 2012/08/16 16:08:15, sadrul wrote:
> Should the tap-event be also forwarded immediately?

Probably not. Doing so would deviate from the pattern of one event at a time.

> 
> and braces!

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:98: DLOG(FATAL) <<
"Unhandled case.\n";
On 2012/08/16 16:08:15, sadrul wrote:
> You can use NOTREACHED here.

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:130: if
(ShouldHandleEventNow())
On 2012/08/16 16:08:15, sadrul wrote:
> braces!

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:138: if
(coalesced_gesture_events_.size() > 1 &&
On 2012/08/16 16:08:15, sadrul wrote:
> Could it be == 1 as well?

No. The 0th event has already been sent to the rendererer. Coalescing with it
would be wrong.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/gesture_event_filter.cc:149: << "Bad gesture type
after coalescing?\n";
On 2012/08/16 16:08:15, sadrul wrote:
> Perhaps just DCHECK at the beginning that gesture_event.type ==
> GestureScrollUpdate

Done.

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

http://codereview.chromium.org/10855200/diff/4001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:133:
select_range_pending_(false),
On 2012/08/16 16:08:15, sadrul wrote:
> unintentional?

Done.

[sky, 2012-08-16 19:25:31]

LGTM

[commit-bot: I haz the power, 2012-08-16 19:43:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/10855200/6001

[commit-bot: I haz the power, 2012-08-16 20:11:43]

Try job failure for 10855200-6001 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-16 21:15:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/10855200/7003

[commit-bot: I haz the power, 2012-08-16 22:54:34]

Try job failure for 10855200-7003 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-18 00:51:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/10855200/1009

[commit-bot: I haz the power, 2012-08-18 03:07:19]

Change committed as 152230