Remove lossy ConvertSizeToPixel() 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: If1297043ae0ac4278b04885d6f908f1178415ede
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419534Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811114}

ash/fast_ink/fast_ink_host.cc

@@ -29,6 +29,7 @@
 #include "ui/aura/window_tree_host.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 namespace fast_ink {
@@ -287,8 +288,12 @@ void FastInkHost::SubmitCompositorFrame() {
                damage_rect_.ToString());
 
   float device_scale_factor = host_window_->layer()->device_scale_factor();
-  gfx::Rect output_rect(gfx::ConvertSizeToPixel(
-      device_scale_factor, host_window_->GetBoundsInScreen().size()));
+
+  gfx::Size window_size_in_dip = host_window_->GetBoundsInScreen().size();
+  // TODO(crbug.com/1131619): Should this be ceil? Why do we choose floor?
+  gfx::Size window_size_in_pixel = gfx::ToFlooredSize(
+      gfx::ConvertSizeToPixels(window_size_in_dip, device_scale_factor));
+  gfx::Rect output_rect(window_size_in_pixel);
 
   gfx::Rect quad_rect;
   gfx::Rect damage_rect;

ash/fast_ink/view_tree_host_root_view.cc

@@ -30,6 +30,7 @@
 #include "ui/display/screen.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 #include "ui/views/widget/widget.h"
 
@@ -385,7 +386,11 @@ void ViewTreeHostRootView::SubmitCompositorFrame() {
 
   float device_scale_factor =
       GetWidget()->GetCompositor()->device_scale_factor();
-  gfx::Rect output_rect(gfx::ConvertSizeToPixel(device_scale_factor, size()));
+
+  // TODO(crbug.com/1131623): Should this be ceil? Why do we choose floor?
+  gfx::Size size_in_pixel =
+      gfx::ToFlooredSize(gfx::ConvertSizeToPixels(size(), device_scale_factor));
+  gfx::Rect output_rect(size_in_pixel);
 
   gfx::Rect quad_rect;
   quad_rect = gfx::Rect(buffer_size_);

components/exo/surface_tree_host.cc

@@ -34,6 +34,7 @@
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/presentation_feedback.h"
 
 namespace exo {
@@ -365,8 +366,10 @@ viz::CompositorFrame SurfaceTreeHost::PrepareToSubmitCompositorFrame() {
   // because  the size is different.
   const float device_scale_factor =
       host_window()->layer()->device_scale_factor();
-  gfx::Size output_surface_size_in_pixels = gfx::ConvertSizeToPixel(
-      device_scale_factor, host_window_->bounds().size());
+  // TODO(crbug.com/1131628): Should this be ceil? Why do we choose floor?
+  gfx::Size output_surface_size_in_pixels =
+      gfx::ToFlooredSize(gfx::ConvertSizeToPixels(host_window_->bounds().size(),
+                                                  device_scale_factor));
   // Viz will crash if the frame size is empty. Ensure it's not empty.
   // crbug.com/1041932.
   if (output_surface_size_in_pixels.IsEmpty())

content/browser/renderer_host/browser_compositor_view_mac.mm

@@ -128,8 +128,10 @@ bool BrowserCompositorMac::UpdateSurfaceFromNSView(
 
   dfh_display_ = new_display;
   dfh_size_dip_ = new_size_dip;
-  dfh_size_pixels_ = gfx::ConvertSizeToPixel(dfh_display_.device_scale_factor(),
-                                             dfh_size_dip_);
+  // The device scale factor is always an integer, so the result here is also
+  // an integer.
+  dfh_size_pixels_ = gfx::ToRoundedSize(gfx::ConvertSizeToPixels(
+      dfh_size_dip_, dfh_display_.device_scale_factor()));
   root_layer_->SetBounds(gfx::Rect(dfh_size_dip_));
 
   if (needs_new_surface_id) {

content/browser/site_per_process_hit_test_browsertest.cc

@@ -6460,16 +6460,17 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessGestureHitTestBrowserTest,
 }
 #endif  // defined(USE_AURA)
 
-// Test that MouseDown and MouseUp to the same coordinates do not result in
-// different coordinates after routing. See bug https://crbug.com/670253.
-#if defined(OS_ANDROID)
 // Android uses fixed scale factor, which makes this test unnecessary.
+// MacOSX does not have fractional device scales.
+#if defined(OS_ANDROID) || defined(OS_MAC)
 #define MAYBE_MouseClickWithNonIntegerScaleFactor \
   DISABLED_MouseClickWithNonIntegerScaleFactor
 #else
 #define MAYBE_MouseClickWithNonIntegerScaleFactor \
   MouseClickWithNonIntegerScaleFactor
 #endif
+// Test that MouseDown and MouseUp to the same coordinates do not result in
+// different coordinates after routing. See bug https://crbug.com/670253.
 IN_PROC_BROWSER_TEST_F(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
                        MAYBE_MouseClickWithNonIntegerScaleFactor) {
   GURL initial_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
@@ -6520,8 +6521,14 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
               event_monitor.event().PositionInWidget().y(), kHitTestTolerance);
 }
 
+// MacOSX does not have fractional device scales.
+#if defined(OS_MAC)
+#define MAYBE_NestedSurfaceHitTestTest DISABLED_NestedSurfaceHitTestTest
+#else
+#define MAYBE_NestedSurfaceHitTestTest NestedSurfaceHitTestTest
+#endif
 IN_PROC_BROWSER_TEST_F(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
-                       NestedSurfaceHitTestTest) {
+                       MAYBE_NestedSurfaceHitTestTest) {
   NestedSurfaceHitTestTestHelper(shell(), embedded_test_server());
 }
 

third_party/blink/web_tests/NeverFixTests

@@ -2054,9 +2054,13 @@ virtual/trust-tokens/http/tests/inspector-protocol/trust-tokens/* [ Pass ]
 virtual/trust-tokens/http/tests/loading/trust-tokens/* [ Pass ]
 virtual/trust-tokens/external/wpt/trust-tokens/end-to-end/* [ Pass ]
 
+# Mac does not ever have fractional device scale factor.
+[ Mac ] virtual/scalefactor150/* [ Skip ]
+
 # Run these tests with under virtual/scalefactor... only.
 crbug.com/567837 fast/hidpi/static/* [ Skip ]
-virtual/scalefactor150/fast/hidpi/static/* [ Pass ]
+[ Linux ] virtual/scalefactor150/fast/hidpi/static/* [ Pass ]
+[ Win ] virtual/scalefactor150/fast/hidpi/static/* [ Pass ]
 virtual/scalefactor200/fast/hidpi/static/* [ Pass ]
 virtual/scalefactor200withzoom/fast/hidpi/static/* [ Pass ]
 

third_party/blink/web_tests/TestExpectations

@@ -1510,18 +1510,17 @@ crbug.com/421283 html/marquee/marquee-scrollamount.html [ Pass Failure ]
 # TODO(oshima): Mac Android are currently not supported.
 crbug.com/567837 [ Mac ] virtual/scalefactor200withzoom/fast/hidpi/static/* [ Skip ]
 
-# Mac does not support fractional scale factor.
-crbug.com/567837 [ Mac ] virtual/scalefactor150/fast/hidpi/static/* [ Skip ]
-
 # TODO(oshima): Move the event scaling code to eventSender and remove this.
 crbug.com/567837 virtual/scalefactor200/fast/hidpi/static/mousewheel-scroll-amount.html [ Skip ]
 crbug.com/567837 virtual/scalefactor200/fast/hidpi/static/gesture-scroll-amount.html [ Skip ]
-crbug.com/567837 virtual/scalefactor150/fast/hidpi/static/mousewheel-scroll-amount.html [ Skip ]
-crbug.com/567837 virtual/scalefactor150/fast/hidpi/static/gesture-scroll-amount.html [ Skip ]
-
-crbug.com/567837 virtual/scalefactor150/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html [ Timeout Failure ]
 crbug.com/567837 virtual/scalefactor200/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html [ Timeout Failure ]
 crbug.com/567837 virtual/scalefactor200withzoom/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html [ Timeout Failure ]
+crbug.com/567837 [ Linux ] virtual/scalefactor150/fast/hidpi/static/mousewheel-scroll-amount.html [ Skip ]
+crbug.com/567837 [ Win ] virtual/scalefactor150/fast/hidpi/static/mousewheel-scroll-amount.html [ Skip ]
+crbug.com/567837 [ Linux ] virtual/scalefactor150/fast/hidpi/static/gesture-scroll-amount.html [ Skip ]
+crbug.com/567837 [ Win ] virtual/scalefactor150/fast/hidpi/static/gesture-scroll-amount.html [ Skip ]
+crbug.com/567837 [ Linux ] virtual/scalefactor150/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html [ Timeout Failure ]
+crbug.com/567837 [ Win ] virtual/scalefactor150/fast/hidpi/static/popup-menu-with-scrollbar-appearance.html [ Timeout Failure ]
 
 # TODO(ojan): These tests aren't flaky. See crbug.com/517144.
 # Release trybots run asserts, but the main waterfall ones don't. So, even
@@ -5486,7 +5485,8 @@ crbug.com/922951 http/tests/security/cookies/basic.html [ Failure Pass Timeout C
 crbug.com/922951 http/tests/webaudio/autoplay-crossorigin.html [ Failure Pass Timeout Crash ]
 crbug.com/922951 media/controls/overflow-menu-hide-on-click-outside-stoppropagation.html [ Failure Pass Timeout Crash ]
 crbug.com/922951 virtual/prefer_compositing_to_lcd_text/scrollbars/resize-scales-with-dpi-150.html [ Failure Pass Timeout Crash ]
-crbug.com/922951 virtual/scalefactor150/fast/events/synthetic-events/tap-on-scaled-screen.html [ Failure Pass Timeout Crash ]
+crbug.com/922951 [ Linux ] virtual/scalefactor150/fast/events/synthetic-events/tap-on-scaled-screen.html [ Failure Pass Timeout Crash ]
+crbug.com/922951 [ Win ] virtual/scalefactor150/fast/events/synthetic-events/tap-on-scaled-screen.html [ Failure Pass Timeout Crash ]
 crbug.com/922951 virtual/threaded/http/tests/devtools/tracing/frame-model-instrumentation.js [ Failure Pass Timeout Crash ]
 crbug.com/922951 virtual/threaded/http/tests/devtools/tracing/timeline-layout/timeline-layout-reason.js [ Failure Pass Timeout Crash ]
 crbug.com/922951 virtual/threaded/http/tests/devtools/tracing/timeline-layout/timeline-layout-with-invalidations.js [ Failure Pass Timeout Crash ]

ui/compositor/dip_util.cc

@@ -28,11 +28,6 @@ gfx::Rect ConvertRectToDIP(const Layer* layer,
   return gfx::ConvertRectToDIP(layer->device_scale_factor(), rect_in_pixel);
 }
 
-gfx::Size ConvertSizeToPixel(const Layer* layer,
-                             const gfx::Size& size_in_dip) {
-  return gfx::ConvertSizeToPixel(layer->device_scale_factor(), size_in_dip);
-}
-
 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

@@ -9,7 +9,6 @@
 #include "ui/gfx/geometry/point_f.h"
 
 namespace gfx {
-class Size;
 class Rect;
 }  // namespace gfx
 
@@ -21,9 +20,6 @@ class Layer;
 COMPOSITOR_EXPORT gfx::Rect ConvertRectToDIP(
     const Layer* layer,
     const gfx::Rect& rect_in_pixel);
-COMPOSITOR_EXPORT gfx::Size ConvertSizeToPixel(
-    const Layer* layer,
-    const gfx::Size& size_in_dip);
 COMPOSITOR_EXPORT gfx::Rect ConvertRectToPixel(
     const Layer* layer,
     const gfx::Rect& rect_in_dip);

ui/gfx/geometry/dip_util.cc

@@ -4,6 +4,8 @@
 
 #include "ui/gfx/geometry/dip_util.h"
 
+#include "base/numerics/safe_conversions.h"
+#include "build/build_config.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/point_f.h"
@@ -14,43 +16,101 @@
 
 namespace gfx {
 
+#if defined(OS_MAC)
+// Returns true if the floating point value is holding an integer, modulo
+// floating point error. The value `f` can be safely converted to its integer
+// form with base::ClampRound().
+static bool IsIntegerInFloat(float f) {
+  return std::abs(f - base::ClampRound(f)) < 0.01f;
+}
+#endif
+
 PointF ConvertPointToDips(const Point& point_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 ScalePoint(PointF(point_in_pixels), 1.f / device_scale_factor);
 }
 
 PointF ConvertPointToDips(const PointF& point_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 ScalePoint(point_in_pixels, 1.f / device_scale_factor);
 }
 
 PointF ConvertPointToPixels(const Point& point_in_dips,
                             float device_scale_factor) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(device_scale_factor));
+#endif
   return ScalePoint(PointF(point_in_dips), device_scale_factor);
 }
 
 PointF ConvertPointToPixels(const PointF& point_in_dips,
                             float device_scale_factor) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(device_scale_factor));
+#endif
   return ScalePoint(point_in_dips, device_scale_factor);
 }
 
 SizeF ConvertSizeToDips(const Size& size_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 ScaleSize(SizeF(size_in_pixels), 1.f / device_scale_factor);
 }
 
 SizeF ConvertSizeToDips(const SizeF& size_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 ScaleSize(size_in_pixels, 1.f / device_scale_factor);
 }
 
+SizeF ConvertSizeToPixels(const Size& size_in_dips, float device_scale_factor) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(device_scale_factor));
+#endif
+  return ScaleSize(SizeF(size_in_dips), device_scale_factor);
+}
+
+SizeF ConvertSizeToPixels(const SizeF& size_in_dips,
+                          float device_scale_factor) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(device_scale_factor));
+#endif
+  return ScaleSize(size_in_dips, device_scale_factor);
+}
+
 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 insets_in_pixel;
   return insets_in_pixel.Scale(1.f / scale_factor);
 }
 
 Rect ConvertRectToDIP(float scale_factor, const Rect& rect_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(
@@ -59,18 +119,20 @@ Rect ConvertRectToDIP(float scale_factor, const Rect& rect_in_pixel) {
 
 Insets ConvertInsetsToPixel(float scale_factor,
                             const gfx::Insets& insets_in_dip) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(scale_factor));
+#endif
   if (scale_factor == 1.f)
     return insets_in_dip;
   return insets_in_dip.Scale(scale_factor);
 }
 
-Size ConvertSizeToPixel(float scale_factor, const Size& size_in_dip) {
-  if (scale_factor == 1.f)
-    return size_in_dip;
-  return ScaleToFlooredSize(size_in_dip, scale_factor);
-}
-
 Rect ConvertRectToPixel(float scale_factor, const Rect& rect_in_dip) {
+#if defined(OS_MAC)
+  // Device scale factor on MacOSX is always an integer.
+  DCHECK(IsIntegerInFloat(scale_factor));
+#endif
   // Use ToEnclosingRect() to ensure we paint all the possible pixels
   // touched. ToEnclosingRect() floors the origin, and ceils the max
   // coordinate. To do otherwise (such as flooring the size) potentially

ui/gfx/geometry/dip_util.h

@@ -47,6 +47,11 @@ GEOMETRY_EXPORT gfx::SizeF ConvertSizeToDips(const gfx::Size& size_in_pixels,
 GEOMETRY_EXPORT gfx::SizeF ConvertSizeToDips(const gfx::SizeF& size_in_pixels,
                                              float device_scale_factor);
 
+GEOMETRY_EXPORT gfx::SizeF ConvertSizeToPixels(const gfx::Size& size_in_dips,
+                                               float device_scale_factor);
+GEOMETRY_EXPORT gfx::SizeF ConvertSizeToPixels(const gfx::SizeF& size_in_dips,
+                                               float device_scale_factor);
+
 GEOMETRY_EXPORT gfx::Insets ConvertInsetsToDIP(
     float scale_factor,
     const gfx::Insets& insets_in_pixel);
@@ -56,8 +61,6 @@ GEOMETRY_EXPORT gfx::Rect ConvertRectToDIP(float scale_factor,
 GEOMETRY_EXPORT gfx::Insets ConvertInsetsToPixel(
     float scale_factor,
     const gfx::Insets& insets_in_dip);
-GEOMETRY_EXPORT gfx::Size ConvertSizeToPixel(float scale_factor,
-                                             const gfx::Size& size_in_dip);
 GEOMETRY_EXPORT gfx::Rect ConvertRectToPixel(float scale_factor,
                                              const gfx::Rect& rect_in_dip);
 }  // gfx

ui/views/cocoa/native_widget_mac_ns_window_host.mm

@@ -25,6 +25,7 @@
 #include "ui/display/screen.h"
 #include "ui/events/cocoa/cocoa_event_utils.h"
 #include "ui/gfx/geometry/dip_util.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/gfx/mac/coordinate_conversion.h"
 #include "ui/native_theme/native_theme_mac.h"
 #include "ui/views/cocoa/text_input_host.h"
@@ -524,11 +525,15 @@ void NativeWidgetMacNSWindowHost::CreateCompositor(
 void NativeWidgetMacNSWindowHost::UpdateCompositorProperties() {
   if (!compositor_)
     return;
-  gfx::Size surface_size_in_dip = content_bounds_in_screen_.size();
-  layer()->SetBounds(gfx::Rect(surface_size_in_dip));
-  compositor_->UpdateSurface(
-      ConvertSizeToPixel(display_.device_scale_factor(), surface_size_in_dip),
-      display_.device_scale_factor(), display_.color_spaces());
+  layer()->SetBounds(gfx::Rect(content_bounds_in_screen_.size()));
+  // Mac device scale factor is always an integer so the result here is an
+  // integer pixel size.
+  gfx::Size content_bounds_in_pixels =
+      gfx::ToRoundedSize(gfx::ConvertSizeToPixels(
+          content_bounds_in_screen_.size(), display_.device_scale_factor()));
+  compositor_->UpdateSurface(content_bounds_in_pixels,
+                             display_.device_scale_factor(),
+                             display_.color_spaces());
 }
 
 void NativeWidgetMacNSWindowHost::DestroyCompositor() {
@@ -983,10 +988,14 @@ void NativeWidgetMacNSWindowHost::OnWindowDisplayChanged(
   bool display_id_changed = display_.id() != new_display.id();
   display_ = new_display;
   if (compositor_) {
-    compositor_->UpdateSurface(
-        ConvertSizeToPixel(display_.device_scale_factor(),
-                           content_bounds_in_screen_.size()),
-        display_.device_scale_factor(), display_.color_spaces());
+    // Mac device scale factor is always an integer so the result here is an
+    // integer pixel size.
+    gfx::Size content_bounds_in_pixels =
+        gfx::ToRoundedSize(gfx::ConvertSizeToPixels(
+            content_bounds_in_screen_.size(), display_.device_scale_factor()));
+    compositor_->UpdateSurface(content_bounds_in_pixels,
+                               display_.device_scale_factor(),
+                               display_.color_spaces());
   }
   if (display_id_changed) {
     display_link_ = ui::DisplayLinkMac::GetForDisplay(display_.id());