Scale to DIPs in ppb_graphics2d_impl for proper invalidation

webkit/plugins/ppapi/ppb_graphics_2d_impl.cc

Index: webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
diff --git a/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc b/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
index 79821bb55a0b06764cdc9dafe0eb6aced70006ef..fc928824145057b0a151e1a3339048e02b597478 100644
--- a/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
+++ b/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
@@ -122,6 +122,17 @@ void ConvertImageData(PPB_ImageData_Impl* src_image, const SkIRect& src_rect,
   }
 }
 
+// Scale the rectangle, taking care to round coordinates outward so a
+// rectangle scaled down then scaled back up by the inverse scale would
+// fully contain the original.

[Wez, 2012/07/19 01:33:37]

nit: Consider expressing this in terms of the goal, being that the scaled
rectangle fully contains the area of the scaled output "affected by" the
original rectangle.

[Josh Horwich, 2012/07/19 22:22:44]

Done.

+void ScaleRectBounds(gfx::Rect& rect, float scale) {

[Wez, 2012/07/19 01:33:37]

nit: gfx::Rect ScaleRectBounds(const gfx::Rect& ...)

[Josh Horwich, 2012/07/19 22:22:44]

Done.

+  int left = static_cast<int>(floorf(rect.x() * scale));
+  int top = static_cast<int>(floorf(rect.y() * scale));
+  int right = static_cast<int>(ceilf((rect.x() + rect.width()) * scale));
+  int bottom = static_cast<int>(ceilf((rect.y() + rect.height()) * scale));
+  rect.SetRect(left, top, right - left, bottom - top);
+}
+
 }  // namespace
 
 struct PPB_Graphics2D_Impl::QueuedOperation {
@@ -350,6 +361,20 @@ int32_t PPB_Graphics2D_Impl::Flush(scoped_refptr<TrackedCallback> callback) {
     // calls, leaving our callback stranded. So we still need to check whether
     // the repainted area is visible to determine how to deal with the callback.
     if (bound_instance_ && !op_rect.IsEmpty()) {
+      // Convert op_rect from context coordinates to logical pixels, taking care
+      // to include partially-covered logical pixels (aka DIPs). If the
+      // operation is a scroll, we also find out if we need to fall back to
+      // InvalidateRect due to the possibility that the scroll cannot accurately
+      // be described in logical pixels due to rounding errors

[Wez, 2012/07/19 01:33:37]

typo: missing .

[Josh Horwich, 2012/07/19 22:22:44]

Done.

+      bool scroll = false;
+      if (operation.type == QueuedOperation::SCROLL) {
+        scroll = ScaleMetrics(scale_,
+                              op_rect,
+                              &operation.scroll_dx,
+                              &operation.scroll_dy);
+      } else {
+        ScaleMetrics(scale_, op_rect, NULL, NULL);

[Wez, 2012/07/19 01:33:37]

nit: Use ScaleRectBounds explicitly here.

[Josh Horwich, 2012/07/19 22:22:44]

Done.

+      }
 
       // Set |nothing_visible| to false if the change overlaps the visible area.
       gfx::Rect visible_changed_rect =
@@ -360,7 +385,7 @@ int32_t PPB_Graphics2D_Impl::Flush(scoped_refptr<TrackedCallback> callback) {
 
       // Notify the plugin of the entire change (op_rect), even if it is
       // partially or completely off-screen.
-      if (operation.type == QueuedOperation::SCROLL) {
+      if (scroll) {
         bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy,
                                     op_rect);
       } else {
@@ -524,6 +549,10 @@ void PPB_Graphics2D_Impl::Paint(WebKit::WebCanvas* canvas,
 
   CGContextClipToRect(canvas, bounds);
 
+  // TODO(jhorwich) Figure out if this code is even active anymore, and if so
+  // how to properly handle scaling.
+  DCHECK_EQ(1.0f, scale_);
+
   // TODO(brettw) bug 56673: do a direct memcpy instead of going through CG
   // if the is_always_opaque_ flag is set. Must ensure bitmap is still clipped.
 
@@ -610,6 +639,32 @@ void PPB_Graphics2D_Impl::ViewFlushedPaint() {
     painted_flush_callback_.Execute(PP_OK);
 }
 
+// static
+bool PPB_Graphics2D_Impl::ScaleMetrics(float scale, gfx::Rect& rect,
+                                       int* dx, int* dy) {
+  if (scale != 1.0f && scale > 0.0f) {
+    gfx::Rect original_rect = rect;
+    ScaleRectBounds(rect, scale);
+    if (dx && dy) {
+      // Ensure that the rectangle and deltas can be scaled and inverse-scaled
+      // without loss of precision. This check is needed when scrolling to
+      // determine if we should fall back to InvalidateRect.
+      gfx::Rect unscaled_rect = rect;
+      int original_dx = *dx;
+      int original_dy = *dy;
+      *dx = static_cast<int>(original_dx * scale);
+      *dy = static_cast<int>(original_dy * scale);
+      ScaleRectBounds(unscaled_rect, 1.0f / scale);
+      if (unscaled_rect != original_rect ||
+          static_cast<int>(*dx / scale) != original_dx ||
+          static_cast<int>(*dy / scale) != original_dy)

[Wez, 2012/07/19 01:33:37]

It feels like it'd be cleaner to keep the ScaleRect and ScaleDelta parts in
separate functions rather than mix the two like this.

[Josh Horwich, 2012/07/19 22:22:44]

Done.

I put the ScaleDelta parts back inline above.

+        return false;
+    }
+  }
+
+  return true;
+}
+
 void PPB_Graphics2D_Impl::ExecutePaintImageData(PPB_ImageData_Impl* image,
                                                 int x, int y,
                                                 const gfx::Rect& src_rect,