Fix computation of scroll amount in scroll actions.

Fix computation of scroll amount in scroll actions.

During page load window.innerHeight/Width can exceed
window.outerHeight/Width, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportHeight/Width
adjusted with chrome.gpuBenchmarking.pageScaleFactor to
compute zoom-independent inner height/width of window.

BUG=chromium:627166
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d2b666834a289f8d109c5383fb16e7a9cba66145

[ulan, 2016-07-11 16:57:49]

ulan@chromium.org changed reviewers:
+ nednguyen@google.com

[ulan, 2016-07-11 16:57:49]

PTAL or redirect.

[nednguyen, 2016-07-11 17:01:35]

nednguyen@google.com changed reviewers:
+ petrcermak@chromium.org

[nednguyen, 2016-07-11 17:01:36]

lgtm if Petr is happy

[nednguyen, 2016-07-11 17:03:22]

On 2016/07/11 17:01:36, nednguyen wrote:
> lgtm if Petr is happy

Actually not ready to land thet.

I think we should push for people to fix the bug on TOT chromium & only do this
workaround fix if reference browser also has this problem. Also how do we test
this?

[ulan, 2016-07-11 18:34:51]

Description was changed from

==========
Work around incorrect window.innerWidth and window.innerHeight values
in scroll actions.

Chrome has a bug that causes window.innerHeight to be sometimes greater
than window.outerHeight (the same for width). This leads to assertion
failure and crash in Chrome while executing a scroll action.

BUG=chromium:627123
==========

to

==========
Work around incorrect window.innerWidth and window.innerHeight values
in scroll actions.

Chrome has a bug that causes window.innerHeight to be sometimes greater
than window.outerHeight (the same for width). This leads to assertion
failure and crash in Chrome while executing a scroll action.

BUG=chromium:627123
==========

[ulan, 2016-07-11 18:34:51]

ulan@chromium.org changed reviewers:
+ dominikg@google.com

[ulan, 2016-07-11 18:47:52]

Description was changed from

==========
Work around incorrect window.innerWidth and window.innerHeight values
in scroll actions.

Chrome has a bug that causes window.innerHeight to be sometimes greater
than window.outerHeight (the same for width). This leads to assertion
failure and crash in Chrome while executing a scroll action.

BUG=chromium:627123
==========

to

==========
Work around incorrect window.innerWidth and window.innerHeight values
in scroll actions.

Chrome has a bug that causes window.innerHeight to be sometimes greater
than window.outerHeight (the same for width). This leads to assertion
failure and crash in Chrome while executing a scroll action.

BUG=chromium:627123,chromium:627166
==========

[ulan, 2016-07-11 18:49:32]

On 2016/07/11 17:03:22, nednguyen wrote:
> On 2016/07/11 17:01:36, nednguyen wrote:
> > lgtm if Petr is happy
> 
> Actually not ready to land thet.
> 
> I think we should push for people to fix the bug on TOT chromium & only do
this
> workaround fix if reference browser also has this problem. Also how do we test
> this?

crbug.com/627123 was closed as WAI, we cannot assume that window.innerHeight <=
window.outerHeight.

So looks like the bug is actually in telemetry code. I filed crbug.com/627166.

Adding dominikg@ to take a look at this CL.

[ulan, 2016-07-12 08:36:53]

Description was changed from

==========
Work around incorrect window.innerWidth and window.innerHeight values
in scroll actions.

Chrome has a bug that causes window.innerHeight to be sometimes greater
than window.outerHeight (the same for width). This leads to assertion
failure and crash in Chrome while executing a scroll action.

BUG=chromium:627123,chromium:627166
==========

to

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerWidth/Height can exceed
window.outerWidth/Height, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch fixes the corner case by taking the minimum
of innerWidth/Height and outerWidth/Height.

BUG=chromium:627166
==========

[ulan, 2016-07-12 08:37:10]

ulan@chromium.org changed reviewers:
+ bokan@chromium.org
- dominikg@google.com

[ulan, 2016-07-12 08:39:02]

nednguyen@, bokan@, ptal.

[petrcermak, 2016-07-12 09:19:21]

LGTM with a few comments.

Thanks,
Petr

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/repeatable_scroll.py:25: # Get the
dimensions of the screen. During page load innerWidth can be
Can you change "innerWidth" to "innerWidth/Height" (and the same for
"outerWidth")?

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/repeatable_scroll.py:28: sizes_js =
('window.innerWidth + "," + window.outerWidth + "," + ' +
Assuming the DevTools protocol can handle returning arrays correctly (which I
think it can), I suggest that we retrieve the values via an array instead:

sizes = tab.EvaluateJavaScript('[window.innerWidth, window.outerWidth, ...]')

Furthermore, I'd destructure the sizes to make the code more readable:

inner_width, outer_width, inner_height, outer_height =
tab.EvaluateJavaScript(...)

As an alternative, you could perform the min calculation in JS to make this
method even simpler:

self._windowsize = tab.EvaluateJavaScript('[Math.min(window.innerWidth,
window.outerWidth), Math.min(window.innerHeight, window.outerHeight)]')

In this case, it would probably be a good idea to add the following assertions
afterwards:

assert len(self._windowsize) == 2
assert all(isinstance(d, numbers.Number) for d in self._windowsize)

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/scroll.js (right):

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/scroll.js:73: // clientHeight is "special"
for the body element. During page load
s/clientHeight/clientWidth/

[ulan, 2016-07-12 09:31:24]

Thanks, awesome comments!

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/repeatable_scroll.py:25: # Get the
dimensions of the screen. During page load innerWidth can be
On 2016/07/12 09:19:20, petrcermak wrote:
> Can you change "innerWidth" to "innerWidth/Height" (and the same for
> "outerWidth")?

Done.

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/repeatable_scroll.py:28: sizes_js =
('window.innerWidth + "," + window.outerWidth + "," + ' +
On 2016/07/12 09:19:20, petrcermak wrote:
> Assuming the DevTools protocol can handle returning arrays correctly (which I
> think it can), I suggest that we retrieve the values via an array instead:
> 
> sizes = tab.EvaluateJavaScript('[window.innerWidth, window.outerWidth, ...]')
> 
> Furthermore, I'd destructure the sizes to make the code more readable:
> 
> inner_width, outer_width, inner_height, outer_height =
> tab.EvaluateJavaScript(...)
> 
> As an alternative, you could perform the min calculation in JS to make this
> method even simpler:
> 
> self._windowsize = tab.EvaluateJavaScript('[Math.min(window.innerWidth,
> window.outerWidth), Math.min(window.innerHeight, window.outerHeight)]')
> 
> In this case, it would probably be a good idea to add the following assertions
> afterwards:
> 
> assert len(self._windowsize) == 2
> assert all(isinstance(d, numbers.Number) for d in self._windowsize)

Done.

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/scroll.js (right):

https://codereview.chromium.org/2138883002/diff/40001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/scroll.js:73: // clientHeight is "special"
for the body element. During page load
On 2016/07/12 09:19:20, petrcermak wrote:
> s/clientHeight/clientWidth/

Done.

[petrcermak, 2016-07-12 09:34:36]

Still LGTM :-)

https://codereview.chromium.org/2138883002/diff/60001/telemetry/telemetry/int...
File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):

https://codereview.chromium.org/2138883002/diff/60001/telemetry/telemetry/int...
telemetry/telemetry/internal/actions/repeatable_scroll.py:31:
'[Math.min(window.innerWidth, window.outerWidth),' +
FYI, you don't need the "+" symbol here (Python automatically concatenates
adjacent string literals)

[ulan, 2016-07-12 09:39:17]

On 2016/07/12 09:34:36, petrcermak wrote:
> Still LGTM :-)
> 
>
https://codereview.chromium.org/2138883002/diff/60001/telemetry/telemetry/int...
> File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):
> 
>
https://codereview.chromium.org/2138883002/diff/60001/telemetry/telemetry/int...
> telemetry/telemetry/internal/actions/repeatable_scroll.py:31:
> '[Math.min(window.innerWidth, window.outerWidth),' +
> FYI, you don't need the "+" symbol here (Python automatically concatenates
> adjacent string literals)

Done. :)

[nednguyen, 2016-07-12 10:13:42]

Do we have any existing test coverage that would catch the crash without this
fix?

[ulan, 2016-07-12 10:38:08]

On 2016/07/12 10:13:42, nednguyen wrote:
> Do we have any existing test coverage that would catch the crash without this
> fix?

The benchmark (not landed yet) in 627166 would catch it.

[nednguyen, 2016-07-12 10:44:03]

On 2016/07/12 10:38:08, ulan wrote:
> On 2016/07/12 10:13:42, nednguyen wrote:
> > Do we have any existing test coverage that would catch the crash without
this
> > fix?
> 
> The benchmark (not landed yet) in 627166 would catch it.

We should make an extra test in telemetry rather than relying on the benchmarks.
Telemetry benchmarks come and go but the test are stable  & run on every CL for
a long time.

[petrcermak, 2016-07-12 10:49:30]

On 2016/07/12 10:13:42, nednguyen wrote:
> Do we have any existing test coverage that would catch the crash without this
> fix?

I don't know.

Ulan: Could you please check and add a unit test if necessary?

Thanks,
Petr

[ulan, 2016-07-12 12:07:04]

On 2016/07/12 10:44:03, nednguyen wrote:
> On 2016/07/12 10:38:08, ulan wrote:
> > On 2016/07/12 10:13:42, nednguyen wrote:
> > > Do we have any existing test coverage that would catch the crash without
> this
> > > fix?
> > 
> > The benchmark (not landed yet) in 627166 would catch it.
> 
> We should make an extra test in telemetry rather than relying on the
benchmarks.
> Telemetry benchmarks come and go but the test are stable  & run on every CL
for
> a long time.

I added unit test for repeatable scroll. Not sure how useful it is since it is
setting fake window.inner/outer/Width/Height and just checking that we take the
minimum.

A better test would be to reliably trigger window.innerHeight >
window.outerHeight in browser and then scroll. I don't know how to do that. In
the benchmark I linked this condition is flaky, happens once in ~5 runs.

[petrcermak, 2016-07-12 12:22:57]

It seems that it's possible to repro the conditions with the following steps
(not sure if the behavior is intended or if it's a Chromium bug):

  1. Zoom out (Ctrl+Scroll down)
  2. Inspect |window.innerWidth| and |window.outerWidth| in DevTools.

You could probably simulate zooming out using a Pinch action
(https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...).
I'm not sure whether it would work on desktop, but I guess it's worth giving a
try (should be very simple to implement in Telemetry).

Petr

https://codereview.chromium.org/2138883002/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:79:
self.assertEquals(i._windowsize[0], 500)
self.assertEquals(i._windowsize, [500, 700])

[ulan, 2016-07-12 14:47:58]

Thank you for suggestion!

It looks like Pinch action can zoom in, but cannot zoom out on desktop. Or I am
doing something wrong.

On mobile, I am getting the following error during navigation. Is there any
other step that should be done besides ./telemetry/bin/run_tests
testRepeatableScrollActionNoRepeatsZoomed ?

Exception KeyError: (<class
'telemetry.core.memory_cache_http_server.MemoryCacheHTTPServer'>,) in <bound
method MemoryCacheHTTPServer.__del__ of
<telemetry.core.memory_cache_http_server.MemoryCacheHTTPServer object at
0x7ff763715c50>> ignored

https://codereview.chromium.org/2138883002/diff/120001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/120001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:79:
self.assertEquals(i._windowsize[0], 500)
On 2016/07/12 12:22:57, petrcermak wrote:
> self.assertEquals(i._windowsize, [500, 700])

Done.

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self._tab.action_runner.PinchPage(scale_factor=0.3)
Any ideas why PinchPage ignores scale_factor < 1.0 on desktop?
If I pass scale_factor > 1, then it is actually zooming in the page.

[nednguyen, 2016-07-12 14:52:27]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 14:52:33]

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-07-12 15:04:26]

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

[commit-bot: I haz the power, 2016-07-12 15:04:27]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[petrcermak, 2016-07-12 15:10:27]

I got your test running on Android with the following command:

$ telemetry/bin/run_tests testRepeatableScrollActionNoRepeatsZoomed
--browser=reference --device=android

It crashes consistently with the following error:

[FATAL:synthetic_gesture_target_base.cc(62)] Check failed:
web_touch.touches[i].state != WebTouchPoint::StatePressed ||
PointIsWithinContents(web_touch.touches[i].position.x,
web_touch.touches[i].position.y). Touch coordinates are not within content
bounds on TouchStart.

I suggest you modify the test to run on Android only. I don't want you to spend
too much time on this, so let's just skip desktop here.

Thanks,
Petr

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self._tab.action_runner.PinchPage(scale_factor=0.3)
On 2016/07/12 14:47:58, ulan wrote:
> Any ideas why PinchPage ignores scale_factor < 1.0 on desktop?
> If I pass scale_factor > 1, then it is actually zooming in the page.

I suppose this is not very surprising given that "pinch" is not really a thing
on desktop.

[bokan, 2016-07-12 15:36:07]

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll.py:32: '
Math.min(window.innerHeight, window.outerHeight)]')
This might be ok as a temporary stopgap to get the tests green again, but this
will be very fragile. Also, if we can't just use outerWidth/outerHeight
directly, won't this fail when outerWidth/outerHeight is the smaller of the two?

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:74:
'window.outerWidth = 500;')
These values aren't normally writable...

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self._tab.action_runner.PinchPage(scale_factor=0.3)
On 2016/07/12 15:10:27, petrcermak wrote:
> On 2016/07/12 14:47:58, ulan wrote:
> > Any ideas why PinchPage ignores scale_factor < 1.0 on desktop?
> > If I pass scale_factor > 1, then it is actually zooming in the page.
> 
> I suppose this is not very surprising given that "pinch" is not really a thing
> on desktop.

Pinch is totally thing on desktop, all you need's a touchscreen or on Mac using
the trackpad to pinch.

You can't zoom out further than 1 because the layout size on desktop matches the
window width so there's no content beyond the window; the minimum-scale on
desktop is always 1. On mobile we regularly make the layout size wider than the
device since desktop pages won't render properly into the small screen sizes so
it makes zooming out possible.

[ulan, 2016-07-12 16:57:04]

Thank you all!

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll.py:32: '
Math.min(window.innerHeight, window.outerHeight)]')
On 2016/07/12 15:36:07, bokan wrote:
> This might be ok as a temporary stopgap to get the tests green again, but this
> will be very fragile. Also, if we can't just use outerWidth/outerHeight
> directly, won't this fail when outerWidth/outerHeight is the smaller of the
two?

As discussed in the bug, I will change it to use pageScaleFactor.
Thank you for the suggestion!

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:74:
'window.outerWidth = 500;')
On 2016/07/12 15:36:07, bokan wrote:
> These values aren't normally writable...

Remove this test as there is better test below.

https://codereview.chromium.org/2138883002/diff/140001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self._tab.action_runner.PinchPage(scale_factor=0.3)
On 2016/07/12 15:36:07, bokan wrote:
> On 2016/07/12 15:10:27, petrcermak wrote:
> > On 2016/07/12 14:47:58, ulan wrote:
> > > Any ideas why PinchPage ignores scale_factor < 1.0 on desktop?
> > > If I pass scale_factor > 1, then it is actually zooming in the page.
> > 
> > I suppose this is not very surprising given that "pinch" is not really a
thing
> > on desktop.
> 
> Pinch is totally thing on desktop, all you need's a touchscreen or on Mac
using
> the trackpad to pinch.
> 
> You can't zoom out further than 1 because the layout size on desktop matches
the
> window width so there's no content beyond the window; the minimum-scale on
> desktop is always 1. On mobile we regularly make the layout size wider than
the
> device since desktop pages won't render properly into the small screen sizes
so
> it makes zooming out possible.

I disabled this test for desktop.

[ulan, 2016-07-13 18:46:26]

Description was changed from

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerWidth/Height can exceed
window.outerWidth/Height, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch fixes the corner case by taking the minimum
of innerWidth/Height and outerWidth/Height.

BUG=chromium:627166
==========

to

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerWidth/Height can exceed
window.outerWidth/Height, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportWidth/Height
to compute scroll amount.

BUG=chromium:627166
==========

[ulan, 2016-07-13 19:04:06]

ulan@chromium.org changed reviewers:
+ ymalik@chromium.org

[ulan, 2016-07-13 19:04:07]

[+ymalik]

Digging around more I found this CL from ymalik@:
https://codereview.chromium.org/1400213002, which is solving the same problem.

There is even TODO to replace innerWidth/Height with
chrome.gpuBenchmarking.visualViewportWidth/Height once they roll in stable.

I checked that visualViewportWidth/Height do not depend on zoom and are
available in reference builds. So I think using them instead of
innerWidth/Height is the proper fix.

The latest patch set applies the fix to all actions.

wdyt?

[bokan, 2016-07-13 19:26:50]

On 2016/07/13 19:04:07, ulan wrote:
> [+ymalik]
> 
> Digging around more I found this CL from ymalik@:
> https://codereview.chromium.org/1400213002, which is solving the same problem.
> 
> There is even TODO to replace innerWidth/Height with
> chrome.gpuBenchmarking.visualViewportWidth/Height once they roll in stable.
> 
> I checked that visualViewportWidth/Height do not depend on zoom and are
> available in reference builds. So I think using them instead of
> innerWidth/Height is the proper fix.
> 
> The latest patch set applies the fix to all actions.
> 
> wdyt?

visualViewportWidth and visualViewportHeight are currently equivalent to
innerWidth/Height. The change ymalik@ made was due to the
--inert-visual-viewport change which makes innerWidth/Height invariant under
pinch-zoom. In order to ship that flag we had to add these methods (some tests
did rely on innerWidth/Height being affected by pinch-zoom). However, we had to
unship --inert-visual-viewport so the two are currently equivalent and using
visualViewportWidth/Height will not help here, they *do* depend on zoom.

[ulan, 2016-07-13 21:09:11]

On 2016/07/13 19:26:50, bokan wrote:
> On 2016/07/13 19:04:07, ulan wrote:
> > [+ymalik]
> > 
> > Digging around more I found this CL from ymalik@:
> > https://codereview.chromium.org/1400213002, which is solving the same
problem.
> > 
> > There is even TODO to replace innerWidth/Height with
> > chrome.gpuBenchmarking.visualViewportWidth/Height once they roll in stable.
> > 
> > I checked that visualViewportWidth/Height do not depend on zoom and are
> > available in reference builds. So I think using them instead of
> > innerWidth/Height is the proper fix.
> > 
> > The latest patch set applies the fix to all actions.
> > 
> > wdyt?
> 
> visualViewportWidth and visualViewportHeight are currently equivalent to
> innerWidth/Height. The change ymalik@ made was due to the
> --inert-visual-viewport change which makes innerWidth/Height invariant under
> pinch-zoom. In order to ship that flag we had to add these methods (some tests
> did rely on innerWidth/Height being affected by pinch-zoom). However, we had
to
> unship --inert-visual-viewport so the two are currently equivalent and using
> visualViewportWidth/Height will not help here, they *do* depend on zoom.

Oh I see, desktop and android differ in these functions and I checked only
desktop.

On desktop I saw this:
window.outerHeight = 1555
window.innerHeight = 5876
chrome.gpuBenchmarking.visualViewportHeight() = 1469
chrome.gpuBenchmarking.pageScaleFactor() = 1

So pageScaleFactor was always 1 and visualViewportHeight represented the real
height.

But on android, visualViewportHeight is indeed equal to innerHeight and
innerHeight * pageScaleFactor = outerHeight.

Combining these desktop and mobile cases, looks like (visualViewportHeight *
pageScaleFactor) is what we need?

[bokan, 2016-07-13 21:13:44]

On 2016/07/13 21:09:11, ulan wrote:
> On 2016/07/13 19:26:50, bokan wrote:
> > On 2016/07/13 19:04:07, ulan wrote:
> > > [+ymalik]
> > > 
> > > Digging around more I found this CL from ymalik@:
> > > https://codereview.chromium.org/1400213002, which is solving the same
> problem.
> > > 
> > > There is even TODO to replace innerWidth/Height with
> > > chrome.gpuBenchmarking.visualViewportWidth/Height once they roll in
stable.
> > > 
> > > I checked that visualViewportWidth/Height do not depend on zoom and are
> > > available in reference builds. So I think using them instead of
> > > innerWidth/Height is the proper fix.
> > > 
> > > The latest patch set applies the fix to all actions.
> > > 
> > > wdyt?
> > 
> > visualViewportWidth and visualViewportHeight are currently equivalent to
> > innerWidth/Height. The change ymalik@ made was due to the
> > --inert-visual-viewport change which makes innerWidth/Height invariant under
> > pinch-zoom. In order to ship that flag we had to add these methods (some
tests
> > did rely on innerWidth/Height being affected by pinch-zoom). However, we had
> to
> > unship --inert-visual-viewport so the two are currently equivalent and using
> > visualViewportWidth/Height will not help here, they *do* depend on zoom.
> 
> Oh I see, desktop and android differ in these functions and I checked only
> desktop.
> 
> On desktop I saw this:
> window.outerHeight = 1555
> window.innerHeight = 5876
> chrome.gpuBenchmarking.visualViewportHeight() = 1469
> chrome.gpuBenchmarking.pageScaleFactor() = 1
> 
> So pageScaleFactor was always 1 and visualViewportHeight represented the real
> height.
> 
> But on android, visualViewportHeight is indeed equal to innerHeight and
> innerHeight * pageScaleFactor = outerHeight.
> 
> Combining these desktop and mobile cases, looks like (visualViewportHeight *
> pageScaleFactor) is what we need?

If pageScaleFactor() == 1 you aren't zoomed in. You were probably using browser
zoom (i.e. ctrl+/-) which is different from pinch-zoom. Both fields should
behave the same way on both desktop and mobile under pinch-zoom (though now I
realize that visualViewportHeight doesn't account for browser zoom, that should
be ok since mobile has no browser-zoom).

In any case, visualViewportHeight/Width * pageScaleFactor is what you want.

[ulan, 2016-07-14 11:51:16]

Description was changed from

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerWidth/Height can exceed
window.outerWidth/Height, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportWidth/Height
to compute scroll amount.

BUG=chromium:627166
==========

to

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerHeight/Width can exceed
window.outerHeight/Width, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportHeight/Width
adjusted with chrome.gpuBenchmarking.pageScaleFactor to
compute zoom-independent inner height/width of window.

BUG=chromium:627166
==========

[ulan, 2016-07-14 11:53:24]

On 2016/07/13 21:13:44, bokan wrote:
> On 2016/07/13 21:09:11, ulan wrote:
> > On 2016/07/13 19:26:50, bokan wrote:
> > > On 2016/07/13 19:04:07, ulan wrote:
> > > > [+ymalik]
> > > > 
> > > > Digging around more I found this CL from ymalik@:
> > > > https://codereview.chromium.org/1400213002, which is solving the same
> > problem.
> > > > 
> > > > There is even TODO to replace innerWidth/Height with
> > > > chrome.gpuBenchmarking.visualViewportWidth/Height once they roll in
> stable.
> > > > 
> > > > I checked that visualViewportWidth/Height do not depend on zoom and are
> > > > available in reference builds. So I think using them instead of
> > > > innerWidth/Height is the proper fix.
> > > > 
> > > > The latest patch set applies the fix to all actions.
> > > > 
> > > > wdyt?
> > > 
> > > visualViewportWidth and visualViewportHeight are currently equivalent to
> > > innerWidth/Height. The change ymalik@ made was due to the
> > > --inert-visual-viewport change which makes innerWidth/Height invariant
under
> > > pinch-zoom. In order to ship that flag we had to add these methods (some
> tests
> > > did rely on innerWidth/Height being affected by pinch-zoom). However, we
had
> > to
> > > unship --inert-visual-viewport so the two are currently equivalent and
using
> > > visualViewportWidth/Height will not help here, they *do* depend on zoom.
> > 
> > Oh I see, desktop and android differ in these functions and I checked only
> > desktop.
> > 
> > On desktop I saw this:
> > window.outerHeight = 1555
> > window.innerHeight = 5876
> > chrome.gpuBenchmarking.visualViewportHeight() = 1469
> > chrome.gpuBenchmarking.pageScaleFactor() = 1
> > 
> > So pageScaleFactor was always 1 and visualViewportHeight represented the
real
> > height.
> > 
> > But on android, visualViewportHeight is indeed equal to innerHeight and
> > innerHeight * pageScaleFactor = outerHeight.
> > 
> > Combining these desktop and mobile cases, looks like (visualViewportHeight *
> > pageScaleFactor) is what we need?
> 
> If pageScaleFactor() == 1 you aren't zoomed in. You were probably using
browser
> zoom (i.e. ctrl+/-) which is different from pinch-zoom. Both fields should
> behave the same way on both desktop and mobile under pinch-zoom (though now I
> realize that visualViewportHeight doesn't account for browser zoom, that
should
> be ok since mobile has no browser-zoom).
> 
> In any case, visualViewportHeight/Width * pageScaleFactor is what you want.

Thank you! The latest patch is now using visualViewportHeight/Width *
pageScaleFactor. PTAL.

[petrcermak, 2016-07-14 13:04:31]

LGTM if Ned is happy (please have a look at my comment regarding creating
gesture_common.py).

Thanks,
Petr

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/key_event_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/key_event_unittest.py:31: # Import
__GestureCommon_GetViewportHeight.
nednguyen: I'm wondering whether we should hide this in a library file (e.g.
telemetry/internal/actions/gesture_common.py) in a method like
InjectCommonJavaScriptGestureFunctions. WDYT?

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:78: if not
self._browser_info.HasRepeatableSynthesizeScrollGesture():
you could combine the two if statements:

if (not ... or
    not ...):
  return

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:91: 
nit: I suggest you remove blank lines 91 and 93 (to keep the action as a visual
block). Feel free to ignore if you disagree ;-)

[bokan, 2016-07-14 14:11:19]

The approach looks fine to me. I have a few notes and questions though.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:40: function
getViewportHeight() {
"Viewport" is a somewhat ambiguous term, I would actually call this
getRendererHeight, getWindowHeight, or getRendererViewportHeight. Something like
that.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:60: var
visualViewportHeight = getViewportHeight();
This method should take page scale into account though.

For example, if you have a device with a screen that's 100x100 pixels and you've
got a page that has a <div> that's 50x50 pixels. The page is unscrolled and the
<div> is at (0, 0). If we zoom in to the top left corner by 2X, the <div> is now
completely filling the screen so the rect we return should be [0, 0, 100, 100].

getBoundingRect(el) will return [0, 0, 50, 50]. getViewportHeight/Width() will
return 100, 100. The code below won't activate and we'll return [0, 0, 50, 50].

Anyway, this is currently broken. I have a patch up that fixes it:
https://codereview.chromium.org/1871453002/ but I've been waiting for the ref
builds to roll to pick up the pageScaleFactor method. You can leave it as is and
I'll fix once that happens.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:83:
self._tab.action_runner.PinchPage(scale_factor=0.1)
What is the intent here? To zoom out as far as possible?

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self.assertGreater(inner_height, outer_height)
This will never be true on desktops or mobile pages with viewport meta
width=device-width

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/scroll_unittest.py:16:
'document.documentElement.scrollTop || document.body.scrollTop'), 0)
Here and in other cases, you can use document.scrollingElement.scrollTop rather
than this ||.  Also, why does it follow that the page has to be unscrolled? I
think this assertion should be made at the callsite rather than in this
function.

[nednguyen, 2016-07-14 14:28:03]

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:8: (function() {
Fan we add some house keeping bit to skip running this if this has been run?

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/key_event_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/key_event_unittest.py:31: # Import
__GestureCommon_GetViewportHeight.
On 2016/07/14 13:04:30, petrcermak wrote:
> nednguyen: I'm wondering whether we should hide this in a library file (e.g.
> telemetry/internal/actions/gesture_common.py) in a method like
> InjectCommonJavaScriptGestureFunctions. WDYT?

This setUp seems quite common between unittest in actions/ dir, so I think we
can just add actions/unittest_base.py & add BaseActionTestCase class with this
setUp defined.

[nednguyen, 2016-07-14 14:33:19]

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:8: (function() {
On 2016/07/14 14:28:02, nednguyen wrote:
> Fan we add some house keeping bit to skip running this if this has been run?

Err, I meant "Can we...?"

[petrcermak, 2016-07-14 15:00:39]

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:8: (function() {
On 2016/07/14 14:33:19, nednguyen wrote:
> On 2016/07/14 14:28:02, nednguyen wrote:
> > Fan we add some house keeping bit to skip running this if this has been run?
> 
> Err, I meant "Can we...?"

+1

[ulan, 2016-07-14 17:20:41]

PTAL

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:8: (function() {
On 2016/07/14 15:00:39, petrcermak wrote:
> On 2016/07/14 14:33:19, nednguyen wrote:
> > On 2016/07/14 14:28:02, nednguyen wrote:
> > > Fan we add some house keeping bit to skip running this if this has been
run?
> > 
> > Err, I meant "Can we...?"
> 
> +1

Done.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:40: function
getViewportHeight() {
On 2016/07/14 14:11:19, bokan wrote:
> "Viewport" is a somewhat ambiguous term, I would actually call this
> getRendererHeight, getWindowHeight, or getRendererViewportHeight. Something
like
> that.

Done.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:60: var
visualViewportHeight = getViewportHeight();
On 2016/07/14 14:11:19, bokan wrote:
> This method should take page scale into account though.
> 
> For example, if you have a device with a screen that's 100x100 pixels and
you've
> got a page that has a <div> that's 50x50 pixels. The page is unscrolled and
the
> <div> is at (0, 0). If we zoom in to the top left corner by 2X, the <div> is
now
> completely filling the screen so the rect we return should be [0, 0, 100,
100].
> 
> getBoundingRect(el) will return [0, 0, 50, 50]. getViewportHeight/Width() will
> return 100, 100. The code below won't activate and we'll return [0, 0, 50,
50].
> 
> Anyway, this is currently broken. I have a patch up that fixes it:
> https://codereview.chromium.org/1871453002/ but I've been waiting for the ref
> builds to roll to pick up the pageScaleFactor method. You can leave it as is
and
> I'll fix once that happens.

Acknowledged.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/key_event_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/key_event_unittest.py:31: # Import
__GestureCommon_GetViewportHeight.
On 2016/07/14 13:04:30, petrcermak wrote:
> nednguyen: I'm wondering whether we should hide this in a library file (e.g.
> telemetry/internal/actions/gesture_common.py) in a method like
> InjectCommonJavaScriptGestureFunctions. WDYT?

Done. I added utils.InjectJavaScript(tab, jsFileName) function.

I would prefer not to move this code into base test class because not all action
tests require injecting js file.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:78: if not
self._browser_info.HasRepeatableSynthesizeScrollGesture():
On 2016/07/14 13:04:30, petrcermak wrote:
> you could combine the two if statements:
> 
> if (not ... or
>     not ...):
>   return

Done.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:83:
self._tab.action_runner.PinchPage(scale_factor=0.1)
On 2016/07/14 14:11:19, bokan wrote:
> What is the intent here? To zoom out as far as possible?

The intent is to zoom out far enough so that scrolling computed with
windown.innerHeight produces touch coordinates outside screen.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self.assertGreater(inner_height, outer_height)
On 2016/07/14 14:11:19, bokan wrote:
> This will never be true on desktops or mobile pages with viewport meta
> width=device-width

It should be ok here because we are loading blank.html.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:91: 
On 2016/07/14 13:04:30, petrcermak wrote:
> nit: I suggest you remove blank lines 91 and 93 (to keep the action as a
visual
> block). Feel free to ignore if you disagree ;-)

Done.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/scroll_unittest.py:16:
'document.documentElement.scrollTop || document.body.scrollTop'), 0)
On 2016/07/14 14:11:19, bokan wrote:
> Here and in other cases, you can use document.scrollingElement.scrollTop
rather
> than this ||.  Also, why does it follow that the page has to be unscrolled? I
> think this assertion should be made at the callsite rather than in this
> function. 

Done.

[nednguyen, 2016-07-14 19:18:27]

lgtm

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/key_event_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/key_event_unittest.py:31: # Import
__GestureCommon_GetViewportHeight.
On 2016/07/14 17:20:41, ulan wrote:
> On 2016/07/14 13:04:30, petrcermak wrote:
> > nednguyen: I'm wondering whether we should hide this in a library file (e.g.
> > telemetry/internal/actions/gesture_common.py) in a method like
> > InjectCommonJavaScriptGestureFunctions. WDYT?
> 
> Done. I added utils.InjectJavaScript(tab, jsFileName) function.
> 
> I would prefer not to move this code into base test class because not all
action
> tests require injecting js file.

Looks like you forgot to upload utils.py?

[bokan, 2016-07-14 21:08:58]

Thanks, just a few more followups

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:83:
self._tab.action_runner.PinchPage(scale_factor=0.1)
On 2016/07/14 17:20:41, ulan wrote:
> On 2016/07/14 14:11:19, bokan wrote:
> > What is the intent here? To zoom out as far as possible?
> 
> The intent is to zoom out far enough so that scrolling computed with
> windown.innerHeight produces touch coordinates outside screen.

I don't get this, we calculate scroll using GetWindowHeight now right? That's
independent of zoom so you shouldn't have to zoom out. In fact, zooming out
might make the page unscrollable again.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self.assertGreater(inner_height, outer_height)
On 2016/07/14 17:20:41, ulan wrote:
> On 2016/07/14 14:11:19, bokan wrote:
> > This will never be true on desktops or mobile pages with viewport meta
> > width=device-width
> 
> It should be ok here because we are loading blank.html.

Right, but on a desktop, for example, it won't be true regardless of the page.

https://codereview.chromium.org/2138883002/diff/280001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/280001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:64: var
visualViewportHeight = getWindowHeight();
s/visualViewport/window

[ulan, 2016-07-15 09:42:42]

PTAL. I added the missing utils.py and addressed comments

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:83:
self._tab.action_runner.PinchPage(scale_factor=0.1)
On 2016/07/14 21:08:58, bokan wrote:
> On 2016/07/14 17:20:41, ulan wrote:
> > On 2016/07/14 14:11:19, bokan wrote:
> > > What is the intent here? To zoom out as far as possible?
> > 
> > The intent is to zoom out far enough so that scrolling computed with
> > windown.innerHeight produces touch coordinates outside screen.
> 
> I don't get this, we calculate scroll using GetWindowHeight now right? That's
> independent of zoom so you shouldn't have to zoom out. In fact, zooming out
> might make the page unscrollable again.

It is intended as a regression test if we ever start to use zoom dependent
window height. Added a comment for this test.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:88:
self.assertGreater(inner_height, outer_height)
On 2016/07/14 21:08:58, bokan wrote:
> On 2016/07/14 17:20:41, ulan wrote:
> > On 2016/07/14 14:11:19, bokan wrote:
> > > This will never be true on desktops or mobile pages with viewport meta
> > > width=device-width
> > 
> > It should be ok here because we are loading blank.html.
> 
> Right, but on a desktop, for example, it won't be true regardless of the page.

I added a comment that the test should be enabled for Android only.

https://codereview.chromium.org/2138883002/diff/280001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/2138883002/diff/280001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/gesture_common.js:64: var
visualViewportHeight = getWindowHeight();
On 2016/07/14 21:08:58, bokan wrote:
> s/visualViewport/window

Done.

[bokan, 2016-07-15 14:19:24]

lgtm, thanks.

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
File telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py (right):

https://codereview.chromium.org/2138883002/diff/260001/telemetry/telemetry/in...
telemetry/telemetry/internal/actions/repeatable_scroll_unittest.py:83:
self._tab.action_runner.PinchPage(scale_factor=0.1)
On 2016/07/15 09:42:41, ulan wrote:
> On 2016/07/14 21:08:58, bokan wrote:
> > On 2016/07/14 17:20:41, ulan wrote:
> > > On 2016/07/14 14:11:19, bokan wrote:
> > > > What is the intent here? To zoom out as far as possible?
> > > 
> > > The intent is to zoom out far enough so that scrolling computed with
> > > windown.innerHeight produces touch coordinates outside screen.
> > 
> > I don't get this, we calculate scroll using GetWindowHeight now right?
That's
> > independent of zoom so you shouldn't have to zoom out. In fact, zooming out
> > might make the page unscrollable again.
> 
> It is intended as a regression test if we ever start to use zoom dependent
> window height. Added a comment for this test.

Ah, sorry, I missed that. Ok, looks good.

[ulan, 2016-07-18 11:31:36]

The CQ bit was checked by ulan@chromium.org

[ulan, 2016-07-18 11:31:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2138883002/#ps300001
(title: "Address comments")

[commit-bot: I haz the power, 2016-07-18 11:31:45]

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-07-18 11:57:24]

Description was changed from

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerHeight/Width can exceed
window.outerHeight/Width, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportHeight/Width
adjusted with chrome.gpuBenchmarking.pageScaleFactor to
compute zoom-independent inner height/width of window.

BUG=chromium:627166
==========

to

==========
Fix computation of scroll amount in scroll actions.

During page load window.innerHeight/Width can exceed
window.outerHeight/Width, which results in synthetic
touch coordinates being outside the screen.

Chrome crashes with assertion failure for such invalid
coordinates.

This patch uses chrome.gpuBenchmarking.visualViewportHeight/Width
adjusted with chrome.gpuBenchmarking.pageScaleFactor to
compute zoom-independent inner height/width of window.

BUG=chromium:627166

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-07-18 11:57:26]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...