cc: Only consider layer clipped content rect for analysis

cc: Only consider layer clipped content rect for analysis

This fixes a problem in which tiles that are on the right and bottom edges of the viewport are not considered solid, even when the visible part of them is solid.

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

[vmpstr, 2013-04-17 16:41:57]

Please take a look.

https://codereview.chromium.org/14205005/diff/1/cc/resources/picture_pile_imp...
File cc/resources/picture_pile_impl.cc (left):

https://codereview.chromium.org/14205005/diff/1/cc/resources/picture_pile_imp...
cc/resources/picture_pile_impl.cc:251: gfx::Rect layer_rect =
gfx::ToEnclosingRect(
This layer_rect was unused, hence the diff looks like I changed it.

[enne (OOO), 2013-04-17 16:53:51]

In general, this looks great.  I assume this addresses the extra upload issue on
the bug?

I just have one edge case concern here, which is what happens at different
scales.  If you scale the canvas by some floating point value < 1, then the edge
pixels on a theoretically opaque layer will not actually be opaque.  This is why
there's code around line 105 to fill with the background color outside of the
rect so that when we sample outside the layer bounds to generate a texel inside
the layer bounds, we sample against something reasonable (i.e. an opaque color
when the layer is opaque, like the root layer).

That code clips (to reduce raster costs), but it is essentially equivalent to
just an unclipped drawColor(background_color_, ...) prior to rastering
everything.  Should we be replicating a similar case here?

Can you add a unit test for this?  I don't think we have any picture pile tests,
but it's a good time to start.

[vmpstr, 2013-04-17 17:09:39]

On 2013/04/17 16:53:51, enne wrote:
> In general, this looks great.  I assume this addresses the extra upload issue
on
> the bug?

It significantly improves it. I'm still getting 1 raster on the page in the bug,
so I'll try to track that down.

> 
> I just have one edge case concern here, which is what happens at different
> scales.  If you scale the canvas by some floating point value < 1, then the
edge
> pixels on a theoretically opaque layer will not actually be opaque.  This is
why
> there's code around line 105 to fill with the background color outside of the
> rect so that when we sample outside the layer bounds to generate a texel
inside
> the layer bounds, we sample against something reasonable (i.e. an opaque color
> when the layer is opaque, like the root layer).
> 
> That code clips (to reduce raster costs), but it is essentially equivalent to
> just an unclipped drawColor(background_color_, ...) prior to rastering
> everything.  Should we be replicating a similar case here?
> 

I think this would come for free. Analysis just constructs a canvas, but raster
into this canvas happens via the PicturePileImpl::Raster call, which means all
of the extra checks that affect raster would also affect analysis. That's one of
the main reasons I wanted to reuse Raster: it wouldn't break if Raster somehow
changed or drew some additional elements, like the fill you're describing.

> Can you add a unit test for this?  I don't think we have any picture pile
tests,
> but it's a good time to start.

Will do. I still have to do unit tests for Picture gather refs thing anyway, so
I might as well spend some more time writing more tests for PicturePileImpl :)

[enne (OOO), 2013-04-17 17:12:04]

On 2013/04/17 17:09:39, vmpstr wrote:
> I think this would come for free. Analysis just constructs a canvas, but
raster
> into this canvas happens via the PicturePileImpl::Raster call, which means all
> of the extra checks that affect raster would also affect analysis. That's one
of
> the main reasons I wanted to reuse Raster: it wouldn't break if Raster somehow
> changed or drew some additional elements, like the fill you're describing.

Oh, my mistake.  You're quite right.  And, this would also be easy to verify in
a unit test.

[vmpstr, 2013-04-17 20:24:46]

Added unittests. I pulled TestablePicturePileImpl from
picture_layer_impl_unittest into test/ (and renamed it to be Fake* instead of
Testable*), so the CL kind of looks large now.

PTAL.

[enne (OOO), 2013-04-17 21:04:28]

lgtm.  I'm curious what the 1 raster on the page is, but this is a clear win and
has tests.  :)

[commit-bot: I haz the power, 2013-04-18 19:05:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14205005/6001

[commit-bot: I haz the power, 2013-04-18 21:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14205005/8004

[commit-bot: I haz the power, 2013-04-19 00:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/14205005/29001

[commit-bot: I haz the power, 2013-04-19 08:39:04]

Change committed as 195103