Ensure touch handler state on Android is accurate after preload

Ensure touch handler state on Android is accurate after preload

In preload scenarios, ContentViewCore is attached to RenderWidgetHostViewAndroid when the contents becomes visible - long after the page was loaded as JS has been run.  If that JS registered a touch event handler, then ContentViewCore won't be notified of it, and future touch events will be dropped on the floor.

Fix this by sending the current touch handler state to ContentViewCore when it's attached.  This required moving the initialization of ContentViewGestureHandler (and related pieces) slightly earlier.

This fix is intentionally minimal to lower the merge risk.  A more complete fix would be to stop worrying about the touch handler state up in java at all - leave it all to the common code in InputRouter (which is already relied upon for Aura).  We will get this essentially for free as part of http://crbug.com/332418.

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

[Rick Byers, 2014-01-16 21:54:03]

Jared, please take a look.

As we discussed, I don't think there's any easy way to test this.  There are no
RenderWidgetHostViewAndroid unit tests today, and I also can't find any preload
integration tests that run on Android (the chrome ones are apparently explicitly
disabled for Android).  Since this is all going away soon anyway (to unified GR)
I think we agree that we'll continue to leave this sort of thing untested for
now.

[jdduke (slow), 2014-01-16 22:53:04]

dtrainor@: Is it fine if RWVHA calls back into CVC (Java) during initialization?
Or should we have CVC query RWHVA after initialization to set whether it has
touch handlers?

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

https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/input_router_impl.cc:501:
TRACE_EVENT1("input", "InputRouterImpl::OnHasTouchEventHandlers",

Is this spammy? If not, this should be fine, otherwise perhaps we could move it
down below the equality check?

https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1330:
content_view_core_->HasTouchEventHandlers(host_->has_touch_handler());

This seems fine.   If we're worried about initialization order, we could always
add some wiring in ContentViewCoreImpl to pull |has_touch_handlers()| from
RWHVA.  That avoids any re-entrance back into ContentViewCore during
initialization.

[Rick Byers, 2014-01-17 15:31:37]

On 2014/01/16 22:53:04, jdduke wrote:
> dtrainor@: Is it fine if RWVHA calls back into CVC (Java) during
initialization?
> Or should we have CVC query RWHVA after initialization to set whether it has
> touch handlers?
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/input/input_router_impl.cc:501:
> TRACE_EVENT1("input", "InputRouterImpl::OnHasTouchEventHandlers",
> 
> Is this spammy? If not, this should be fine, otherwise perhaps we could move
it
> down below the equality check?

No, it's not really spammy (certainly not compared to input events) - just
called once during initialization, and then when it changes (as far as I can
see).  I thought it was better to put it before the equality check to help
understand issues like this (where we're trying to update the state but possibly
failing), but it's no big deal - tracing is no substitute for debugging, just
figured it was worthwhile having SOME trace around this important state change. 
I'm happy to move it if you prefer.

>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/render_widget_host_view_android.cc:1330:
> content_view_core_->HasTouchEventHandlers(host_->has_touch_handler());
> 
> This seems fine.   If we're worried about initialization order, we could
always
> add some wiring in ContentViewCoreImpl to pull |has_touch_handlers()| from
> RWHVA.  That avoids any re-entrance back into ContentViewCore during
> initialization.

Yep, I'm happy to go that way if you prefer.  Let me know...

[jdduke (slow), 2014-01-17 15:43:13]

On 2014/01/17 15:31:37, Rick Byers wrote:
> On 2014/01/16 22:53:04, jdduke wrote:
> > dtrainor@: Is it fine if RWVHA calls back into CVC (Java) during
> initialization?
> > Or should we have CVC query RWHVA after initialization to set whether it has
> > touch handlers?
> > 
> >
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> > File content/browser/renderer_host/input/input_router_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> > content/browser/renderer_host/input/input_router_impl.cc:501:
> > TRACE_EVENT1("input", "InputRouterImpl::OnHasTouchEventHandlers",
> > 
> > Is this spammy? If not, this should be fine, otherwise perhaps we could move
> it
> > down below the equality check?
> 
> No, it's not really spammy (certainly not compared to input events) - just
> called once during initialization, and then when it changes (as far as I can
> see).  I thought it was better to put it before the equality check to help
> understand issues like this (where we're trying to update the state but
possibly
> failing), but it's no big deal - tracing is no substitute for debugging, just
> figured it was worthwhile having SOME trace around this important state
change. 
> I'm happy to move it if you prefer.
> 

Nah, as it is looks just fine.


> >
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> > content/browser/renderer_host/render_widget_host_view_android.cc:1330:
> > content_view_core_->HasTouchEventHandlers(host_->has_touch_handler());
> > 
> > This seems fine.   If we're worried about initialization order, we could
> always
> > add some wiring in ContentViewCoreImpl to pull |has_touch_handlers()| from
> > RWHVA.  That avoids any re-entrance back into ContentViewCore during
> > initialization.
> 
> Yep, I'm happy to go that way if you prefer.  Let me know...

I'm fairly indifferent, I'll defer to dtrainor@ if he has a preference (I'm less
familiar with ContentViewCore initialization).

[David Trainor- moved to gerrit, 2014-01-17 18:01:01]

On 2014/01/16 22:53:04, jdduke wrote:
> dtrainor@: Is it fine if RWVHA calls back into CVC (Java) during
initialization?
> Or should we have CVC query RWHVA after initialization to set whether it has
> touch handlers?
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/input/input_router_impl.cc:501:
> TRACE_EVENT1("input", "InputRouterImpl::OnHasTouchEventHandlers",
> 
> Is this spammy? If not, this should be fine, otherwise perhaps we could move
it
> down below the equality check?
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/140833005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/render_widget_host_view_android.cc:1330:
> content_view_core_->HasTouchEventHandlers(host_->has_touch_handler());
> 
> This seems fine.   If we're worried about initialization order, we could
always
> add some wiring in ContentViewCoreImpl to pull |has_touch_handlers()| from
> RWHVA.  That avoids any re-entrance back into ContentViewCore during
> initialization.

I'm fine with pushing it here.  The only other way would be to pull it every
time we set a new RWHVA.

[jdduke (slow), 2014-01-17 18:12:22]

content/browser/renderer_host/input/ and content/public/android/ lgtm.

[Rick Byers, 2014-01-17 18:19:52]

On 2014/01/17 18:12:22, jdduke wrote:
> content/browser/renderer_host/input/ and content/public/android/ lgtm.

Thanks.  +aelias for OWNERS of
content/browser/renderer_host/render_widget_host_view_android.cc

[aelias_OOO_until_Jul13, 2014-01-17 19:33:31]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-17 20:22:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/140833005/1

[commit-bot: I haz the power, 2014-01-17 22:55:51]

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

[commit-bot: I haz the power, 2014-01-18 00:35:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/140833005/1

[commit-bot: I haz the power, 2014-01-18 02:23:35]

Change committed as 245723