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()) {
+      // 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);
 
-      // 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);
+          Intersect(dip_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) {
         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);
       }
     }
   }
   queued_operations_.clear();
 
   if (nothing_visible) {
     // There's nothing visible to invalidate so just schedule the callback to
     // execute in the next round of the message loop.
     ScheduleOffscreenCallback(FlushCallbackData(callback));
   } else {
@@ skipping 322 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