Allow painting multiple paint rects....

Allow painting multiple paint rects.

Previously, if there was no scrolling, then we would always paint the union of
any invalidations.  With this change, if the area of the invalidation rects is
sufficiently less than the area of the union, then we will just paint the
individual invalidation rects.

This may regress page cycler performance (see http://crbug.com/29589).

R=brettw
BUG=50303
TEST=updated unit tests


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

[brettw, 2010-12-06 19:09:29]

http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregator.h
File chrome/renderer/paint_aggregator.h (right):

http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregato...
chrome/renderer/paint_aggregator.h:46: const PendingUpdate& GetPendingUpdate();
It kind of sucks that this function modifies the update inside the paint
aggregator out from under you. Normally I wouldn't expect this to happen for a
functionc alled GetFoo().

Maybe we can rename the function, like CoalesceAndGetPendingUpdate()?

[darin (slow to review), 2010-12-06 19:11:31]

On Mon, Dec 6, 2010 at 11:09 AM, <brettw@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregator.h
> File chrome/renderer/paint_aggregator.h (right):
>
>
>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregato...
> chrome/renderer/paint_aggregator.h:46: const PendingUpdate&
> GetPendingUpdate();
> It kind of sucks that this function modifies the update inside the paint
> aggregator out from under you. Normally I wouldn't expect this to happen
> for a functionc alled GetFoo().
>
> Maybe we can rename the function, like CoalesceAndGetPendingUpdate()?
>
>
I had a similar thought at first, but I think of it more as a lazy
algorithm.  The caller doesn't need to care since this is the only way for
the caller to access the PendingUpdate structure.  I could keep
GetPendingUpdate const, and make update_ mutable to further the illusion.

-Darin



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

[brettw, 2010-12-06 19:27:01]

On Mon, Dec 6, 2010 at 11:11 AM, Darin Fisher <darin@chromium.org> wrote:
> On Mon, Dec 6, 2010 at 11:09 AM, <brettw@chromium.org> wrote:
>>
>>
>>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregator.h
>> File chrome/renderer/paint_aggregator.h (right):
>>
>>
>>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregato...
>> chrome/renderer/paint_aggregator.h:46: const PendingUpdate&
>> GetPendingUpdate();
>> It kind of sucks that this function modifies the update inside the paint
>> aggregator out from under you. Normally I wouldn't expect this to happen
>> for a functionc alled GetFoo().
>>
>> Maybe we can rename the function, like CoalesceAndGetPendingUpdate()?
>
> I had a similar thought at first, but I think of it more as a lazy
> algorithm.  The caller doesn't need to care since this is the only way for
> the caller to access the PendingUpdate structure.  I could keep
> GetPendingUpdate const, and make update_ mutable to further the illusion.

Yeah, but on the other hand there's nothing magical about
GetPendingUpdate: it doesn't finalize anything and you have to call
ClearPendingUpdate when you're done. So theoretically one could call
GetPendingUpdate at any time, and still queue up more updates. Then
the result would be wrong because there would be intermediate
coalescing. Although I realize that this probably won't happen, but it
still violates my expectation given the API.

Brett

[darin (slow to review), 2010-12-06 19:50:05]

On Mon, Dec 6, 2010 at 11:19 AM, Brett Wilson <brettw@chromium.org> wrote:

> On Mon, Dec 6, 2010 at 11:11 AM, Darin Fisher <darin@chromium.org> wrote:
> > On Mon, Dec 6, 2010 at 11:09 AM, <brettw@chromium.org> wrote:
> >>
> >>
> >>
>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregator.h
> >> File chrome/renderer/paint_aggregator.h (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregato...
> >> chrome/renderer/paint_aggregator.h:46: const PendingUpdate&
> >> GetPendingUpdate();
> >> It kind of sucks that this function modifies the update inside the paint
> >> aggregator out from under you. Normally I wouldn't expect this to happen
> >> for a functionc alled GetFoo().
> >>
> >> Maybe we can rename the function, like CoalesceAndGetPendingUpdate()?
> >
> > I had a similar thought at first, but I think of it more as a lazy
> > algorithm.  The caller doesn't need to care since this is the only way
> for
> > the caller to access the PendingUpdate structure.  I could keep
> > GetPendingUpdate const, and make update_ mutable to further the illusion.
>
> Yeah, but on the other hand there's nothing magical about
> GetPendingUpdate: it doesn't finalize anything and you have to call
> ClearPendingUpdate when you're done. So theoretically one could call
> GetPendingUpdate at any time, and still queue up more updates. Then
> the result would be wrong because there would be intermediate
> coalescing. Although I realize that this probably won't happen, but it
> still violates my expectation given the API.
>
> Brett
>


Ah, how about I rename it GetAndClearPendingUpdate?  The current callsite
does this:

  PaintAggregator::PendingUpdate update =
paint_aggregator_.GetPendingUpdate();
  paint_aggregator_.ClearPendingUpdate();

I could change that to:

  PaintAggregator::PendingUpdate update;
  paint_aggregator_.GetAndClearPendingUpdate(&update);

Other shorter names might work too:  PopPendingUpdate?

-Darin

[brettw, 2010-12-06 19:55:42]

On Mon, Dec 6, 2010 at 11:49 AM, Darin Fisher <darin@chromium.org> wrote:
> On Mon, Dec 6, 2010 at 11:19 AM, Brett Wilson <brettw@chromium.org> wrote:
>>
>> On Mon, Dec 6, 2010 at 11:11 AM, Darin Fisher <darin@chromium.org> wrote:
>> > On Mon, Dec 6, 2010 at 11:09 AM, <brettw@chromium.org> wrote:
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregator.h
>> >> File chrome/renderer/paint_aggregator.h (right):
>> >>
>> >>
>> >>
>> >>
http://codereview.chromium.org/5613002/diff/1/chrome/renderer/paint_aggregato...
>> >> chrome/renderer/paint_aggregator.h:46: const PendingUpdate&
>> >> GetPendingUpdate();
>> >> It kind of sucks that this function modifies the update inside the
>> >> paint
>> >> aggregator out from under you. Normally I wouldn't expect this to
>> >> happen
>> >> for a functionc alled GetFoo().
>> >>
>> >> Maybe we can rename the function, like CoalesceAndGetPendingUpdate()?
>> >
>> > I had a similar thought at first, but I think of it more as a lazy
>> > algorithm.  The caller doesn't need to care since this is the only way
>> > for
>> > the caller to access the PendingUpdate structure.  I could keep
>> > GetPendingUpdate const, and make update_ mutable to further the
>> > illusion.
>>
>> Yeah, but on the other hand there's nothing magical about
>> GetPendingUpdate: it doesn't finalize anything and you have to call
>> ClearPendingUpdate when you're done. So theoretically one could call
>> GetPendingUpdate at any time, and still queue up more updates. Then
>> the result would be wrong because there would be intermediate
>> coalescing. Although I realize that this probably won't happen, but it
>> still violates my expectation given the API.
>>
>> Brett
>
>
> Ah, how about I rename it GetAndClearPendingUpdate?  The current callsite
> does this:
>   PaintAggregator::PendingUpdate update =
> paint_aggregator_.GetPendingUpdate();
>   paint_aggregator_.ClearPendingUpdate();
> I could change that to:
>   PaintAggregator::PendingUpdate update;
>   paint_aggregator_.GetAndClearPendingUpdate(&update);

SGTM.

[darin (slow to review), 2010-12-06 22:41:46]

OK, please take another look.  I went with PopPendingUpdate.

[brettw, 2010-12-06 23:16:18]

LGTM

http://codereview.chromium.org/5613002/diff/16001/chrome/renderer/paint_aggre...
File chrome/renderer/paint_aggregator.h (right):

http://codereview.chromium.org/5613002/diff/16001/chrome/renderer/paint_aggre...
chrome/renderer/paint_aggregator.h:46: // Returns and clears the pending update.
Returns -> Fills |update|