Reland (2): cc: Make tile manager object persist for the length of LTHI.

Reland (2): cc: Make tile manager object persist for the length of LTHI.

This patch makes TileManager be persistent throughout LTHI instead of
deleting and recreating it. This makes it possible to store items
on it that don't have to go away (such as PictureLayerTilingSets for
example).

R=enne
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/79191844c9fed08df7a274d8935e1a0fd9c57e53
Cr-Commit-Position: refs/heads/master@{#338348}

[vmpstr, 2015-07-01 22:11:19]

Please take a look. I'm not 100% on all of the function names, so if you have
better suggestions I'd appreciate it.

[enne (OOO), 2015-07-06 18:07:26]

Looks good in general.

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:260: }
Should this call FinishTasksAndCleanUp? Or at least DCHECK that it's been done?

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

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.h#new...
cc/tiles/tile_manager.h:124: // Set the new given resource pool and tile task
runner.
Should you leave a comment here that you need to call FinishTasksAndCleanUp
before calling this a second time? Or should that happen automatically in
SetResources rather than DCHECKing?

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:340: if (!did_prepare_tiles) {
Would it be cleaner to just have PrepareTiles always signal rather than make the
caller sometimes handle it?

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2057: void
LayerTreeHostImpl::RecreateTileManagerResources() {
bikeshedding: This seems less Recreate and more just Create.  I guess I expect a
"recreate" to also tear down vs "create" is just create?

[vmpstr, 2015-07-06 18:41:36]

PTAL

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:260: }
On 2015/07/06 18:07:25, enne wrote:
> Should this call FinishTasksAndCleanUp? Or at least DCHECK that it's been
done?

Done.

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

https://codereview.chromium.org/1210073020/diff/1/cc/tiles/tile_manager.h#new...
cc/tiles/tile_manager.h:124: // Set the new given resource pool and tile task
runner.
On 2015/07/06 18:07:25, enne wrote:
> Should you leave a comment here that you need to call FinishTasksAndCleanUp
> before calling this a second time? Or should that happen automatically in
> SetResources rather than DCHECKing?

In order to ensure that the caller is aware that FinishTasksAndCleanUp is
happening, I'd prefer to do the comment route instead of an implicit call to it.
Added the comment.

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:340: if (!did_prepare_tiles) {
On 2015/07/06 18:07:26, enne wrote:
> Would it be cleaner to just have PrepareTiles always signal rather than make
the
> caller sometimes handle it?

Maybe. I've left a TODO here, because I think it's a bit more involved on the
tile manager side... Also, some uses of PrepareTiles in LTHI I think would not
expect activation if tiles aren't dirty or something like that.

https://codereview.chromium.org/1210073020/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2057: void
LayerTreeHostImpl::RecreateTileManagerResources() {
On 2015/07/06 18:07:26, enne wrote:
> bikeshedding: This seems less Recreate and more just Create.  I guess I expect
a
> "recreate" to also tear down vs "create" is just create?

Done.

[enne (OOO), 2015-07-06 19:47:51]

lgtm

[vmpstr, 2015-07-06 20:05:19]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-07-06 20:06:47]

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

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

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[vmpstr, 2015-07-07 19:48:57]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-07-07 19:49:06]

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

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

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

[commit-bot: I haz the power, 2015-07-07 23:54:24]

Exceeded global retry quota

[vmpstr, 2015-07-08 17:51:47]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-07-08 17:53:54]

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

[commit-bot: I haz the power, 2015-07-08 18:04:35]

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

[commit-bot: I haz the power, 2015-07-08 18:04:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vmpstr, 2015-07-08 21:19:18]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-07-08 21:19:19]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1210073020/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-07-08 21:19:48]

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

[commit-bot: I haz the power, 2015-07-08 22:21:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-08 22:22:12]

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

[esprehn, 2015-07-08 23:28:47]

This appears to be causing the memory builders to fail:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...

[vmpstr, 2015-07-08 23:52:04]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1227183002/ by vmpstr@chromium.org.

The reason for reverting is: Causing memory issues:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v...
.

[vmpstr, 2015-07-09 22:54:08]

Ok, I'm going to try landing this again. I couldn't repro the memory issues with
the original patch, but I did fix some webkit tests that happened as a result.
The stack trace in the memory failure points to the same variable as was the
cause of the webkit failures, so I'm reasonably confident that it's fixed.

[vmpstr, 2015-07-09 22:54:36]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-07-09 22:54:36]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1210073020/#ps80001
(title: "update")

[commit-bot: I haz the power, 2015-07-09 22:56:26]

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

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-09 23:05:27]

Patchset 5 (id:??) landed as
https://crrev.com/e1437e24549275dcafbb8a7646ba22cf55408c84
Cr-Commit-Position: refs/heads/master@{#338179}

[zhaoqin1, 2015-07-10 03:46:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1229223003/ by zhaoqin@chromium.org.

The reason for reverting is: This appears to be causing the memory builders to
fail:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests...
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20M...

BUG=508792.

[vmpstr, 2015-07-10 20:34:21]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-07-10 20:34:21]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1210073020/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-07-10 20:34:58]

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

[commit-bot: I haz the power, 2015-07-10 20:40:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-10 20:42:55]

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