Adjust view unittest to work with impl-side-painting

Adjust view unittest to work with impl-side-painting

View unittest builds on assumption of exact invalidation rect being
used. Impl side would change that assumption. Adjust unittests to test
for both impl-side-painting and non-impl-side-painting.

R=danakj
BUG=314185
Committed: https://crrev.com/98bc520d5bef9bb2b3699f10e1b9fdc306de9c6a
Cr-Commit-Position: refs/heads/master@{#319566}

[weiliangc, 2015-03-04 01:09:44]

Still kinda magic number but good enough?

[weiliangc, 2015-03-04 01:10:59]

weiliangc@chromium.org changed reviewers:
+ sky@chromium.org

[weiliangc, 2015-03-04 01:11:47]

Trying to fix bot failures in https://codereview.chromium.org/962833003/

[sky, 2015-03-04 15:58:20]

On 2015/03/04 01:11:47, weiliangc wrote:
> Trying to fix bot failures in https://codereview.chromium.org/962833003/

Why does impl side painting result in painting a bigger region that was damaged?

[danakj, 2015-03-04 16:33:53]

On Wed, Mar 4, 2015 at 7:58 AM, <sky@chromium.org> wrote:

> On 2015/03/04 01:11:47, weiliangc wrote:
>
>> Trying to fix bot failures in https://codereview.chromium.org/962833003/
>>
>
> Why does impl side painting result in painting a bigger region that was
> damaged?
>

Impl side painting records tiles. When an invalidation occurs, all tiles
that intersect it are re-recorded (this behaviour will change again as we
move to display lists).

In old compositor, the size of the paint rect == the number of rastered
pixels. In impl-side painting the painting step is simply a recording. We
later raster from those tiled recordings into bitmaps via other mechanisms.


>
> https://codereview.chromium.org/976923002/
>

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

[sky, 2015-03-04 16:35:54]

On 2015/03/04 16:33:53, danakj wrote:
> On Wed, Mar 4, 2015 at 7:58 AM, <mailto:sky@chromium.org> wrote:
> 
> > On 2015/03/04 01:11:47, weiliangc wrote:
> >
> >> Trying to fix bot failures in https://codereview.chromium.org/962833003/
> >>
> >
> > Why does impl side painting result in painting a bigger region that was
> > damaged?
> >
> 
> Impl side painting records tiles. When an invalidation occurs, all tiles
> that intersect it are re-recorded (this behaviour will change again as we
> move to display lists).
> 
> In old compositor, the size of the paint rect == the number of rastered
> pixels. In impl-side painting the painting step is simply a recording. We
> later raster from those tiled recordings into bitmaps via other mechanisms.
> 
> 
> >
> > https://codereview.chromium.org/976923002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, thanks. Is it possible for tests to change the tile size? Seems like a tile
size of 1x1 would mean no changes are necessary, right?

[danakj, 2015-03-04 16:49:05]

On Wed, Mar 4, 2015 at 8:35 AM, <sky@chromium.org> wrote:

> On 2015/03/04 16:33:53, danakj wrote:
>
>  On Wed, Mar 4, 2015 at 7:58 AM, <mailto:sky@chromium.org> wrote:
>>
>
>  > On 2015/03/04 01:11:47, weiliangc wrote:
>> >
>> >> Trying to fix bot failures in https://codereview.chromium.
>> org/962833003/
>> >>
>> >
>> > Why does impl side painting result in painting a bigger region that was
>> > damaged?
>> >
>>
>
>  Impl side painting records tiles. When an invalidation occurs, all tiles
>> that intersect it are re-recorded (this behaviour will change again as we
>> move to display lists).
>>
>
>  In old compositor, the size of the paint rect == the number of rastered
>> pixels. In impl-side painting the painting step is simply a recording. We
>> later raster from those tiled recordings into bitmaps via other
>> mechanisms.
>>
>
>
>  >
>> > https://codereview.chromium.org/976923002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Ok, thanks. Is it possible for tests to change the tile size? Seems like a
> tile
> size of 1x1 would mean no changes are necessary, right?
>

It would be possible by replacing ui::Layer's PictureLayers with
FakePictureLayers
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_pictu...
and passing in a recording source that you have changed the tile size for,
by allowing FakePicturePile to override the tile size
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...
.

This requires adding test hooks/subclass to ui::Layer that
virtualizes/indirects the PictureLayer creation, and making use of cc/test/
support stuff (maybe a new dependency for the views tests). It's possible.
Does that sound like something you want?

It's worth noting here that when we switch to DisplayLists instead of
PicturePiles backing the PictureLayer tho, any invalidation will just ask
for the whole DisplayList structure again, there won't be any rects at all
(changes will simply be stored in the DisplayList by the view then handed
to the compositor, it won't have to tell the compositor invalidation rects
at all). But then these tests (and maybe the cull set structure itself)
make less sense when we get to that.


>
> https://codereview.chromium.org/976923002/
>

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

[enne (OOO), 2015-03-04 17:20:01]

> Ok, thanks. Is it possible for tests to change the tile size? Seems like a
tile size of 1x1 would mean no changes are necessary, right?

The paint rect size is an implementation detail of cc.  The API to cc::Layer is
that the embedder sends in damage rects, cc merges them and then decides based
on the visible rect and its own internal data structures what paint rect to ask
for back.  The fact that this depended on getting a 1x1 paint rect back is just
depending on implementation details that are no longer true.

Changing to a tile size of 1x1 would only fix this problem if you also changed
the minimum scale to 1 so that border pixels weren't added.  Even so, this would
then make the test only true until the next time the implementation changed.  I
don't think that's the right answer.

[danakj, 2015-03-05 16:21:46]

I think I understand what we need to do here.. the problem is that this is
testing the compositor at all. It's sending in a paint request and hoping
to get back the right thing. What these tests should do is just sent that
paint rect that they know they want directly thru the "OnPaint" whatever
methods that the compositor /would/ send it to, but bypassing the
compositor entirely, and just testing the systems and APIs that it is
meaning to test. This is totally the wrong level to be testing what rects
come from the compositor, instead it should just be testing "if i get this
rect i do this" which it can do by causing itself to "get this rect"
directly. Then we don't have to make any geometric changes to these tests
(for now).

On Wed, Mar 4, 2015 at 9:20 AM, <enne@chromium.org> wrote:

> Ok, thanks. Is it possible for tests to change the tile size? Seems like a
>>
> tile size of 1x1 would mean no changes are necessary, right?
>
> The paint rect size is an implementation detail of cc.  The API to
> cc::Layer is
> that the embedder sends in damage rects, cc merges them and then decides
> based
> on the visible rect and its own internal data structures what paint rect
> to ask
> for back.  The fact that this depended on getting a 1x1 paint rect back is
> just
> depending on implementation details that are no longer true.
>
> Changing to a tile size of 1x1 would only fix this problem if you also
> changed
> the minimum scale to 1 so that border pixels weren't added.  Even so, this
> would
> then make the test only true until the next time the implementation
> changed.  I
> don't think that's the right answer.
>
> https://codereview.chromium.org/976923002/
>

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

[weiliangc, 2015-03-06 18:10:43]

Updated:
view unittests shouldn't go through cc to test cull set related functionalities.
Add function in unittest to just test paint to widget's root view instead of go
thru cc.

[danakj, 2015-03-06 18:21:59]

LGTM

https://codereview.chromium.org/976923002/diff/20001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/976923002/diff/20001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:181: float image_scale = 1;
1.f

https://codereview.chromium.org/976923002/diff/20001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:183: gfx::Canvas
gcanvas(widget->GetRootView()->bounds().size(), image_scale,
nit: just |canvas| ?

[weiliangc, 2015-03-06 21:31:35]

sky PTAL.

[sky, 2015-03-06 23:33:17]

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:180: void PaintToViewInRect(views::Widget* widget,
const gfx::Rect& rect) {
As this takes a Widget, how about naming it PaintWidget?

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:181: float image_scale = 1.f;
nit: const on these two.

[sky, 2015-03-06 23:33:25]

LGTM with those two changes.

[weiliangc, 2015-03-07 00:22:52]

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:180: void PaintToViewInRect(views::Widget* widget,
const gfx::Rect& rect) {
On 2015/03/06 23:33:17, sky wrote:
> As this takes a Widget, how about naming it PaintWidget?

Done.

https://codereview.chromium.org/976923002/diff/40001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:181: float image_scale = 1.f;
On 2015/03/06 23:33:17, sky wrote:
> nit: const on these two.

Done.

[weiliangc, 2015-03-07 00:22:58]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-03-07 00:22:59]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/976923002/#ps60001 (title:
"address review comments")

[commit-bot: I haz the power, 2015-03-07 00:23:40]

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

[commit-bot: I haz the power, 2015-03-07 02:18:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 02:18:44]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[weiliangc, 2015-03-07 21:12:29]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2015-03-07 21:13:05]

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

[commit-bot: I haz the power, 2015-03-07 23:03:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-07 23:04:22]

Patchset 4 (id:??) landed as
https://crrev.com/98bc520d5bef9bb2b3699f10e1b9fdc306de9c6a
Cr-Commit-Position: refs/heads/master@{#319566}