Fix problems correctly invalidating/repainting when our updated paint rect fo...

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13681

[darin (slow to review), 2009-04-09 23:15:52]

LGTM

http://codereview.chromium.org/66013/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/66013/diff/1/3#newcode94
Line 94: // The view should ensure the given rect is eventually copied onto the
screen.
there is a subtle detail here.  it is important to note that when we return from
DidPaintRect, the renderer will be sent an ACK that tells it that it is OK to
paint again.  this is why the old comment says that the view should copy to the
screen.  if that doesn't happen, then the renderer can end up dominating the CPU
generating updates to the backingstore and the backingstore may never actually
make its way to the screen.

the new comment doesn't quite convey the right message since it suggests that
always calling InvalidateRect would be OK, when indeed it would not.

how to reword it??  hmm...

http://codereview.chromium.org/66013/diff/1/2
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/66013/diff/1/2#newcode236
Line 236: // True if we're in the midst of a paint operation.
nit: we may also want to just scope the GetBackingStore call...

[darin (slow to review), 2009-04-09 23:16:13]

please make sure that the mac and linux guys know to update their views in a
similar fashion.

[Peter Kasting, 2009-04-09 23:51:16]

Changed approach slightly and re-snapshotted.

Now instead of invalidating in DidPaintRect() we actually Redraw(), but we
subtract off the rect we already want to paint.  This is a bit of a smaller diff
than the other change, should result in less tearing, and while susceptible to
overpaint, shouldn't really be worse than the previous solution.  It also makes
more sense to me :P

[darin (slow to review), 2009-04-09 23:54:06]

i think the original solution is better...

http://codereview.chromium.org/66013/diff/2003/2008
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/66013/diff/2003/2008#newcode403
Line 403: Redraw(redraw_rect);
this means recursively calling GetBackingStore, which is something that i really
don't think is a good idea.

[Peter Kasting, 2009-04-10 00:09:45]

Take 3 uploaded, which is fairly similar to take 1 but with better comments and
a few small tweaks.

[darin (slow to review), 2009-04-10 06:01:27]

LGTM

http://codereview.chromium.org/66013/diff/2010/2012
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/66013/diff/2010/2012#newcode99
Line 99: // events which can cause renderer resizes, drag-resizing can starve
painting;
a slightly simpler example is just mouse input events that lead to web page
animations (e.g., dragging the map in google maps).

http://codereview.chromium.org/66013/diff/2010/2015
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/66013/diff/2010/2015#newcode507
Line 507: BackingStore* backing_store = render_widget_host_->GetBackingStore();
it just occured to me that we could probably reuse the in_get_backing_store flag
on RenderWidgetHost if we really wanted to.

[Peter Kasting, 2009-04-10 16:35:52]

Sadly, the DCHECK() that we don't call GetBackingStore() recursively causes
numerous test failures on Linux.  I guess I'll glance at the Linux code to see
if the problems are obvious, and if not, check in with that DCHECK commented out
and ask the Linux folks to enable once they fix, or something.

http://codereview.chromium.org/66013/diff/2010/2012
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/66013/diff/2010/2012#newcode99
Line 99: // events which can cause renderer resizes, drag-resizing can starve
painting;
On 2009/04/10 06:01:27, darin wrote:
> a slightly simpler example is just mouse input events that lead to web page
> animations (e.g., dragging the map in google maps).

Do you think it adds clarity to add this example as well?  Maybe I should just
stick "e.g." before "drag-resizing".

http://codereview.chromium.org/66013/diff/2010/2015
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/66013/diff/2010/2015#newcode507
Line 507: BackingStore* backing_store = render_widget_host_->GetBackingStore();
On 2009/04/10 06:01:27, darin wrote:
> it just occured to me that we could probably reuse the in_get_backing_store
flag
> on RenderWidgetHost if we really wanted to.

We could, but I'd have to make it non-private.  I think that's suboptimal, since
this feels more like two coincident  private details than a true shared concept.
 I dunno, they're somewhat coupled, so I don't feel strongly, but I'm inclined
to leave it as-is.

[darin (slow to review), 2009-04-10 17:21:35]

http://codereview.chromium.org/66013/diff/2010/2012
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/66013/diff/2010/2012#newcode99
Line 99: // events which can cause renderer resizes, drag-resizing can starve
painting;
OK to "e.g."

http://codereview.chromium.org/66013/diff/2010/2015
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/66013/diff/2010/2015#newcode507
Line 507: BackingStore* backing_store = render_widget_host_->GetBackingStore();
OK

[Avi (use Gerrit), 2009-04-12 18:43:03]

Looks reasonable, though I don't have a machine to test it on right now. Any
indications that the recursive drawing that is caught on Linux happens on the
Mac?