Refactor browser process coordinate transformation methods

content/browser/renderer_host/delegated_frame_host.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/delegated_frame_host.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 205 matching lines @@
   cc::SurfaceHittest hittest(delegate, GetSurfaceManager());
   gfx::Transform target_transform;
   cc::SurfaceId target_surface_id =
       hittest.GetTargetSurfaceAtPoint(surface_id_, point, &target_transform);
   *transformed_point = point;
   if (!target_surface_id.is_null())
     target_transform.TransformPoint(transformed_point);
   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 {
   // Should skip a frame only when another frame from the renderer is guaranteed
   // to replace it. Otherwise may cause hangs when the renderer is waiting for
   // the completion of latency infos (such as when taking a Snapshot.)
   if (can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
       can_lock_compositor_ == NO_PENDING_COMMIT || !resize_lock_.get())
     return false;
 
@@ skipping 649 matching lines @@
     cc::SurfaceManager* manager = factory->GetSurfaceManager();
     new_layer->SetShowSurface(
         surface_id_, base::Bind(&SatisfyCallback, base::Unretained(manager)),
         base::Bind(&RequireCallback, base::Unretained(manager)),
         current_surface_size_, current_scale_factor_,
         current_frame_size_in_dip_);
   }
 }
 
 }  // namespace content