Fix coordinate calculations under pinch-zoom

telemetry/telemetry/internal/actions/gesture_common.js

Index: telemetry/telemetry/internal/actions/gesture_common.js
diff --git a/telemetry/telemetry/internal/actions/gesture_common.js b/telemetry/telemetry/internal/actions/gesture_common.js
index ec76ebaedb258ba8a7a2db3eec455dd3baee7273..66b0e23174c33ef04c4de63478b92d95a9a93df4 100644
--- a/telemetry/telemetry/internal/actions/gesture_common.js
+++ b/telemetry/telemetry/internal/actions/gesture_common.js
@@ -11,7 +11,7 @@
   if (window.__GestureCommon_GetBoundingVisibleRect)
     return;
 
-  // Returns the bounding rectangle wrt to the top-most document.
+  // Returns the bounding rectangle wrt to the layout viewport.
   function getBoundingRect(el) {
     var clientRect = el.getBoundingClientRect();
     var bound = { left: clientRect.left,
@@ -32,35 +32,44 @@
     return bound;
   }
 
-  // TODO(ulan): Remove this function once
-  // chrome.gpuBenchmarking.pageScaleFactor is available in reference builds.
-  function getPageScaleFactor() {
-    if (chrome.gpuBenchmarking.pageScaleFactor)
-      return chrome.gpuBenchmarking.pageScaleFactor();
-    return 1;
-  }
-
   // Zoom-independent window height. See crbug.com/627123 for more details.
   function getWindowHeight() {
-    return getPageScaleFactor() * chrome.gpuBenchmarking.visualViewportHeight();
+    return chrome.gpuBenchmarking.pageScaleFactor() *
+        chrome.gpuBenchmarking.visualViewportHeight();
   }
 
   // Zoom-independent window width. See crbug.com/627123 for more details.
   function getWindowWidth() {
-    return getPageScaleFactor() * chrome.gpuBenchmarking.visualViewportWidth();
+    return chrome.gpuBenchmarking.pageScaleFactor() *
+        chrome.gpuBenchmarking.visualViewportWidth();
   }
 
   function clamp(min, value, max) {
     return Math.min(Math.max(min, value), max);
   }
 
+  // Returns the bounding rect in the visual viewport's coordinates.
   function getBoundingVisibleRect(el) {
     // Get the element bounding rect.
     var rect = getBoundingRect(el);
 
+    // TODO(bokan): Remove this once gpuBenchmarking is changed to take all
+    // coordinates in viewport space. crbug.com/610021.
+    if ('gesturesExpectedInViewportCoordinates' in chrome.gpuBenchmarking) {

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

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?

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

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.

+      // Apply the visual viewport transform (i.e. pinch-zoom) to the bounding
+      // rect.
+      const scale = chrome.gpuBenchmarking.pageScaleFactor();
+      const visualViewportX = chrome.gpuBenchmarking.visualViewportX();
+      const visualViewportY = chrome.gpuBenchmarking.visualViewportY();
+      rect.top = (rect.top - visualViewportY) * scale;
+      rect.left = (rect.left - visualViewportX) * scale;
+      rect.width *= scale;

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

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]

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.

+      rect.height *= scale;
+    }
+
     // Get the window dimensions.
-    var windowHeight = getWindowHeight();
-    var windowWidth = getWindowWidth();
+    const windowHeight = getWindowHeight();
+    const windowWidth = getWindowWidth();
 
     // Then clip the rect to the screen size.
     rect.top = clamp(0, rect.top, windowHeight);