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..a55251ec17e12e843c2a4b92b0772d45fdd48655 100644
--- a/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
+++ b/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
@@ -350,11 +350,17 @@ 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()) {
-
       // Set |nothing_visible| to false if the change overlaps the visible area.

[Wez, 2012/07/13 21:29:57]

nit: This comment belongs with the if statement below. You probably want a
comment here to clarify why you are scaling |op_rect| here.

[Josh Horwich, 2012/07/13 22:01:23]

Agree with the nit - will fix so comment stays with relevant code.

I am potentially going to move this bounds computation somewhere more common
(will check with oshima if it's ok to put in gfx::Rect since it might be
generally useful, not just for pepper) - in that (newer) version of the patch I
do explain the reason for scaling |op_rect|...

[Josh Horwich, 2012/07/16 23:26:52]

Done.

+      int left, top, right, bottom;
+      left = floor(op_rect.x() * scale_);

[Wez, 2012/07/16 17:41:43]

Do we need to consider the impact of introducing float multiplications here, or
can we safely assume all our target architectures have H/W FP-multiply?

[Josh Horwich, 2012/07/16 23:26:52]

Looking elsewhere (e.g. ui/gfx) I see lots of FP multiply / divides occurring.
However, I'll only do any of it if scale_ isn't 1.0 as my later patch does other
computations that are only relevant for non-identity scale.

+      top = floor(op_rect.y() * scale_);
+      right = ceil((op_rect.x() + op_rect.width()) * scale_);

[Wez, 2012/07/17 17:10:16]

Out of interest, why does Graphics2D even need an explicit scale to be
specified? We know the context and view dimensions in pixels, so can't we just
scale using those?

+      bottom = ceil((op_rect.y() + op_rect.height()) * scale_);
+      gfx::Rect dip_op_rect(left, top, right - left, bottom - top);
+
       gfx::Rect visible_changed_rect =
           PP_ToGfxRect(bound_instance_->view_data().clip_rect).
-          Intersect(op_rect);
+          Intersect(dip_op_rect);
       if (!visible_changed_rect.IsEmpty())
         nothing_visible = false;
 
@@ -362,9 +368,9 @@ int32_t PPB_Graphics2D_Impl::Flush(scoped_refptr<TrackedCallback> callback) {
       // partially or completely off-screen.
       if (operation.type == QueuedOperation::SCROLL) {
         bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy,

[Wez, 2012/07/13 21:29:57]

Do you need to scale the scroll dx & dy values as well?

[Josh Horwich, 2012/07/13 22:01:23]

Likely I do actually - thank you for noticing this.
I will also need to think a bit about what to do if the delta*scale is
non-integral (which for Invalidate wasn't a big deal since "rounding outward" is
ok). Hmmmm.

[Wez, 2012/07/16 17:41:43]

By the time we reach here we've already "executed" the scroll in the
back-buffer, so if the scroll dx/dy end up non-integer then you can fall back to
InvalidateRect().  If we're expecting to usually see 1x and 2x zooms then in
practice we'll never hit that case.

[Josh Horwich, 2012/07/16 23:26:52]

Actually, I believe we will hit this case reasonably often (when device_scale is
2x, the scale between context and logical pixels/DIPs is .5, so a plugin
scrolling by "5 device pixels" would hit this, or scrolling an area that doesn't
exactly align on integral 'logical pixels' in origin or size). Anyway, I've
added code in the new patchset to fall back to Invalidate in any of those cases.

[Wez, 2012/07/17 17:10:16]

True; you really need to fix the underlying implementation to use device pixels
throughout so that just the interfaces have to do grotty coordinate conversion.

-                                    op_rect);
+                                    dip_op_rect);
       } else {
-        bound_instance_->InvalidateRect(op_rect);
+        bound_instance_->InvalidateRect(dip_op_rect);
       }
     }
   }