Send array of paint rects and bitmaps as opposed to a Union

To help resolve the performance issue introduced when enabling the resize corner, we now keep all non-intersecting rects separately and send an array of invalidation bitmaps via IPC as opposed to a single unionized rect :-)

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

[MAD, 2009-05-21 16:08:07]

Salut,

   please review this sensitive change about drawing invalidations using
multiple sub-rects from a single, bigger bitmap...

   There are changes to platform specific code, so please review the code for
the platform you are most familiar with (I'm guessing Darin for Windows, Adam
for Linux and Amanda for Mac), and feel free to look at the rest of the code...
But I think Darin should be the owner of the Go/NoGo for the rest... Right?

   If needed, please forward to anybody else you think should look at this...

Thanks!

BYE
MAD

[Amanda Walker, 2009-05-21 16:22:09]

Mac related changes LGTM.

[darin (slow to review), 2009-05-21 17:03:06]

just some initial feedback...

http://codereview.chromium.org/108040/diff/2129/3116
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/2129/3116#newcode60
Line 60: rect = rect.Union(*it);
what if this union intersects other elements of rects_?  it seems like a
recursive algorithm in which you remove the intersecting rect, compute the
union, and then call Add on the union might be better.  however, i can see that
being a potentially expensive algorithm.

shouldn't we place a limit on the number of independent damage rects?  at some
point it seems like we might benefit from coalescing.

[MAD, 2009-05-21 17:15:49]

On top of answers to Darin's comment, I have some additional stuff coming up.

I just realized that I may need to make another change... There seems to be an
intermittent case where the following asserts pop up while running the
ui_tests... sometimes...
[backing_store_win.cc(59) Check failed: bitmap_rect.Contains(paint_rect)]

First, this reminds me that I need to add this assert to the other platforms...
And second I think I know how it can happen... There are cases where the
invalidation rect is outside of the view size... We clip them as they are sent
to DidInvalidateRect(), but if the view size changes after we added a rect to
the array, then we don't clip it... I'll fix those and send another update...

Thanks!

BYE
MAD

http://codereview.chromium.org/108040/diff/2129/3116
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/2129/3116#newcode60
Line 60: rect = rect.Union(*it);
About the limit, I thought about it but I'm not sure which number would be good,
do you have an idea? 10???

About the union, it is fine... Using this trick, we remove all existing rect
that intersects with the new one before we push it, so we can be sure that none
of the existing rects intersect with it... So... incrementally... None of the
rects in the vector, at any given time... should intersect with each other...
right? Or did I miss something...

[darin (slow to review), 2009-05-21 17:33:17]

i'm not sure what a good limit should be on the number of rects.  can you devise
some kind of illustrative test that would help guide us here?  i think it would
be valuable to understand how expensive each paint call is.

http://codereview.chromium.org/108040/diff/2129/3116
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/2129/3116#newcode60
Line 60: rect = rect.Union(*it);
given rects A and B already in the vector, if C intersects B but not A, it is
possible for the union of B and C to intersect A.

      X  Y  W  H
A = { 1, 1, 1, 1 }
B = { 0, 3, 4, 1 }
C = { 3, 0, 1, 4 }

[MAD, 2009-05-21 17:40:17]

quick answers to Darin's last comments...

http://codereview.chromium.org/108040/diff/2129/3116
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/2129/3116#newcode60
Line 60: rect = rect.Union(*it);
D'ho! You're right... Sorry... I didn't see this... I'll fix it...

As for the number of rects... I'll try to cook up something...

Thanks!

[agl, 2009-05-21 22:31:20]

Just looking at the Linux bits:

http://codereview.chromium.org/108040/diff/2129/3121
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/2129/3121#newcode80
Line 80: const gfx::Rect &bitmap_rect,
nit: & on the left. (my fault)

http://codereview.chromium.org/108040/diff/2129/3121#newcode102
Line 102: const int x_offset = paint_rect.x() - bitmap_rect.x();
These rectangles are controlled by the renderer, which is untrusted. I think
there might be a check for negative sizes in the code path already, but a bad
renderer could make this difference negative, which probably gets stuffed into
an unsigned at some point triggering all kinds of panic. You should sanity check
these numbers.

I think:
bitmap_rect.Contains(paint_rect) &&
paint_rect.[xy] < 100000

http://codereview.chromium.org/108040/diff/2129/3121#newcode103
Line 103: const int y_offset = paint_rect.y() - bitmap_rect.y();
ditto

http://codereview.chromium.org/108040/diff/2129/3121#newcode128
Line 128: for (int y = 0; y < bitmap_height; ++y) {
Can't you start at the right row rather than iterating through?

http://codereview.chromium.org/108040/diff/2129/3121#newcode164
Line 164: 0, 0 /* source x, y */, paint_width, paint_height,
The 0, 0 for the source x,y is wrong, right? Shouldn't it be something like
x_offset?

http://codereview.chromium.org/108040/diff/2129/3121#newcode165
Line 165: paint_rect.x(), paint_rect.y() /* dest x, y */);
dest x,y seem wrong too. Maybe add the bitmap_rect.x()?

http://codereview.chromium.org/108040/diff/2129/3121#newcode233
Line 233: image.data = static_cast<char*>(bitmap->memory());
This works, but it might be inefficient. Can you put in a TODO(agl) to check and
file a bug against me for the same?

http://codereview.chromium.org/108040/diff/2129/3112
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/2129/3112#newcode710
Line 710: for (size_t i = 0; i < paint_rects.size(); ++i) {
When this lands, please file a bug against me to reduce the number of X server
round trips.

[MAD, 2009-05-22 14:25:42]

Some changes done, some answers to question...

Still looking at the numbers trying to find a way to threshold the number of
sub-rects...

BYE
MAD

http://codereview.chromium.org/108040/diff/2129/3121
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/2129/3121#newcode80
Line 80: const gfx::Rect &bitmap_rect,
On 2009/05/21 22:31:20, agl wrote:
> nit: & on the left. (my fault)

Done.

http://codereview.chromium.org/108040/diff/2129/3121#newcode102
Line 102: const int x_offset = paint_rect.x() - bitmap_rect.x();
the paint_rect are constrained within the bitmap at the render_widget level, and
I also added a DCHECK for containment...

I don't understand the need for the other check though, pos x & y < 100000???

http://codereview.chromium.org/108040/diff/2129/3121#newcode128
Line 128: for (int y = 0; y < bitmap_height; ++y) {
On 2009/05/21 22:31:20, agl wrote:
> Can't you start at the right row rather than iterating through?

D'ho!... Of course... Sorry about that... I don't know why I didn't do it here,
I did it in the other dimension... weird...

http://codereview.chromium.org/108040/diff/2129/3121#newcode164
Line 164: 0, 0 /* source x, y */, paint_width, paint_height,
On 2009/05/21 22:31:20, agl wrote:
> The 0, 0 for the source x,y is wrong, right? Shouldn't it be something like
> x_offset?

No, since we copies only the bits we wanted into the bitmap24, so we take the
whole pixmap, right?

http://codereview.chromium.org/108040/diff/2129/3121#newcode165
Line 165: paint_rect.x(), paint_rect.y() /* dest x, y */);
On 2009/05/21 22:31:20, agl wrote:
> dest x,y seem wrong too. Maybe add the bitmap_rect.x()?

I don't think so, since the paint_rect coord are absolute, not relative to the
bitmap_rect... They are were we want to see them on screen. OK?

http://codereview.chromium.org/108040/diff/2129/3121#newcode233
Line 233: image.data = static_cast<char*>(bitmap->memory());
Done.
http://crbug.com/12491

http://codereview.chromium.org/108040/diff/2129/3112
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/2129/3112#newcode710
Line 710: for (size_t i = 0; i < paint_rects.size(); ++i) {
Done.
http://crbug.com/12493

[agl, 2009-05-22 16:28:37]

Linux code: LGTM

http://codereview.chromium.org/108040/diff/2129/3121
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/2129/3121#newcode102
Line 102: const int x_offset = paint_rect.x() - bitmap_rect.x();
On 2009/05/22 14:25:42, mad1 wrote:
> I don't understand the need for the other check though, pos x & y < 100000???

(I was guilty of this in the original code too)

Consider a bad renderer setting paint_width and paint_height to 2^16. Later we
malloc 3 * width * height, which will be 0 in this case, then malloc returns
NULL and bang, you're dead.

Worst still if the renderer sets width to 2**16+1 instead, malloc returns a
buffer which is too short and we write renderer controlled data over heap
metadata.

http://codereview.chromium.org/108040/diff/2129/3121#newcode164
Line 164: 0, 0 /* source x, y */, paint_width, paint_height,
On 2009/05/22 14:25:42, mad1 wrote:
> No, since we copies only the bits we wanted into the bitmap24, so we take the
> whole pixmap, right?

You're right. Sorry.

http://codereview.chromium.org/108040/diff/2129/3121#newcode165
Line 165: paint_rect.x(), paint_rect.y() /* dest x, y */);
On 2009/05/22 14:25:42, mad1 wrote:
> On 2009/05/21 22:31:20, agl wrote:
> > dest x,y seem wrong too. Maybe add the bitmap_rect.x()?
> 
> I don't think so, since the paint_rect coord are absolute, not relative to the
> bitmap_rect... They are were we want to see them on screen. OK?

Yep, same misunderstanding as above.

[agl, 2009-05-22 16:55:46]

http://codereview.chromium.org/108040/diff/2129/3121
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/2129/3121#newcode102
Line 102: const int x_offset = paint_rect.x() - bitmap_rect.x();
Of course, I've just demonstrated that by haphazard condition is also
insufficient to protect against these attacks :) You can also cast to a
uint64_t and carefully check for overflows.

[MAD, 2009-05-25 21:00:24]

Made some changes to the linux code as requested by agl and also put a threshold
on the number of rects as Darin requested...

Please confirm that this is OK...

Thanks!

BYE
MAD

http://codereview.chromium.org/108040/diff/2129/3121
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/2129/3121#newcode102
Line 102: const int x_offset = paint_rect.x() - bitmap_rect.x();
Ha OK, now I understand what you meant...

We should be OK because we clip the rects to the view port rect (which is (0,0),
(view_size))... But I added the asserts as requested just to make sure...
Thanks!

[MAD, 2009-05-28 15:53:10]

Ping?

BYE
MAD

[darin (slow to review), 2009-05-29 00:10:36]

more feedback.  really sorry for the slow turnaround time.

http://codereview.chromium.org/108040/diff/3192/2201
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/108040/diff/3192/2201#newcode69
Line 69: const gfx::Rect& paint_rect);
nit: please add to the comments to explain what paint_rect and bitmap_rect mean.

http://codereview.chromium.org/108040/diff/3192/2202
File chrome/browser/renderer_host/backing_store_mac.cc (right):

http://codereview.chromium.org/108040/diff/3192/2202#newcode27
Line 27: paint_rect.x() < 100000 && paint_rect.y() < 100000);
how did you pick these numbers?

usually it is better to define a constant for a magic number and then write a
comment explaining how you chose the value.

http://codereview.chromium.org/108040/diff/3192/2200
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/3192/2200#newcode60
Line 60: paint_rect.x() < 100000 && paint_rect.y() < 100000);
ditto for magic numbers

http://codereview.chromium.org/108040/diff/3192/2203
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/3192/2203#newcode83
Line 83: paint_rect.x() < 100000 && paint_rect.y() < 100000);
ditto

http://codereview.chromium.org/108040/diff/3192/2194
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3192/2194#newcode17
Line 17: #include "skia/ext/platform_canvas.h"
nit: "s" before "v"

http://codereview.chromium.org/108040/diff/3192/2194#newcode713
Line 713: BackingStoreManager::PrepareBackingStore(this, view_size,
calling PrepareBackingStore in a loop seems non-ideal.  instead, it seems like
we should get the backingstore (possibly allocating it) and then we should
update it N times.

also, what happens if you have N non-intersecting rects that in combination
provide complete coverage of the backingstore?  if the backingstore is freshly
allocated, then wno't needs_full_paint be set to true causing extra painting?

http://codereview.chromium.org/108040/diff/3192/2192
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/108040/diff/3192/2192#newcode153
Line 153: views::NativeScrollBar::GetHorizontalScrollBarHeight());
nit: for readability it would be nice to line up the parameters one under the
other.

http://codereview.chromium.org/108040/diff/3192/2196
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/108040/diff/3192/2196#newcode167
Line 167: // The position and size of the rects where to paint the bitmap.
Suggested wording change: "The position and size of the rects where the bitmap
has data to be painted to the view."

It might be nice if an empty vector implied that the entire bitmap should be
painted.

http://codereview.chromium.org/108040/diff/3192/2198
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/3192/2198#newcode35
Line 35: namespace {
nit: please add a newline after this line

http://codereview.chromium.org/108040/diff/3192/2198#newcode37
Line 37: class RectVectorHelper {
A better name for this class might be PaintRegion.

http://codereview.chromium.org/108040/diff/3192/2198#newcode39
Line 39: explicit RectVectorHelper(std::vector<gfx::Rect>* rects)
you should probably mark this parameter as "const" since you only require read
access to the vector.  perhaps it should also be a const reference instead of a
const pointer.

http://codereview.chromium.org/108040/diff/3192/2198#newcode44
Line 44: bool IsDamagedRectBigEnough(const gfx::Size& min_size) const {
I'm having trouble parsing the name of this function along with the name of the
parameter.  what is the "damaged rect" that we are checking?

Maybe you should just have a method to compute the smallest bounding rect. 
GetBoundingRect?  And, then you could perform the min_size comparison elsewhere?

http://codereview.chromium.org/108040/diff/3192/2198#newcode88
Line 88: void ClipRects(const gfx::Rect& clip_rect) {
maybe just call this method Clip since the caller doesn't really need to know
that the PaintRegion is a vector of rects.

http://codereview.chromium.org/108040/diff/3192/2198#newcode103
Line 103: }
nit: newline before "private" would be nice for readability since you have
newlines below the other methods.

http://codereview.chromium.org/108040/diff/3192/2198#newcode107
Line 107: static size_t nb_rects_threshold_;
nit: "nb" is not a word.  can you use a word for this instead?

http://codereview.chromium.org/108040/diff/3192/2198#newcode116
Line 116: size_t RectVectorHelper::nb_rects_threshold_ = 25;
shouldn't this be declared as a constant?  and named with kFooBar style?

http://codereview.chromium.org/108040/diff/3192/2197
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/108040/diff/3192/2197#newcode119
Line 119: void PaintThisRect(gfx::Rect rect, skia::PlatformCanvas* canvas);
"const gfx::Rect& rect"

[MAD, 2009-05-29 19:49:47]

Salut again,

   Answers to questions from Darin as well as fixes as suggested...

   I also needed to update the Linux version again because a new code path has
been added for 16bits drawing... So I would appreciate if Adam would look at
it... again... sorry about that... :-)

Thanks again!

BYE
MAD

http://codereview.chromium.org/108040/diff/3192/2201
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/108040/diff/3192/2201#newcode69
Line 69: const gfx::Rect& paint_rect);
On 2009/05/29 00:10:37, darin wrote:
> nit: please add to the comments to explain what paint_rect and bitmap_rect
mean.

Done.

http://codereview.chromium.org/108040/diff/3192/2202
File chrome/browser/renderer_host/backing_store_mac.cc (right):

http://codereview.chromium.org/108040/diff/3192/2202#newcode27
Line 27: paint_rect.x() < 100000 && paint_rect.y() < 100000);
This was a request from Adam.
I'll use a static const data member and comment it

http://codereview.chromium.org/108040/diff/3192/2200
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/3192/2200#newcode60
Line 60: paint_rect.x() < 100000 && paint_rect.y() < 100000);
On 2009/05/29 00:10:37, darin wrote:
> ditto for magic numbers

Done.

http://codereview.chromium.org/108040/diff/3192/2203
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/108040/diff/3192/2203#newcode83
Line 83: paint_rect.x() < 100000 && paint_rect.y() < 100000);
On 2009/05/29 00:10:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/108040/diff/3192/2194
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3192/2194#newcode17
Line 17: #include "skia/ext/platform_canvas.h"
oops... :-)
Done.

http://codereview.chromium.org/108040/diff/3192/2194#newcode713
Line 713: BackingStoreManager::PrepareBackingStore(this, view_size,
I'm not sure I completely understand what you're trying to say here.

I was also hesitant to do it like that, but I then realized that the backing
store is allocated for the whole view anyway, so we always have one available
(and we reuse it for all calls). I decided to keep the loop here, so that the
concept of the list of rects would stay in the rendering host, and only upgrade
the backing store (and its manager) to allow for drawing of a sub-rectangle. If
you prefer it otherwise, I don't mind changing it, but I think it is more
semantic than an execution change.

The only way we get to need a full repaint is if we need a new backing store for
the given size, and this should only happen on the first call since we make all
calls with the same backing store size (which is the view size)... Unless...
again... I missed something... which is highly probably... :-)

http://codereview.chromium.org/108040/diff/3192/2192
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/108040/diff/3192/2192#newcode153
Line 153: views::NativeScrollBar::GetHorizontalScrollBarHeight());
On 2009/05/29 00:10:37, darin wrote:
> nit: for readability it would be nice to line up the parameters one under the
> other.

Done.

http://codereview.chromium.org/108040/diff/3192/2196
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/108040/diff/3192/2196#newcode167
Line 167: // The position and size of the rects where to paint the bitmap.
Good idea... Done!

http://codereview.chromium.org/108040/diff/3192/2198
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/3192/2198#newcode35
Line 35: namespace {
On 2009/05/29 00:10:37, darin wrote:
> nit: please add a newline after this line

Done.

http://codereview.chromium.org/108040/diff/3192/2198#newcode37
Line 37: class RectVectorHelper {
On 2009/05/29 00:10:37, darin wrote:
> A better name for this class might be PaintRegion.

Done.

http://codereview.chromium.org/108040/diff/3192/2198#newcode39
Line 39: explicit RectVectorHelper(std::vector<gfx::Rect>* rects)
I do modify it in the Add method... This is why it is a pointer, otherwise, it
would be a const & of course :-)

http://codereview.chromium.org/108040/diff/3192/2198#newcode44
Line 44: bool IsDamagedRectBigEnough(const gfx::Size& min_size) const {
The reason I did it like this is to avoid having to compute the smallest
bounding rect for nothing when the DCHECK are not enabled. Since this is only
used in a DCHECK right now.

Do you still want me to change it anyway? I could rename the function, as well
as the argument and add comments about the fact that it is currently only called
from a DCHECK?

http://codereview.chromium.org/108040/diff/3192/2198#newcode88
Line 88: void ClipRects(const gfx::Rect& clip_rect) {
Good point...
Done.

http://codereview.chromium.org/108040/diff/3192/2198#newcode103
Line 103: }
On 2009/05/29 00:10:37, darin wrote:
> nit: newline before "private" would be nice for readability since you have
> newlines below the other methods.

Done.

http://codereview.chromium.org/108040/diff/3192/2198#newcode107
Line 107: static size_t nb_rects_threshold_;
I thought nb was allowed, so I double checked the coding style, and it isn't nb
but num that is allowed, so would num_rects_threshold_ be OK with you?
Otherwise, suggestions are welcome... :-)

http://codereview.chromium.org/108040/diff/3192/2198#newcode116
Line 116: size_t RectVectorHelper::nb_rects_threshold_ = 25;
Oops... Sorry... 
Done.

http://codereview.chromium.org/108040/diff/3192/2197
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/108040/diff/3192/2197#newcode119
Line 119: void PaintThisRect(gfx::Rect rect, skia::PlatformCanvas* canvas);
Oops... One of my experiments to make it work was reusing rect as a local var...
Not anymore... Sorry about that..

Done.

[darin (slow to review), 2009-05-30 16:43:44]

http://codereview.chromium.org/108040/diff/3192/2194
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3192/2194#newcode713
Line 713: BackingStoreManager::PrepareBackingStore(this, view_size,
suppose the view_size is 2x2.  suppose you have 4 paint rects: 
{ 0, 0, 1, 1 }
{ 0, 1, 1, 1 }
{ 1, 0, 1, 1 }
{ 1, 1, 1, 1 }

Now imagine that the backing store is newly allocated on the first call to
PrepareBackingStore.  needs_full_paint would be set to true, despite the fact
that you actually have paint rects that would allow you to do a full paint.

Q: Can this edge case happen given the way you coalesce paint rects in the
renderer?  If not, then we are fine here.  But, if it can, then perhaps we
should do something differently here.

[darin (slow to review), 2009-05-30 16:49:54]

http://codereview.chromium.org/108040/diff/3192/2198
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/3192/2198#newcode44
Line 44: bool IsDamagedRectBigEnough(const gfx::Size& min_size) const {
Ah, yeah... I see.  Since this is just debug code how about this:

DCHECK_GE(RectVectorHelper(&paint_rects_).GetBoundingRect().width(),
          new_size.width());
DCHECK_GE(RectVectorHelper(&paint_rects_).GetBoundingRect().height(),
          new_size.height());

I just find it really hard to parse the name IsDamagedRectBigEnough.  "Bounding
Rect" is a term usually used for the smallest rectangle to contain a region, so
using that term in the function name is helpful.

http://codereview.chromium.org/108040/diff/3192/2198#newcode107
Line 107: static size_t nb_rects_threshold_;
num_ is fine.  thanks!

[MAD, 2009-06-01 21:21:37]

This covers the most recent comments from Darin, and while testing them, I
stumbled on a bug in StretchDIBits that I had seen before but couldn't
isolate... Now I did, and wrote a work around for it, so please re-review
chrome\browser\renderer_host\backing_store_win.cc

Thanks!

BYE
MAD

http://codereview.chromium.org/108040/diff/3192/2194
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3192/2194#newcode713
Line 713: BackingStoreManager::PrepareBackingStore(this, view_size,
OK, now I understand what you mean. Even though we stop the loop here, and won't
try to process the other rects, we have already painted them in the bitmap, and
we will ask for a full repaint which would not be needed if we have a complete
coverage from the given list of rects. Right?

The way we coalesce doesn't take full coverage into account, and I don't think
it is worth the extra computation needed to do it at the source (rendering)
level. So I added it here, only in the case where we want to look for it, which
should not happen often since most of the time (if not always), the backing
store will have been allocated by the first invalidation of the whole view size.

Let me know if this works for you... Another option (though more complex and I
don't think it is worth it at this point) would be to have some sort of
heuristic at the source to combine rects as they gain more coverage of the
quadrant where they lie. And we could figure out the optimal granularity of the
number of quad levels we would want to go... But not now... please :-)

http://codereview.chromium.org/108040/diff/3192/2198
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/108040/diff/3192/2198#newcode44
Line 44: bool IsDamagedRectBigEnough(const gfx::Size& min_size) const {
OK, will do...
I wanted to avoid that since we will compute the bounding rect twice, but I
guess it is OK in a debug build.

I was using the damaged term because it was used at a few places in this file,
but I'm find with bounding :-)
Done.

[darin (slow to review), 2009-06-02 16:52:53]

Thanks for dealing with the complete coverage case.  I don't know how often we
will hit that case...

http://codereview.chromium.org/108040/diff/3255/3258
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/3255/3258#newcode88
Line 88: y_destination = -1;
do i understand this correctly:  we have to add in an off-by-one pixel bug in
order to paint correctly?  there has to be a better way.  this will surely cause
rendering problems, no?

http://codereview.chromium.org/108040/diff/3255/3262
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3255/3262#newcode715
Line 715: if (paint_rects.size() == 0) {
nit: using .empty() in cases like this is usually preferred

http://codereview.chromium.org/108040/diff/3255/3262#newcode731
Line 731: // We should not need a full paint more than once for a given view
size
nit: it would be nice to extract this chunk of code into a helper function:

static bool CoversArea(const std::vector<gfx::Rect>& rects, const gfx::Size&
size);

http://codereview.chromium.org/108040/diff/3255/3262#newcode744
Line 744: } else {
nit: you can remove this else and thereby reduce indentation of the following
bit of code.

[MAD, 2009-06-02 21:12:29]

There you go...

Thanks!

BYE
MAD

http://codereview.chromium.org/108040/diff/3255/3258
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/3255/3258#newcode88
Line 88: y_destination = -1;
Oops... I forgot to also increment the height of the source, and actually got
the source/destination terms mixed up, sorry about that...

But thanks for pushing me further, I did some more research and found another
workaround, is this new version OK?

http://codereview.chromium.org/108040/diff/3255/3262
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/3255/3262#newcode715
Line 715: if (paint_rects.size() == 0) {
Done.

As well as two other places where I saw the same thing.

http://codereview.chromium.org/108040/diff/3255/3262#newcode731
Line 731: // We should not need a full paint more than once for a given view
size
On 2009/06/02 16:52:53, darin wrote:
> nit: it would be nice to extract this chunk of code into a helper function:
> 
> static bool CoversArea(const std::vector<gfx::Rect>& rects, const gfx::Size&
> size);

Done.

http://codereview.chromium.org/108040/diff/3255/3262#newcode744
Line 744: } else {
On 2009/06/02 16:52:53, darin wrote:
> nit: you can remove this else and thereby reduce indentation of the following
> bit of code.

Done.

[MAD, 2009-06-05 20:26:56]

Ping?

Thanks!

BYE
MAD

[MAD, 2009-06-10 16:17:46]

Re-Ping?

BYE
MAD

[darin (slow to review), 2009-06-10 17:23:24]

Looks good to me.  Final nits:

http://codereview.chromium.org/108040/diff/5003/4004
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/5003/4004#newcode89
Line 89: // so we must a mirror blit trick as proposed here:
nit: // so we must [use] a mirror blit...

http://codereview.chromium.org/108040/diff/5003/4008
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/5003/4008#newcode697
Line 697: static bool CoversArea(const std::vector<gfx::Rect>& rects,
nit: please move helper functions to the top of the file (around line 42 would
be good).  this section of the file is for RenderWidgetHost methods.

[MAD, 2009-06-10 19:00:13]

Cool...

I resolved a few merge conflicts as well as these changes and am now testing a
full build and will commit after that... 

Thanks to all!

BYE
MAD

http://codereview.chromium.org/108040/diff/5003/4004
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/108040/diff/5003/4004#newcode89
Line 89: // so we must a mirror blit trick as proposed here:
On 2009/06/10 17:23:24, darin wrote:
> nit: // so we must [use] a mirror blit...

Done.

http://codereview.chromium.org/108040/diff/5003/4008
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/108040/diff/5003/4008#newcode697
Line 697: static bool CoversArea(const std::vector<gfx::Rect>& rects,
On 2009/06/10 17:23:24, darin wrote:
> nit: please move helper functions to the top of the file (around line 42 would
> be good).  this section of the file is for RenderWidgetHost methods.

Done.

[darin (slow to review), 2009-06-10 19:42:45]

LGTM