Remove lossy ConvertRectToDIP() 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=ccameron@chromium.org, sky@chromium.org

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

ash/host/transformer_helper.cc

@@ -10,13 +10,14 @@
 #include "ash/host/root_window_transformer.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
-#include "ui/compositor/dip_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_animation_element.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
+#include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/geometry/rect_f.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/size_conversions.h"
@@ -43,10 +44,12 @@ class SimpleRootWindowTransformer : public RootWindowTransformer {
   }
 
   gfx::Rect GetRootWindowBounds(const gfx::Size& host_size) const override {
-    gfx::Rect bounds(host_size);
-    gfx::RectF new_bounds(ui::ConvertRectToDIP(root_window_->layer(), bounds));
-    GetInverseTransform().TransformRect(&new_bounds);
-    return gfx::Rect(gfx::ToFlooredSize(new_bounds.size()));
+    gfx::Rect host_bounds_in_pixels(host_size);
+    gfx::RectF host_bounds_in_dips = gfx::ConvertRectToDips(
+        host_bounds_in_pixels, root_window_->layer()->device_scale_factor());
+    gfx::RectF root_window_bounds = host_bounds_in_dips;
+    GetInverseTransform().TransformRect(&root_window_bounds);
+    return gfx::Rect(gfx::ToFlooredSize(root_window_bounds.size()));
   }
 
   gfx::Insets GetHostInsets() const override { return gfx::Insets(); }

chrome/browser/ui/views/frame/glass_browser_frame_view.cc

@@ -37,6 +37,7 @@
 #include "ui/display/win/screen_win.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/icon_util.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/scoped_canvas.h"
@@ -310,8 +311,17 @@ int GlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
                                         DWMWA_CAPTION_BUTTON_BOUNDS,
                                         &button_bounds,
                                         sizeof(button_bounds)))) {
-      gfx::Rect buttons = GetMirroredRect(gfx::ConvertRectToDIP(
-          display::win::GetDPIScale(), gfx::Rect(button_bounds)));
+      gfx::RectF button_bounds_in_dips = gfx::ConvertRectToDips(
+          gfx::Rect(button_bounds), display::win::GetDPIScale());
+      // TODO(crbug.com/1131681): GetMirroredRect() requires an integer rect,
+      // but the size in DIPs may not be an integer with a fractional device
+      // scale factor. If we want to keep using integers, the choice to use
+      // ToFlooredRectDeprecated() seems to be doing the wrong thing given the
+      // comment below about insetting 1 DIP instead of 1 physical pixel. We
+      // should probably use ToEnclosedRect() and then we could have inset 1
+      // physical pixel here.
+      gfx::Rect buttons =
+          GetMirroredRect(gfx::ToFlooredRectDeprecated(button_bounds_in_dips));
 
       // There is a small one-pixel strip right above the caption buttons in
       // which the resize border "peeks" through.

components/exo/input_method_surface.cc

@@ -10,6 +10,7 @@
 #include "ui/base/class_property.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/views/accessibility/view_accessibility.h"
 
 DEFINE_UI_CLASS_PROPERTY_KEY(exo::InputMethodSurface*,
@@ -69,10 +70,16 @@ void InputMethodSurface::OnSurfaceCommit() {
     manager_->AddSurface(this);
   }
 
-  const gfx::Rect new_bounds = gfx::ConvertRectToDIP(
-      GetScale(), root_surface()->hit_test_region().bounds());
-  if (input_method_bounds_ != new_bounds) {
-    input_method_bounds_ = new_bounds;
+  gfx::RectF new_bounds_in_dips = gfx::ConvertRectToDips(
+      root_surface()->hit_test_region().bounds(), GetScale());
+  // TODO(crbug.com/1131682): We should avoid dropping precision to integers
+  // here if we want to know the true rectangle bounds in DIPs. If not, we
+  // should use ToEnclosingRect() if we want to include DIPs that partly overlap
+  // the physical pixel bounds, or ToEnclosedRect() if we do not.
+  gfx::Rect int_bounds_in_dips =
+      gfx::ToFlooredRectDeprecated(new_bounds_in_dips);
+  if (input_method_bounds_ != int_bounds_in_dips) {
+    input_method_bounds_ = int_bounds_in_dips;
     manager_->OnTouchableBoundsChanged(this);
 
     GetViewAccessibility().OverrideBounds(gfx::RectF(input_method_bounds_));

components/viz/service/frame_sinks/video_detector.cc

@@ -4,13 +4,17 @@
 
 #include "components/viz/service/frame_sinks/video_detector.h"
 
+#include <memory>
+#include <utility>
+#include <vector>
+
 #include "base/time/time.h"
 #include "components/viz/common/quads/compositor_frame.h"
 #include "components/viz/service/surfaces/surface.h"
 #include "components/viz/service/surfaces/surface_manager.h"
 #include "mojo/public/cpp/bindings/remote.h"
-#include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 
 namespace viz {
 
@@ -38,8 +42,8 @@ class VideoDetector::ClientInfo {
     const CompositorFrame& frame = surface->GetActiveFrame();
 
     gfx::Rect damage =
-        gfx::ConvertRectToDIP(frame.device_scale_factor(),
-                              frame.render_pass_list.back()->damage_rect);
+        gfx::ScaleToEnclosingRect(frame.render_pass_list.back()->damage_rect,
+                                  1.f / frame.device_scale_factor());
 
     if (damage.width() < kMinDamageWidth || damage.height() < kMinDamageHeight)
       return false;

ui/accelerated_widget_mac/ca_layer_tree_unittest_mac.mm

@@ -12,6 +12,7 @@
 #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/geometry/rect_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"
@@ -457,9 +458,10 @@ class CALayerTreePropertyUpdatesTest : public CALayerTreeTest {
 
       // Validate the clip and sorting context layer.
       EXPECT_TRUE([clip_and_sorting_layer masksToBounds]);
-      EXPECT_EQ(gfx::ConvertRectToDIP(properties.scale_factor,
-                                      gfx::Rect(properties.clip_rect.size())),
-                gfx::Rect([clip_and_sorting_layer bounds]));
+      EXPECT_EQ(
+          gfx::ToFlooredRectDeprecated(gfx::ConvertRectToDips(
+              gfx::Rect(properties.clip_rect.size()), properties.scale_factor)),
+          gfx::Rect([clip_and_sorting_layer bounds]));
       EXPECT_EQ(gfx::ToFlooredPoint(gfx::ConvertPointToDips(
                     properties.clip_rect.origin(), properties.scale_factor)),
                 gfx::Point([clip_and_sorting_layer position]));
@@ -484,9 +486,10 @@ class CALayerTreePropertyUpdatesTest : public CALayerTreeTest {
       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())),
-                gfx::Rect([content_layer bounds]));
+      EXPECT_EQ(
+          gfx::ToFlooredRectDeprecated(gfx::ConvertRectToDips(
+              gfx::Rect(properties.rect.size()), properties.scale_factor)),
+          gfx::Rect([content_layer bounds]));
       EXPECT_EQ(kCALayerBottomEdge, [content_layer edgeAntialiasingMask]);
       EXPECT_EQ(properties.opacity, [content_layer opacity]);
       if ([content_layer respondsToSelector:(@selector(contentsScale))])

ui/compositor/dip_util.cc

@@ -23,11 +23,6 @@
 
 namespace ui {
 
-gfx::Rect ConvertRectToDIP(const Layer* layer,
-                           const gfx::Rect& rect_in_pixel) {
-  return gfx::ConvertRectToDIP(layer->device_scale_factor(), rect_in_pixel);
-}
-
 gfx::Rect ConvertRectToPixel(const Layer* layer,
                              const gfx::Rect& rect_in_dip) {
   return gfx::ConvertRectToPixel(layer->device_scale_factor(), rect_in_dip);

ui/compositor/dip_util.h

@@ -6,7 +6,6 @@
 #define UI_COMPOSITOR_DIP_UTIL_H_
 
 #include "ui/compositor/compositor_export.h"
-#include "ui/gfx/geometry/point_f.h"
 
 namespace gfx {
 class Rect;
@@ -15,11 +14,6 @@ class Rect;
 namespace ui {
 class Layer;
 
-// Utility functions that convert point/size/rect between
-// DIP and pixel coordinates system.
-COMPOSITOR_EXPORT gfx::Rect ConvertRectToDIP(
-    const Layer* layer,
-    const gfx::Rect& rect_in_pixel);
 COMPOSITOR_EXPORT gfx::Rect ConvertRectToPixel(
     const Layer* layer,
     const gfx::Rect& rect_in_dip);

ui/gfx/geometry/dip_util.cc

@@ -95,26 +95,32 @@ SizeF ConvertSizeToPixels(const SizeF& size_in_dips,
   return ScaleSize(size_in_dips, device_scale_factor);
 }
 
-Insets ConvertInsetsToDIP(float scale_factor,
-                          const gfx::Insets& insets_in_pixel) {
+RectF ConvertRectToDips(const Rect& rect_in_pixels, float device_scale_factor) {
 #if defined(OS_MAC)
   // Device scale factor on MacOSX is always an integer.
-  DCHECK(IsIntegerInFloat(scale_factor));
+  DCHECK(IsIntegerInFloat(device_scale_factor));
 #endif
-  if (scale_factor == 1.f)
-    return insets_in_pixel;
-  return insets_in_pixel.Scale(1.f / scale_factor);
+  return ScaleRect(RectF(rect_in_pixels), 1.f / device_scale_factor);
+}
+
+RectF ConvertRectToDips(const RectF& rect_in_pixels,
+                        float device_scale_factor) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(device_scale_factor));
+#endif
+  return ScaleRect(rect_in_pixels, 1.f / device_scale_factor);
 }
 
-Rect ConvertRectToDIP(float scale_factor, const Rect& rect_in_pixel) {
+Insets ConvertInsetsToDIP(float scale_factor,
+                          const gfx::Insets& insets_in_pixel) {
 #if defined(OS_MAC)
   // Device scale factor on MacOSX is always an integer.
   DCHECK(IsIntegerInFloat(scale_factor));
 #endif
   if (scale_factor == 1.f)
-    return rect_in_pixel;
-  return ToFlooredRectDeprecated(
-      ScaleRect(RectF(rect_in_pixel), 1.f / scale_factor));
+    return insets_in_pixel;
+  return insets_in_pixel.Scale(1.f / scale_factor);
 }
 
 Insets ConvertInsetsToPixel(float scale_factor,

ui/gfx/geometry/dip_util.h

@@ -13,6 +13,7 @@ class Insets;
 class Point;
 class PointF;
 class Rect;
+class RectF;
 class Size;
 class SizeF;
 
@@ -52,11 +53,14 @@ GEOMETRY_EXPORT gfx::SizeF ConvertSizeToPixels(const gfx::Size& size_in_dips,
 GEOMETRY_EXPORT gfx::SizeF ConvertSizeToPixels(const gfx::SizeF& size_in_dips,
                                                float device_scale_factor);
 
+GEOMETRY_EXPORT gfx::RectF ConvertRectToDips(const gfx::Rect& rect_in_pixels,
+                                             float scale_factor);
+GEOMETRY_EXPORT gfx::RectF ConvertRectToDips(const gfx::RectF& rect_in_pixels,
+                                             float scale_factor);
+
 GEOMETRY_EXPORT gfx::Insets ConvertInsetsToDIP(
     float scale_factor,
     const gfx::Insets& insets_in_pixel);
-GEOMETRY_EXPORT gfx::Rect ConvertRectToDIP(float scale_factor,
-                                           const gfx::Rect& rect_in_pixel);
 
 GEOMETRY_EXPORT gfx::Insets ConvertInsetsToPixel(
     float scale_factor,

ui/ozone/platform/x11/x11_screen_ozone.cc

@@ -11,6 +11,7 @@
 #include "ui/gfx/font_render_params.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/point_conversions.h"
+#include "ui/gfx/geometry/rect_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"
@@ -110,11 +111,12 @@ display::Display X11ScreenOzone::GetDisplayNearestPoint(
 }
 
 display::Display X11ScreenOzone::GetDisplayMatching(
-    const gfx::Rect& match_rect) const {
+    const gfx::Rect& match_rect_in_pixels) const {
+  gfx::Rect match_rect = gfx::ToEnclosingRect(
+      gfx::ConvertRectToDips(match_rect_in_pixels, GetDeviceScaleFactor()));
   const display::Display* matching_display =
       display::FindDisplayWithBiggestIntersection(
-          x11_display_manager_->displays(),
-          gfx::ConvertRectToDIP(GetDeviceScaleFactor(), match_rect));
+          x11_display_manager_->displays(), match_rect);
   return matching_display ? *matching_display : GetPrimaryDisplay();
 }
 

ui/ozone/platform/x11/x11_screen_ozone.h

@@ -46,7 +46,7 @@ class X11ScreenOzone : public PlatformScreen,
   display::Display GetDisplayNearestPoint(
       const gfx::Point& point) const override;
   display::Display GetDisplayMatching(
-      const gfx::Rect& match_rect) const override;
+      const gfx::Rect& match_rect_in_pixels) const override;
   void AddObserver(display::DisplayObserver* observer) override;
   void RemoveObserver(display::DisplayObserver* observer) override;
   std::string GetCurrentWorkspace() override;

ui/views/widget/desktop_aura/desktop_screen_x11.cc

@@ -22,6 +22,7 @@
 #include "ui/gfx/font_render_params.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/point_conversions.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/switches.h"
 #include "ui/platform_window/x11/x11_topmost_window_finder.h"
@@ -121,9 +122,10 @@ display::Display DesktopScreenX11::GetDisplayNearestWindow(
         DesktopWindowTreeHostLinux::GetHostForWidget(
             host->GetAcceleratedWidget());
     if (desktop_host) {
-      const gfx::Rect pixel_rect = desktop_host->GetBoundsInPixels();
-      return GetDisplayMatching(
-          gfx::ConvertRectToDIP(GetXDisplayScaleFactor(), pixel_rect));
+      gfx::Rect match_rect_in_pixels = desktop_host->GetBoundsInPixels();
+      gfx::Rect match_rect = gfx::ToEnclosingRect(gfx::ConvertRectToDips(
+          match_rect_in_pixels, GetXDisplayScaleFactor()));
+      return GetDisplayMatching(match_rect);
     }
   }