cc: refcount resources as we send them to the parent

cc: refcount resources as we send them to the parent

This allows us to fully specify the resources used by the frame, so that we can:
- allow more than one frame in flight
- allow duplicating the frame for aura::Window::RecreateLayers

For every frame we generate, we increase the "exported_count" in the child.
Every time we import the resources for a frame, we increase the "imported count"
in the parent. The contract is that we return resources exactly once for every
time it's exported.

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

[piman, 2013-08-14 00:16:23]

Note: right now I'm returning resources multiple times in the array, which
doesn't really scale - I have a follow up in
https://codereview.chromium.org/23097005 to make it cleaner (I think), but I
wanted to separate the semantic changes from the syntactic ones.

Many tests were testing some of the logic that was needed before (reusing
recycled textures), and I was torn between removing them or not. Instead I tried
to extend them and test the new logic. Let me know what you think.

[danakj, 2013-08-15 21:04:32]

Some cc tests are failing still it seems, or are flaky (see win_rel).

[piman, 2013-08-15 21:22:38]

On Thu, Aug 15, 2013 at 2:04 PM, <danakj@chromium.org> wrote:

> Some cc tests are failing still it seems, or are flaky (see win_rel).
>

Ah, yes, I need to make the test resilient to the order in the resource
list.


>
>
https://codereview.chromium.**org/23023005/<https://codereview.chromium.org/2...
>

[danakj, 2013-08-15 21:30:52]

Some comments on the non-test changes.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:195: return !!resource->lock_for_read_count ||
!!resource->exported_count;
nit: i like x>0 a lot more than !!x for integers

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:314: DCHECK(!resource->marked_for_deletion);
Can we DCHECK(!imported_count)? Is it okay to delete a resource if we have a
copy from a child?

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:841: for (unsigned i = 0; i <
resources_[*it].imported_count; ++i)
int

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:842: list->push_back(resource);
You'd need to --imported_count to DCHECK in the DeleteResource() call. This loop
could instead be while (imported_count > 0) {}

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:870:
resources_[child_info.child_to_parent_map[it->id]].imported_count++;
We just did a find() on line 868. Save the iterator returned there and use it
here instead of operator[]?

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:913: DCHECK(!!resource->exported_count);
DCHECK_GT?

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provider.h
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:357: unsigned imported_count;
use int here.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types

[danakj, 2013-08-15 21:32:01]

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:314: DCHECK(!resource->marked_for_deletion);
On 2013/08/15 21:30:53, danakj wrote:
> Can we DCHECK(!imported_count)? Is it okay to delete a resource if we have a
> copy from a child?

Oops, bad reviewer. I mean DCHECK_EQ(0, imported_count).

[danakj, 2013-08-15 22:20:18]

Thanks for updating the tests! I think that will be useful/helpful.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:515: // Both frames' resources
should be in the parent's resource provider.
Comment's wrong now, but why'd you choose to remove 999 in the second frame? Why
not just send both resources? I think this test can be useful testing that
sending a resource in two consecutive frames it doesn't get lost (kinda what it
was testing before)?

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:926: EXPECT_EQ(2u,
resources.size());
Verify which resources are in the array.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1248: frame =
CreateFrameData(gfx::Rect(0, 0, 1, 1), gfx::Rect(0, 0, 1, 1));
// Send a frame with no resources in it.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1254: // TODO(danakj): The test
should work without this.
I see what you did there :)

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1258: resources.clear();
// The unused resource 555 should be returned.

[piman, 2013-08-16 04:29:54]

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:195: return !!resource->lock_for_read_count ||
!!resource->exported_count;
On 2013/08/15 21:30:53, danakj wrote:
> nit: i like x>0 a lot more than !!x for integers

Done.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:314: DCHECK(!resource->marked_for_deletion);
On 2013/08/15 21:32:01, danakj wrote:
> On 2013/08/15 21:30:53, danakj wrote:
> > Can we DCHECK(!imported_count)? Is it okay to delete a resource if we have a
> > copy from a child?
> 
> Oops, bad reviewer. I mean DCHECK_EQ(0, imported_count).

DCHECK_EQ(imported_count, 0).
DCHECK doesn't differentiate between the left and right, as EXPECT does.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:841: for (unsigned i = 0; i <
resources_[*it].imported_count; ++i)
On 2013/08/15 21:30:53, danakj wrote:
> int

Done.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:842: list->push_back(resource);
On 2013/08/15 21:30:53, danakj wrote:
> You'd need to --imported_count to DCHECK in the DeleteResource() call. This
loop
> could instead be while (imported_count > 0) {}

Done.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:870:
resources_[child_info.child_to_parent_map[it->id]].imported_count++;
On 2013/08/15 21:30:53, danakj wrote:
> We just did a find() on line 868. Save the iterator returned there and use it
> here instead of operator[]?

Done.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.cc:913: DCHECK(!!resource->exported_count);
On 2013/08/15 21:30:53, danakj wrote:
> DCHECK_GT?

Done.

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provider.h
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/23023005/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:357: unsigned imported_count;
On 2013/08/15 21:30:53, danakj wrote:
> use int here.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types

Disagree, but done.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:515: // Both frames' resources
should be in the parent's resource provider.
On 2013/08/15 22:20:19, danakj wrote:
> Comment's wrong now, but why'd you choose to remove 999 in the second frame?
Why
> not just send both resources? I think this test can be useful testing that
> sending a resource in two consecutive frames it doesn't get lost (kinda what
it
> was testing before)?

TBH I don't remember (this CL has been pending for a few weeks now). I put it
back. I think it was that we don't merge the resources anymore so this test
didn't make sense anymore and I wanted to repurpose.... But whatever was tested
here is tested below too in LayerTreeHostDelegatedTestDontLeakResource

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:926: EXPECT_EQ(2u,
resources.size());
On 2013/08/15 22:20:19, danakj wrote:
> Verify which resources are in the array.

Done.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1248: frame =
CreateFrameData(gfx::Rect(0, 0, 1, 1), gfx::Rect(0, 0, 1, 1));
On 2013/08/15 22:20:19, danakj wrote:
> // Send a frame with no resources in it.

Done.

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1254: // TODO(danakj): The test
should work without this.
On 2013/08/15 22:20:19, danakj wrote:
> I see what you did there :)

copy & paste :)

https://codereview.chromium.org/23023005/diff/1/cc/trees/layer_tree_host_unit...
cc/trees/layer_tree_host_unittest_delegated.cc:1258: resources.clear();
On 2013/08/15 22:20:19, danakj wrote:
> // The unused resource 555 should be returned.

Done.

[danakj, 2013-08-16 16:49:44]

Thanks, LGTM

https://codereview.chromium.org/23023005/diff/25001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/23023005/diff/25001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_delegated.cc:530: TransferableResourceArray
returned_resources;
// The resource 999 from frame1 is returned since it is still on the main
thread.

[commit-bot: I haz the power, 2013-08-16 20:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/23023005/35001

[piman, 2013-08-16 20:43:18]

https://codereview.chromium.org/23023005/diff/25001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/23023005/diff/25001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_delegated.cc:530: TransferableResourceArray
returned_resources;
On 2013/08/16 16:49:44, danakj wrote:
> // The resource 999 from frame1 is returned since it is still on the main
> thread.

Done.

[commit-bot: I haz the power, 2013-08-17 13:53:13]

Change committed as 218173