[Android] Disable the touch ack timeout on mobile sites

[Android] Disable the touch ack timeout on mobile sites

Disable the touch ack timeout on pages that have a width=device-width or
narrow viewport (indicative of the page being mobile-optimized or responsive).
Previously, this was disabled only on mobile sites with a fixed page scale.

BUG=260732, 313741
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245219

[jdduke (slow), 2014-01-14 21:33:01]

I must have missed this case when I first ported the timeout to C++.  We had it
briefly after johnme@'s double-tap optimization
https://codereview.chromium.org/18850005/ landed, when the timeout in
ContentViewGestureHandler relied on the |isDoubleTapDisabled()| query to
determine if a timeout should be started.

[jdduke (slow), 2014-01-14 21:40:57]

My math is off, I need to convert the content width coordinates... Patch will be
up soon.

[jdduke (slow), 2014-01-14 21:54:17]

On 2014/01/14 21:40:57, jdduke wrote:
> My math is off, I need to convert the content width coordinates... Patch will
be
> up soon.

Just kidding, I think the math is good :)

[Xianzhu, 2014-01-14 21:54:19]

I happened to read the related code in this morning :)

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:113: if
(metadata.root_layer_size.width() <= window_width_dip)
Can we simply skip timeout for all pages with viewport tag?

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:116: return view_flags;
Will the individual bits of view_flags be meaningful to InputRouter or be used
for purpose other than touch_ack_timeout?

Raised the question when I was seeking other conditions to opt out the timeout,
which would need new bits, or just InputRouter::DisableTouchAckTimeout().

[jdduke (slow), 2014-01-14 21:56:37]

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:113: if
(metadata.root_layer_size.width() <= window_width_dip)
On 2014/01/14 21:54:19, Xianzhu wrote:
> Can we simply skip timeout for all pages with viewport tag?

Where do we get that data?

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:116: return view_flags;
On 2014/01/14 21:54:19, Xianzhu wrote:
> Will the individual bits of view_flags be meaningful to InputRouter or be used
> for purpose other than touch_ack_timeout?
> 
> Raised the question when I was seeking other conditions to opt out the
timeout,
> which would need new bits, or just InputRouter::DisableTouchAckTimeout().

Currently, no, it's only useful for the ack timeout.  However, 
the flag additions were somewhat speculative... I've had thoughts of filtering
out pinch update events if the page scale is fixed, or event scroll updates if
we know there is no affordance in the scroll offset.

[Xianzhu, 2014-01-14 23:56:18]

lgtm

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:113: if
(metadata.root_layer_size.width() <= window_width_dip)
On 2014/01/14 21:56:38, jdduke wrote:
> On 2014/01/14 21:54:19, Xianzhu wrote:
> > Can we simply skip timeout for all pages with viewport tag?
> 
> Where do we get that data?

My fault. I thought it is available in the metadata.

[johnme, 2014-01-15 02:15:09]

LGTM if the answer to my questions is yes :)

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:113: if
(metadata.root_layer_size.width() <= window_width_dip)
On 2014/01/14 21:54:19, Xianzhu wrote:
> Can we simply skip timeout for all pages with viewport tag?

No, many desktop sites have viewport tags like <meta name="viewport"
content="width=1024"> (including, say, theverge.com) and we shouldn't treat
those differently from other desktop sites.

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:112:
std::ceil(metadata.page_scale_factor * metadata.viewport_size.width());
Can you confirm that metadata.viewport_size.width gives the window width in CSS
pixels (as opposed to the screen width, layout viewport width, or content
width)? For example if you load a width=device-width page in a 200DIP wide
Chrome-powered Android WebView, this should initially report 200 pixels, but if
you then pinch zoom in by 2x it should report 100 pixels, and hence after
multiplying by the page_scale_factor this should evaluate to 200 pixels at both
zoom levels.

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:113: const float
content_width_css = metadata.root_layer_size.width();
Can you confirm that metadata.root_layer_size.width is the content width (i.e.
it determines how far the page can be scrolled) rather than the layout viewport
width (set by meta viewport, and which determines the width of the initial
containing block, hence the content width of a flexible width page), and that it
is in CSS pixels not DIPs? For example if you load a width=device-width page
that contains a <div style="width:2000px; height:5px">, this should report 2000
pixels, irrespective of zoom level.

[jdduke (slow), 2014-01-15 03:06:49]

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:112:
std::ceil(metadata.page_scale_factor * metadata.viewport_size.width());
On 2014/01/15 02:15:09, johnme wrote:
> Can you confirm that metadata.viewport_size.width gives the window width in
CSS
> pixels (as opposed to the screen width, layout viewport width, or content
> width)? For example if you load a width=device-width page in a 200DIP wide
> Chrome-powered Android WebView, this should initially report 200 pixels, but
if
> you then pinch zoom in by 2x it should report 100 pixels, and hence after
> multiplying by the page_scale_factor this should evaluate to 200 pixels at
both
> zoom levels.

I'll double check (under basic testing the |window_width_dip| was invariant
under pinch zoom).  I should note that these are the same values that propagate
to RenderCoordinates via ContentViewCore, so this math should be equivalent to
that in |RenderCoordinates.hasMobileViewport|.

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:113: const float
content_width_css = metadata.root_layer_size.width();
On 2014/01/15 02:15:09, johnme wrote:
> Can you confirm that metadata.root_layer_size.width is the content width (i.e.
> it determines how far the page can be scrolled) rather than the layout
viewport
> width (set by meta viewport, and which determines the width of the initial
> containing block, hence the content width of a flexible width page), and that
it
> is in CSS pixels not DIPs? For example if you load a width=device-width page
> that contains a <div style="width:2000px; height:5px">, this should report
2000
> pixels, irrespective of zoom level.

Again, I'll double check. The only difference between this (content width) value
and that used in RenderCoordinates is that the latter is max'ed with
|mViewportWidthPix| (converted to DIP) from ContentViewCore.  Would that have a
meaningful effect on the result?  I guess I could max with
|RWHVA::GetViewBounds()| so we're consistent.

[jdduke (slow), 2014-01-15 21:28:50]

Thanks for review.

aelias@: Owner review? Thanks.

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:112:
std::ceil(metadata.page_scale_factor * metadata.viewport_size.width());
On 2014/01/15 03:06:49, jdduke wrote:
> On 2014/01/15 02:15:09, johnme wrote:
> > Can you confirm that metadata.viewport_size.width gives the window width in
> CSS
> > pixels (as opposed to the screen width, layout viewport width, or content
> > width)? For example if you load a width=device-width page in a 200DIP wide
> > Chrome-powered Android WebView, this should initially report 200 pixels, but
> if
> > you then pinch zoom in by 2x it should report 100 pixels, and hence after
> > multiplying by the page_scale_factor this should evaluate to 200 pixels at
> both
> > zoom levels.
> 
> I'll double check (under basic testing the |window_width_dip| was invariant
> under pinch zoom).  I should note that these are the same values that
propagate
> to RenderCoordinates via ContentViewCore, so this math should be equivalent to
> that in |RenderCoordinates.hasMobileViewport|.

Confirmed.

https://codereview.chromium.org/138893003/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:113: const float
content_width_css = metadata.root_layer_size.width();
On 2014/01/15 03:06:49, jdduke wrote:
> On 2014/01/15 02:15:09, johnme wrote:
> > Can you confirm that metadata.root_layer_size.width is the content width
(i.e.
> > it determines how far the page can be scrolled) rather than the layout
> viewport
> > width (set by meta viewport, and which determines the width of the initial
> > containing block, hence the content width of a flexible width page), and
that
> it
> > is in CSS pixels not DIPs? For example if you load a width=device-width page
> > that contains a <div style="width:2000px; height:5px">, this should report
> 2000
> > pixels, irrespective of zoom level.
> 
> Again, I'll double check. The only difference between this (content width)
value
> and that used in RenderCoordinates is that the latter is max'ed with
> |mViewportWidthPix| (converted to DIP) from ContentViewCore.  Would that have
a
> meaningful effect on the result?  I guess I could max with
> |RWHVA::GetViewBounds()| so we're consistent.

Confirmed.

[aelias_OOO_until_Jul13, 2014-01-15 22:51:13]

lgtm

[commit-bot: I haz the power, 2014-01-15 23:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/138893003/110001

[commit-bot: I haz the power, 2014-01-16 01:25:16]

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

[commit-bot: I haz the power, 2014-01-16 02:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/138893003/110001

[commit-bot: I haz the power, 2014-01-16 17:16:49]

Change committed as 245219