Fix problems correctly invalidating/repainting when our updated paint rect for a large area came back while we were repainting a smaller area.

Review URL: http://codereview.chromium.org/66013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13681 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 62 insertions, 20 deletions

diff --git a/chrome/browser/renderer_host/render_widget_host.cc b/chrome/browser/renderer_host/render_widget_host.cc
index 6d62b75..dd14812 100644
--- a/chrome/browser/renderer_host/render_widget_host.cc
+++ b/chrome/browser/renderer_host/render_widget_host.cc
@@ -54,10 +54,10 @@ RenderWidgetHost::RenderWidgetHost(RenderProcessHost* process,
       is_hidden_(false),
       repaint_ack_pending_(false),
       resize_ack_pending_(false),
-      suppress_view_updating_(false),
       mouse_move_pending_(false),
       needs_repainting_on_restore_(false),
       is_unresponsive_(false),
+      in_get_backing_store_(false),
       view_being_painted_(false),
       text_direction_updated_(false),
       text_direction_(WEB_TEXT_DIRECTION_LTR),
@@ -209,6 +209,11 @@ BackingStore* RenderWidgetHost::GetBackingStore() {
   // then it means that our consumer failed to call WasRestored.
   DCHECK(!is_hidden_) << "GetBackingStore called while hidden!";
 
+  // We should never be called recursively; this can theoretically lead to
+  // infinite recursion and almost certainly leads to lower performance.
+  DCHECK(!in_get_backing_store_) << "GetBackingStore called recursively!";
+  in_get_backing_store_ = true;
+
   // We might have a cached backing store that we can reuse!
   BackingStore* backing_store =
       BackingStoreManager::GetBackingStore(this, current_size_);
@@ -228,14 +233,13 @@ BackingStore* RenderWidgetHost::GetBackingStore() {
     IPC::Message msg;
     TimeDelta max_delay = TimeDelta::FromMilliseconds(kPaintMsgTimeoutMS);
     if (process_->WaitForPaintMsg(routing_id_, max_delay, &msg)) {
-      suppress_view_updating_ = true;
       ViewHostMsg_PaintRect::Dispatch(
           &msg, this, &RenderWidgetHost::OnMsgPaintRect);
-      suppress_view_updating_ = false;
       backing_store = BackingStoreManager::GetBackingStore(this, current_size_);
     }
   }
 
+  in_get_backing_store_ = false;
   return backing_store;
 }
 
@@ -506,11 +510,9 @@ void RenderWidgetHost::OnMsgPaintRect(
   // Now paint the view. Watch out: it might be destroyed already.
   if (view_) {
     view_->MovePluginWindows(params.plugin_window_moves);
-    if (!suppress_view_updating_) {
-      view_being_painted_ = true;
-      view_->DidPaintRect(params.bitmap_rect);
-      view_being_painted_ = false;
-    }
+    view_being_painted_ = true;
+    view_->DidPaintRect(params.bitmap_rect);
+    view_being_painted_ = false;
   }
 
   if (paint_observer_.get())
diff --git a/chrome/browser/renderer_host/render_widget_host.h b/chrome/browser/renderer_host/render_widget_host.h
index 66aef1f..b6fd4d0 100644
--- a/chrome/browser/renderer_host/render_widget_host.h
+++ b/chrome/browser/renderer_host/render_widget_host.h
@@ -357,10 +357,6 @@ class RenderWidgetHost : public IPC::Channel::Listener {
   // The current size of the RenderWidget.
   gfx::Size current_size_;
 
-  // If true, then we should not ask our view to repaint when our backingstore
-  // is updated.
-  bool suppress_view_updating_;
-
   // True if a mouse move event was sent to the render view and we are waiting
   // for a corresponding ViewHostMsg_HandleInputEvent_ACK message.
   bool mouse_move_pending_;
@@ -391,6 +387,9 @@ class RenderWidgetHost : public IPC::Channel::Listener {
   // Optional observer that listens for notifications of painting.
   scoped_ptr<PaintObserver> paint_observer_;
 
+  // Flag to detect recurive calls to GetBackingStore().
+  bool in_get_backing_store_;
+
   // Set when we call DidPaintRect/DidScrollRect on the view.
   bool view_being_painted_;
 
diff --git a/chrome/browser/renderer_host/render_widget_host_view.h b/chrome/browser/renderer_host/render_widget_host_view.h
index d071b5a..db894f8 100644
--- a/chrome/browser/renderer_host/render_widget_host_view.h
+++ b/chrome/browser/renderer_host/render_widget_host_view.h
@@ -91,8 +91,19 @@ class RenderWidgetHostView {
   virtual void IMEUpdateStatus(int control, const gfx::Rect& caret_rect) = 0;
 
   // Informs the view that a portion of the widget's backing store was painted.
-  // The view should copy the given rect from the backing store of the render
-  // widget onto the screen.
+  // The view should ensure this gets copied to the screen.
+  //
+  // There are subtle performance implications here.  The RenderWidget gets sent
+  // a paint ack after this returns, so if the view only ever invalidates in
+  // response to this, then on Windows, where WM_PAINT has lower priority than
+  // events which can cause renderer resizes/paint rect updates, e.g.
+  // drag-resizing can starve painting; this function thus provides the view its
+  // main chance to ensure it stays painted and not just invalidated.  On the
+  // other hand, if this always blindly paints, then if we're already in the
+  // midst of a paint on the callstack, we can double-paint unnecessarily.
+  // (Worse, we might recursively call RenderWidgetHost::GetBackingStore().)
+  // Thus implementers should generally paint as much of |rect| as possible
+  // synchronously with as little overpainting as possible.
   virtual void DidPaintRect(const gfx::Rect& rect) = 0;
 
   // Informs the view that a portion of the widget's backing store was scrolled
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
index 32135f9..eb1b966 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
@@ -159,6 +159,7 @@ RenderWidgetHostViewGtk::RenderWidgetHostViewGtk(RenderWidgetHost* widget_host)
       parent_(NULL),
       popup_signal_id_(0),
       activatable_(true),
+      about_to_validate_and_paint_(false),
       is_loading_(false) {
   host_->set_view(this);
 }
@@ -277,7 +278,10 @@ void RenderWidgetHostViewGtk::IMEUpdateStatus(int control,
 }
 
 void RenderWidgetHostViewGtk::DidPaintRect(const gfx::Rect& rect) {
-  Paint(rect);
+  if (about_to_validate_and_paint_)
+    invalid_rect_ = invalid_rect_.Union(rect);
+  else
+    Paint(rect);
 }
 
 void RenderWidgetHostViewGtk::DidScrollRect(const gfx::Rect& rect, int dx,
@@ -338,7 +342,13 @@ void RenderWidgetHostViewGtk::PasteFromSelectionClipboard() {
 }
 
 void RenderWidgetHostViewGtk::Paint(const gfx::Rect& damage_rect) {
+  DCHECK(!about_to_validate_and_paint_);
+
+  invalid_rect_ = damage_rect;
+  about_to_validate_and_paint_ = true;
   BackingStore* backing_store = host_->GetBackingStore();
+  // Calling GetBackingStore maybe have changed |invalid_rect_|...
+  about_to_validate_and_paint_ = false;
 
   GdkWindow* window = view_.get()->window;
   if (backing_store) {
@@ -347,7 +357,7 @@ void RenderWidgetHostViewGtk::Paint(const gfx::Rect& damage_rect) {
     // Destroy()ed yet and it receives paint messages...
     if (window) {
       backing_store->ShowRect(
-          damage_rect, x11_util::GetX11WindowFromGtkWidget(view_.get()));
+          invalid_rect_, x11_util::GetX11WindowFromGtkWidget(view_.get()));
     }
   } else {
     if (window)
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.h b/chrome/browser/renderer_host/render_widget_host_view_gtk.h
index 215549d..cd49a8d 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.h
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.h
@@ -101,6 +101,12 @@ class RenderWidgetHostViewGtk : public RenderWidgetHostView {
   // activatable popup: <select> dropdown. Example of non-activatable popup:
   // form autocomplete.
   bool activatable_;
+  // This is true when we are currently painting and thus should handle extra
+  // paint requests by expanding the invalid rect rather than actually
+  // painting.
+  bool about_to_validate_and_paint_;
+  // This is the rectangle which we'll paint.
+  gfx::Rect invalid_rect_;
 
   // Whether we are currently loading.
   bool is_loading_;
diff --git a/chrome/browser/renderer_host/render_widget_host_view_win.cc b/chrome/browser/renderer_host/render_widget_host_view_win.cc
index 9b3182c..fb243d3 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_win.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_win.cc
@@ -134,6 +134,7 @@ RenderWidgetHostViewWin::RenderWidgetHostViewWin(RenderWidgetHost* widget)
       track_mouse_leave_(false),
       ime_notification_(false),
       is_hidden_(false),
+      about_to_validate_and_paint_(false),
       close_on_deactivate_(false),
       tooltip_hwnd_(NULL),
       tooltip_showing_(false),
@@ -447,7 +448,10 @@ void RenderWidgetHostViewWin::DidPaintRect(const gfx::Rect& rect) {
   if (is_hidden_)
     return;
 
-  Redraw(rect);
+  if (about_to_validate_and_paint_)
+    InvalidateRect(&rect.ToRECT(), false);
+  else
+    Redraw(rect);
 }
 
 void RenderWidgetHostViewWin::DidScrollRect(
@@ -552,11 +556,16 @@ void RenderWidgetHostViewWin::OnDestroy() {
 void RenderWidgetHostViewWin::OnPaint(HDC dc) {
   DCHECK(render_widget_host_->process()->channel());
 
-  CPaintDC paint_dc(m_hWnd);
-  HBRUSH white_brush = reinterpret_cast<HBRUSH>(GetStockObject(WHITE_BRUSH));
-
+  about_to_validate_and_paint_ = true;
   BackingStore* backing_store = render_widget_host_->GetBackingStore();
 
+  // We initialize |paint_dc| (and thus call BeginPaint()) after calling
+  // GetBackingStore(), so that if it updates the invalid rect we'll catch the
+  // changes and repaint them.
+  about_to_validate_and_paint_ = false;
+  CPaintDC paint_dc(m_hWnd);
+
+  HBRUSH white_brush = reinterpret_cast<HBRUSH>(GetStockObject(WHITE_BRUSH));
   if (backing_store) {
     gfx::Rect damaged_rect(paint_dc.m_ps.rcPaint);
 
diff --git a/chrome/browser/renderer_host/render_widget_host_view_win.h b/chrome/browser/renderer_host/render_widget_host_view_win.h
index ceec701..893e129 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_win.h
+++ b/chrome/browser/renderer_host/render_widget_host_view_win.h
@@ -233,6 +233,11 @@ class RenderWidgetHostViewWin :
   // true if the View is not visible.
   bool is_hidden_;
 
+  // True if we're in the midst of a paint operation and should respond to
+  // DidPaintRect() notifications by merely invalidating.  See comments on
+  // render_widget_host_view.h:DidPaintRect().
+  bool about_to_validate_and_paint_;
+
   // true if the View should be closed when its HWND is deactivated (used to
   // support SELECT popups which are closed when they are deactivated).
   bool close_on_deactivate_;