Refactor browser process coordinate transformation methods

content/browser/renderer_host/delegated_frame_host.cc

Index: content/browser/renderer_host/delegated_frame_host.cc
diff --git a/content/browser/renderer_host/delegated_frame_host.cc b/content/browser/renderer_host/delegated_frame_host.cc
index 3ea311ce025522466402e7eb7e7258f9aaf67485..7d1fc46ebd3395802cdaf45472e1e90aac026744 100644
--- a/content/browser/renderer_host/delegated_frame_host.cc
+++ b/content/browser/renderer_host/delegated_frame_host.cc
@@ -223,21 +223,27 @@ cc::SurfaceId DelegatedFrameHost::SurfaceIdAtPoint(
   return target_surface_id;
 }
 
-void DelegatedFrameHost::TransformPointToLocalCoordSpace(
+gfx::Point DelegatedFrameHost::TransformPointToLocalCoordSpace(
     const gfx::Point& point,
-    const cc::SurfaceId& original_surface,
-    gfx::Point* transformed_point) {
-  *transformed_point = point;
+    const cc::SurfaceId& original_surface) {
   if (surface_id_.is_null() || original_surface == surface_id_)
-    return;
+    return point;
 
-  gfx::Transform transform;
+  gfx::Point transformed_point = point;
   cc::SurfaceHittest hittest(nullptr, GetSurfaceManager());
-  if (hittest.GetTransformToTargetSurface(surface_id_, original_surface,
-                                          &transform) &&
-      transform.GetInverse(&transform)) {
-    transform.TransformPoint(transformed_point);
-  }
+  if (!hittest.TransformPointToTargetSurface(original_surface, surface_id_,
+                                             &transformed_point))
+    DCHECK(false);

[nasko, 2016/07/29 14:19:46]

This DCHECK is hitting in an OOPIF fullscreen test.

Another note - without {} around the DCHECK, would it be removed by the
preprocessor in release build, putting the return statement to be in the
conditional? Maybe just add {} to be on the safe side?

[kenrb, 2016/07/29 16:20:22]

This is annoying, but I think the best thing is to remove the DCHECK here. The
problem seems to be that in tests we can load a page and then hide the tab
before the compositor frames have all finished propagating, making coordinate
transformation impossible. This is a race condition and potentially a lot of
tests could do things like that, so it might be best just to allow the
transformation to silently fail in that case. It shouldn't be a problem because
the page with the bad transformation is not visible.
DCHECK is defined as (void (0)) in release configurations to prevent that sort
of thing. If it was completely removed this method wouldn't compile because it
would have a path with no return statement.

+  return transformed_point;
+}
+
+gfx::Point DelegatedFrameHost::TransformPointToCoordSpaceForView(
+    const gfx::Point& point,
+    RenderWidgetHostViewBase* target_view) {
+  if (surface_id_.is_null())
+    return point;
+
+  return target_view->TransformPointToLocalCoordSpace(point, surface_id_);
 }
 
 bool DelegatedFrameHost::ShouldSkipFrame(gfx::Size size_in_dip) const {