cc: SolidColorLayerImpl::AppendSolidQuads takes the visible content rect

cc: SolidColorLayerImpl::AppendSolidQuads uses visible content rect

Huge solid layers can cause a lot of memory to be allocated because
append quads function would use the content_bounds, not the
visible_content_rect.

PLI now uses visible rect, but SCLI is still using the content
bounds, this should probably be addressed in a follow up, but I'm
not too worried about this since it always worked this way and
this is mostly used by unit tests.  We can't simply pass the
visible rect because none of the unit tests set the visible rect,
and the visible rect is not passed from SCL to SCLI for tests that
don't directly interact with the SCLI.

BUG=423950
Committed: https://crrev.com/1610bcc9cf8f30c55d81d3d9aa2391f993667ee5
Cr-Commit-Position: refs/heads/master@{#301134}

[hendrikw, 2014-10-23 23:24:51]

hendrikw@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-10-23 23:25:09]

PTAL

[danakj, 2014-10-23 23:27:28]

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
File cc/layers/solid_color_layer_impl.cc (right):

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
cc/layers/solid_color_layer_impl.cc:79: gfx::Rect(content_bounds()),
why isn't this the visible rect too? same problem with a huge solid color layer.

[danakj, 2014-10-23 23:38:21]

https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:1457:
EXPECT_TRUE(visible_rect.Contains(quad.rect));
the visible_rect is not used at all except here. how is it not an input to the
test's behaviour also? is it the viewport on the host_impl_? can you tie that
more explicitly?

https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:4430:
active_layer_->draw_properties().visible_content_rect = layer_rect;
why isn't this set by updatedrawproperties?

do you need to adjust the hostimpl's viewport instead? or call updatedrawprops
above? this doesn't seem like a good thing to do here

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
File cc/layers/solid_color_layer_impl.cc (right):

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
cc/layers/solid_color_layer_impl.cc:79: gfx::Rect(content_bounds()),
please leave a TODO pointing to a bug.

https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
File cc/test/fake_picture_pile_impl.h (right):

https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
cc/test/fake_picture_pile_impl.h:73: void set_is_solid_color() { is_solid_color_
= true; }
pass a bool in so we don't have to add that later?

[hendrikw, 2014-10-23 23:53:49]

Good catches, thanks!

PTAL

https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:1457:
EXPECT_TRUE(visible_rect.Contains(quad.rect));
On 2014/10/23 23:38:21, danakj wrote:
> the visible_rect is not used at all except here. how is it not an input to the
> test's behaviour also? is it the viewport on the host_impl_? can you tie that
> more explicitly?

My bad, I meant to remove the one below, and keep the code that set the visible
rect in this function.

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
File cc/layers/solid_color_layer_impl.cc (right):

https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
cc/layers/solid_color_layer_impl.cc:79: gfx::Rect(content_bounds()),
On 2014/10/23 23:38:21, danakj wrote:
> please leave a TODO pointing to a bug.

Acknowledged.

https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
File cc/test/fake_picture_pile_impl.h (right):

https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
cc/test/fake_picture_pile_impl.h:73: void set_is_solid_color() { is_solid_color_
= true; }
On 2014/10/23 23:38:21, danakj wrote:
> pass a bool in so we don't have to add that later?

Acknowledged.  It's funny, the last review I sent, someone had me remove the
bool (since it was only ever set to true), so this time I left it out.

[hendrikw, 2014-10-23 23:55:04]

On 2014/10/23 23:53:49, Hendrik wrote:
> Good catches, thanks!
> 
> PTAL
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
> File cc/layers/picture_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/layers/picture_layer_...
> cc/layers/picture_layer_impl_unittest.cc:1457:
> EXPECT_TRUE(visible_rect.Contains(quad.rect));
> On 2014/10/23 23:38:21, danakj wrote:
> > the visible_rect is not used at all except here. how is it not an input to
the
> > test's behaviour also? is it the viewport on the host_impl_? can you tie
that
> > more explicitly?
> 
> My bad, I meant to remove the one below, and keep the code that set the
visible
> rect in this function.
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
> File cc/layers/solid_color_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/layers/solid_color_la...
> cc/layers/solid_color_layer_impl.cc:79: gfx::Rect(content_bounds()),
> On 2014/10/23 23:38:21, danakj wrote:
> > please leave a TODO pointing to a bug.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
> File cc/test/fake_picture_pile_impl.h (right):
> 
>
https://codereview.chromium.org/675903002/diff/40001/cc/test/fake_picture_pil...
> cc/test/fake_picture_pile_impl.h:73: void set_is_solid_color() {
is_solid_color_
> = true; }
> On 2014/10/23 23:38:21, danakj wrote:
> > pass a bool in so we don't have to add that later?
> 
> Acknowledged.  It's funny, the last review I sent, someone had me remove the
> bool (since it was only ever set to true), so this time I left it out.

oops, I removed the wrong one :)

[hendrikw, 2014-10-23 23:57:24]

PTAL for real!  :)

[danakj, 2014-10-24 14:58:39]

https://codereview.chromium.org/675903002/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/675903002/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1459:
EXPECT_TRUE(visible_rect.Contains(quad.rect));
I think you mean Intersects here. How did you get lucky and Contains works with
such round numbers?

I think what you want to use is something like this:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_test...

[hendrikw, 2014-10-24 15:32:15]

https://codereview.chromium.org/675903002/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/675903002/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1459:
EXPECT_TRUE(visible_rect.Contains(quad.rect));
On 2014/10/24 14:58:39, danakj wrote:
> I think you mean Intersects here. How did you get lucky and Contains works
with
> such round numbers?
> 
> I think what you want to use is something like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_test...

No, I meant to use Contains, we go from left to right, top to bottom of the
visible rect when appending the quads, so unless I'm missing something (which I
could be), the quads should always be contained within it.  Luck had nothing to
do with it :)

I'm testing for quads outside of the visible region, the test you pointed me to
tests for full coverage.  Full coverage isn't what I'm fixing, but I can do this
too if you like?

[danakj, 2014-10-24 15:35:03]

On Fri, Oct 24, 2014 at 11:32 AM, <hendrikw@chromium.org> wrote:

>
> https://codereview.chromium.org/675903002/diff/100001/cc/
> layers/picture_layer_impl_unittest.cc
> File cc/layers/picture_layer_impl_unittest.cc (right):
>
> https://codereview.chromium.org/675903002/diff/100001/cc/
> layers/picture_layer_impl_unittest.cc#newcode1459
> cc/layers/picture_layer_impl_unittest.cc:1459:
> EXPECT_TRUE(visible_rect.Contains(quad.rect));
> On 2014/10/24 14:58:39, danakj wrote:
>
>> I think you mean Intersects here. How did you get lucky and Contains
>>
> works with
>
>> such round numbers?
>>
>
>  I think what you want to use is something like this:
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/cc/test/layer_test_common.cc&rcl=1413930987&l=39
>
> No, I meant to use Contains, we go from left to right, top to bottom of
> the visible rect when appending the quads, so unless I'm missing
> something (which I could be), the quads should always be contained
> within it.  Luck had nothing to do with it :)
>

If a quad would be only half visible, we should still append it though.

I'm testing for quads outside of the visible region, the test you
> pointed me to tests for full coverage.  Full coverage isn't what I'm
> fixing, but I can do this too if you like?
>

It's hard to have confidence that the test isn't just not appending
anything and passing without some sanity check in the test to show that
though?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hendrikw, 2014-10-24 15:41:14]

On 2014/10/24 15:35:03, danakj wrote:
> On Fri, Oct 24, 2014 at 11:32 AM, <mailto:hendrikw@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/675903002/diff/100001/cc/
> > layers/picture_layer_impl_unittest.cc
> > File cc/layers/picture_layer_impl_unittest.cc (right):
> >
> > https://codereview.chromium.org/675903002/diff/100001/cc/
> > layers/picture_layer_impl_unittest.cc#newcode1459
> > cc/layers/picture_layer_impl_unittest.cc:1459:
> > EXPECT_TRUE(visible_rect.Contains(quad.rect));
> > On 2014/10/24 14:58:39, danakj wrote:
> >
> >> I think you mean Intersects here. How did you get lucky and Contains
> >>
> > works with
> >
> >> such round numbers?
> >>
> >
> >  I think what you want to use is something like this:
> >>
> >
> > https://code.google.com/p/chromium/codesearch#chromium/
> > src/cc/test/layer_test_common.cc&rcl=1413930987&l=39
> >
> > No, I meant to use Contains, we go from left to right, top to bottom of
> > the visible rect when appending the quads, so unless I'm missing
> > something (which I could be), the quads should always be contained
> > within it.  Luck had nothing to do with it :)
> >
> 
> If a quad would be only half visible, we should still append it though.

We do, but it would be half the size, and still contained within the visible
rect.

> 
> I'm testing for quads outside of the visible region, the test you
> > pointed me to tests for full coverage.  Full coverage isn't what I'm
> > fixing, but I can do this too if you like?
> >
> 
> It's hard to have confidence that the test isn't just not appending
> anything and passing without some sanity check in the test to show that
> though?

I'll check for full coverage too.

[danakj, 2014-10-24 15:43:20]

On Fri, Oct 24, 2014 at 11:41 AM, <hendrikw@chromium.org> wrote:

> On 2014/10/24 15:35:03, danakj wrote:
>
>> On Fri, Oct 24, 2014 at 11:32 AM, <mailto:hendrikw@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/675903002/diff/100001/cc/
>> > layers/picture_layer_impl_unittest.cc
>> > File cc/layers/picture_layer_impl_unittest.cc (right):
>> >
>> > https://codereview.chromium.org/675903002/diff/100001/cc/
>> > layers/picture_layer_impl_unittest.cc#newcode1459
>> > cc/layers/picture_layer_impl_unittest.cc:1459:
>> > EXPECT_TRUE(visible_rect.Contains(quad.rect));
>> > On 2014/10/24 14:58:39, danakj wrote:
>> >
>> >> I think you mean Intersects here. How did you get lucky and Contains
>> >>
>> > works with
>> >
>> >> such round numbers?
>> >>
>> >
>> >  I think what you want to use is something like this:
>> >>
>> >
>> > https://code.google.com/p/chromium/codesearch#chromium/
>> > src/cc/test/layer_test_common.cc&rcl=1413930987&l=39
>> >
>> > No, I meant to use Contains, we go from left to right, top to bottom of
>> > the visible rect when appending the quads, so unless I'm missing
>> > something (which I could be), the quads should always be contained
>> > within it.  Luck had nothing to do with it :)
>> >
>>
>
>  If a quad would be only half visible, we should still append it though.
>>
>
> We do, but it would be half the size, and still contained within the
> visible
> rect.


Ah.. right, cuz it's the |visible_rect| not the |rect| you're testing here,
okay!


>
>
>
>  I'm testing for quads outside of the visible region, the test you
>> > pointed me to tests for full coverage.  Full coverage isn't what I'm
>> > fixing, but I can do this too if you like?
>> >
>>
>
>  It's hard to have confidence that the test isn't just not appending
>> anything and passing without some sanity check in the test to show that
>> though?
>>
>
> I'll check for full coverage too.
>

Thanks!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hendrikw, 2014-10-24 15:50:27]

Added tests for full coverage.  PTAL

[danakj, 2014-10-24 15:58:24]

LGTM

[hendrikw, 2014-10-24 16:32:51]

I had an issue in an older test, PictureLayerImplTest.DrawSolidQuads, I wasn't
calling UpdateDrawProperties, so the visible rect wasn't being copied over. 
Fixed!

[danakj, 2014-10-24 16:34:05]

LGTNM

[danakj, 2014-10-24 16:34:18]

On 2014/10/24 16:34:05, danakj wrote:
> LGTNM

er... -N :D

[hendrikw, 2014-10-24 16:37:11]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:38:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675903002/130001

[commit-bot: I haz the power, 2014-10-24 17:28:52]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2014-10-24 17:29:35]

Patchset 8 (id:??) landed as
https://crrev.com/1610bcc9cf8f30c55d81d3d9aa2391f993667ee5
Cr-Commit-Position: refs/heads/master@{#301134}