Scale to DIPs in ppb_graphics2d_impl for proper invalidation

webkit/plugins/ppapi/ppb_graphics_2d_impl.cc

 // Copyright (c) 2012 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 "webkit/plugins/ppapi/ppb_graphics_2d_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
@@ skipping 332 matching lines @@
         break;
     }
 
     // For correctness with accelerated compositing, we must issue an invalidate
     // on the full op_rect even if it is partially or completely off-screen.
     // However, if we issue an invalidate for a clipped-out region, WebKit will
     // do nothing and we won't get any ViewWillInitiatePaint/ViewFlushedPaint
     // 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.
+      bool scroll = false;
+      if (operation.type == QueuedOperation::SCROLL) {
+        if (scale_ == 1.0f) {
+          scroll = true;

[Wez, 2012/07/20 00:01:41]

Is the float multiplication overhead sufficient to justify a special-case for
SCROLL but not for PAINT? Or should the scale_ test be outside this scaling
block?

[Josh Horwich, 2012/07/20 02:30:22]

Done.

This motivated me to refactor this section a bit so it should be simpler to
understand as well (I hope).

+        } else if (scale_ > 0.0f) {
+          gfx::Rect original_rect = op_rect;
+          gfx::Point original_delta(operation.scroll_dx, operation.scroll_dy);
+          gfx::Point delta_scaled = original_delta.Scale(scale_);
+          float inverse_scale = 1.0f / scale_;
+          op_rect = ScaleRectBounds(op_rect, scale_);
+          operation.scroll_dx = delta_scaled.x();
+          operation.scroll_dy = delta_scaled.y();

[Wez, 2012/07/20 00:01:41]

nit: Rather than copy the point back into the operation, why not copy the scroll
fields to a gfx::Point |scroll_delta|, and simplify this to scroll_delta =
original_delta.Scale(scale_)?

[Josh Horwich, 2012/07/20 02:30:22]

Done.

+          if (original_rect == ScaleRectBounds(op_rect, inverse_scale) &&
+              original_delta == delta_scaled.Scale(inverse_scale))

[Wez, 2012/07/20 00:01:41]

nit: Put {} around the body of multi-line-condition ifs.

[Josh Horwich, 2012/07/20 02:30:22]

Done.

+            scroll = true;
+        }
+      } else {
+        op_rect = ScaleRectBounds(op_rect, scale_);
+      }
 
       // Set |nothing_visible| to false if the change overlaps the visible area.
       gfx::Rect visible_changed_rect =
           PP_ToGfxRect(bound_instance_->view_data().clip_rect).
           Intersect(op_rect);
       if (!visible_changed_rect.IsEmpty())
         nothing_visible = false;
 
       // 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 {
         bound_instance_->InvalidateRect(op_rect);
       }
     }
   }
   queued_operations_.clear();
 
   if (nothing_visible) {
@@ skipping 143 matching lines @@
 
   CGRect bounds;
   bounds.origin.x = plugin_rect.origin().x();
   bounds.origin.y = window_height - plugin_rect.origin().y() -
       plugin_rect.height();
   bounds.size.width = plugin_rect.width();
   bounds.size.height = plugin_rect.height();
 
   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.
 
   CGContextDrawImage(canvas, bitmap_rect, image);
   CGContextRestoreGState(canvas);
 #else
   SkRect sk_plugin_rect = SkRect::MakeXYWH(
       SkIntToScalar(plugin_rect.origin().x()),
       SkIntToScalar(plugin_rect.origin().y()),
       SkIntToScalar(plugin_rect.width()),
@@ skipping 66 matching lines @@
 }
 
 void PPB_Graphics2D_Impl::ViewFlushedPaint() {
   TRACE_EVENT0("pepper", "PPB_Graphics2D_Impl::ViewFlushedPaint");
   // Notify any "painted" callback. See |unpainted_flush_callback_| in the
   // header for more.
   if (!painted_flush_callback_.is_null())
     painted_flush_callback_.Execute(PP_OK);
 }
 
+// static
+gfx::Rect PPB_Graphics2D_Impl::ScaleRectBounds(const gfx::Rect& rect,
+                                               float scale) {
+  // 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 entire area affected by the original rectangle.

[Wez, 2012/07/20 00:01:41]

nit: Move this comment to the header, since it's a part of the class's public
API.

[Josh Horwich, 2012/07/20 02:30:22]

Done.

+  if (scale != 1.0f && scale > 0.0f) {
+    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));
+    return gfx::Rect(left, top, right - left, bottom - top);
+  }
+
+  return rect;
+}
+
 void PPB_Graphics2D_Impl::ExecutePaintImageData(PPB_ImageData_Impl* image,
                                                 int x, int y,
                                                 const gfx::Rect& src_rect,
                                                 gfx::Rect* invalidated_rect) {
   // Ensure the source image is mapped to read from it.
   ImageDataAutoMapper auto_mapper(image);
   if (!auto_mapper.is_valid())
     return;
 
   // Portion within the source image to cut out.
@@ skipping 77 matching lines @@
 }
 
 bool PPB_Graphics2D_Impl::HasPendingFlush() const {
   return !unpainted_flush_callback_.is_null() ||
          !painted_flush_callback_.is_null() ||
          offscreen_flush_pending_;
 }
 
 }  // namespace ppapi
 }  // namespace webkit