Fix coordinate calculations under pinch-zoom

Fix coordinate calculations under pinch-zoom

This CL fixes how telemetry calculates coordinates under pinch-zoom.
This has been broken because pinch-zoom is more than just a
multiplication by a scale, it also includes a translation to account for
the visual viewport's offset within the root frame. Thus, to convert a
coordinate in the root frame to a coordinate in the visual viewport
(i.e. the user's screen), we use:

(x, y) = ((x1, y1) - visualViewportOffset) * pageScaleFactor

However, this change also requires that gpuBenchmarking's methods know
that incoming coordinates are relative to the visual viewport.
gpuBenchmarking today tries to apply page scale to most coordinates.
There is a follow up patch that removes the pageScaleFactor application
in gpuBenchmarking: https://crrev.com/c/668376

Hoewver, until gpuBenchmarking is changed, we don't want to change the
coordinate system we use in Catapult but we can't change gpuBenchmarking
until the Catapult repo changes are rolled into Chrome. This, I've gated
the changes here on an attribute that the above patch will add to
gpuBenchmarking. Once both changes land I can remove the attribute and
the old code in this patch.

In addition, the PinchAction supported pinching-in on a specific
Element. This functionality is broken and unused so I've remoted it. The
telemetry code always tries to zoom into the center of the viewport.
I've also updated pageScaleFactor calculation to take account of the
inert visualViewport changes which means that innerWidth / outerWidth
no longer represents the scale factor.

[bokan, 2017-09-15 09:52:22]

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

[commit-bot: I haz the power, 2017-09-15 09: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, 2017-09-15 09:55:22]

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

[commit-bot: I haz the power, 2017-09-15 09:55:23]

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

[bokan, 2017-09-15 10:36:30]

Description was changed from

==========
Fix coordinate calculations under pinch-zoom

This CL fixes how telemetry calculates coordinates under pinch-zoom.
This has been broken because pinch-zoom is more than just a
multiplication by a scale, it also includes a translation to account for
the visual viewport's offset within the root frame. Thus, to convert a
coordinate in the root frame to a coordinate in the visual viewport
(i.e. the user's screen), we use:

(x, y) = ((x1, y1) - visualViewportOffset) * pageScaleFactor

However, this change also requires that gpuBenchmarking's methods know
that incoming coordinates are relative to the visual viewport.
gpuBenchmarking today tries to apply page scale to most coordinates.
There is a follow up patch that removes the pageScaleFactor application
in gpuBenchmarking: https://crrev.com/c/668376

Hoewver, until gpuBenchmarking is changed, we don't want to change the
coordinate system we use in Catapult but we can't change gpuBenchmarking
until the Catapult repo changes are rolled into Chrome. This, I've gated
the changes here on an attribute that the above patch will add to
gpuBenchmarking. Once both changes land I can remove the attribute and
the old code in this patch.

In addition, the PinchAction supported pinching-in on a specific
Element. This functionality is broken and unused so I've remoted it. The
telemetry code always tries to zoom into the center of the viewport.
I've also updated pageScaleFactor calculation to take account of the
inert visualViewport changes which means that innerWidth / outerWidth
no longer represents the scale factor.
==========

to

==========
Fix coordinate calculations under pinch-zoom

This CL fixes how telemetry calculates coordinates under pinch-zoom.
This has been broken because pinch-zoom is more than just a
multiplication by a scale, it also includes a translation to account for
the visual viewport's offset within the root frame. Thus, to convert a
coordinate in the root frame to a coordinate in the visual viewport
(i.e. the user's screen), we use:

(x, y) = ((x1, y1) - visualViewportOffset) * pageScaleFactor

However, this change also requires that gpuBenchmarking's methods know
that incoming coordinates are relative to the visual viewport.
gpuBenchmarking today tries to apply page scale to most coordinates.
There is a follow up patch that removes the pageScaleFactor application
in gpuBenchmarking: https://crrev.com/c/668376

Hoewver, until gpuBenchmarking is changed, we don't want to change the
coordinate system we use in Catapult but we can't change gpuBenchmarking
until the Catapult repo changes are rolled into Chrome. This, I've gated
the changes here on an attribute that the above patch will add to
gpuBenchmarking. Once both changes land I can remove the attribute and
the old code in this patch.

In addition, the PinchAction supported pinching-in on a specific
Element. This functionality is broken and unused so I've remoted it. The
telemetry code always tries to zoom into the center of the viewport.
I've also updated pageScaleFactor calculation to take account of the
inert visualViewport changes which means that innerWidth / outerWidth
no longer represents the scale factor.
==========

[bokan, 2017-09-15 10:36:30]

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

[bokan, 2017-09-15 10:37:27]

Hi Ned,

Not really sure who should review this - feel free to punt to someone else if
you know.

I set this up to be gated on the changes that will also need to be landed in
gpuBenchmarking, is this typically how it's done?

[nednguyen, 2017-09-15 10:49:58]

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

[nednguyen, 2017-09-15 10:49:59]

I am not an expert to review the math here, Eric: can you review this as you are
owner of smoothness.desktop_tough_pinch_zoom_cases ?

[ericrk, 2017-09-15 16:41:04]

I think this looks good. A few small questions.

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/action_runner.py (left):

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/action_runner.py:374: def
PinchElement(self, selector=None, text=None, element_function=None,
Thanks for cleaning this up!

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/gesture_common.js:58: if
('gesturesExpectedInViewportCoordinates' in chrome.gpuBenchmarking) {
So, to make sure I understand, this code is the new code that will run once
https://chromium-review.googlesource.com/c/chromium/src/+/668376 lands? So is
this the permanent code, and once everything's done the "if ('gesture...." will
be gone but the code block will remain?

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/gesture_common.js:66: rect.width *= scale;
So, I think your code is correct, but I want to make sure I'm understanding
correctly:

chrome.gpuBenchmarking.visualViewportX/Y return a value in non-scaled page
coordinates, meaning that when we subtract / scale below, everything works out.
This isn't immediately intuitive to me, as it feels like these values could
either be pre or post-scale. Can you maybe add a comment indicating that
visualViewportX/Y are non-scaled coordinates in the page?

[bokan, 2017-09-17 05:48:51]

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
File telemetry/telemetry/internal/actions/gesture_common.js (right):

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/gesture_common.js:58: if
('gesturesExpectedInViewportCoordinates' in chrome.gpuBenchmarking) {
On 2017/09/15 16:41:04, ericrk wrote:
> So, to make sure I understand, this code is the new code that will run once
> https://chromium-review.googlesource.com/c/chromium/src/+/668376 lands? So is
> this the permanent code, and once everything's done the "if ('gesture...."
will
> be gone but the code block will remain?

That's right. Once that CL lands the code guarded by
gesturesExpectedInViewportCoordinates will be in effect so I'll remove the guard
here (and the `else` block in pinch.js). I'll then remove
gesturesExpectedInViewportCoordinates from gpuBenchmarking.

https://codereview.chromium.org/3017493002/diff/1/telemetry/telemetry/interna...
telemetry/telemetry/internal/actions/gesture_common.js:66: rect.width *= scale;
On 2017/09/15 16:41:04, ericrk wrote:
> So, I think your code is correct, but I want to make sure I'm understanding
> correctly:
> 
> chrome.gpuBenchmarking.visualViewportX/Y return a value in non-scaled page
> coordinates, meaning that when we subtract / scale below, everything works
out.
> This isn't immediately intuitive to me, as it feels like these values could
> either be pre or post-scale. Can you maybe add a comment indicating that
> visualViewportX/Y are non-scaled coordinates in the page?

Your understanding is correct. These values are in CSS pixels, when we scale the
page, we're changing the size of the CSS pixel so the values themselves don't
change. Width/Height are also in CSS pixels but they change as we scale because
the physical viewport is a fixed size so fewer or more CSS pixels now fit into
it.

Thanks for pointing out the lack of documentation, I sometimes forget this stuff
isn't intimitely familiar to anyone but me :). I've added documentation here and
in gpuBenchmarking.

[bokan, 2017-09-19 04:18:11]

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

[commit-bot: I haz the power, 2017-09-19 04:18:19]

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, 2017-09-19 04:21:48]

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

[commit-bot: I haz the power, 2017-09-19 04:21:50]

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

[bokan, 2017-09-25 19:55:13]

Post-BlinkON ping :)