Refactors RenderWidget to extract a PaintAggregator class....

Refactors RenderWidget to extract a PaintAggregator class.

After this change, I plan on changing the PaintAggregator algorithm.

Some things to note:

1- Previously, it was possible to send overlapping ViewHostMsg_PaintRect and
ViewHostMsg_ScrollRect messages.  This happened when scrolling a page since the
scrollbar would need to be repainted while the contents of the page are being
scrolled.  With this CL, this overlapping behavior is a bit more explicit.

2- There was a TODO about view_size clipping that I've eliminated.  I was able
to eliminate it because I realized that it is correct to clip the rects passed
by didInvalidateRect and didScrollRect to the size of the RenderWidget.
Apparently WebKit can sometimes invalidate regions outside the view.

R=brettw
BUG=25905
TEST=none


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

[brettw, 2009-11-18 17:22:54]

http://codereview.chromium.org/403005/diff/3001/3007
File chrome/renderer/paint_aggregator.cc (right):

http://codereview.chromium.org/403005/diff/3001/3007#newcode30
Line 30: void PaintAggregator::ScrollRect(int dx, int dy, const gfx::Rect&
clip_rect) {
It seems like there's a really good opportunity for a few unit tests for this
code. You said you're going to change the algorithm. Are there aspects of it
that won't change that make sense to write unit tests for now?

http://codereview.chromium.org/403005/diff/3001/3006
File chrome/renderer/paint_aggregator.h (right):

http://codereview.chromium.org/403005/diff/3001/3006#newcode25
Line 25: struct PendingUpdate {
Since this is a type, it should technically go first in the public section.

http://codereview.chromium.org/403005/diff/3001/3006#newcode26
Line 26: gfx::Point scroll_delta;
It might be good if you commented that the scroll_delta can only be in one
direction.

http://codereview.chromium.org/403005/diff/3001/3004
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/403005/diff/3001/3004#newcode415
Line 415: if (update.scroll_delta.x()) {
It seems like this complicated computation could be moved to a helper function
on the PendingUpdate or somewhere like that. It would be much easier to follow
the update code if it said "gfx::Rect damaged_rect = update.GetScrollDamage();"
or something instead of these 30 lines.

http://codereview.chromium.org/403005/diff/3001/3004#newcode426
Line 426: } else {
It might be nice to DCHECK that the "other" scroll direction is 0 in both arms
of this update code.

[brettw, 2009-11-18 17:33:07]

> http://codereview.chromium.org/403005/diff/3001/3004#newcode415
> Line 415: if (update.scroll_delta.x()) {
> It seems like this complicated computation could be moved to a helper function
> on the PendingUpdate or somewhere like that. It would be much easier to follow
> the update code if it said "gfx::Rect damaged_rect =
update.GetScrollDamage();"
> or something instead of these 30 lines.

Also, if this is split out, it's easy to write a unit test for this nontrivial
computation.

[darin (slow to review), 2009-11-18 17:45:27]

On Wed, Nov 18, 2009 at 9:22 AM, <brettw@chromium.org> wrote:

>
> http://codereview.chromium.org/403005/diff/3001/3007
> File chrome/renderer/paint_aggregator.cc (right):
>
> http://codereview.chromium.org/403005/diff/3001/3007#newcode30
> Line 30: void PaintAggregator::ScrollRect(int dx, int dy, const
> gfx::Rect& clip_rect) {
> It seems like there's a really good opportunity for a few unit tests for
> this code. You said you're going to change the algorithm. Are there
> aspects of it that won't change that make sense to write unit tests for
> now?
>

Yeah, I should have said that the reason for this refactor was to make
unit testing easier.  I was going to write unit tests once I switch the
algorithm since all but the simplest unit tests will be invalidated.  I can
add the very simple unit tests now.



>
> http://codereview.chromium.org/403005/diff/3001/3006
> File chrome/renderer/paint_aggregator.h (right):
>
> http://codereview.chromium.org/403005/diff/3001/3006#newcode25
> Line 25: struct PendingUpdate {
> Since this is a type, it should technically go first in the public
> section.
>

OK, I favored keeping it near the output methods, but I can certainly change
it.



>
> http://codereview.chromium.org/403005/diff/3001/3006#newcode26
> Line 26: gfx::Point scroll_delta;
> It might be good if you commented that the scroll_delta can only be in
> one direction.
>

OK



>
> http://codereview.chromium.org/403005/diff/3001/3004
> File chrome/renderer/render_widget.cc (right):
>
> http://codereview.chromium.org/403005/diff/3001/3004#newcode415
> Line 415: if (update.scroll_delta.x()) {
> It seems like this complicated computation could be moved to a helper
> function on the PendingUpdate or somewhere like that. It would be much
> easier to follow the update code if it said "gfx::Rect damaged_rect =
> update.GetScrollDamage();" or something instead of these 30 lines.
>
>
I want to keep PendingUpdate as a simple struct.  I'll look at moving this
computation to be a private method of PaintAggregator.



> http://codereview.chromium.org/403005/diff/3001/3004#newcode426
> Line 426: } else {
> It might be nice to DCHECK that the "other" scroll direction is 0 in
> both arms of this update code.


OK


>
>
> http://codereview.chromium.org/403005
>

[darin (slow to review), 2009-11-18 17:50:55]

On Wed, Nov 18, 2009 at 9:45 AM, Darin Fisher <darin@chromium.org> wrote:

> On Wed, Nov 18, 2009 at 9:22 AM, <brettw@chromium.org> wrote:
>
>>  <http://codereview.chromium.org/403005/diff/3001/3004>
>
> ...

> http://codereview.chromium.org/403005/diff/3001/3004
>
> File chrome/renderer/render_widget.cc (right):
>>
>> http://codereview.chromium.org/403005/diff/3001/3004#newcode415
>> Line 415: if (update.scroll_delta.x()) {
>> It seems like this complicated computation could be moved to a helper
>> function on the PendingUpdate or somewhere like that. It would be much
>> easier to follow the update code if it said "gfx::Rect damaged_rect =
>> update.GetScrollDamage();" or something instead of these 30 lines.
>>
>>
> I want to keep PendingUpdate as a simple struct.  I'll look at moving this
> computation to be a private method of PaintAggregator.
>
>
Actually, I see what you mean now.  That does sound like a good idea.
-Darin



>
>
>>
>>
>> http://codereview.chromium.org/403005
>>
>
>

[darin (slow to review), 2009-11-18 21:52:04]

OK, please take another look.  All suggestions incorporated.

[brettw, 2009-11-18 23:58:37]

sweet, LGTM