cc: Refactor how picture layers are exposed to the tile manager.

cc: Refactor how picture layers are exposed to the tile manager.

This moves the vector of registered picture layers from
the tile manager to LTHI. Allows layers to be registered
in ctor and unregistered in dtor instead of having to
unregister them from the tile manager and then re-register
them as needed.

The picture layer vector is exposed to the tile manager
using a client function and this vector will include all
existing PictureLayerImpl instances. Whether it makes
sense for the tile manager to use a layer or not depends
on the return value of
PictureLayerImpl::HasValidTilePriorities().

BUG=

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

[reveman, 2014-06-05 22:15:18]

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

https://codereview.chromium.org/322443002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1383: bool
PictureLayerImpl::IsViableSourceOfRasterAndEvictionTiles() const {
What else is needed here? Does UpdateTilePriorities just need to be called once
before this can return true? Or do we need to check if the current version of
tile priorities is a result of the last call to update draw properties and in
case it's not, return false?

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:1200: void TileManager::GetPairedPictureLayers(
Would it be easier and more efficient to maintain this vector in the client
instead of having to rebuild a vector here every time?

[vmpstr, 2014-06-05 22:38:02]

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

https://codereview.chromium.org/322443002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1383: bool
PictureLayerImpl::IsViableSourceOfRasterAndEvictionTiles() const {
On 2014/06/05 22:15:18, reveman wrote:
> What else is needed here? Does UpdateTilePriorities just need to be called
once
> before this can return true? Or do we need to check if the current version of
> tile priorities is a result of the last call to update draw properties and in
> case it's not, return false?

IsDrawnRenderSurfaceLayerListMember being true implies priorities were updated.

You also have to make sure to remove all tiles when the layer is no longer a
part of the render surface layer list, since it still might have tiles with
resources.

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:1200: void TileManager::GetPairedPictureLayers(
On 2014/06/05 22:15:18, reveman wrote:
> Would it be easier and more efficient to maintain this vector in the client
> instead of having to rebuild a vector here every time?

You would have to update it every time there's a switch between creating a
pending tree and when it becomes recycled.

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:1613: if
(!(*it)->AllTilesRequiredForActivationAreReadyToDraw())
I'm not sure you want to check all of the layers here. If checking a layer with
out of date tile priorities is fine, then this is fine.

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager_un...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager_un...
cc/resources/tile_manager_unittest.cc:800: picture_layers_.clear();
Do you need this?

https://codereview.chromium.org/322443002/diff/1/cc/test/fake_tile_manager_cl...
File cc/test/fake_tile_manager_client.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/test/fake_tile_manager_cl...
cc/test/fake_tile_manager_client.cc:12:
base::LazyInstance<std::vector<PictureLayerImpl*> > g_picture_layers;
Why is this not a member? You need to include base/lazy_instance.h

[reveman, 2014-06-06 17:27:24]

PTAL

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

https://codereview.chromium.org/322443002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1383: bool
PictureLayerImpl::IsViableSourceOfRasterAndEvictionTiles() const {
On 2014/06/05 22:38:01, vmpstr wrote:
> On 2014/06/05 22:15:18, reveman wrote:
> > What else is needed here? Does UpdateTilePriorities just need to be called
> once
> > before this can return true? Or do we need to check if the current version
of
> > tile priorities is a result of the last call to update draw properties and
in
> > case it's not, return false?
> 
> IsDrawnRenderSurfaceLayerListMember being true implies priorities were
updated.
> 
> You also have to make sure to remove all tiles when the layer is no longer a
> part of the render surface layer list, since it still might have tiles with
> resources.

We'll handle that as discussed. Renamed this function to HasValidTilePriorities
in latest patch.

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:1613: if
(!(*it)->AllTilesRequiredForActivationAreReadyToDraw())
On 2014/06/05 22:38:01, vmpstr wrote:
> I'm not sure you want to check all of the layers here. If checking a layer
with
> out of date tile priorities is fine, then this is fine.

Updated AllTilesRequiredForActivationAreReadyToDraw.

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager_un...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/resources/tile_manager_un...
cc/resources/tile_manager_unittest.cc:800: picture_layers_.clear();
On 2014/06/05 22:38:02, vmpstr wrote:
> Do you need this?

Nope. Done.

https://codereview.chromium.org/322443002/diff/1/cc/test/fake_tile_manager_cl...
File cc/test/fake_tile_manager_client.cc (right):

https://codereview.chromium.org/322443002/diff/1/cc/test/fake_tile_manager_cl...
cc/test/fake_tile_manager_client.cc:12:
base::LazyInstance<std::vector<PictureLayerImpl*> > g_picture_layers;
On 2014/06/05 22:38:02, vmpstr wrote:
> Why is this not a member?

Either is fine. I just put it here because I like hiding things in the .cc file
when possible. Changed to a member in latest patch.

[vmpstr, 2014-06-11 18:49:46]

Let me know if my comments make sense. I think we should land this instead of my
patch. Could you put in a description as well when you get a chance?

[vmpstr, 2014-06-11 19:00:35]

Disregard my last comment. lgtm after you put in a description. Thanks

https://codereview.chromium.org/322443002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/322443002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1396: if (!layer_tree_impl()->IsPendingTree())
As an optimization, it might still be worth to keep this one and && it with the
other condition

[reveman, 2014-06-11 20:09:00]

Added a description.

https://codereview.chromium.org/322443002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/322443002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1396: if (!layer_tree_impl()->IsPendingTree())
On 2014/06/11 19:00:35, vmpstr wrote:
> As an optimization, it might still be worth to keep this one and && it with
the
> other condition

Done.

[reveman, 2014-06-11 20:09:11]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:12:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/322443002/40001

[commit-bot: I haz the power, 2014-06-11 21:00:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15606)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18743)

[commit-bot: I haz the power, 2014-06-11 21:04:26]

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

[commit-bot: I haz the power, 2014-06-11 21:04:27]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15635)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18772)

[reveman, 2014-06-11 21:19:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:22:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/322443002/60001

[commit-bot: I haz the power, 2014-06-11 22:42:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15657)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18792)

[commit-bot: I haz the power, 2014-06-11 22:43:52]

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

[commit-bot: I haz the power, 2014-06-11 22:43:53]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15735)

[reveman, 2014-06-12 03:18:29]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:21:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/322443002/60001

[commit-bot: I haz the power, 2014-06-12 14:19:20]

Change committed as 276665