Refactor compositor event handling path to be callback-based

Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)

2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)

3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpHBo/edit?usp=sharing

BUG=637393
Committed: https://crrev.com/78f276e96eac5a447149fafac0b4953eb5936979
Cr-Commit-Position: refs/heads/master@{#417788}

[chongz, 2016-08-22 23:16:51]

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

[commit-bot: I haz the power, 2016-08-22 23:17:20]

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, 2016-08-23 01:36:21]

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

[commit-bot: I haz the power, 2016-08-23 01:36:23]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-08-23 04:21:59]

Description was changed from

==========
Changed event path to callback-based

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where the
events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

BUG=637393
==========

[chongz, 2016-08-23 04:22:35]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where the
events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where
eventually events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

BUG=637393
==========

[chongz, 2016-08-23 04:35:23]

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

[chongz, 2016-08-23 04:35:24]

dtapuska@ PTAL, thanks!

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_event_filter.h (left):

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:127:
std::unique_ptr<ui::DidOverscrollParams>* current_overscroll_params_;
|DidOverscrollParams| handling moved to |InputHandlerProxy|.

https://codereview.chromium.org/2265393002/diff/1/content/renderer/mus/compos...
File content/renderer/mus/compositor_mus_connection.cc (right):

https://codereview.chromium.org/2265393002/diff/1/content/renderer/mus/compos...
content/renderer/mus/compositor_mus_connection.cc:131: std::move(*ack_callback);
Original implementation only takes ownership if we need to send ack, but with
the callback-based implementation we don't have enough information in advance.

https://codereview.chromium.org/2265393002/diff/1/content/renderer/mus/compos...
File content/renderer/mus/compositor_mus_connection_unittest.cc (right):

https://codereview.chromium.org/2265393002/diff/1/content/renderer/mus/compos...
content/renderer/mus/compositor_mus_connection_unittest.cc:443:
EXPECT_EQ(EventResult::UNHANDLED, test_callback->result());
Changed behavior since |CompositorMusConnection::OnWindowInputEvent| has to
always take ownership, but it can pass back |EventResult::UNHANDLED| in these
case.

https://codereview.chromium.org/2265393002/diff/1/ui/events/blink/input_handl...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2265393002/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy.cc:287:
std::move(current_overscroll_params_));
Handles event immediately.

https://codereview.chromium.org/2265393002/diff/1/ui/events/blink/input_handl...
ui/events/blink/input_handler_proxy.cc:1165: bundle_ack_with_triggering_event);
Have to pass raw data instead of |DidOverscrollParams| for
|InputHandlerProxyTest|.

[dtapuska, 2016-08-23 19:34:10]

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

[dtapuska, 2016-08-23 19:34:11]

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_manager.h:91: void DidOverscroll(int
routing_id,
I find this DidOverscroll vs DidHandleInputEvent confusing. I wonder if the
callback should be something like a progress callback.

If there is an event associated with the callback then an InputEventAck will be
sent. If there is no associated event; then send the InputHostMsg_DidOverscroll.

I know you researched whether we could just do a direct class callback and that
the problem was that mus wanted to be an alternate handler for input messages.
Can MUS and Chrome IPC be used at the same time? If not I wonder if this could
just be setting the client interface to a new handle.

tdresser@ thoughts; callbacks seem to generate spaghetti code callgraphs to me.

[chongz, 2016-08-23 23:38:06]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where
eventually events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where
eventually events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

[chongz, 2016-08-23 23:40:34]

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2265393002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_handler_manager.h:91: void DidOverscroll(int
routing_id,
On 2016/08/23 19:34:10, dtapuska wrote:
> I find this DidOverscroll vs DidHandleInputEvent confusing. I wonder if the
> callback should be something like a progress callback.
> 
> If there is an event associated with the callback then an InputEventAck will
be
> sent. If there is no associated event; then send the
InputHostMsg_DidOverscroll.
> 
> I know you researched whether we could just do a direct class callback and
that
> the problem was that mus wanted to be an alternate handler for input messages.
> Can MUS and Chrome IPC be used at the same time? If not I wonder if this could
> just be setting the client interface to a new handle.
> 
> tdresser@ thoughts; callbacks seem to generate spaghetti code callgraphs to
me.

I've also updated the design doc (@chromium) which discussed some tradeoffs for
the approach.
PTAL, thanks!
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

[chongz, 2016-08-29 22:19:08]

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

[commit-bot: I haz the power, 2016-08-29 22:49:06]

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, 2016-08-30 03:25:03]

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

[commit-bot: I haz the power, 2016-08-30 03:25:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[chongz, 2016-08-30 13:38:26]

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

[chongz, 2016-08-30 13:38:36]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-30 13:38:42]

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, 2016-08-30 13:40:36]

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

[commit-bot: I haz the power, 2016-08-30 13:40:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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, 2016-08-30 15:05:09]

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

[chongz, 2016-08-30 15:05:19]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-08-30 15:05:25]

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, 2016-08-30 15:58:41]

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

[commit-bot: I haz the power, 2016-08-30 15:58:43]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-08-30 16:27:34]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

This CL is the pre-work for compositor VSync aligned input, where
eventually events will be queued and handled async.

Currently the events are still handled synchronously even through we 
are using callback. Following up CLs will actually queue events in
|InputHandlerProxy| and dispatch async.

Note:
This CL also affects MUS and |DidOverscrollParams| since they are
related to the event path.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                     |DidForwardToHandlerAndOverscroll|
                V                                                 ^
    |InputHandlerManager|            |DidHandleInputEventAndOverscroll|
               V                                                  ^
    |InputHandlerProxy|            ->      (handle event)


2. |CompositorMusConnection|      |DidHandle...AndOverscroll|
                V                                                  ^
    |InputHandlerManager|               |DidHandleInputEventAndOverscroll|
                V                                                  ^
    |InputHandlerProxy|            ->      (handle event)  


3. (other sources)                Wrapper->Manager->Filter::DidOverscroll
                V                                                  ^
    |InputHandlerProxy|           ->       (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

[chongz, 2016-08-30 16:31:24]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                     |DidForwardToHandlerAndOverscroll|
                V                                                 ^
    |InputHandlerManager|            |DidHandleInputEventAndOverscroll|
               V                                                  ^
    |InputHandlerProxy|            ->      (handle event)


2. |CompositorMusConnection|      |DidHandle...AndOverscroll|
                V                                                  ^
    |InputHandlerManager|               |DidHandleInputEventAndOverscroll|
                V                                                  ^
    |InputHandlerProxy|            ->      (handle event)  


3. (other sources)                Wrapper->Manager->Filter::DidOverscroll
                V                                                  ^
    |InputHandlerProxy|           ->       (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)                Wrapper->Manager->Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

[chongz, 2016-08-30 16:32:34]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)                Wrapper->Manager->Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

[chongz, 2016-08-30 16:33:05]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as |CompositorMusConnection|
already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

[chongz, 2016-08-30 17:20:22]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...
==========

[chongz, 2016-08-30 17:28:38]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

[chongz, 2016-08-30 17:58:03]

dtapuska@ I've made a compromise solution to our last discussion and updated CL
description, PTAL, thanks!

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:95: void DidOverscroll(int
routing_id, const ui::DidOverscrollParams& params);
I cannot fully merge |DidOverscroll| and |DidHandleInputEvent| into a single
callback as other overscroll sources require the client interface.

A compromise solution is to rename |DidHandleInputEvent| =>
|DidHandleInputEventAndOverscroll|, such that:
  1. All InputEvent triggered overscroll will go into
|DidHandleInputEventAndOverscroll|
  2. Other sources of overscroll with go into |DidOverscroll|

Please see CL description for more detailed event path.

[dtapuska, 2016-08-30 18:33:50]

seems reasonable to me.

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_manager.cc:224: base::Unretained(this),
callback));
What is the life cycle here?

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_manager_client.h (right):

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:44: // InputEvent
associated overscroll will be returned along with the callback.
Likely should reference what callback you are indicating here that it it is part
of the HandleInputEvent method args.

[chongz, 2016-08-30 19:36:42]

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

[commit-bot: I haz the power, 2016-08-30 19:37:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-08-30 19:47:04]

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

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_manager.cc:224: base::Unretained(this),
callback));
On 2016/08/30 18:33:50, dtapuska wrote:
> What is the life cycle here?

According to my knowledge |InputHandlerManager| was created and destroyed along
with |RenderThreadImpl|, and |InputHandlerManager| is managing
|InputHandlerWrapper|s.

But just for safety I've changed it to WeakPtr.

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
File content/renderer/input/input_handler_manager_client.h (right):

https://codereview.chromium.org/2265393002/diff/60001/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:44: // InputEvent
associated overscroll will be returned along with the callback.
On 2016/08/30 18:33:50, dtapuska wrote:
> Likely should reference what callback you are indicating here that it it is
part
> of the HandleInputEvent method args.

Done.

[commit-bot: I haz the power, 2016-08-30 20:41:39]

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

[commit-bot: I haz the power, 2016-08-30 20:41:40]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-08-31 14:56:47]

tdresser@ PTAL, thanks!

[tdresser, 2016-09-01 16:58:45]

This LGTM to me at a high level, but wait for Dave's more thorough review.

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:129: using EventDisposition =
ui::InputHandlerProxy::EventDisposition;
I'm not sure it's worth this "using" for two uses.

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
File content/renderer/mus/compositor_mus_connection.cc (right):

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
content/renderer/mus/compositor_mus_connection.cc:132: ui::ScopedWebInputEvent
web_event = ui::WebInputEventTraits::Clone(
Why do we need to clone the event now?

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
content/renderer/mus/compositor_mus_connection.cc:180: base::Passed(&web_event),
ack));
It isn't clear to me why this changed from std::move.

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

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:64: const bool
kBundleAckWithTriggeringEvent = true;
The point of this bool is just to make this easy to revert, right? It seems like
it's adding some complexity, and I'm not sure it's worth it.

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:1114: bool
bundle_ack_with_triggering_event) {
Can we tweak this naming? bundle_overscroll_params_with_ack perhaps?

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy_client.h (right):

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy_client.h:35: // Overscroll triggered by
|HandleInputEvent/WithLatencyInfo| will response to
This sentence doesn't quite make sense.

"|HandleInputEvent/WithLatencyInfo| will respond to overscroll by calling the
passed in callback." perhaps?

[chongz, 2016-09-01 20:09:10]

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

[commit-bot: I haz the power, 2016-09-01 20:09:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-09-01 20:13:13]

dtapuska@ PTAL, thanks!

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:129: using EventDisposition =
ui::InputHandlerProxy::EventDisposition;
On 2016/09/01 16:58:45, tdresser wrote:
> I'm not sure it's worth this "using" for two uses.

Removed.

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
File content/renderer/mus/compositor_mus_connection.cc (right):

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
content/renderer/mus/compositor_mus_connection.cc:132: ui::ScopedWebInputEvent
web_event = ui::WebInputEventTraits::Clone(
On 2016/09/01 16:58:45, tdresser wrote:
> Why do we need to clone the event now?

Sorry I misused Clone. Fixed.

https://codereview.chromium.org/2265393002/diff/80001/content/renderer/mus/co...
content/renderer/mus/compositor_mus_connection.cc:180: base::Passed(&web_event),
ack));
On 2016/09/01 16:58:45, tdresser wrote:
> It isn't clear to me why this changed from std::move.

Changed back to |std::move|.

There is no actual difference. I'm just applying the syntactic sugar introduced
in "bind_helpers.h:433" and to keep consistence with my new code:
  141           this, base::Passed(&callback)));

See "bind_helpers.h:433":
https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=base::passed&sq=pa...

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

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:64: const bool
kBundleAckWithTriggeringEvent = true;
On 2016/09/01 16:58:45, tdresser wrote:
> The point of this bool is just to make this easy to revert, right? It seems
like
> it's adding some complexity, and I'm not sure it's worth it.

Removed.

I was just trying to avoid passing raw 'true'/'false'. I thought I read it from
code style but I might be wrong.

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:1114: bool
bundle_ack_with_triggering_event) {
On 2016/09/01 16:58:45, tdresser wrote:
> Can we tweak this naming? bundle_overscroll_params_with_ack perhaps? 

Done.

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
File ui/events/blink/input_handler_proxy_client.h (right):

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy_client.h:35: // Overscroll triggered by
|HandleInputEvent/WithLatencyInfo| will response to
On 2016/09/01 16:58:45, tdresser wrote:
> This sentence doesn't quite make sense.
> 
> "|HandleInputEvent/WithLatencyInfo| will respond to overscroll by calling the
> passed in callback." perhaps?

Done.

[commit-bot: I haz the power, 2016-09-01 21:07:50]

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

[commit-bot: I haz the power, 2016-09-01 21:07:51]

Dry run: This issue passed the CQ dry run.

[tdresser, 2016-09-01 21:08:11]

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

https://codereview.chromium.org/2265393002/diff/80001/ui/events/blink/input_h...
ui/events/blink/input_handler_proxy.cc:64: const bool
kBundleAckWithTriggeringEvent = true;
On 2016/09/01 20:13:13, chongz wrote:
> On 2016/09/01 16:58:45, tdresser wrote:
> > The point of this bool is just to make this easy to revert, right? It seems
> like
> > it's adding some complexity, and I'm not sure it's worth it.
> 
> Removed.
> 
> I was just trying to avoid passing raw 'true'/'false'. I thought I read it
from
> code style but I might be wrong.

Yup, that is general good practice. I'd tend to do it by making the parameter a
two valued enum type though.

[dtapuska, 2016-09-02 15:20:52]

lgtm

[chongz, 2016-09-02 15:34:23]

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

[chongz, 2016-09-02 15:34:24]

ben@ PTAL, thanks!

[chongz, 2016-09-07 17:16:21]

ben@ can you take a look at this CL please? Thanks!

[chongz, 2016-09-09 17:25:07]

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

[chongz, 2016-09-09 17:25:08]

creis@ PTAL, thanks! (Seems Ben is not available)

[Charlie Reis, 2016-09-09 20:57:57]

RS LGTM.

https://codereview.chromium.org/2265393002/diff/100001/content/renderer/mus/c...
File content/renderer/mus/compositor_mus_connection.h (right):

https://codereview.chromium.org/2265393002/diff/100001/content/renderer/mus/c...
content/renderer/mus/compositor_mus_connection.h:95:
std::unique_ptr<ui::DidOverscrollParams>);
nit: Please name parameters.

[Ben Goodger (Google), 2016-09-09 21:08:21]

Sorry I didn't see this until now (sometimes I aggressively archive too much +
vacation). Please feel free to ping me via IM if I don't get back to you within
24hrs.

[chongz, 2016-09-10 00:03:04]

https://codereview.chromium.org/2265393002/diff/100001/content/renderer/mus/c...
File content/renderer/mus/compositor_mus_connection.h (right):

https://codereview.chromium.org/2265393002/diff/100001/content/renderer/mus/c...
content/renderer/mus/compositor_mus_connection.h:95:
std::unique_ptr<ui::DidOverscrollParams>);
On 2016/09/09 20:57:57, Charlie Reis (slow) wrote:
> nit: Please name parameters.

Done.

[chongz, 2016-09-10 00:03:13]

The CQ bit was checked by chongz@chromium.org

[chongz, 2016-09-10 00:03:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, creis@chromium.org, dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2265393002/#ps120001
(title: "creis's review, rebase")

[commit-bot: I haz the power, 2016-09-10 00:03:59]

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, 2016-09-10 01:09:18]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

[commit-bot: I haz the power, 2016-09-10 01:09:20]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-09-10 01:11:37]

Description was changed from

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)


2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)  


3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393
==========

to

==========
Refactor compositor event handling path to be callback-based

(A pre-work for compositor VSync aligned input.)

This CL adds |DidHandleInputEventAndOverscroll| method, and will
be passed to |HandleInputEvent| as callback.
(It's hard to use interface-based implementation as
|CompositorMusConnection| already has callback-based method
baked in.)

New event path:
1. |InputEventFilter|                |DidForwardToHandlerAndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)

2. |CompositorMusConnection|         |DidHandle...AndOverscroll|
          V                                        ^
   |InputHandlerManager|             |DidHandleInputEventAndOverscroll|
          V                                        ^
   |InputHandlerProxy|        ->      (handle event)

3. (other sources)           ..Wrapper->..Manager->..Filter::DidOverscroll
          V                                        ^
   |InputHandlerProxy|        ->      (did overscroll)

Note about overscroll:
Only one of |DidOverscroll| or |DidHandleInputEventAndOverscroll|
will be fired, and it's decided by whether it's caused by an InputEvent.

Note about event queue:
Events are still handled synchronously in |InputHandlerProxy|, following
up CLs will support event queue.

Design Doc: (@chromium.org)
https://docs.google.com/a/chromium.org/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1...

BUG=637393

Committed: https://crrev.com/78f276e96eac5a447149fafac0b4953eb5936979
Cr-Commit-Position: refs/heads/master@{#417788}
==========

[commit-bot: I haz the power, 2016-09-10 01:11:39]

Patchset 5 (id:??) landed as
https://crrev.com/78f276e96eac5a447149fafac0b4953eb5936979
Cr-Commit-Position: refs/heads/master@{#417788}