cc: Discard SkCanvas (frame-buffer) before rasterization.

cc: Discard SkCanvas (frame-buffer) before rasterization.

BUG=361266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269958

[alokp, 2014-05-08 18:45:36]

I ran repaint benchmark on key-mobile-sites. I did not see any regressions or
improvements. The improvements may be too tiny to measure probably.

[reveman, 2014-05-09 00:56:29]

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.cc:101: canvas_rect.height()));
Should we really require this? In that case, should |canvas_rect| be
|canvas_origin| instead of passing more data to this function than needed?

[alokp, 2014-05-09 20:13:04]

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.cc:101: canvas_rect.height()));
On 2014/05/09 00:56:29, reveman wrote:
> Should we really require this? In that case, should |canvas_rect| be
> |canvas_origin| instead of passing more data to this function than needed?

We do not require it as far as the API is concerned. But in practice we always
create a new canvas for each raster. The API allows you to offset multiple
rasters into a single canvas. But we have gradually started making the
assumption that it is OK to write outside canvas_rect - see line 143 below where
we clear the entire canvas. I wanted to add a DCHECK to ensure this assumption.

You are right that we should change the API to make this assumption explicit.
But I think it is outside the scope of this patch.

[reveman, 2014-05-10 00:34:52]

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.cc:101: canvas_rect.height()));
On 2014/05/09 20:13:04, Alok Priyadarshi wrote:
> On 2014/05/09 00:56:29, reveman wrote:
> > Should we really require this? In that case, should |canvas_rect| be
> > |canvas_origin| instead of passing more data to this function than needed?
> 
> We do not require it as far as the API is concerned. But in practice we always
> create a new canvas for each raster. The API allows you to offset multiple
> rasters into a single canvas. But we have gradually started making the
> assumption that it is OK to write outside canvas_rect - see line 143 below
where
> we clear the entire canvas. I wanted to add a DCHECK to ensure this
assumption.
> 
> You are right that we should change the API to make this assumption explicit.
> But I think it is outside the scope of this patch.

I think I prefer if you didn't add this DCHECK. Without it, this API can be
interpreted as "Here's a canvas of some size, you can do whatever with it but
canvas_rect is the area I like correct contents to be produced for". If this
flexibility is unnecessary, I'd rather see us change the API right away than add
temporary DCHECKs.

[alokp, 2014-05-12 18:45:32]

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.cc:101: canvas_rect.height()));
OK. Removed DCHECK. I think we should make it clear through documentation or
otherwise that this function may write over area outside canvas_rect.

[reveman, 2014-05-12 18:51:38]

lgtm

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/232843006/diff/20001/cc/resources/picture_pil...
cc/resources/picture_pile_impl.cc:101: canvas_rect.height()));
On 2014/05/12 18:45:33, Alok Priyadarshi wrote:
> OK. Removed DCHECK. I think we should make it clear through documentation or
> otherwise that this function may write over area outside canvas_rect.

Yea, that wouldn't hurt. Feel free to add a comment to picture_pile_impl.h as
part of this patch if you like.

[alokp, 2014-05-12 21:25:11]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-05-12 21:26:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/232843006/70001

[commit-bot: I haz the power, 2014-05-13 01:33:28]

Change committed as 269958