ppapi Graphics2d should invalidate all changes without clipping to visible area.

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 89cba977ea524a0df0f064edeed3802ea76f2257..fae5c1d2d01688b6398ec7001785ad402ec11c57 100644
--- a/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
+++ b/webkit/plugins/ppapi/ppb_graphics_2d_impl.cc
@@ -344,24 +344,32 @@ int32_t PPB_Graphics2D_Impl::Flush(PP_CompletionCallback callback) {
         break;
     }
 
-    // We need the rect to be in terms of the current clip rect of the plugin
-    // since that's what will actually be painted. 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.
-    gfx::Rect visible_changed_rect;
-    if (bound_instance_ && !op_rect.IsEmpty())
-      visible_changed_rect =PP_ToGfxRect(bound_instance_->view_data().clip_rect).
-          Intersect(op_rect);
-
-    if (bound_instance_ && !visible_changed_rect.IsEmpty()) {
+    // 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_) {
+
+      // Set |nothing_visible| to false if the change overlaps the visible area.
+      if (!op_rect.IsEmpty()) {
+        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

[Wez, 2012/04/26 21:13:29]

Doesn't this block want to be inside the !op_rect.IsEmpty() block, to match the
original logic?

[shawnsingh, 2012/04/26 22:35:50]

Yes - thanks.  That should also simplify the ugly nested if-statements,

+      // partially or completely off-screen.
       if (operation.type == QueuedOperation::SCROLL) {
         bound_instance_->ScrollRect(operation.scroll_dx, operation.scroll_dy,
-                                    visible_changed_rect);
+                                    op_rect);
       } else {
-        bound_instance_->InvalidateRect(visible_changed_rect);
+        bound_instance_->InvalidateRect(op_rect);
       }
-      nothing_visible = false;
     }
   }
   queued_operations_.clear();