Add a flag to disable partial raster in renderer

Add a flag to disable partial raster in renderer

Partial raster was intended to be disabled in renderers, but it was
disabled indirectly through a GPUMemoryBuffer property, which didn't
apply to all rasterization modes / tile workers.

This change adds a flag "--enable-partial-raster" which will cause
partial raster to be enabled (current state) if passed. The default
behavior is now that we block partial raster at a higher level, but
not providing invalidated IDs to tiles.

BUG=537722
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/1d17f75189e66a12ef287af75e0d6adb5449db3d
Cr-Commit-Position: refs/heads/master@{#354966}

[ericrk, 2015-10-02 20:15:13]

ericrk@chromium.org changed reviewers:
+ vmpstr@chromium.org

[ericrk, 2015-10-02 20:15:14]

https://codereview.chromium.org/1381163002/diff/1/cc/layers/picture_layer_impl.h
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/1381163002/diff/1/cc/layers/picture_layer_imp...
cc/layers/picture_layer_impl.h:55: gfx::Rect GetEnclosingRectInTargetSpace()
const override;
Moved this up from below - was in the wrong section.

[ericrk, 2015-10-02 20:15:54]

ericrk@chromium.org changed reviewers:
+ jbroman@chromium.org

[ericrk, 2015-10-02 20:15:55]

+jbroman as well

[jbroman, 2015-10-05 13:38:38]

jbroman@chromium.org changed reviewers:
+ danakj@chromium.org

[jbroman, 2015-10-05 13:38:38]

Seems reasonable to me in principle. Since this was the purpose of the
--enable-persistent-gpu-memory-buffer flag, you can probably remove that flag if
you add this one.

I believe danakj added the original partial raster support (for the UI
compositor), and would probably be a better reviewer than me for that angle (I
was lead to believe that the GPUMemoryBuffer restriction was sufficient).

[ericrk, 2015-10-05 20:28:02]

On 2015/10/05 at 13:38:38, jbroman wrote:
> Seems reasonable to me in principle. Since this was the purpose of the
--enable-persistent-gpu-memory-buffer flag, you can probably remove that flag if
you add this one.
> 
> I believe danakj added the original partial raster support (for the UI
compositor), and would probably be a better reviewer than me for that angle (I
was lead to believe that the GPUMemoryBuffer restriction was sufficient).

The previous flag (enable-persistent-gpu-memory-buffer) was only used in the
one-copy tile task worker pool, as far as I can tell. Doesn't seem to impact
zero copy or GPU - which is where I was seeing this. I'll remove your previous
flag and just add this one, as it covers all bases.

I'll also add Dana.

[vmpstr, 2015-10-06 18:35:49]

https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
cc/test/fake_picture_layer_tiling_client.h:50: void
set_enable_partial_Raster(bool enable_partial_raster) {
nit: capitalization typo

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:343: tile->SetInvalidated(invalid_content_rect,
old_tile->id());
You probably need a similar check here. (with test please)

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1822: EXPECT_FALSE(tiling_->TileAt(0,
0)->invalidated_id());
can you make this EXPECT_EQ(0, ...);

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1824: EXPECT_TRUE(tiling_->TileAt(0,
0)->invalidated_id());
Can you save off the original tile id and EXPECT_EQ it?

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1843: EXPECT_FALSE(tiling_->TileAt(0,
0)->invalidated_id());
Same here.

[ericrk, 2015-10-06 19:57:54]

On 2015/10/06 at 18:35:49, vmpstr wrote:
>
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> File cc/test/fake_picture_layer_tiling_client.h (right):
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> cc/test/fake_picture_layer_tiling_client.h:50: void
set_enable_partial_Raster(bool enable_partial_raster) {
> nit: capitalization typo
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> File cc/tiles/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> cc/tiles/picture_layer_tiling.cc:343:
tile->SetInvalidated(invalid_content_rect, old_tile->id());
> You probably need a similar check here. (with test please)
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> File cc/tiles/picture_layer_tiling_unittest.cc (right):
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> cc/tiles/picture_layer_tiling_unittest.cc:1822:
EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> can you make this EXPECT_EQ(0, ...);
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> cc/tiles/picture_layer_tiling_unittest.cc:1824: EXPECT_TRUE(tiling_->TileAt(0,
0)->invalidated_id());
> Can you save off the original tile id and EXPECT_EQ it?
> 
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> cc/tiles/picture_layer_tiling_unittest.cc:1843:
EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> Same here.

Thanks for the feedback - trying to decide what the best approach is here.

jbroman - do you think there's some value in keeping the persistent
GPUMemoryBuffer switch? I see that we currently do use this value in a few
places to turn persistent buffers on and off. Is there any value to persistent
buffers being off over just providing a way to disable partial raster? If we
remove the flag should we just make everything use persistent buffers?

[jbroman, 2015-10-06 20:22:59]

On 2015/10/06 at 19:57:54, ericrk wrote:
> On 2015/10/06 at 18:35:49, vmpstr wrote:
> >
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> > File cc/test/fake_picture_layer_tiling_client.h (right):
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> > cc/test/fake_picture_layer_tiling_client.h:50: void
set_enable_partial_Raster(bool enable_partial_raster) {
> > nit: capitalization typo
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > File cc/tiles/picture_layer_tiling.cc (right):
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > cc/tiles/picture_layer_tiling.cc:343:
tile->SetInvalidated(invalid_content_rect, old_tile->id());
> > You probably need a similar check here. (with test please)
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > File cc/tiles/picture_layer_tiling_unittest.cc (right):
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > cc/tiles/picture_layer_tiling_unittest.cc:1822:
EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> > can you make this EXPECT_EQ(0, ...);
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > cc/tiles/picture_layer_tiling_unittest.cc:1824:
EXPECT_TRUE(tiling_->TileAt(0, 0)->invalidated_id());
> > Can you save off the original tile id and EXPECT_EQ it?
> > 
> >
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > cc/tiles/picture_layer_tiling_unittest.cc:1843:
EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> > Same here.
> 
> Thanks for the feedback - trying to decide what the best approach is here.
> 
> jbroman - do you think there's some value in keeping the persistent
GPUMemoryBuffer switch? I see that we currently do use this value in a few
places to turn persistent buffers on and off. Is there any value to persistent
buffers being off over just providing a way to disable partial raster? If we
remove the flag should we just make everything use persistent buffers?

Unless someone else has started using it for something else, the only purpose of
--enable-persistent-gpu-memory-buffer is to disable renderer partial raster.

[ericrk, 2015-10-07 17:11:56]

Patchset #2 (id:20001) has been deleted

[ericrk, 2015-10-07 17:16:34]

On 2015/10/06 20:22:59, jbroman wrote:
> On 2015/10/06 at 19:57:54, ericrk wrote:
> > On 2015/10/06 at 18:35:49, vmpstr wrote:
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> > > File cc/test/fake_picture_layer_tiling_client.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
> > > cc/test/fake_picture_layer_tiling_client.h:50: void
> set_enable_partial_Raster(bool enable_partial_raster) {
> > > nit: capitalization typo
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > File cc/tiles/picture_layer_tiling.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > cc/tiles/picture_layer_tiling.cc:343:
> tile->SetInvalidated(invalid_content_rect, old_tile->id());
> > > You probably need a similar check here. (with test please)
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > File cc/tiles/picture_layer_tiling_unittest.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > cc/tiles/picture_layer_tiling_unittest.cc:1822:
> EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> > > can you make this EXPECT_EQ(0, ...);
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > cc/tiles/picture_layer_tiling_unittest.cc:1824:
> EXPECT_TRUE(tiling_->TileAt(0, 0)->invalidated_id());
> > > Can you save off the original tile id and EXPECT_EQ it?
> > > 
> > >
>
https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
> > > cc/tiles/picture_layer_tiling_unittest.cc:1843:
> EXPECT_FALSE(tiling_->TileAt(0, 0)->invalidated_id());
> > > Same here.
> > 
> > Thanks for the feedback - trying to decide what the best approach is here.
> > 
> > jbroman - do you think there's some value in keeping the persistent
> GPUMemoryBuffer switch? I see that we currently do use this value in a few
> places to turn persistent buffers on and off. Is there any value to persistent
> buffers being off over just providing a way to disable partial raster? If we
> remove the flag should we just make everything use persistent buffers?
> 
> Unless someone else has started using it for something else, the only purpose
of
> --enable-persistent-gpu-memory-buffer is to disable renderer partial raster.

Ok, thanks! I'll actually do this in a follow-up CL, as there are quite a lot of
changes necessary to remove the flag and it's somewhat separate from this
change.

[ericrk, 2015-10-07 17:32:33]

Patchset #3 (id:60001) has been deleted

[ericrk, 2015-10-07 17:59:08]

Thanks for the feedback!

https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/1381163002/diff/1/cc/test/fake_picture_layer_...
cc/test/fake_picture_layer_tiling_client.h:50: void
set_enable_partial_Raster(bool enable_partial_raster) {
On 2015/10/06 18:35:48, vmpstr wrote:
> nit: capitalization typo

Done.

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:343: tile->SetInvalidated(invalid_content_rect,
old_tile->id());
On 2015/10/06 18:35:48, vmpstr wrote:
> You probably need a similar check here. (with test please)

Yup - missed this and had a unit test bug =/... I've updated this as well as the
tests. Thanks!

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1822: EXPECT_FALSE(tiling_->TileAt(0,
0)->invalidated_id());
On 2015/10/06 18:35:49, vmpstr wrote:
> can you make this EXPECT_EQ(0, ...);

Done.

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1824: EXPECT_TRUE(tiling_->TileAt(0,
0)->invalidated_id());
On 2015/10/06 18:35:49, vmpstr wrote:
> Can you save off the original tile id and EXPECT_EQ it?

Done.

https://codereview.chromium.org/1381163002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:1843: EXPECT_FALSE(tiling_->TileAt(0,
0)->invalidated_id());
On 2015/10/06 18:35:49, vmpstr wrote:
> Same here.

Done.

[ericrk, 2015-10-07 23:35:24]

Patchset #4 (id:100001) has been deleted

[ericrk, 2015-10-07 23:40:14]

Patchset #4 (id:120001) has been deleted

[ericrk, 2015-10-08 00:11:13]

Patchset #4 (id:140001) has been deleted

[ericrk, 2015-10-08 00:12:20]

Updated this to combine it with "--enable-persistent-gpu-memory-buffer". There
are rare cases in which you might want two different flags, but I figured it's
better to start simple and add more complexity in later if we really need the
functionality.

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
Is there a reason we had a disable flag as well as an enable flag? Only enable
was used. Is this just the style?

[jbroman, 2015-10-08 14:08:22]

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
On 2015/10/08 at 00:12:20, ericrk wrote:
> Is there a reason we had a disable flag as well as an enable flag? Only enable
> was used. Is this just the style?

It wasn't necessary to forward it, no.

[danakj, 2015-10-08 15:18:43]

One q: Why do you prefer not setting ids on the tiles in PictureLayerTiling vs
just not grabbing an old resource from the pool with that id in gpu/bitmap
worker pools?

Right now there are if statements in the one copy pool for partial_raster && old
id.

But you're making the old id == 0 if partial raster is off. We should choose
one: check in worker pools or set ids to always zero?

I kinda slightly prefer if statements in the worker pools. One copy has to
branch on it anyways for deciding persistent map or not, so it has to be plumbed
to the worker pools. PLT doesn't need it otherwise.

Thoughts?

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:144: if (client_->IsPartialRasterEnabled() &&
tile && invalidation &&
can you move this virtual call outside of the loop?

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:343: if (client_->IsPartialRasterEnabled() &&
tile)
ditto

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
On 2015/10/08 14:08:22, jbroman wrote:
> On 2015/10/08 at 00:12:20, ericrk wrote:
> > Is there a reason we had a disable flag as well as an enable flag? Only
enable
> > was used. Is this just the style?
> 
> It wasn't necessary to forward it, no.

We dont use it in compositor_util either. Delete?

https://codereview.chromium.org/1381163002/diff/160001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/ui/compositor/composit...
ui/compositor/compositor.cc:143: // UI compositor always uses partial raster.
Use PERSISTENT_MAP memory buffers
aside: i guess we shouldn't do this if use_zero_copy is true

[jbroman, 2015-10-08 15:19:59]

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
On 2015/10/08 at 15:18:43, danakj_OOO_til_10-12 wrote:
> On 2015/10/08 14:08:22, jbroman wrote:
> > On 2015/10/08 at 00:12:20, ericrk wrote:
> > > Is there a reason we had a disable flag as well as an enable flag? Only
enable
> > > was used. Is this just the style?
> > 
> > It wasn't necessary to forward it, no.
> 
> We dont use it in compositor_util either. Delete?

Well, the disabling flag exists because at one point this feature was on and we
needed a flag to disable it. Right now it's off by default, so the disabling
flag is a no-op. :P

[danakj, 2015-10-08 15:21:28]

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
On 2015/10/08 15:19:59, jbroman wrote:
> On 2015/10/08 at 15:18:43, danakj_OOO_til_10-12 wrote:
> > On 2015/10/08 14:08:22, jbroman wrote:
> > > On 2015/10/08 at 00:12:20, ericrk wrote:
> > > > Is there a reason we had a disable flag as well as an enable flag? Only
> enable
> > > > was used. Is this just the style?
> > > 
> > > It wasn't necessary to forward it, no.
> > 
> > We dont use it in compositor_util either. Delete?
> 
> Well, the disabling flag exists because at one point this feature was on and
we
> needed a flag to disable it. Right now it's off by default, so the disabling
> flag is a no-op. :P

Ah, that's what happened. Usually you'd only have one flag and rename it when
changing it. Or check both but have one dominate the other in the is-enabled
code. Either way there's not an unused cmd line flag hanging around. :)

[ericrk, 2015-10-13 23:03:45]

Moved away from modifying PictureLayerTiling to avoid generating invalidated IDs
and instead block off the partial raster path in TileManager.

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:144: if (client_->IsPartialRasterEnabled() &&
tile && invalidation &&
this code has been removed.

https://codereview.chromium.org/1381163002/diff/160001/cc/tiles/picture_layer...
cc/tiles/picture_layer_tiling.cc:343: if (client_->IsPartialRasterEnabled() &&
tile)
On 2015/10/08 15:18:43, danakj_OOO_til_10-12 wrote:
> ditto

this code has been removed.

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisablePartialRaster,
On 2015/10/08 15:21:27, danakj_OOO_til_10-12 wrote:
> On 2015/10/08 15:19:59, jbroman wrote:
> > On 2015/10/08 at 15:18:43, danakj_OOO_til_10-12 wrote:
> > > On 2015/10/08 14:08:22, jbroman wrote:
> > > > On 2015/10/08 at 00:12:20, ericrk wrote:
> > > > > Is there a reason we had a disable flag as well as an enable flag?
Only
> > enable
> > > > > was used. Is this just the style?
> > > > 
> > > > It wasn't necessary to forward it, no.
> > > 
> > > We dont use it in compositor_util either. Delete?
> > 
> > Well, the disabling flag exists because at one point this feature was on and
> we
> > needed a flag to disable it. Right now it's off by default, so the disabling
> > flag is a no-op. :P
> 
> Ah, that's what happened. Usually you'd only have one flag and rename it when
> changing it. Or check both but have one dominate the other in the is-enabled
> code. Either way there's not an unused cmd line flag hanging around. :)

I'll remove this then.

https://codereview.chromium.org/1381163002/diff/160001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1381163002/diff/160001/ui/compositor/composit...
ui/compositor/compositor.cc:143: // UI compositor always uses partial raster.
Use PERSISTENT_MAP memory buffers
On 2015/10/08 15:18:43, danakj_OOO_til_10-12 wrote:
> aside: i guess we shouldn't do this if use_zero_copy is true

Added some more logic here to handle this case.

[danakj, 2015-10-16 18:13:08]

LGTM

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager.h
File cc/tiles/tile_manager.h (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager....
cc/tiles/tile_manager.h:294: bool use_partial_raster_;
const

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1737: // raster is disabled.
copypasta alert: comment does not match test

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1740: const int kLayerId = 7;
It would be nice to share code. It looks like you could put all the test code in
a static method that takes a LTHI* and a bool partial_raster?

https://codereview.chromium.org/1381163002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_tiles.cc:207:
PartialRaster_SingleThread_Software) {
Should we add non-partial versions of software and gpu tests? That might be the
simplest way to ensure we're not doing the wrong thing.

[ericrk, 2015-10-16 20:27:12]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[ericrk, 2015-10-16 20:27:13]

+piman for owners on remaining files.

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager.h
File cc/tiles/tile_manager.h (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager....
cc/tiles/tile_manager.h:294: bool use_partial_raster_;
On 2015/10/16 18:13:08, danakj wrote:
> const

Done.

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1737: // raster is disabled.
On 2015/10/16 18:13:08, danakj wrote:
> copypasta alert: comment does not match test

Done.

https://codereview.chromium.org/1381163002/diff/180001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1740: const int kLayerId = 7;
On 2015/10/16 18:13:08, danakj wrote:
> It would be nice to share code. It looks like you could put all the test code
in
> a static method that takes a LTHI* and a bool partial_raster?

yup, done - had to transfer ownership of host_impl_ to ensure proper cleanup
order, but otherwise pretty straightforward.

https://codereview.chromium.org/1381163002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1381163002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_pixeltest_tiles.cc:207:
PartialRaster_SingleThread_Software) {
On 2015/10/16 18:13:08, danakj wrote:
> Should we add non-partial versions of software and gpu tests? That might be
the
> simplest way to ensure we're not doing the wrong thing.

good suggestion - done

[ericrk, 2015-10-19 21:25:27]

Description was changed from

==========
Add a flag to disable partial raster in renderer

Partial raster was intended to be disabled in renderers, but it was
disabled indirectly through a GPUMemoryBuffer property, which didn't
apply to all rasterization modes / tile workers.

This change adds a flag "--enable-partial-raster" which will cause
partial raster to be enabled (current state) if passed. The default
behavior is now that we block partial raster at a higher level, but
not providing invalidated IDs to tiles.

BUG=537722
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add a flag to disable partial raster in renderer

Partial raster was intended to be disabled in renderers, but it was
disabled indirectly through a GPUMemoryBuffer property, which didn't
apply to all rasterization modes / tile workers.

This change adds a flag "--enable-partial-raster" which will cause
partial raster to be enabled (current state) if passed. The default
behavior is now that we block partial raster at a higher level, but
not providing invalidated IDs to tiles.

BUG=537722
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2015-10-19 21:25:27]

ericrk@chromium.org changed reviewers:
- piman@chromium.org

[ericrk, 2015-10-19 21:27:24]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-10-20 01:53:16]

lgtm

[ericrk, 2015-10-20 02:00:17]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-10-20 02:00:18]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1381163002/#ps190001
(title: "feedback + UT refactoring")

[commit-bot: I haz the power, 2015-10-20 02:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381163002/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381163002/190001

[commit-bot: I haz the power, 2015-10-20 03:03:15]

Committed patchset #6 (id:190001)

[commit-bot: I haz the power, 2015-10-20 03:04:26]

Patchset 6 (id:??) landed as
https://crrev.com/1d17f75189e66a12ef287af75e0d6adb5449db3d
Cr-Commit-Position: refs/heads/master@{#354966}