[VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary

[VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|.

BUG=704732
Review-Url: https://codereview.chromium.org/2869823003
Cr-Original-Commit-Position: refs/heads/master@{#480202}
Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074d5b7f5f65338
Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#481130}
Committed: https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a772fba104e56a

[chongz, 2017-05-08 22:26:34]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 22:29:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-05-08 22:33:13]

Description was changed from

==========
Forward gesture events generated by blocking touch events immediately

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based 
    |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
    kBlocking|.

BUG=704732
==========

[chongz, 2017-05-08 22:37:50]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based 
    |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
    kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based 
    |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
    kBlocking|.

* Alternative approaches for 2. could be:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|.
  b. Set |source_touch_event_dispatch_type| in 
    |WindowEventDispatcher::ProcessedTouchEvent()|.

BUG=704732
==========

[chongz, 2017-05-08 22:38:34]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based 
    |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
    kBlocking|.

* Alternative approaches for 2. could be:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|.
  b. Set |source_touch_event_dispatch_type| in 
    |WindowEventDispatcher::ProcessedTouchEvent()|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2. could be:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.

BUG=704732
==========

[chongz, 2017-05-08 22:43:29]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2. could be:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2.:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|
     * Might be overkill? Acks would always be |INPUT_EVENT_ACK_STATE_IGNORED|
       when |send_touch_events_async_ == true|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.
     * We probably don't want to expose too much logic to
|WindowEventDispatcher|?
BUG=704732
==========

[chongz, 2017-05-08 22:44:44]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2.:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|
     * Might be overkill? Acks would always be |INPUT_EVENT_ACK_STATE_IGNORED|
       when |send_touch_events_async_ == true|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.
     * We probably don't want to expose too much logic to
|WindowEventDispatcher|?
BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2.:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|
     * Might be overkill? We always simulate |INPUT_EVENT_ACK_STATE_IGNORED|
       when |send_touch_events_async_ == true|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.
     * We probably don't want to expose too much logic to
|WindowEventDispatcher|?
BUG=704732
==========

[commit-bot: I haz the power, 2017-05-09 00:21:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 00:21:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chongz, 2017-05-09 14:40:33]

chongz@chromium.org changed reviewers:
+ dtapuska@chromium.org

[chongz, 2017-05-09 14:40:34]

dtapuska@ PTAL, thanks!

[chongz, 2017-05-10 14:36:44]

On 2017/05/09 14:40:34, chongz wrote:
> dtapuska@ PTAL, thanks!

dtapuska@ Can you take a look at this patch? Thanks!

[dtapuska, 2017-05-10 14:49:02]

https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:192:
touch_event_queue_->SendTouchEventsAsync()
I don't think we really should query the state here. But instead use the ack
result of the message to determine if we should send the gesture as blocking or
not.

[chongz, 2017-05-23 22:13:26]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 22:14:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-05-23 22:17:03]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  *2. Set |source_touch_event_dispatch_type| based on
      |TouchEventQueue::send_touch_events_async_|.
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

* Alternative approaches for 2.:
  a. Cache |InputEventAckState| based on |unique_touch_event_id|
     * Might be overkill? We always simulate |INPUT_EVENT_ACK_STATE_IGNORED|
       when |send_touch_events_async_ == true|.
  b. Set |source_touch_event_dispatch_type| in 
     |WindowEventDispatcher::ProcessedTouchEvent()|.
     * We probably don't want to expose too much logic to
|WindowEventDispatcher|?
BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

   event.
    * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
      |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:17:21]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

   event.
    * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
      |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

   event.
     * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
       |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:17:36]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

   event.
     * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
       |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
         |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:26:18]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[chongz, 2017-05-23 22:26:34]

Patchset #2 (id:20001) has been deleted

[chongz, 2017-05-23 22:26:46]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
         |ui::EventResult::ER_UNKNOWN_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
         |ui::EventResult::ER_IGNORED_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[commit-bot: I haz the power, 2017-05-23 22:27:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-05-23 22:27:52]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => 
         |ui::EventResult::ER_IGNORED_NON_BLOCKING|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
                 => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
                 => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:28:08]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
                 => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
                 => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
           => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
           => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:28:28]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
           => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
           => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
             => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
             => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:29:06]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |INPUT_EVENT_ACK_STATE_IGNORED|
             => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
             => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-23 22:29:35]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack result of the touch

     event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[commit-bot: I haz the power, 2017-05-23 22:51:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 22:51:32]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2017-05-23 23:16:09]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 23:20:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-05-23 23:23:51]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-24 01:04:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 01:04:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2017-05-24 13:47:09]

dtapuska@ I've updated CL to use the ack. PTAL again, thanks!

https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_impl.cc:192:
touch_event_queue_->SendTouchEventsAsync()
On 2017/05/10 14:49:00, dtapuska wrote:
> I don't think we really should query the state here. But instead use the ack
> result of the message to determine if we should send the gesture as blocking
or
> not.

Done.

[dtapuska, 2017-05-25 15:40:12]

https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1668: const
ui::EventResult event_result =
Why is this different than aura?

https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1044: case
INPUT_EVENT_ACK_STATE_IGNORED:
I'm not sure we should map Ignored to ignored_non_blocking; I believe the
Renderer can still return IGNORED. I think you really want to map the
SET_NON_BLOCKING/SET_NON_BLOCKING_FLING states don't you?

https://codereview.chromium.org/2869823003/diff/60001/ui/events/event_constan...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/2869823003/diff/60001/ui/events/event_constan...
ui/events/event_constants.h:156: ER_IGNORED_NON_BLOCKING =
The name is slightly confusing because this is a bit set.

sadrul@ might have a comment here.

https://codereview.chromium.org/2869823003/diff/60001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/2869823003/diff/60001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:116: ack_state_ =
(result == ER_UNHANDLED || result == ER_IGNORED_NON_BLOCKING)
I think these are bit sets.. so comparing them directly seems wrong.

[chongz, 2017-05-25 19:29:53]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

Queuing all events could increase |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-05-25 20:25:28]

chongz@chromium.org changed reviewers:
+ sadrul@chromium.org

[chongz, 2017-05-25 20:25:29]

sadrul@ Thanks for the help! Can you take another look at the additional
question below? Thanks!

https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:387:
HandleInputEvent(event_with_callback->event());
sadrul@ Just to recall: During our in-person discussion you suggested that we
should store |InputEventAckState| (same as |EventDisposition|) on renderer for
the Touch event and use on next Gesture event.

However events could switch between blocking and non-blocking in one scroll, and
it seems to me that the following sequence may happen:
1. TouchMove1
2. TouchMove2
3. GestureScroll1 (from TouchMove1)
4. GestureScroll2 (from TouchMove2)

Or is there anything I'm missing?

[chongz, 2017-05-30 22:35:09]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 22:36:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 22:52:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 22:52:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chongz, 2017-05-31 15:33:39]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 15:33:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-05-31 15:34:59]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-31 17:00:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 17:00:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chongz, 2017-05-31 17:57:50]

sadrul@ I'm still concerned about the out-of-order event sequence... 

I've made |set_non_blocking| a separate parameter as you've mentioned last time,
PTAL, thanks!

[sadrul, 2017-06-01 19:57:18]

https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:387:
HandleInputEvent(event_with_callback->event());
On 2017/05/25 20:25:28, chongz wrote:
> sadrul@ Just to recall: During our in-person discussion you suggested that we
> should store |InputEventAckState| (same as |EventDisposition|) on renderer for
> the Touch event and use on next Gesture event.
> 
> However events could switch between blocking and non-blocking in one scroll,
and
> it seems to me that the following sequence may happen:
> 1. TouchMove1
> 2. TouchMove2
> 3. GestureScroll1 (from TouchMove1)
> 4. GestureScroll2 (from TouchMove2)
> 
> Or is there anything I'm missing?

@dtapuska@: Can you confirm if this can happen?

Actually, even if this happens, you still want to release the queue immediately,
right? So shouldn't you just do a PostTask (and from RenderWidgetInputHandler
too, maybe?) to DispatchQueuedInputEvents()?

My feedback here is that sending the relevant information from main-thread to
compositor-thread should happen directly, instead of having to route through all
of browser/aura.

[dtapuska, 2017-06-05 18:11:42]

On 2017/06/01 19:57:18, sadrul wrote:
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> File ui/events/blink/input_handler_proxy.cc (right):
> 
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> ui/events/blink/input_handler_proxy.cc:387:
> HandleInputEvent(event_with_callback->event());
> On 2017/05/25 20:25:28, chongz wrote:
> > sadrul@ Just to recall: During our in-person discussion you suggested that
we
> > should store |InputEventAckState| (same as |EventDisposition|) on renderer
for
> > the Touch event and use on next Gesture event.
> > 
> > However events could switch between blocking and non-blocking in one scroll,
> and
> > it seems to me that the following sequence may happen:
> > 1. TouchMove1
> > 2. TouchMove2
> > 3. GestureScroll1 (from TouchMove1)
> > 4. GestureScroll2 (from TouchMove2)
> > 
> > Or is there anything I'm missing?
> 
> @dtapuska@: Can you confirm if this can happen?
> 
> Actually, even if this happens, you still want to release the queue
immediately,
> right? So shouldn't you just do a PostTask (and from RenderWidgetInputHandler
> too, maybe?) to DispatchQueuedInputEvents()?
> 
> My feedback here is that sending the relevant information from main-thread to
> compositor-thread should happen directly, instead of having to route through
all
> of browser/aura.

Yes that sequence can happen.

The filtering of the event is done on the browser side so the gesture isn't
released from the queue until the ACK is returned for the event as being
unhandled. I'm not sure what dispatching between the main thread and compositor
you are talking about but the compositor won't even have the relevant event
until the ack is returned and processed by the browser.

An alternate implementation of this is to have a table in the renderer
compositor that knows about the dispatch type of previous touch events and maps
those into whether they should be aligned or not. But I'd prefer not having a
table of IDs as that can get complicated to manage and lead to memory leaks.

[sadrul, 2017-06-05 20:48:20]

On 2017/06/05 18:11:42, dtapuska wrote:
> On 2017/06/01 19:57:18, sadrul wrote:
> >
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> > File ui/events/blink/input_handler_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> > ui/events/blink/input_handler_proxy.cc:387:
> > HandleInputEvent(event_with_callback->event());
> > On 2017/05/25 20:25:28, chongz wrote:
> > > sadrul@ Just to recall: During our in-person discussion you suggested that
> we
> > > should store |InputEventAckState| (same as |EventDisposition|) on renderer
> for
> > > the Touch event and use on next Gesture event.
> > > 
> > > However events could switch between blocking and non-blocking in one
scroll,
> > and
> > > it seems to me that the following sequence may happen:
> > > 1. TouchMove1
> > > 2. TouchMove2
> > > 3. GestureScroll1 (from TouchMove1)
> > > 4. GestureScroll2 (from TouchMove2)
> > > 
> > > Or is there anything I'm missing?
> > 
> > @dtapuska@: Can you confirm if this can happen?
> > 
> > Actually, even if this happens, you still want to release the queue
> immediately,
> > right? So shouldn't you just do a PostTask (and from
RenderWidgetInputHandler
> > too, maybe?) to DispatchQueuedInputEvents()?
> > 
> > My feedback here is that sending the relevant information from main-thread
to
> > compositor-thread should happen directly, instead of having to route through
> all
> > of browser/aura.
> 
> Yes that sequence can happen.
> 
> The filtering of the event is done on the browser side so the gesture isn't
> released from the queue until the ACK is returned for the event as being
> unhandled.

Of course, the browser will need to continue to receive the acks. I am not
saying it should not.

> I'm not sure what dispatching between the main thread and compositor
> you are talking about but the compositor won't even have the relevant event
> until the ack is returned and processed by the browser.

The new information being added in ui in this CL (i.e. touch-event non-blocking)
is never set, or used, from ui (aura, browser), as far as I can see. It is set
only when the renderer tells it to set it, and this information is sent back to
the renderer. This is what I was referring to when I said communicating between
main-thread and compositor (but I guess it can be compositor-thread back to
compositor-thread too).

> An alternate implementation of this is to have a table in the renderer
> compositor that knows about the dispatch type of previous touch events and
maps
> those into whether they should be aligned or not. But I'd prefer not having a
> table of IDs as that can get complicated to manage and lead to memory leaks.

I would much rather isolate the complexity to where it is actually needed, and
not spread it across all layers. Having said that, adding a table does sound ...
not very nice.

[sadrul, 2017-06-05 20:51:08]

OK, I looked at the CL, and it does not actually look too bad. Would you mind
updating the CL description please?

[sadrul, 2017-06-05 20:51:53]

On 2017/06/05 20:51:08, sadrul wrote:
> OK, I looked at the CL, and it does not actually look too bad

+ anymore (for context) :)

> . Would you mind
> updating the CL description please?

[chongz, 2017-06-05 21:07:50]

Description was changed from

==========
[VSync Queue] Dispatch gesture events generated by blocking touch events
immediately

Queuing all events could increase |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we want to flush the vsync queue immediately if it
was blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

[chongz, 2017-06-05 21:19:06]

Description was changed from

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we want to flush the vsync queue immediately if it
was blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

This CL:
  1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|.
  2. Set |source_touch_event_dispatch_type| based on the ack of the touch event.
       * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED|
              => |ui::EventResult::ER_IGNORED_NON_BLOCKING|
              => |WebInputEvent::DispatchType::kEventNonBlocking|
  3. Dispatch gesture events immediately if |source_touch_event_dispatch_type ==
     kBlocking|.

BUG=704732
==========

to

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732
==========

[chongz, 2017-06-05 21:33:49]

On 2017/06/05 20:48:20, sadrul wrote:
> On 2017/06/05 18:11:42, dtapuska wrote:
> > On 2017/06/01 19:57:18, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> > > File ui/events/blink/input_handler_proxy.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h...
> > > ui/events/blink/input_handler_proxy.cc:387:
> > > HandleInputEvent(event_with_callback->event());
> > > On 2017/05/25 20:25:28, chongz wrote:
> > > > sadrul@ Just to recall: During our in-person discussion you suggested
that
> > we
> > > > should store |InputEventAckState| (same as |EventDisposition|) on
renderer
> > for
> > > > the Touch event and use on next Gesture event.
> > > > 
> > > > However events could switch between blocking and non-blocking in one
> scroll,
> > > and
> > > > it seems to me that the following sequence may happen:
> > > > 1. TouchMove1
> > > > 2. TouchMove2
> > > > 3. GestureScroll1 (from TouchMove1)
> > > > 4. GestureScroll2 (from TouchMove2)
> > > > 
> > > > Or is there anything I'm missing?
> > > 
> > > @dtapuska@: Can you confirm if this can happen?
> > > 
> > > Actually, even if this happens, you still want to release the queue
> > immediately,
> > > right? So shouldn't you just do a PostTask (and from
> RenderWidgetInputHandler
> > > too, maybe?) to DispatchQueuedInputEvents()?
> > > 
> > > My feedback here is that sending the relevant information from main-thread
> to
> > > compositor-thread should happen directly, instead of having to route
through
> > all
> > > of browser/aura.
> > 
> > Yes that sequence can happen.
> > 
> > The filtering of the event is done on the browser side so the gesture isn't
> > released from the queue until the ACK is returned for the event as being
> > unhandled.
> 
> Of course, the browser will need to continue to receive the acks. I am not
> saying it should not.
> 
> > I'm not sure what dispatching between the main thread and compositor
> > you are talking about but the compositor won't even have the relevant event
> > until the ack is returned and processed by the browser.
> 
> The new information being added in ui in this CL (i.e. touch-event
non-blocking)
> is never set, or used, from ui (aura, browser), as far as I can see. It is set
> only when the renderer tells it to set it, and this information is sent back
to
> the renderer. This is what I was referring to when I said communicating
between
> main-thread and compositor (but I guess it can be compositor-thread back to
> compositor-thread too).
> 
> > An alternate implementation of this is to have a table in the renderer
> > compositor that knows about the dispatch type of previous touch events and
> maps
> > those into whether they should be aligned or not. But I'd prefer not having
a
> > table of IDs as that can get complicated to manage and lead to memory leaks.
> 
> I would much rather isolate the complexity to where it is actually needed, and
> not spread it across all layers. Having said that, adding a table does sound
...
> not very nice.

I'm not sure if I fully understand your suggestion: Do you prefer a table
[touch_id -> ack] on the renderer side (so it won't be exposed to the browser),
or are you ok with the current approach?
Or actually you'd like something different than these two?

On 2017/06/05 20:51:53, sadrul wrote:
> On 2017/06/05 20:51:08, sadrul wrote:
> > OK, I looked at the CL, and it does not actually look too bad
> 
> + anymore (for context) :)
> 
> > . Would you mind
> > updating the CL description please?

Done. Updated CL description to put more focus on event passing.

[sadrul, 2017-06-05 21:37:07]

> I'm not sure if I fully understand your suggestion: Do you prefer a table
> [touch_id -> ack] on the renderer side (so it won't be exposed to the
browser),
> or are you ok with the current approach?
> Or actually you'd like something different than these two?

Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable. I
will look at it more closely later tonight.

[chongz, 2017-06-07 17:41:59]

On 2017/06/05 21:37:07, sadrul wrote:
> > I'm not sure if I fully understand your suggestion: Do you prefer a table
> > [touch_id -> ack] on the renderer side (so it won't be exposed to the
> browser),
> > or are you ok with the current approach?
> > Or actually you'd like something different than these two?
> 
> Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable. I
> will look at it more closely later tonight.

Gentle ping.

[sadrul, 2017-06-08 20:31:38]

On 2017/06/07 17:41:59, chongz wrote:
> On 2017/06/05 21:37:07, sadrul wrote:
> > > I'm not sure if I fully understand your suggestion: Do you prefer a table
> > > [touch_id -> ack] on the renderer side (so it won't be exposed to the
> > browser),
> > > or are you ok with the current approach?
> > > Or actually you'd like something different than these two?
> > 
> > Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable.
I
> > will look at it more closely later tonight.
> 
> Gentle ping.

Sorry about the review delay :(

The non-blink code lgtm

[sadrul, 2017-06-08 20:31:48]

lgtm

https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
File content/browser/renderer_host/ui_events_helper.h (right):

https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
content/browser/renderer_host/ui_events_helper.h:41: bool
InputEventAckStateIsSetNonBlocking(InputEventAckState);
Document.

https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto& gesture
: gestures_.container())
Add {}

[chongz, 2017-06-09 14:51:44]

dtapuska@ PTAL again, thanks!

https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
File content/browser/renderer_host/ui_events_helper.h (right):

https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
content/browser/renderer_host/ui_events_helper.h:41: bool
InputEventAckStateIsSetNonBlocking(InputEventAckState);
On 2017/06/08 20:31:48, sadrul wrote:
> Document.

Done.

https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto& gesture
: gestures_.container())
On 2017/06/08 20:31:48, sadrul wrote:
> Add {}

Done.

[dtapuska, 2017-06-09 15:07:40]

On 2017/06/09 14:51:44, chongz wrote:
> dtapuska@ PTAL again, thanks!
> 
>
https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
> File content/browser/renderer_host/ui_events_helper.h (right):
> 
>
https://codereview.chromium.org/2869823003/diff/100001/content/browser/render...
> content/browser/renderer_host/ui_events_helper.h:41: bool
> InputEventAckStateIsSetNonBlocking(InputEventAckState);
> On 2017/06/08 20:31:48, sadrul wrote:
> > Document.
> 
> Done.
> 
>
https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
> File ui/events/gesture_detection/gesture_event_data_packet.cc (right):
> 
>
https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete...
> ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto&
gesture
> : gestures_.container())
> On 2017/06/08 20:31:48, sadrul wrote:
> > Add {}
> 
> Done.

lgtm

[chongz, 2017-06-09 15:27:42]

chongz@chromium.org changed reviewers:
+ pfeldman@chromium.org

[chongz, 2017-06-09 15:27:43]

pfeldman@ PTAL at "content/browser/BUILD.gn" and 2 "third_party/WebKit/" files,
thanks!

[pfeldman, 2017-06-09 18:38:08]

https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
Are you relying upon all the call sites to initialize these? Looks like an
anti-pattern to me.

[chongz, 2017-06-09 18:58:58]

pfeldman@ I've updated as per your comments, PTAL again, thanks!

https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
On 2017/06/09 18:38:08, pfeldman wrote:
> Are you relying upon all the call sites to initialize these? Looks like an
> anti-pattern to me.

Done. Thanks for catching! Yes it would be safer to initialize it in ctors.

[chongz, 2017-06-14 14:06:08]

On 2017/06/09 18:58:58, chongz wrote:
> pfeldman@ I've updated as per your comments, PTAL again, thanks!
> 
>
https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/WebGestureEvent.h (right):
> 
>
https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
> is_source_touch_event_set_non_blocking;
> On 2017/06/09 18:38:08, pfeldman wrote:
> > Are you relying upon all the call sites to initialize these? Looks like an
> > anti-pattern to me.
> 
> Done. Thanks for catching! Yes it would be safer to initialize it in ctors.

pfeldman@ Can you take a look again? Thanks!

[pfeldman, 2017-06-15 18:56:42]

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking =
false;

[dtapuska, 2017-06-15 19:18:30]

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
On 2017/06/15 18:56:42, pfeldman wrote:
> lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking =
> false;

Actually the initializer false, isn't needed here... This struct is actually
memset(0) in the WebInputEvent ctor.. So chong you can remove the
is_source_touch_event_set_non_blocking in the ctors...

[chongz, 2017-06-15 19:37:37]

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
On 2017/06/15 19:18:29, dtapuska wrote:
> On 2017/06/15 18:56:42, pfeldman wrote:
> > lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking =
> > false;
> 
> Actually the initializer false, isn't needed here... This struct is actually
> memset(0) in the WebInputEvent ctor.. So chong you can remove the
> is_source_touch_event_set_non_blocking in the ctors...

Removed. Confirmed with dtapuska offline and the TODO about removing |memset|
won't affect decision here.

[chongz, 2017-06-16 16:04:00]

The CQ bit was checked by chongz@chromium.org

[chongz, 2017-06-16 16:04:00]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
dtapuska@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2869823003/#ps160001
(title: "dtapuska's comment: Remove |is_source_touch_event_set_non_blocking|
from ctor")

[commit-bot: I haz the power, 2017-06-16 16:04:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 17:39:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 17:39:44]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2017-06-16 20:05:42]

The CQ bit was checked by chongz@chromium.org

[chongz, 2017-06-16 20:05:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, sadrul@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2869823003/#ps180001
(title: "Remove empty test "stylus-generated-tap.html", added by mistake")

[commit-bot: I haz the power, 2017-06-16 20:06:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 22:37:37]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497643542288960,
"parent_rev": "8b52edff46258108f273b22906c9e800e7787024", "commit_rev":
"80553a96dc6712a24a7d0a3e3074d5b7f5f65338"}

[commit-bot: I haz the power, 2017-06-16 22:37:53]

Description was changed from

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732
==========

to

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732

Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#480202}
Committed:
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
==========

[commit-bot: I haz the power, 2017-06-16 22:37:56]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...

[findit-for-me, 2017-06-17 04:07:22]

Findit (https://goo.gl/kROfz5) identified this CL at revision 480202 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Lei Zhang, 2017-06-17 06:00:45]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2017-06-17 06:00:46]

Probably should have initialized |is_source_touch_event_set_non_blocking_|
normally rather than relying on some other code to memset() the struct. Are all
the users going to memset() the struct? Looks like no because the MSAN bots are
unhappy.

[dcheng, 2017-06-17 07:36:41]

A revert of this CL (patchset #7 id:180001) has been created in
https://codereview.chromium.org/2943883002/ by dcheng@chromium.org.

The reason for reverting is: Likely causing failures on Linux ChromiumOS MSan
Tests..

[chongz, 2017-06-19 20:15:10]

Description was changed from

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732

Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#480202}
Committed:
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
==========

to

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732

Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#480202}
Committed:
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
==========

[chongz, 2017-06-19 20:16:57]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 20:17:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2017-06-19 20:24:40]

dtapuska@ PTAL again, thanks!

It turns out that |WebGestureEvent| was initialized correctly, but I was then
setting its value from an un-initialized |GestureEventDetail|.

https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebGestureEvent.h (right):

https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
is_source_touch_event_set_non_blocking;
Initialize |is_source_touch_event_set_non_blocking| here Won't fix the issue.

https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.cc:631:
details.is_source_touch_event_set_non_blocking();
Remove this code Will fix the issue.

https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even...
File ui/events/gesture_event_details.h (right):

https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even...
ui/events/gesture_event_details.h:237: bool
is_source_touch_event_set_non_blocking_ = false;
|GestureEventDetails.is_source_touch_event_set_non_blocking_ = false| is the
actual fix.

[commit-bot: I haz the power, 2017-06-19 23:06:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 23:06:15]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-06-20 21:04:11]

On 2017/06/19 20:24:40, chongz wrote:
> dtapuska@ PTAL again, thanks!
> 
> It turns out that |WebGestureEvent| was initialized correctly, but I was then
> setting its value from an un-initialized |GestureEventDetail|.
> 
>
https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/WebGestureEvent.h (right):
> 
>
https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebGestureEvent.h:40: bool
> is_source_touch_event_set_non_blocking;
> Initialize |is_source_touch_event_set_non_blocking| here Won't fix the issue.
> 
>
https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_...
> File ui/events/blink/blink_event_util.cc (right):
> 
>
https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_...
> ui/events/blink/blink_event_util.cc:631:
> details.is_source_touch_event_set_non_blocking();
> Remove this code Will fix the issue.
> 
>
https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even...
> File ui/events/gesture_event_details.h (right):
> 
>
https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even...
> ui/events/gesture_event_details.h:237: bool
> is_source_touch_event_set_non_blocking_ = false;
> |GestureEventDetails.is_source_touch_event_set_non_blocking_ = false| is the
> actual fix.

lgtm, you should be able to reland this.

[chongz, 2017-06-21 03:52:48]

The CQ bit was checked by chongz@chromium.org

[chongz, 2017-06-21 03:52:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2869823003/#ps200001
(title: "Fix MSAN Use-of-uninitialized-value: Initialize GestureEventDetails")

[commit-bot: I haz the power, 2017-06-21 03:52:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 07:04:37]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498017168325540,
"parent_rev": "b8247ff416deaf64d757907aabe6d9931a444fed", "commit_rev":
"44f78150b462c4c9daaba7ae47a772fba104e56a"}

[commit-bot: I haz the power, 2017-06-21 07:04:54]

Description was changed from

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732

Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#480202}
Committed:
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
==========

to

==========
[VSync Queue] Plug touch ack to gesture events and flush vsync queue if
necessary

For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.

This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.

After CL the process would be:
  1. Browser -> Renderer: Send touch
  2. Renderer -> Browser: Touch Ack
  3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack
was 
    |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
  4. Browser -> Renderer: Send generated gesture events with 
    |is_source_touch_event_set_non_blocking| bit
  5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking ==
true|.

BUG=704732

Review-Url: https://codereview.chromium.org/2869823003
Cr-Original-Commit-Position: refs/heads/master@{#480202}
Committed:
https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#481130}
Committed:
https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a7...
==========

[commit-bot: I haz the power, 2017-06-21 07:04:59]

Committed patchset #8 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a7...

[vabr (Chromium), 2017-06-21 07:40:48]

The builder
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu...
fails with:

FAILED: gesture_detection.dll gesture_detection.dll.lib
gesture_detection.dll.pdb 
C:/b/depot_tools/python276_bin/python.exe
../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
link.exe /nologo /IMPLIB:./gesture_detection.dll.lib /DLL
/OUT:./gesture_detection.dll /PDB:./gesture_detection.dll.pdb
@./gesture_detection.dll.rsp
LINK : fatal error LNK1104: cannot open file 'gfx_switches.dll.lib'

Suspecting that it is related to this CL.

Today's sheriff.

[vabr (Chromium), 2017-06-21 07:43:31]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/2953563002/ by vabr@chromium.org.

The reason for reverting is: Speculative revert.

The builder
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu...
fails with:

FAILED: gesture_detection.dll gesture_detection.dll.lib
gesture_detection.dll.pdb 
C:/b/depot_tools/python276_bin/python.exe
../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
link.exe /nologo /IMPLIB:./gesture_detection.dll.lib /DLL
/OUT:./gesture_detection.dll /PDB:./gesture_detection.dll.pdb
@./gesture_detection.dll.rsp
LINK : fatal error LNK1104: cannot open file 'gfx_switches.dll.lib'

The reverted CL touches code with gestures and gfx.

Today's sheriff..