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

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 326 matching lines @@
       case QueuedOperation::SCROLL:
         ExecuteScroll(operation.scroll_clip_rect,
                       operation.scroll_dx, operation.scroll_dy,
                       &op_rect);
         break;
       case QueuedOperation::REPLACE:
         ExecuteReplaceContents(operation.replace_image, &op_rect);
         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);
+    // 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_) {
 
-    if (bound_instance_ && !visible_changed_rect.IsEmpty()) {
+      // 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();
 
   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(
         scoped_refptr<TrackedCallback>(new TrackedCallback(this, callback))));
   } else {
@@ skipping 305 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