Don't send touch move to renderer while scrolling

Don't send touch move to renderer while scrolling
    
    This patch tries to optimize this case:
    
    A scrollable webpage has a touch handler listening to touch move but
    does not prevent-default on touch move.
    
    When the touch handler does not call prevent default on touch move,
    the touch move events will come back to browser process and can
    generate scroll events. While the page is scrolled, we want to stop
    sending the touch move events to the touch handler, trying to avoid
    the scrolling being blocked on waiting for each touch move to be ACKed
    from renderer. We will still send any touch end events to the page.
    
    BUG=240735, 251810
TEST=1. enable the flag #enable-no-touch-to-renderer-while-scrolling
    in chrome://flags
    2. go to www.rbyers.net/eventTest.html
    scroll the page, one should see touch move stops being received
    when scroll stops, one should see touch end to all existing touches
    3. go to www.rbyers.net/janky-touch-scroll.html
    scroll the page and one should see smooth scrolling

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217454

[Rick Byers, 2013-05-30 01:53:29]

A couple drive-by comments while you and Sadrul work to figure out how best to
structure this.  

+aelias from chrome android.  Where is the logic for android?  Would it make
sense to share any of it?

https://codereview.chromium.org/16114003/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16114003/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6549: +      <message
name="IDS_FLAGS_ENABLE_TOUCH_CANCEL_AFTER_SCROLL_NAME" desc="Name of the flag
that enables sending touch cancel after scroll.">
Nit: "on scroll" would make more sense for the name/description than "after
scroll".  "after" to me implies after the scroll has ended.

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2478: if
(switches::IsTouchCancelAfterScrollEnabled()) {
This code should apply to windows (non-aura) as well (and perhaps there's even
some opportunity to share logic with Android).  So it probably doesn't belong in
an Aura specific file.  

I know you and Sadrul are discussing where best to put this - I'm not sure
myself which option I think is best.

[sadrul, 2013-05-30 02:44:16]

https://codereview.chromium.org/16114003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16114003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1265:
ENABLE_DISABLE_VALUE_TYPE(switches::kEnableTouchCancelAfterScroll,
Use ENABLE_DISABLE_VALUE_TYPE_AND_VALUE(
  kTouchCancelAfterScroll, "1",
  ..., "0")

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2485:
host_->scroll_update_in_progress() = true;
ew no

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2495:
host_->skip_next_acked_touch_cancel() = true;
ew no

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2502:
host_->scroll_update_in_progress() = false;
stahp

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2504: }
There may be a few more touch-move events in the touch-event queue when
scrolling starts. So it is necessary to flush all those events to the GR
immediately when this happens.

This is not a good place for doing this. It would be better to do this in
GestureEventFilter (although still a little awkward, perhaps), or near that part
of the code.

https://codereview.chromium.org/16114003/diff/1/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/16114003/diff/1/ui/base/ui_base_switches.h#ne...
ui/base/ui_base_switches.h:24: UI_EXPORT extern const char
kEnableTouchCancelAfterScroll[];
You don't need both versions of the flag at the same time. Remove one. And these
flags are used only in content/. Can they move into
content/public/common/content_switches instead?

[Yufeng Shen (Slow to review), 2013-05-30 22:34:37]

https://codereview.chromium.org/16114003/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16114003/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:6549: +      <message
name="IDS_FLAGS_ENABLE_TOUCH_CANCEL_AFTER_SCROLL_NAME" desc="Name of the flag
that enables sending touch cancel after scroll.">
On 2013/05/30 01:53:29, Rick Byers wrote:
> Nit: "on scroll" would make more sense for the name/description than "after
> scroll".  "after" to me implies after the scroll has ended.

Done.

https://codereview.chromium.org/16114003/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16114003/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1265:
ENABLE_DISABLE_VALUE_TYPE(switches::kEnableTouchCancelAfterScroll,
On 2013/05/30 02:44:16, sadrul wrote:
> Use ENABLE_DISABLE_VALUE_TYPE_AND_VALUE(
>   kTouchCancelAfterScroll, "1",
>   ..., "0")

Done.

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2478: if
(switches::IsTouchCancelAfterScrollEnabled()) {
On 2013/05/30 01:53:29, Rick Byers wrote:
> This code should apply to windows (non-aura) as well (and perhaps there's even
> some opportunity to share logic with Android).  So it probably doesn't belong
in
> an Aura specific file.  
> 
> I know you and Sadrul are discussing where best to put this - I'm not sure
> myself which option I think is best.


Moved the logic to render_widget_host_impl.cc

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2485:
host_->scroll_update_in_progress() = true;
On 2013/05/30 02:44:16, sadrul wrote:
> ew no

Done.

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2495:
host_->skip_next_acked_touch_cancel() = true;
On 2013/05/30 02:44:16, sadrul wrote:
> ew no

Done.

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2502:
host_->scroll_update_in_progress() = false;
On 2013/05/30 02:44:16, sadrul wrote:
> stahp

Done.

https://codereview.chromium.org/16114003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_aura.cc:2504: }
On 2013/05/30 02:44:16, sadrul wrote:
> There may be a few more touch-move events in the touch-event queue when
> scrolling starts. So it is necessary to flush all those events to the GR
> immediately when this happens.
> 
> This is not a good place for doing this. It would be better to do this in
> GestureEventFilter (although still a little awkward, perhaps), or near that
part
> of the code.

moved to render_widge_host_impl.cc

https://codereview.chromium.org/16114003/diff/1/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/16114003/diff/1/ui/base/ui_base_switches.h#ne...
ui/base/ui_base_switches.h:24: UI_EXPORT extern const char
kEnableTouchCancelAfterScroll[];
On 2013/05/30 02:44:16, sadrul wrote:
> You don't need both versions of the flag at the same time. Remove one. And
these
> flags are used only in content/. Can they move into
> content/public/common/content_switches instead?

Done.

[Yufeng Shen (Slow to review), 2013-06-03 18:51:40]

Ping.

[Yufeng Shen (Slow to review), 2013-06-17 19:50:17]

On 2013/06/03 18:51:40, Yufeng Shen wrote:
> Ping.

hey Sadrul, can you take a look at this patch ?

We are targeting this for M29 (with default to be disabled).

[Yufeng Shen (Slow to review), 2013-06-19 17:26:35]

rebased PTAL. Thanks.

[Yufeng Shen (Slow to review), 2013-06-19 20:14:53]

Fix the case touch event queue could be empty when calling
GetLatestEvent().

[sadrul, 2013-06-20 08:51:27]

/cc jdduke@ as an FYI

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1073: if
(gesture_event.type == WebInputEvent::GestureScrollUpdate &&
You should do this on GestureScrollBegin event, which marks the beginning of the
scroll. So the touch-cancel should go to the renderer before the scrolling
starts.

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2317:
!scroll_update_in_progress_;
Can you expose gesture_event_filter_->scrolling_in_progress(), and use that here
instead of maintaining the same state here in RWHI?

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:4256: if
(host_->ShouldForwardTouchEvent())
Use EXPECT to verify the expectation, since the state of the host_ should be
known here.

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/touch_event_queue.cc (left):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/touch_event_queue.cc:177: return
touch_queue_.back()->coalesced_event();
When is touch_queue_ empty here?

[aelias_OOO_until_Jul13, 2013-06-20 17:52:30]

This change seems complicated/potentially bug-prone and I think we should stop
making major changes to touch_event_queue now that work is started on
BrowserInputController.  We should make this kind of optimization as part of
that system.

[Yufeng Shen (Slow to review), 2013-06-20 17:56:44]

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1073: if
(gesture_event.type == WebInputEvent::GestureScrollUpdate &&
On 2013/06/20 08:51:27, sadrul wrote:
> You should do this on GestureScrollBegin event, which marks the beginning of
the
> scroll. So the touch-cancel should go to the renderer before the scrolling
> starts.

I have a comment in the description of the CL about why using
GestureScrollUpdate
instead of GestureScrollBegin.

Mainly it is for the 2 finger case. With current implementation of gesture
recognizer, if touch move is prevented default, and we use 2 fingers scroll, we
can still generate GestureScrollBegin (but not GestureScrollUpdate). And if we
send cancel on GestureScrollBegin, it is wrong since the page wants to consume
touch move.

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2317:
!scroll_update_in_progress_;
On 2013/06/20 08:51:27, sadrul wrote:
> Can you expose gesture_event_filter_->scrolling_in_progress(), and use that
here
> instead of maintaining the same state here in RWHI?

gesture_event_filter->scrolling_in_progress_  is designed for debouncing purpose
and it has the feature that if it is set on GestureScrollUpdate, but not renewed
in debounce_interval_time_ms_, the debounce_deferring_timer callback will clear
it, that means if we scroll slowly, there would be

GestureScrollUpdate
-> scrolling_in_progress_ = true
-> 30ms later scrolling_in_progress_ = false
-> touch events are sent to renderer (wrong!)
-> new GestureScrollUpdate arrives
-> scrolling_in_progress_ = true

So it does not quite carry the semantics of the scroll_update_in_progress_ we 
maintain in RWHI.

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_unittest.cc:4256: if
(host_->ShouldForwardTouchEvent())
On 2013/06/20 08:51:27, sadrul wrote:
> Use EXPECT to verify the expectation, since the state of the host_ should be
> known here.

Done.

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/touch_event_queue.cc (left):

https://codereview.chromium.org/16114003/diff/18002/content/browser/renderer_...
content/browser/renderer_host/touch_event_queue.cc:177: return
touch_queue_.back()->coalesced_event();
On 2013/06/20 08:51:27, sadrul wrote:
> When is touch_queue_ empty here?

The PopTouchEventToView() below first pops the touch event in the queue, which
can make the queue empty. Then the popped touch event is sent to View, which can
result in a gesture event (e.g. ScrollUpdate). Then on receiving
GestureScrollUpdate, we want to make a cancel event from the latest event from
touch event queue, which could be empty.

[Yufeng Shen (Slow to review), 2013-06-20 18:06:25]

On 2013/06/20 17:52:30, aelias wrote:
> This change seems complicated/potentially bug-prone and I think we should stop
> making major changes to touch_event_queue now that work is started on
> BrowserInputController.  We should make this kind of optimization as part of
> that system.

This is all behind a flag which is default off. Chrome on Android already has
this
optimization, but lacks the the supporting data that proves this optimization's
usefulness (or you guys do have but I am not aware of). We are working on a more
complete telemetry scroll latency test and plan to get concrete latency
improvement
stats out for this optimization. So we need to implement this first (even though
it is under the flag), and it is also beneficial if later on we do decide to
implement
this optimization based on latency improvement stats we have, then we already
have the
implementation we can borrow from.

[aelias_OOO_until_Jul13, 2013-06-20 18:37:47]

OK, thanks for the explanation.  Fine by me then because it's behind a flag.

[Yufeng Shen (Slow to review), 2013-06-21 17:44:19]

Hey Sadrul:  do you have more comments ?

[jdduke (slow), 2013-06-21 18:07:05]

On 2013/06/21 17:44:19, Yufeng Shen wrote:
> Hey Sadrul:  do you have more comments ?

If at all possible, I'd love it if we could defer this patch until
crrev.com/15789009 has landed.

This work seems to add a good amount of additional coupling between
TouchEventQueue and RenderWidgetHost, which is in part what crrev.com/15789009
is trying to address. 

I have no issues with what this patch does, especially as it's behind a flag,
but I wonder if it wouldn't be better to land after the big refactoring?

[Rick Byers, 2013-06-21 18:18:11]

On 2013/06/21 18:07:05, jdduke wrote:
> On 2013/06/21 17:44:19, Yufeng Shen wrote:
> > Hey Sadrul:  do you have more comments ?
> 
> If at all possible, I'd love it if we could defer this patch until
> crrev.com/15789009 has landed.
> 
> This work seems to add a good amount of additional coupling between
> TouchEventQueue and RenderWidgetHost, which is in part what crrev.com/15789009
> is trying to address. 
> 
> I have no issues with what this patch does, especially as it's behind a flag,
> but I wonder if it wouldn't be better to land after the big refactoring?

That's alright with me - we definitely don't want to make things any harder for
you.

[Yufeng Shen (Slow to review), 2013-08-06 17:39:24]

Now that jdduke@'s refactor has landed, I would like to revive this CL.

And based on what we agreed in the offline meeting, this is
switched to match the ios model that "once scroll starts, stop sending touch
move to renderer; and after scroll stops, send touch end to renderer".

Alex, Rick & Sadrul, PTAL!

[sadrul, 2013-08-06 19:34:19]

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:562:
switches::kNoTouchToRendererWhileScrolling) == "1") {
Can you use a static bool here?
  static bool stop_touch_while_scrolling = CommandLine::...() == "1";
  if (!stop_touch_while_scrolling)
    return;
  ....

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:566:
!no_touch_to_renderer_while_scrolling_) {
ShouldForwardTouchEvent() already implies !no_touch_to_renderer_while_scrolling_

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:567:
no_touch_to_renderer_while_scrolling_ = true;
What happens to the touch-events that may already be in the queue when this
happens?

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.h (right):

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.h:130: void
HandleGestureScroll(
doc

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.cc:197: return
latest_event_;
Why is this necessary?

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/16114003/diff/42001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.h:57: // so we also clear
the queue to not process the ack to the touch end.
I think with this patchset, the sequence of events is:
 1. Browser queues up a bunch of events in the queue.
 2. Renderer doesn't prevent-default the touches.
 3. Browser starts sending gesture-scroll events.
 4. The RWHView implementations directly process the subsequent touch-events to
generate more scroll events.
 5. Scroll ends. At this point, the queued touch-move events upto step 3 are
sent to the RWHView. The touch-event is sent to the renderer.

Is this correct? If so, then this is buggy. The events queued up until step 3
needs to be processed by the RWHV before any of the subsequent touches.

[jdduke (slow), 2013-08-07 00:15:52]

I know this is behind a flag, but it's not really clear to me if the IIR/TEQ are
the best places to insert this logic.  Generally, the input router should queue,
send and/or delegate; it seems like the origin of the scroll gesture and/or
touch events would be a better place for making specific decisions about how to
manage input event ordering.

In any case, the BufferedInputRouter will do away with the TouchEventQueue
entirely; higher level decisions like this will need to live elsewhere.

[sadrul, 2013-08-07 02:30:01]

On 2013/08/07 00:15:52, jdduke wrote:
> I know this is behind a flag, but it's not really clear to me if the IIR/TEQ
are
> the best places to insert this logic.  Generally, the input router should
queue,
> send and/or delegate; it seems like the origin of the scroll gesture and/or
> touch events would be a better place for making specific decisions about how
to
> manage input event ordering.
> 
> In any case, the BufferedInputRouter will do away with the TouchEventQueue
> entirely; higher level decisions like this will need to live elsewhere.

The decision of whether to send touch-events to the renderer at all or not would
still need to be made at the browser, in InputRouter::ShouldForwardTouchEvent().
The implementation in this patchset attempts to update the implementation in
ImmediateInputRouter to make sure touch-move events aren't dispatched when
scrolling starts. I suspect the implementation for BufferedInputRouter will also
need something similar.

[Rick Byers, 2013-08-07 14:01:32]

https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
content/browser/renderer_host/input/immediate_input_router.cc:279:
!no_touch_to_renderer_while_scrolling_;
This will queue the event rather than discard it, right?  That's not what you
want.

https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
content/browser/renderer_host/input/immediate_input_router.cc:559: void
ImmediateInputRouter::HandleGestureScroll(
It does seem unfortunate to have the input router have to understand this stuff
here.

Here's another potential idea (which I haven't fully considered the implications
of): what if we give the gesture recognizer the option to consume touch events
before they're sent anywhere (analogous to how web applications can 'capture'
events).  Then the GR can use it's own state to decide that certain touchmove
events should be handled directly by the GR (generating scroll update events)
rather than being sent anywhere else.

I suspect we may want to do something similar for pinch, so I think it makes
sense to let the GR decide when it will swallow events.

On the down side however, that will probably require duplicate code for Aura and
Android (since we don't share a GR).  So maybe filtering downstream of the GR
somewhere is better to keep the platforms consistent.

[jdduke (slow), 2013-08-07 14:31:01]

On 2013/08/07 14:01:32, Rick Byers wrote:
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> File content/browser/renderer_host/input/immediate_input_router.cc (right):
> 
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> content/browser/renderer_host/input/immediate_input_router.cc:279:
> !no_touch_to_renderer_while_scrolling_;
> This will queue the event rather than discard it, right?  That's not what you
> want.
> 
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> content/browser/renderer_host/input/immediate_input_router.cc:559: void
> ImmediateInputRouter::HandleGestureScroll(
> It does seem unfortunate to have the input router have to understand this
stuff
> here.
> 
> Here's another potential idea (which I haven't fully considered the
implications
> of): what if we give the gesture recognizer the option to consume touch events
> before they're sent anywhere (analogous to how web applications can 'capture'
> events).  Then the GR can use it's own state to decide that certain touchmove
> events should be handled directly by the GR (generating scroll update events)
> rather than being sent anywhere else.
> 
> I suspect we may want to do something similar for pinch, so I think it makes
> sense to let the GR decide when it will swallow events.
> 
> On the down side however, that will probably require duplicate code for Aura
and
> Android (since we don't share a GR).  So maybe filtering downstream of the GR
> somewhere is better to keep the platforms consistent.

Along those lines, we're going to need a sensible, common place to stick
browser-driven flings.  When that time comes, perhaps the GEF (or something like
the GEF) could evolve to provide a home for some of this shared logic?  

That may be too far down the horizon.  Until such time, I'm not sure that this
implementation of deferred touch-down is compatible with Android's GR. That is,
it doesn't expect gesture events to mutate/inject touch events downstream, so we
may have to duplicate this logic for now anyhow.  As long as all of the added
logic stays behind a flag, I suppose I won't complain too loudly.

[Rick Byers, 2013-08-07 15:03:09]

On 2013/08/07 14:31:01, jdduke wrote:
> On 2013/08/07 14:01:32, Rick Byers wrote:
> >
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> > File content/browser/renderer_host/input/immediate_input_router.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> > content/browser/renderer_host/input/immediate_input_router.cc:279:
> > !no_touch_to_renderer_while_scrolling_;
> > This will queue the event rather than discard it, right?  That's not what
you
> > want.
> > 
> >
>
https://chromiumcodereview.appspot.com/16114003/diff/42001/content/browser/re...
> > content/browser/renderer_host/input/immediate_input_router.cc:559: void
> > ImmediateInputRouter::HandleGestureScroll(
> > It does seem unfortunate to have the input router have to understand this
> stuff
> > here.
> > 
> > Here's another potential idea (which I haven't fully considered the
> implications
> > of): what if we give the gesture recognizer the option to consume touch
events
> > before they're sent anywhere (analogous to how web applications can
'capture'
> > events).  Then the GR can use it's own state to decide that certain
touchmove
> > events should be handled directly by the GR (generating scroll update
events)
> > rather than being sent anywhere else.
> > 
> > I suspect we may want to do something similar for pinch, so I think it makes
> > sense to let the GR decide when it will swallow events.
> > 
> > On the down side however, that will probably require duplicate code for Aura
> and
> > Android (since we don't share a GR).  So maybe filtering downstream of the
GR
> > somewhere is better to keep the platforms consistent.
> 
> Along those lines, we're going to need a sensible, common place to stick
> browser-driven flings.  When that time comes, perhaps the GEF (or something
like
> the GEF) could evolve to provide a home for some of this shared logic?  
> 
> That may be too far down the horizon.  Until such time, I'm not sure that this
> implementation of deferred touch-down is compatible with Android's GR. That
is,
> it doesn't expect gesture events to mutate/inject touch events downstream, so
we
> may have to duplicate this logic for now anyhow.  As long as all of the added
> logic stays behind a flag, I suppose I won't complain too loudly.

The plan is not to leave it behind a flag for too long, so we should probably
figure out the right way of doing this now.

[Yufeng Shen (Slow to review), 2013-08-07 21:18:41]

I realized that many of the complication of the previous patch arises from using

ImmediateInputRouter::ShouldForwardTouchEvent()
to decide whether or not sending touch events to renderer,
particularly when stopping sending touch moves to renderer, one has to flush the
TEQ first so these events are in the right
order with upcoming touch move events which will be filtered out at
ShouldForwardTouchEvent().

I now move the decision to TouchEventQueue::ShouldForwardToRenderer(). So if
scrolling is in progress, touch events will be popped from TEQ to views as if
there is no touch handler at renderer.  The logic is much simpler to follow now.

PTAL, thanks.

[sadrul, 2013-08-08 08:46:42]

I think this looks pretty good. I will do a detailed review on Friday (I am out
on Thurs). In the meantime, please add tests.

[Yufeng Shen (Slow to review), 2013-08-08 20:17:00]

tests added.

[Yufeng Shen (Slow to review), 2013-08-13 16:26:24]

Ping.

[sadrul, 2013-08-13 19:40:03]

LGTM

https://codereview.chromium.org/16114003/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/16114003/diff/64001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:563: if
(gesture_event.event.type == WebInputEvent::GestureScrollUpdate)
Please consider using GestureScrollBegin instead of Update.

https://codereview.chromium.org/16114003/diff/64001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/16114003/diff/64001/content/browser/renderer_...
content/browser/renderer_host/input/touch_event_queue.h:94: // is scrolling.
+ "This behaviour is currently enabled only on aura behind a flag."

[Yufeng Shen (Slow to review), 2013-08-13 20:17:27]

offline talk to jddkue@ and he has no objection to this CL.


+ joi@ for OWNER of content/public/common/content_switches.*

https://chromiumcodereview.appspot.com/16114003/diff/64001/content/browser/re...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://chromiumcodereview.appspot.com/16114003/diff/64001/content/browser/re...
content/browser/renderer_host/input/immediate_input_router.cc:563: if
(gesture_event.event.type == WebInputEvent::GestureScrollUpdate)
On 2013/08/13 19:40:04, sadrul wrote:
> Please consider using GestureScrollBegin instead of Update.

Done.

https://chromiumcodereview.appspot.com/16114003/diff/64001/content/browser/re...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://chromiumcodereview.appspot.com/16114003/diff/64001/content/browser/re...
content/browser/renderer_host/input/touch_event_queue.h:94: // is scrolling.
On 2013/08/13 19:40:04, sadrul wrote:
> + "This behaviour is currently enabled only on aura behind a flag."

Done.

[Jói, 2013-08-13 21:54:42]

//content/public/common LGTM.

[commit-bot: I haz the power, 2013-08-13 22:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/16114003/75001

[commit-bot: I haz the power, 2013-08-14 00:20:53]

Retried try job too often on linux_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-14 01:29:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/16114003/75001

[commit-bot: I haz the power, 2013-08-14 02:37:23]

Change committed as 217454

[jam, 2013-08-25 13:06:40]

https://chromiumcodereview.appspot.com/16114003/diff/75001/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/16114003/diff/75001/content/public/com...
content/public/common/content_switches.cc:786: const char
kNoTouchToRendererWhileScrolling[] =
here too

https://chromiumcodereview.appspot.com/16114003/diff/75001/content/public/com...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/16114003/diff/75001/content/public/com...
content/public/common/content_switches.h:227: CONTENT_EXPORT extern const char
kNoTouchToRendererWhileScrolling[];
nit: this file is ordered, please follow that