Remove lossy ConvertPointToDIP() methods.

The Convert*ToDIP() and Convert*ToPixel() functions are problematic in
that they hide what decision is being taken in the underlying code
when moving from floating point to integer, and consumers should be
thinking about what they want for correctness. They also act in
inconsistent ways when converting a Rect vs its components. This
removes the functions that perform float->int conversions in order to
have callers show what they intend to happen and make it clear that
data is being lost in some fashion.

R=sky@chromium.org

Bug: 1130050
Change-Id: I5fc16143e7425d84ee0d6cfc55b863063d725f94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2418805Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Auto-Submit: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809789}

ash/display/cursor_window_controller.cc

@@ -35,6 +35,7 @@
 #include "ui/display/screen.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/views/widget/widget.h"
@@ -321,16 +322,17 @@ void CursorWindowController::UpdateCursorImage() {
       ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(original_scale));
 
   gfx::ImageSkia image;
+  gfx::Point hot_point_in_physical_pixels;
   if (cursor_.type() == ui::mojom::CursorType::kCustom) {
     const SkBitmap& bitmap = cursor_.custom_bitmap();
     if (bitmap.isNull())
       return;
     image = gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
-    hot_point_ = cursor_.custom_hotspot();
+    hot_point_in_physical_pixels = cursor_.custom_hotspot();
   } else {
     int resource_id;
     if (!ui::GetCursorDataFor(cursor_size_, cursor_.type(), cursor_scale,
-                              &resource_id, &hot_point_)) {
+                              &resource_id, &hot_point_in_physical_pixels)) {
       return;
     }
     image =
@@ -350,14 +352,17 @@ void CursorWindowController::UpdateCursorImage() {
     resized = gfx::ImageSkiaOperations::CreateResizedImage(
         image, skia::ImageOperations::ResizeMethod::RESIZE_GOOD,
         gfx::ScaleToCeiledSize(image.size(), rescale));
-    hot_point_ = gfx::ScaleToCeiledPoint(hot_point_, rescale);
+    hot_point_in_physical_pixels =
+        gfx::ScaleToCeiledPoint(hot_point_in_physical_pixels, rescale);
   }
 
   const gfx::ImageSkiaRep& image_rep = resized.GetRepresentation(cursor_scale);
   delegate_->SetCursorImage(resized.size(),
                             gfx::ImageSkia(gfx::ImageSkiaRep(
                                 GetAdjustedBitmap(image_rep), cursor_scale)));
-  hot_point_ = gfx::ConvertPointToDIP(cursor_scale, hot_point_);
+  // TODO(danakj): Should this be rounded? Or kept as a floating point?
+  hot_point_ = gfx::ToFlooredPoint(
+      gfx::ConvertPointToDips(hot_point_in_physical_pixels, cursor_scale));
 
   if (cursor_view_widget_) {
     static_cast<cursor::CursorView*>(cursor_view_widget_->GetContentsView())

ash/magnifier/magnification_controller.cc

@@ -27,7 +27,6 @@
 #include "ui/base/ime/chromeos/ime_bridge.h"
 #include "ui/base/ime/input_method.h"
 #include "ui/base/ime/text_input_client.h"
-#include "ui/compositor/dip_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/display/display.h"
@@ -35,6 +34,7 @@
 #include "ui/events/event.h"
 #include "ui/events/gestures/gesture_provider_aura.h"
 #include "ui/events/types/event_type.h"
+#include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/point3_f.h"
 #include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/rect_conversions.h"
@@ -593,13 +593,14 @@ ui::EventDispatchDetails MagnificationController::RewriteEvent(
   return SendEvent(continuation, &event);
 }
 
-bool MagnificationController::Redraw(const gfx::PointF& position,
-                                     float scale,
-                                     bool animate) {
-  const gfx::PointF position_in_dip =
-      ui::ConvertPointToDIP(root_window_->layer(), position);
-  return RedrawDIP(position_in_dip, scale,
-                   animate ? kDefaultAnimationDurationInMs : 0,
+bool MagnificationController::Redraw(
+    const gfx::PointF& position_in_physical_pixels,
+    float scale,
+    bool animate) {
+  gfx::PointF position =
+      gfx::ConvertPointToDips(position_in_physical_pixels,
+                              root_window_->layer()->device_scale_factor());
+  return RedrawDIP(position, scale, animate ? kDefaultAnimationDurationInMs : 0,
                    kDefaultAnimationTweenType);
 }
 

ash/magnifier/magnification_controller.h

@@ -172,7 +172,9 @@ class ASH_EXPORT MagnificationController : public ui::EventHandler,
   // given scale. Returns true if the window is changed; otherwise, false.
   // These methods should be called internally just after the scale and/or
   // the position are changed to redraw the window.
-  bool Redraw(const gfx::PointF& position, float scale, bool animate);
+  bool Redraw(const gfx::PointF& position_in_physical_pixels,
+              float scale,
+              bool animate);
 
   // Redraws the magnification window with the given origin position in dip and
   // the given scale. Returns true if the window is changed; otherwise, false.

content/browser/renderer_host/render_widget_host_input_event_router.cc

@@ -562,7 +562,7 @@ RenderWidgetTargetResult RenderWidgetHostInputEventRouter::FindViewAtLocation(
   frame_sink_id = target.frame_sink_id;
   if (frame_sink_id.is_valid()) {
     *transformed_point =
-        gfx::ConvertPointToDIP(device_scale_factor, target.location_in_target);
+        gfx::ConvertPointToDips(target.location_in_target, device_scale_factor);
   } else {
     *transformed_point = point;
   }

content/browser/renderer_host/render_widget_host_view_base.cc

@@ -802,12 +802,14 @@ bool RenderWidgetHostViewBase::TransformPointToTargetCoordSpace(
                               &transform_root_to_original);
   if (!transform_root_to_original.TransformPointReverse(&point_in_pixels))
     return false;
+  gfx::PointF transformed_point_in_physical_pixels;
   if (!query->TransformLocationForTarget(
-          target_ancestors, point_in_pixels.AsPointF(), transformed_point)) {
+          target_ancestors, point_in_pixels.AsPointF(),
+          &transformed_point_in_physical_pixels)) {
     return false;
   }
-  *transformed_point =
-      gfx::ConvertPointToDIP(device_scale_factor, *transformed_point);
+  *transformed_point = gfx::ConvertPointToDips(
+      transformed_point_in_physical_pixels, device_scale_factor);
   return true;
 }
 

third_party/blink/renderer/platform/widget/widget_base.cc

@@ -1255,15 +1255,17 @@ bool WidgetBase::ComputePreferCompositingToLCDText() {
 gfx::PointF WidgetBase::DIPsToBlinkSpace(const gfx::PointF& point) {
   if (!use_zoom_for_dsf_)
     return point;
-  return gfx::ConvertPointToPixel(
-      client_->GetOriginalScreenInfo().device_scale_factor, point);
+  // TODO(danakj): Should this be GetScreenInfo() so it changes under emulation?
+  return gfx::ScalePoint(point,
+                         client_->GetOriginalScreenInfo().device_scale_factor);
 }
 
 gfx::PointF WidgetBase::BlinkSpaceToDIPs(const gfx::PointF& point) {
   if (!use_zoom_for_dsf_)
     return point;
-  return gfx::ConvertPointToDIP(
-      client_->GetOriginalScreenInfo().device_scale_factor, point);
+  // TODO(danakj): Should this be GetScreenInfo() so it changes under emulation?
+  return gfx::ScalePoint(
+      point, 1.f / client_->GetOriginalScreenInfo().device_scale_factor);
 }
 
 gfx::Size WidgetBase::DIPsToBlinkSpace(const gfx::Size& size) {

ui/accelerated_widget_mac/ca_layer_tree_unittest_mac.mm

@@ -11,6 +11,7 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/accelerated_widget_mac/ca_renderer_layer_tree.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/mac/io_surface.h"
 #include "ui/gl/ca_renderer_layer_params.h"
 #include "ui/gl/gl_image_io_surface.h"
@@ -459,8 +460,8 @@ class CALayerTreePropertyUpdatesTest : public CALayerTreeTest {
       EXPECT_EQ(gfx::ConvertRectToDIP(properties.scale_factor,
                                       gfx::Rect(properties.clip_rect.size())),
                 gfx::Rect([clip_and_sorting_layer bounds]));
-      EXPECT_EQ(gfx::ConvertPointToDIP(properties.scale_factor,
-                                       properties.clip_rect.origin()),
+      EXPECT_EQ(gfx::ToFlooredPoint(gfx::ConvertPointToDips(
+                    properties.clip_rect.origin(), properties.scale_factor)),
                 gfx::Point([clip_and_sorting_layer position]));
       EXPECT_EQ(-properties.clip_rect.origin().x() / properties.scale_factor,
                 [clip_and_sorting_layer sublayerTransform].m41);
@@ -480,8 +481,8 @@ class CALayerTreePropertyUpdatesTest : public CALayerTreeTest {
                 [content_layer contents]);
       EXPECT_EQ(properties.contents_rect,
                 gfx::RectF([content_layer contentsRect]));
-      EXPECT_EQ(gfx::ConvertPointToDIP(properties.scale_factor,
-                                       properties.rect.origin()),
+      EXPECT_EQ(gfx::ToFlooredPoint(gfx::ConvertPointToDips(
+                    properties.rect.origin(), properties.scale_factor)),
                 gfx::Point([content_layer position]));
       EXPECT_EQ(gfx::ConvertRectToDIP(properties.scale_factor,
                                       gfx::Rect(properties.rect.size())),

ui/compositor/dip_util.cc

@@ -23,16 +23,6 @@
 
 namespace ui {
 
-gfx::Point ConvertPointToDIP(const Layer* layer,
-                             const gfx::Point& point_in_pixel) {
-  return gfx::ConvertPointToDIP(layer->device_scale_factor(), point_in_pixel);
-}
-
-gfx::PointF ConvertPointToDIP(const Layer* layer,
-                              const gfx::PointF& point_in_pixel) {
-  return gfx::ConvertPointToDIP(layer->device_scale_factor(), point_in_pixel);
-}
-
 gfx::Size ConvertSizeToDIP(const Layer* layer,
                            const gfx::Size& size_in_pixel) {
   return gfx::ConvertSizeToDIP(layer->device_scale_factor(), size_in_pixel);

ui/compositor/dip_util.h

@@ -19,12 +19,6 @@ class Layer;
 
 // Utility functions that convert point/size/rect between
 // DIP and pixel coordinates system.
-COMPOSITOR_EXPORT gfx::Point ConvertPointToDIP(
-    const Layer* layer,
-    const gfx::Point& point_in_pixel);
-COMPOSITOR_EXPORT gfx::PointF ConvertPointToDIP(
-    const Layer* layer,
-    const gfx::PointF& point_in_pixel);
 COMPOSITOR_EXPORT gfx::Size ConvertSizeToDIP(
     const Layer* layer,
     const gfx::Size& size_in_pixel);

ui/gfx/geometry/dip_util.cc

@@ -15,23 +15,21 @@
 
 namespace gfx {
 
-Insets ConvertInsetsToDIP(float scale_factor,
-                          const gfx::Insets& insets_in_pixel) {
-  if (scale_factor == 1.f)
-    return insets_in_pixel;
-  return insets_in_pixel.Scale(1.f / scale_factor);
+PointF ConvertPointToDips(const Point& point_in_pixels,
+                          float device_scale_factor) {
+  return ScalePoint(PointF(point_in_pixels), 1.f / device_scale_factor);
 }
 
-Point ConvertPointToDIP(float scale_factor, const Point& point_in_pixel) {
-  if (scale_factor == 1.f)
-    return point_in_pixel;
-  return ScaleToFlooredPoint(point_in_pixel, 1.f / scale_factor);
+PointF ConvertPointToDips(const PointF& point_in_pixels,
+                          float device_scale_factor) {
+  return ScalePoint(point_in_pixels, 1.f / device_scale_factor);
 }
 
-PointF ConvertPointToDIP(float scale_factor, const PointF& point_in_pixel) {
+Insets ConvertInsetsToDIP(float scale_factor,
+                          const gfx::Insets& insets_in_pixel) {
   if (scale_factor == 1.f)
-    return point_in_pixel;
-  return ScalePoint(point_in_pixel, 1.f / scale_factor);
+    return insets_in_pixel;
+  return insets_in_pixel.Scale(1.f / scale_factor);
 }
 
 Size ConvertSizeToDIP(float scale_factor, const Size& size_in_pixel) {

ui/gfx/geometry/dip_util.h

@@ -15,14 +15,27 @@ class PointF;
 class Rect;
 class Size;
 
+// This file contains helper functions to move between DIPs (device-independent
+// pixels) and physical pixels, by multiplying or dividing by device scale
+// factor. These help show the intent of the caller by naming the operation,
+// instead of directly performing a scale operation. More complicated
+// transformations between coordinate spaces than DIP<->physical pixels should
+// be done via more explicit means.
+//
+// Note that functions that receive integer values will convert them to floating
+// point values, which can itself be a lossy operation for large integers. The
+// intention of these methods is to be used for UI values which are relatively
+// small.
+GEOMETRY_EXPORT gfx::PointF ConvertPointToDips(
+    const gfx::Point& point_in_pixels,
+    float device_scale_factor);
+GEOMETRY_EXPORT gfx::PointF ConvertPointToDips(
+    const gfx::PointF& point_in_pixels,
+    float device_scale_factor);
+
 GEOMETRY_EXPORT gfx::Insets ConvertInsetsToDIP(
     float scale_factor,
     const gfx::Insets& insets_in_pixel);
-GEOMETRY_EXPORT gfx::Point ConvertPointToDIP(float scale_factor,
-                                             const gfx::Point& point_in_pixel);
-GEOMETRY_EXPORT gfx::PointF ConvertPointToDIP(
-    float scale_factor,
-    const gfx::PointF& point_in_pixel);
 GEOMETRY_EXPORT gfx::Size ConvertSizeToDIP(float scale_factor,
                                            const gfx::Size& size_in_pixel);
 GEOMETRY_EXPORT gfx::Rect ConvertRectToDIP(float scale_factor,

ui/ozone/platform/x11/x11_screen_ozone.cc

@@ -10,6 +10,7 @@
 #include "ui/events/platform/x11/x11_event_source.h"
 #include "ui/gfx/font_render_params.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/platform_window/x11/x11_topmost_window_finder.h"
 #include "ui/platform_window/x11/x11_window.h"
@@ -29,10 +30,6 @@ float GetDeviceScaleFactor() {
   return device_scale_factor;
 }
 
-gfx::Point PixelToDIPPoint(const gfx::Point& pixel_point) {
-  return gfx::ConvertPointToDIP(GetDeviceScaleFactor(), pixel_point);
-}
-
 }  // namespace
 
 X11ScreenOzone::X11ScreenOzone()
@@ -74,14 +71,20 @@ display::Display X11ScreenOzone::GetDisplayForAcceleratedWidget(
 }
 
 gfx::Point X11ScreenOzone::GetCursorScreenPoint() const {
+  base::Optional<gfx::Point> point_in_pixels;
   if (ui::X11EventSource::HasInstance()) {
-    base::Optional<gfx::Point> point =
-        ui::X11EventSource::GetInstance()
-            ->GetRootCursorLocationFromCurrentEvent();
-    if (point)
-      return PixelToDIPPoint(point.value());
+    point_in_pixels = ui::X11EventSource::GetInstance()
+                          ->GetRootCursorLocationFromCurrentEvent();
+  }
+  if (!point_in_pixels) {
+    // This call is expensive so we explicitly only call it when
+    // |point_in_pixels| is not set. We note that base::Optional::value_or()
+    // would cause it to be called regardless.
+    point_in_pixels = GetCursorLocation();
   }
-  return PixelToDIPPoint(GetCursorLocation());
+  // TODO(danakj): Should this be rounded? Or kept as a floating point?
+  return gfx::ToFlooredPoint(
+      gfx::ConvertPointToDips(*point_in_pixels, GetDeviceScaleFactor()));
 }
 
 gfx::AcceleratedWidget X11ScreenOzone::GetAcceleratedWidgetAtScreenPoint(

ui/views/widget/desktop_aura/desktop_screen_x11.cc

@@ -21,6 +21,7 @@
 #include "ui/display/util/display_util.h"
 #include "ui/gfx/font_render_params.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/switches.h"
 #include "ui/platform_window/x11/x11_topmost_window_finder.h"
@@ -48,14 +49,18 @@ void DesktopScreenX11::Init() {
 gfx::Point DesktopScreenX11::GetCursorScreenPoint() {
   TRACE_EVENT0("views", "DesktopScreenX11::GetCursorScreenPoint()");
 
-  base::Optional<gfx::Point> point;
+  base::Optional<gfx::Point> point_in_pixels;
   if (const auto* const event_source = ui::X11EventSource::GetInstance())
-    point = event_source->GetRootCursorLocationFromCurrentEvent();
-  return gfx::ConvertPointToDIP(
-      GetXDisplayScaleFactor(),
-      // NB: Do NOT call value_or() here, since that would defeat the purpose of
-      // caching |point|.
-      point ? point.value() : x11_display_manager_->GetCursorLocation());
+    point_in_pixels = event_source->GetRootCursorLocationFromCurrentEvent();
+  if (!point_in_pixels) {
+    // This call is expensive so we explicitly only call it when
+    // |point_in_pixels| is not set. We note that base::Optional::value_or()
+    // would cause it to be called regardless.
+    point_in_pixels = x11_display_manager_->GetCursorLocation();
+  }
+  // TODO(danakj): Should this be rounded? Or kept as a floating point?
+  return gfx::ToFlooredPoint(
+      gfx::ConvertPointToDips(*point_in_pixels, GetXDisplayScaleFactor()));
 }
 
 bool DesktopScreenX11::IsWindowUnderCursor(gfx::NativeWindow window) {