sync compositor: Merge input path with chrome

sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628
Committed: https://crrev.com/de5b75b1ccd0ef7f478318d805b3ba1c8ebf7130
Cr-Commit-Position: refs/heads/master@{#380563}

[boliu, 2016-02-26 00:39:01]

Description was changed from

==========
[NOT FOR COMMIT] hack to unify input path with chrome
==========

to

==========
[NOT FOR COMMIT] hack to unify input path with chrome

BUG=545628
==========

[boliu, 2016-02-27 20:25:49]

Description was changed from

==========
[NOT FOR COMMIT] hack to unify input path with chrome

BUG=545628
==========

to

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, and everything just works.

BUG=545628
==========

[boliu, 2016-02-27 23:02:05]

Description was changed from

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, and everything just works.

BUG=545628
==========

to

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

BUG=545628
==========

[boliu, 2016-02-27 23:06:22]

Description was changed from

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

BUG=545628
==========

to

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628
==========

[boliu, 2016-02-29 15:55:45]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org, tdresser@chromium.org

[boliu, 2016-02-29 15:55:46]

PTAL

tdresser: content/renderer/input
sievers: everything else

[tdresser, 2016-02-29 16:42:48]

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

[tdresser, 2016-02-29 16:42:49]

aelias@ would be a better reviewer for this change - I'm not very familiar with
WebView logic.

"sync compositor can use SynchronousInputHandlerProxy even without routing input
synchronously". I'm finding the naming in this patch pretty hard to follow,
though that may primarily be lack of context.

It seems incorrect to me that we have a SynchronousInputHandlerProxy that
doesn't route input synchronously.

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:308: //
Intentional No-op.
Comment why this should be a no-op.

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:95: void
DidOverscrollInProc(const DidOverscrollParams& params);
Can we de-abbreviate this?

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

https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:63: class CONTENT_EXPORT
InputHandlerManagerSyncHandlerProxyClient {
This name is hard to parse.

Isn't the second "Handler" redundant?
Would something like:

SyncInputHandlerManagerProxyClient 

mean the same thing?

[boliu, 2016-02-29 18:58:53]

No new patch set.

On 2016/02/29 16:42:49, tdresser wrote:
> aelias@ would be a better reviewer for this change - I'm not very familiar
with
> WebView logic.
> 
> "sync compositor can use SynchronousInputHandlerProxy even without routing
input
> synchronously". I'm finding the naming in this patch pretty hard to follow,
> though that may primarily be lack of context.
> 
> It seems incorrect to me that we have a SynchronousInputHandlerProxy that
> doesn't route input synchronously.

Perhaps SynchronousInputHandlerProxy is badly named then. It has nothing to do
with delivering input events; it's used by webview to control root layer scroll,
and animate flings. This is additional functionality on top of the regular
"InputHanlderProxy", and it doesn't really matter whether the functions of
InputHandlerProxy is synchronous or not.

Note though event ordering can't change, which still holds

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

https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:63: class CONTENT_EXPORT
InputHandlerManagerSyncHandlerProxyClient {
On 2016/02/29 16:42:49, tdresser wrote:
> This name is hard to parse.
> 
> Isn't the second "Handler" redundant?
> Would something like:
> 
> SyncInputHandlerManagerProxyClient 
> 
> mean the same thing?

Yeah I'm terribe with names. How about just SynchronousInputHandlerProxyClient?
To match SynchronousInputHandlerProxy

[tdresser, 2016-02-29 19:10:25]

Thanks for the clarification. I'm not thrilled with
SynchronousInputHandlerProxy, but it's probably fine to leave it.

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

https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:63: class CONTENT_EXPORT
InputHandlerManagerSyncHandlerProxyClient {
On 2016/02/29 18:58:53, boliu wrote:
> On 2016/02/29 16:42:49, tdresser wrote:
> > This name is hard to parse.
> > 
> > Isn't the second "Handler" redundant?
> > Would something like:
> > 
> > SyncInputHandlerManagerProxyClient 
> > 
> > mean the same thing?
> 
> Yeah I'm terribe with names. How about just
SynchronousInputHandlerProxyClient?
> To match SynchronousInputHandlerProxy

SGTM.

[boliu, 2016-02-29 21:03:17]

Patchset #5 (id:70001) has been deleted

[boliu, 2016-02-29 21:04:40]

New patch set up

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:308: //
Intentional No-op.
On 2016/02/29 16:42:49, tdresser wrote:
> Comment why this should be a no-op.

Actually should really a NOTREACHED. Done.

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.h (right):

https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.h:95: void
DidOverscrollInProc(const DidOverscrollParams& params);
On 2016/02/29 16:42:49, tdresser wrote:
> Can we de-abbreviate this?

Done.

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

https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:63: class CONTENT_EXPORT
InputHandlerManagerSyncHandlerProxyClient {
On 2016/02/29 19:10:25, tdresser wrote:
> On 2016/02/29 18:58:53, boliu wrote:
> > On 2016/02/29 16:42:49, tdresser wrote:
> > > This name is hard to parse.
> > > 
> > > Isn't the second "Handler" redundant?
> > > Would something like:
> > > 
> > > SyncInputHandlerManagerProxyClient 
> > > 
> > > mean the same thing?
> > 
> > Yeah I'm terribe with names. How about just
> SynchronousInputHandlerProxyClient?
> > To match SynchronousInputHandlerProxy
> 
> SGTM.

Done.

[tdresser, 2016-03-03 16:15:57]

On 2016/02/29 21:04:40, boliu wrote:
> New patch set up
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
> File content/browser/android/in_process/synchronous_compositor_impl.cc
(right):
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
> content/browser/android/in_process/synchronous_compositor_impl.cc:308: //
> Intentional No-op.
> On 2016/02/29 16:42:49, tdresser wrote:
> > Comment why this should be a no-op.
> 
> Actually should really a NOTREACHED. Done.
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
> File content/browser/android/in_process/synchronous_compositor_impl.h (right):
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/browser/android...
> content/browser/android/in_process/synchronous_compositor_impl.h:95: void
> DidOverscrollInProc(const DidOverscrollParams& params);
> On 2016/02/29 16:42:49, tdresser wrote:
> > Can we de-abbreviate this?
> 
> Done.
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
> File content/renderer/input/input_handler_manager_client.h (right):
> 
>
https://codereview.chromium.org/1620053002/diff/40002/content/renderer/input/...
> content/renderer/input/input_handler_manager_client.h:63: class CONTENT_EXPORT
> InputHandlerManagerSyncHandlerProxyClient {
> On 2016/02/29 19:10:25, tdresser wrote:
> > On 2016/02/29 18:58:53, boliu wrote:
> > > On 2016/02/29 16:42:49, tdresser wrote:
> > > > This name is hard to parse.
> > > > 
> > > > Isn't the second "Handler" redundant?
> > > > Would something like:
> > > > 
> > > > SyncInputHandlerManagerProxyClient 
> > > > 
> > > > mean the same thing?
> > > 
> > > Yeah I'm terribe with names. How about just
> > SynchronousInputHandlerProxyClient?
> > > To match SynchronousInputHandlerProxy
> > 
> > SGTM.
> 
> Done.

aelias@, you've got a fair bit more context on this than I do, can you take a
look?

[aelias_OOO_until_Jul13, 2016-03-04 07:16:29]

lgtm

I'm mostly convinced by Bo's argument that synchronous input isn't needed
because only the vsync truly needs to be synchronous in WebView.  Let's give
this a try.

[tdresser, 2016-03-04 19:42:25]

LGTM

https://codereview.chromium.org/1620053002/diff/90001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/90001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:308:
NOTREACHED();  // In-process path does not use async DidOverscroll.
Can you add to this comment indicating this path does receive information about
overscrolling?

[boliu, 2016-03-05 01:28:46]

(Before sievers looks..) also reversed the switch. The async path is still
default, but can be disabled at run time by adding the switch

https://codereview.chromium.org/1620053002/diff/90001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/90001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:308:
NOTREACHED();  // In-process path does not use async DidOverscroll.
On 2016/03/04 19:42:25, tdresser wrote:
> Can you add to this comment indicating this path does receive information
about
> overscrolling?

Done.

[boliu, 2016-03-08 16:01:40]

sievers: ping

[boliu, 2016-03-09 19:29:25]

On 2016/03/08 16:01:40, boliu wrote:
> sievers: ping

ping x2

[no sievers, 2016-03-10 20:37:06]

lgtm

https://codereview.chromium.org/1620053002/diff/130001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/130001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1184:
cmd_line->HasSwitch(switches::kSyncInputForSyncCompositor);
nit: DCHECK_IMPLIES(sync_input_for_sync_compositing,
using_ipc_sync_compositing);

[boliu, 2016-03-10 21:04:28]

https://codereview.chromium.org/1620053002/diff/130001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1620053002/diff/130001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1184:
cmd_line->HasSwitch(switches::kSyncInputForSyncCompositor);
On 2016/03/10 20:37:06, sievers wrote:
> nit: DCHECK_IMPLIES(sync_input_for_sync_compositing,
> using_ipc_sync_compositing);

Done. DCHECK_IMPLIES isn't a thing anymore, so it's the DCHECK(!foo || bar)
flavour.

[boliu, 2016-03-10 21:05:05]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-03-10 21:05:06]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
tdresser@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1620053002/#ps150001
(title: "review")

[commit-bot: I haz the power, 2016-03-10 21:05:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620053002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620053002/150001

[commit-bot: I haz the power, 2016-03-10 21:41:14]

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

[commit-bot: I haz the power, 2016-03-10 21:41:16]

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

[boliu, 2016-03-10 22:44:22]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-03-10 22:44:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620053002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620053002/150001

[commit-bot: I haz the power, 2016-03-10 23:33:27]

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

[commit-bot: I haz the power, 2016-03-10 23:33:28]

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

[boliu, 2016-03-10 23:37:20]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:38:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620053002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620053002/150001

[commit-bot: I haz the power, 2016-03-11 00:13:25]

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

[commit-bot: I haz the power, 2016-03-11 00:13:26]

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

[boliu, 2016-03-11 00:14:46]

A lot of purple on main waterfall and these failing trybots. Looks like we are
experience some widespread bot issues :/

[boliu, 2016-03-11 05:18:21]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-03-11 05:19:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1620053002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1620053002/150001

[commit-bot: I haz the power, 2016-03-11 07:02:22]

Description was changed from

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628
==========

to

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628
==========

[commit-bot: I haz the power, 2016-03-11 07:02:23]

Committed patchset #8 (id:150001)

[commit-bot: I haz the power, 2016-03-11 07:03:23]

Description was changed from

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628
==========

to

==========
sync compositor: Merge input path with chrome

Use chrome's async input delivery path in Android WebView. See bug
for potential incompatibility risk. Existing code path is not removed
in case compatibility is an issue.

Refactor out ui::SynchronousInputHandlerProxy parts from
InputHandlerManagerClient into a separate interface so that sync
compositor can use SynchronousInputHandlerProxy even without
routing input synchronously. Then just stop filtering input events in
sync compositor, hook up overscroll, and everything just works.

Tests:
testNoSpuriousOverScrolls requires synchronous input to maintain
the property that a single scroll call happens. Asynchronously the
test is not valid
AwContents.zoomIn is no longer synchronous which breaks
testPinchZoomUpdatesScrollRangeSynchronously. Can fix this
independently if this becomes a problem later.

BUG=545628

Committed: https://crrev.com/de5b75b1ccd0ef7f478318d805b3ba1c8ebf7130
Cr-Commit-Position: refs/heads/master@{#380563}
==========

[commit-bot: I haz the power, 2016-03-11 07:03:25]

Patchset 8 (id:??) landed as
https://crrev.com/de5b75b1ccd0ef7f478318d805b3ba1c8ebf7130
Cr-Commit-Position: refs/heads/master@{#380563}