Still produce quad when tile OOM and on demand raster is not allowed

Still produce quad when tile OOM and on demand raster is not allowed

When on demand raster is not allowed, when tiles are OOM, checkerboard
or solid color quad should be produced rather than nothing.

Add a unittest for this exact same scenario.

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

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

[weiliangc, 2014-08-13 16:04:55]

overall: danakj

[danakj, 2014-08-13 16:06:03]

> or solid color quad should be produced rather than nother.

"nother"? :)

[danakj, 2014-08-13 16:26:42]

Thanks! Mostly trying to wrap my head around what the test needs to do and
doesn't

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

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3126:
host_impl_.CreateFakeTileManager();
FakeTileManager seems like it mostly exists to private access to private stuff
that you don't use in this test. You're not inserting tiles into the manager in
weird ways, and maybe you can use ManageTiles() below and not need to use
FakeTileManager?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3133:
host_impl_.SetViewportSize(gfx::Size(1000, 1000));
use layer_bounds instead of 1000,1000?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3135:
active_layer_->set_fixed_tile_size(tile_size);
why is this needed?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3147: 
random whitespace?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3148: state.hard_memory_limit_in_bytes
= state.soft_memory_limit_in_bytes * 2;
why * 2?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3157: ->AssignMemoryToTiles(state);
Would it work if you did ManageTiles() here instead of
InitializeTilesWithResourcesForTesting above and this call here? You've done
UpdateDrawProps so you've updated the tile priorities so would that work?

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3169:
EXPECT_NE(render_pass->quad_list.back()->material,
Can you verify explicit materials?

[weiliangc, 2014-08-13 17:45:15]

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

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3126:
host_impl_.CreateFakeTileManager();
On 2014/08/13 16:26:42, danakj wrote:
> FakeTileManager seems like it mostly exists to private access to private stuff
> that you don't use in this test. You're not inserting tiles into the manager
in
> weird ways, and maybe you can use ManageTiles() below and not need to use
> FakeTileManager?

Mostly I want to call that AssignMemoryToTiles function that access protected
part of TileManager, which is used in the similar test in tile_manager_unittest.

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3133:
host_impl_.SetViewportSize(gfx::Size(1000, 1000));
On 2014/08/13 16:26:42, danakj wrote:
> use layer_bounds instead of 1000,1000?

Done.

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3135:
active_layer_->set_fixed_tile_size(tile_size);
On 2014/08/13 16:26:42, danakj wrote:
> why is this needed?

So active layer takes tile size from pending tree, instead of using default tile
size from settings (I think).

No need to use it if I don't specify tile size, which I don't think I need to.

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3147: 
On 2014/08/13 16:26:42, danakj wrote:
> random whitespace?

Done.

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3148: state.hard_memory_limit_in_bytes
= state.soft_memory_limit_in_bytes * 2;
On 2014/08/13 16:26:42, danakj wrote:
> why * 2?

Magic number copied from tile_manager_unittest. Not needed will delete.

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3157: ->AssignMemoryToTiles(state);
On 2014/08/13 16:26:42, danakj wrote:
> Would it work if you did ManageTiles() here instead of
> InitializeTilesWithResourcesForTesting above and this call here? You've done
> UpdateDrawProps so you've updated the tile priorities so would that work?

ManageTiles produces invalid tiles, which won't hit code that is in the bug.

(Not sure how ManageTiles would does that though)

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3169:
EXPECT_NE(render_pass->quad_list.back()->material,
On 2014/08/13 16:26:42, danakj wrote:
> Can you verify explicit materials?

Done.

[weiliangc, 2014-08-13 20:01:23]

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

https://codereview.chromium.org/468983002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3157: ->AssignMemoryToTiles(state);
On 2014/08/13 17:45:15, weiliangc wrote:
> On 2014/08/13 16:26:42, danakj wrote:
> > Would it work if you did ManageTiles() here instead of
> > InitializeTilesWithResourcesForTesting above and this call here? You've done
> > UpdateDrawProps so you've updated the tile priorities so would that work?
> 
> ManageTiles produces invalid tiles, which won't hit code that is in the bug.
> 
> (Not sure how ManageTiles would does that though)

Aha!
So I'm wrong, I can use ManageTiles just need to call differently. Update coming
up :D

[weiliangc, 2014-08-13 20:15:17]

PTAL.
No more hacks with FakeTileManager.

[danakj, 2014-08-13 21:48:26]

Awesome job LGTM

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

https://codereview.chromium.org/468983002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:3118: MockOcclusionTracker<LayerImpl>
occlusion_tracker;
can you make this down closer to where it's used

https://codereview.chromium.org/468983002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:3138: // directly set state to
host_impl_, so we try set policy that would change
s/try//

https://codereview.chromium.org/468983002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:3152:
occlusion_tracker.set_occluded_target_rect(occluded);
do you need?

https://codereview.chromium.org/468983002/diff/60001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/468983002/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.h:127:
resource_pool_->AcquireResource(tiles[i]->tile_size_.size());
tile_size_ is a Rect... weird. theres tiles[i]->size() you could use also

[weiliangc, 2014-08-13 22:24:41]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:26:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/468983002/100001

[commit-bot: I haz the power, 2014-08-14 07:29:43]

Committed patchset #5 (100001) as 289480

[weiliangc, 2014-08-14 16:36:09]

PLAT.

Reverted because fail Linux ASAN.
Order of tiles in PrioritizedTileSet NOW_AND_READY_TO_DRAW_BIN is depended on
order of iteration over TileMap. Which leads to different tiles being in
PICTURE_PILE_MODE when memory is limited. Sort it by Id as tie breaker.

[danakj, 2014-08-14 16:38:10]

vmpstr/reveman PTAL at cc/resources/prioritized_tile_set.cc

[weiliangc, 2014-08-18 18:34:08]

On 2014/08/14 16:38:10, danakj wrote:
> vmpstr/reveman PTAL at cc/resources/prioritized_tile_set.cc

Friendly ping?

[vmpstr, 2014-08-18 18:53:06]

prioritized_tile_set lgtm. Out of curiousity, what is the change in there
fixing? Ie, why do we require a fixed order?

[weiliangc, 2014-08-18 19:02:56]

On 2014/08/18 18:53:06, vmpstr wrote:
> prioritized_tile_set lgtm. Out of curiousity, what is the change in there
> fixing? Ie, why do we require a fixed order?

So to test my fix for original bug, I wrote a unittest that force OOM on tiles,
and check that the first quad generated is one with resource, and last quad
generated is without resource.

This failed on Linux ASAN build, where the last quad generated is the one with
resource, and first one is without.

Digging back, the order to decide to keep resources for which tile is depended
on how PrioritizedTileSet is iterated through, where NOW_AND_READY_TO_DRAW bin
is not sorted. Its order is depended on how tiles are added in, which is by
iterating through a TileMap. Hashmap iteration order is undefined and different
on Linux ASAN build.

[weiliangc, 2014-08-20 17:04:41]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-08-20 17:06:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/468983002/160001

[commit-bot: I haz the power, 2014-08-20 18:43:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 19:16:46]

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

[commit-bot: I haz the power, 2014-08-20 19:16:47]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2014-08-21 15:56:01]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-08-21 15:57:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/468983002/160001

[commit-bot: I haz the power, 2014-08-21 16:55:57]

Committed patchset #8 (160001) as 291101