cc: Remove DrawQuad::IterateResoruces

cc: Remove DrawQuad::IterateResoruces

This patch removes IterateResources function in favor of
iterating resources directly on the quad. In order to
accomplish this, each derived quad uses new resources
object on the base class to store all of the resources
it needs. This allows the calling code that used to call
IterateResources with a callback to instead directly
iterate all of the ids and manipulate them in any way
that is required.

This improves the performance of the IterateResources
test by ~30%.

BUG=492765
R=danakj, piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0eca2e87110112c678c5f4c89b7f99e2de6026bb
Cr-Commit-Position: refs/heads/master@{#332476}

[vmpstr, 2015-05-28 16:49:58]

Hi, please take a look. I still have to write a perftest, but I would appreciate
an early review.

[danakj, 2015-05-28 17:10:06]

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:138: for (const auto& quad :
pass->quad_list)
{}

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:139: quad->IterateResources(
Does this look nicer/worse with just walking thru an array? It seems like it'd
be simpler to me, did you try it?

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:142: return
ResourceRemapHelper(&invalid_frame, resource_map,
I *think* I'd just put the body of this helper here.. unless that's difficult to
read. I'm new to lambdas.

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (left):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:251: #if DCHECK_IS_ON()
The idea here is that the function is a no-op in the header so compiles away
easily when DCHECKS are off.

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
File cc/layers/tiled_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
cc/layers/tiled_layer_impl_unittest.cc:272: EXPECT_NE(0u, quad->resource_id())
<< LayerTestCommon::quad_string
You could check resources.count == 1 and resources[0] != 0 ? Then you don't need
to add things to the subclasses except index constants for quads with > 1
resource?

[piman, 2015-05-28 18:37:59]

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:139: quad->IterateResources(
On 2015/05/28 17:10:05, danakj wrote:
> Does this look nicer/worse with just walking thru an array? It seems like it'd
> be simpler to me, did you try it?

Actually... I think we may be able to do something nice if we just add a begin()
and end() to DrawQuad::Resources, then I think we can do something like
for (ResourceId& resource_id : quad->resources()) {
  resource_id = ...
}

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:194: &referenced_resources, resource_id);
nit: I would inline ValidateResourceHelper here. It was only really needed for
Bind.

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:430: return
ResourceRemapHelper(child_to_parent_map, resource_id);
nit: same here, I would probably inline ResourceRemapHelper too

[vmpstr, 2015-05-28 18:38:45]

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:138: for (const auto& quad :
pass->quad_list)
On 2015/05/28 17:10:05, danakj wrote:
> {}

Done.

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:139: quad->IterateResources(
On 2015/05/28 17:10:05, danakj wrote:
> Does this look nicer/worse with just walking thru an array? It seems like it'd
> be simpler to me, did you try it?

Yep, that does look better. In the current version there's no
IterateResources...

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:142: return
ResourceRemapHelper(&invalid_frame, resource_map,
On 2015/05/28 17:10:05, danakj wrote:
> I *think* I'd just put the body of this helper here.. unless that's difficult
to
> read. I'm new to lambdas.

Yeah, I could, but I prefer keeping lambdas very small... otherwise you're just
reading another function inside a function, which is kinda weird...

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (left):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:251: #if DCHECK_IS_ON()
On 2015/05/28 17:10:05, danakj wrote:
> The idea here is that the function is a no-op in the header so compiles away
> easily when DCHECKS are off.

OK. I'm not too sure how much of an impact this makes, but I'd rather leave this
as it was then :) (I undid my changes)

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
File cc/layers/tiled_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
cc/layers/tiled_layer_impl_unittest.cc:272: EXPECT_NE(0u, quad->resource_id())
<< LayerTestCommon::quad_string
On 2015/05/28 17:10:05, danakj wrote:
> You could check resources.count == 1 and resources[0] != 0 ? Then you don't
need
> to add things to the subclasses except index constants for quads with > 1
> resource?

I kind of prefer having this type of accessor... I feel like it's more burden on
the caller(s) to do the count/resources[0] check.. since it's really up to the
quad where the resource is stored. Wdyt?

[danakj, 2015-05-28 19:02:29]

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
File cc/layers/tiled_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
cc/layers/tiled_layer_impl_unittest.cc:272: EXPECT_NE(0u, quad->resource_id())
<< LayerTestCommon::quad_string
On 2015/05/28 18:38:45, vmpstr wrote:
> On 2015/05/28 17:10:05, danakj wrote:
> > You could check resources.count == 1 and resources[0] != 0 ? Then you don't
> need
> > to add things to the subclasses except index constants for quads with > 1
> > resource?
> 
> I kind of prefer having this type of accessor... I feel like it's more burden
on
> the caller(s) to do the count/resources[0] check.. since it's really up to the
> quad where the resource is stored. Wdyt?

What if each DrawQuad subclass had a public kNumResources?

[vmpstr, 2015-05-28 19:18:51]

PTAL (with range based for-loops)

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
File cc/layers/tiled_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/layers/tiled_layer_i...
cc/layers/tiled_layer_impl_unittest.cc:272: EXPECT_NE(0u, quad->resource_id())
<< LayerTestCommon::quad_string
On 2015/05/28 19:02:29, danakj wrote:
> On 2015/05/28 18:38:45, vmpstr wrote:
> > On 2015/05/28 17:10:05, danakj wrote:
> > > You could check resources.count == 1 and resources[0] != 0 ? Then you
don't
> > need
> > > to add things to the subclasses except index constants for quads with > 1
> > > resource?
> > 
> > I kind of prefer having this type of accessor... I feel like it's more
burden
> on
> > the caller(s) to do the count/resources[0] check.. since it's really up to
the
> > quad where the resource is stored. Wdyt?
> 
> What if each DrawQuad subclass had a public kNumResources?

It's not really a constant (it's determined when SetAll/SetNew is called
sometimes).. I mean that's what resources.count is meant to represent.

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:194: &referenced_resources, resource_id);
On 2015/05/28 18:37:59, piman (Very slow to review) wrote:
> nit: I would inline ValidateResourceHelper here. It was only really needed for
> Bind.

Done.

https://codereview.chromium.org/1152473006/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:430: return
ResourceRemapHelper(child_to_parent_map, resource_id);
On 2015/05/28 18:37:59, piman (Very slow to review) wrote:
> nit: same here, I would probably inline ResourceRemapHelper too

Done.

[piman, 2015-05-28 19:35:47]

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:127: continue;
You can break here actually.

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:772: for (const ResourceId& resource_id :
quad->resources)
nit: ResourceId instead of const ResourceId&

https://codereview.chromium.org/1152473006/diff/60001/cc/output/delegating_re...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/output/delegating_re...
cc/output/delegating_renderer.cc:88: for (const ResourceId& resource_id :
quad->resources)
nit: ResourceId instead of const ResourceId&

https://codereview.chromium.org/1152473006/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:492: for (const ResourceId& resource_id :
quad->resources)
nit: ResourceId instead of const ResourceId&

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h
File cc/quads/draw_quad.h (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:159: };
I don't think you need this - you should be able to just do
ResourceId* begin() { return ids; }
ResourceId* end() { return ids+count; }

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:175: for (const ResourceId& resource_id :
quad->resources) {
nit: ResourceId instead of const ResourceId&

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:180: continue;
nit: break

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:416: for (size_t i = 0; i <
dest_quad->resources.count; ++i) {
You can use range for loop here too:
for (ResourceId& resource_id : quad->resources) {
  resource_id = ResourceRemapHelper(child_to_parent_map, resource_id);
}

https://codereview.chromium.org/1152473006/diff/60001/cc/test/fake_delegated_...
File cc/test/fake_delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/test/fake_delegated_...
cc/test/fake_delegated_renderer_layer_impl.cc:48: for (const ResourceId&
resource_id : quad->resources) {
nit: ResourceId instead of const ResourceId&

[danakj, 2015-05-28 20:57:37]

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h
File cc/quads/draw_quad.h (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:137: return index_ == other.index_ && resources_ ==
other.resources_;
I know it's safer but you could drop the resources == check here probly.
Iterators from different quads don't need to be comparable?

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:159: };
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> I don't think you need this - you should be able to just do
> ResourceId* begin() { return ids; }
> ResourceId* end() { return ids+count; }

yaa

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:166: bool IsCountValid() const { return count <=
kMaxResourceIdCount; }
not sure why this is here?

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
File cc/quads/draw_quad_unittest.cc (left):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
cc/quads/draw_quad_unittest.cc:813: EXPECT_EQ(0, IterateAndCount(quad_new));
Oh hm. This made it so we didn't iterate over 0 id resources right? A 0 would
make a frame be "invalid" if we iterate over it. So I think we need iterator++
to be smarter than that. And this test should still exist.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
cc/quads/draw_quad_unittest.cc:728: ResourceId IncrementResourceId(ResourceId
id) {
unused now

[vmpstr, 2015-05-28 22:37:00]

PTAL

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_impl.cc:127: continue;
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> You can break here actually.

Done. Should resource_id be set to 0 here? It's inconsistent with
cc/surfaces/surface_aggregator.cc and it makes more sense to leave the id
untouched to me, but I'm a bit unsure if that value being reset to 0 is
important here.

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:772: for (const ResourceId& resource_id :
quad->resources)
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: ResourceId instead of const ResourceId&

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/output/delegating_re...
File cc/output/delegating_renderer.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/output/delegating_re...
cc/output/delegating_renderer.cc:88: for (const ResourceId& resource_id :
quad->resources)
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: ResourceId instead of const ResourceId&

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:492: for (const ResourceId& resource_id :
quad->resources)
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: ResourceId instead of const ResourceId&

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h
File cc/quads/draw_quad.h (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:137: return index_ == other.index_ && resources_ ==
other.resources_;
On 2015/05/28 20:57:37, danakj wrote:
> I know it's safer but you could drop the resources == check here probly.
> Iterators from different quads don't need to be comparable?

Yeah I had that initially... Now I don't have the Iterator class at all.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:159: };
On 2015/05/28 20:57:37, danakj wrote:
> On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> > I don't think you need this - you should be able to just do
> > ResourceId* begin() { return ids; }
> > ResourceId* end() { return ids+count; }
> 
> yaa

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:166: bool IsCountValid() const { return count <=
kMaxResourceIdCount; }
On 2015/05/28 20:57:37, danakj wrote:
> not sure why this is here?

This is used in cc_messages.cc. Initially I had DCHECK_LE(count,
cc::DrawQuad::Resources::kMaxResourceIdCount) there, but the linker complains
that it can't find cc::DrawQuad::Resources::kMaxResourceIdCount when it's
declared as static const. Alternatively I had declared kMaxResourceIdCount as an
enum and that worked, but it's inconsistent with enum naming.

If you know how I can make the linker happy with using the static const in
cc_messages.cc, then I can change it to that.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
File cc/quads/draw_quad_unittest.cc (left):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad_unit...
cc/quads/draw_quad_unittest.cc:813: EXPECT_EQ(0, IterateAndCount(quad_new));
On 2015/05/28 20:57:37, danakj wrote:
> Oh hm. This made it so we didn't iterate over 0 id resources right? A 0 would
> make a frame be "invalid" if we iterate over it. So I think we need iterator++
> to be smarter than that. And this test should still exist.

This is handled differently now in that when mask_resource_id is 0, the resource
count is 0. I readded a version of the test, the only thing that I can't do here
is do quad_new->mask_resource_id = 0, since the only way this is set is through
SetAll/SetNew.

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:175: for (const ResourceId& resource_id :
quad->resources) {
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: ResourceId instead of const ResourceId&

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:180: continue;
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: break

Done.

https://codereview.chromium.org/1152473006/diff/60001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:416: for (size_t i = 0; i <
dest_quad->resources.count; ++i) {
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> You can use range for loop here too:
> for (ResourceId& resource_id : quad->resources) {
>   resource_id = ResourceRemapHelper(child_to_parent_map, resource_id);
> }

Oops, missed this one. Thanks

https://codereview.chromium.org/1152473006/diff/60001/cc/test/fake_delegated_...
File cc/test/fake_delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/test/fake_delegated_...
cc/test/fake_delegated_renderer_layer_impl.cc:48: for (const ResourceId&
resource_id : quad->resources) {
On 2015/05/28 19:35:46, piman (Very slow to review) wrote:
> nit: ResourceId instead of const ResourceId&

Done.

[danakj, 2015-05-28 23:00:15]

LGTM

A perf test would be good for your perf though.

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h
File cc/quads/draw_quad.h (right):

https://codereview.chromium.org/1152473006/diff/60001/cc/quads/draw_quad.h#ne...
cc/quads/draw_quad.h:166: bool IsCountValid() const { return count <=
kMaxResourceIdCount; }
On 2015/05/28 22:36:59, vmpstr wrote:
> On 2015/05/28 20:57:37, danakj wrote:
> > not sure why this is here?
> 
> This is used in cc_messages.cc. Initially I had DCHECK_LE(count,
> cc::DrawQuad::Resources::kMaxResourceIdCount) there, but the linker complains
> that it can't find cc::DrawQuad::Resources::kMaxResourceIdCount when it's
> declared as static const. Alternatively I had declared kMaxResourceIdCount as
an
> enum and that worked, but it's inconsistent with enum naming.
> 
> If you know how I can make the linker happy with using the static const in
> cc_messages.cc, then I can change it to that.

FWIW I'm fine with an enum with a single value that's a constant named kFoo.

https://codereview.chromium.org/1152473006/diff/100001/cc/layers/delegated_re...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/layers/delegated_re...
cc/layers/delegated_renderer_layer_impl.cc:125: resource_id = 0;
You don't have to set it to 0, we're going to drop the frame and not use its
resources. Unless there's a unit test that makes me a liar.

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/draw_quad_uni...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/draw_quad_uni...
cc/quads/draw_quad_unittest.cc:810: ResourceId new_mask_resource_id = 0;
Well done.

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/render_pass_d...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/render_pass_d...
cc/quads/render_pass_draw_quad.cc:70: resources.count =
resources.ids[kMaskResourceIdIndex] ? 1 : 0;
nit: or use mask_resource_id again

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/yuv_video_dra...
File cc/quads/yuv_video_draw_quad.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/yuv_video_dra...
cc/quads/yuv_video_draw_quad.cc:42: resources.count =
resources.ids[kAPlaneResourceIdIndex] ? 4 : 3;
nitto

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:230: provider_ ?
provider_->GetChildToParentMap(ChildIdForSurface(surface))
can you // TODO provider check is a hack for unit tests like we have in
VerifyResources() ?

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:432: provider_ ?
provider_->GetChildToParentMap(ChildIdForSurface(surface))
todo-o

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:433: : ResourceProvider::ResourceIdMap();
I'm kinda surprised this doesn't cause the tests to DCHECK on l407 now. Nothing
would be in the map instead of just not checking anything. Maybe provider_ isn't
null in unit tests anymroe?

https://codereview.chromium.org/1152473006/diff/100001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/content/common/cc_mess...
content/common/cc_messages.cc:816: size_t to_reserve = sizeof(p.count) + p.count
* sizeof(p.ids[0]);
space for this is already reserved in ReserveSizeForRenderPassWrite no? the size
of the ids etc is all part of sizeof the largest draw quad?

[vmpstr, 2015-05-28 23:35:52]

I think I'll write a perftest as a separate patch to land before this one, since
we want to compare it to the IterateResources way of doing things.

https://codereview.chromium.org/1152473006/diff/100001/cc/layers/delegated_re...
File cc/layers/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/layers/delegated_re...
cc/layers/delegated_renderer_layer_impl.cc:125: resource_id = 0;
On 2015/05/28 23:00:15, danakj wrote:
> You don't have to set it to 0, we're going to drop the frame and not use its
> resources. Unless there's a unit test that makes me a liar.

Done.

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/draw_quad_uni...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/draw_quad_uni...
cc/quads/draw_quad_unittest.cc:810: ResourceId new_mask_resource_id = 0;
On 2015/05/28 23:00:15, danakj wrote:
> Well done.

Acknowledged.

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/render_pass_d...
File cc/quads/render_pass_draw_quad.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/render_pass_d...
cc/quads/render_pass_draw_quad.cc:70: resources.count =
resources.ids[kMaskResourceIdIndex] ? 1 : 0;
On 2015/05/28 23:00:15, danakj wrote:
> nit: or use mask_resource_id again

Done.

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/yuv_video_dra...
File cc/quads/yuv_video_draw_quad.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/quads/yuv_video_dra...
cc/quads/yuv_video_draw_quad.cc:42: resources.count =
resources.ids[kAPlaneResourceIdIndex] ? 4 : 3;
On 2015/05/28 23:00:15, danakj wrote:
> nitto

Done.

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:230: provider_ ?
provider_->GetChildToParentMap(ChildIdForSurface(surface))
On 2015/05/28 23:00:15, danakj wrote:
> can you // TODO provider check is a hack for unit tests like we have in
> VerifyResources() ?

Done.

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:432: provider_ ?
provider_->GetChildToParentMap(ChildIdForSurface(surface))
On 2015/05/28 23:00:15, danakj wrote:
> todo-o

Done.

https://codereview.chromium.org/1152473006/diff/100001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:433: : ResourceProvider::ResourceIdMap();
On 2015/05/28 23:00:15, danakj wrote:
> I'm kinda surprised this doesn't cause the tests to DCHECK on l407 now.
Nothing
> would be in the map instead of just not checking anything. Maybe provider_
isn't
> null in unit tests anymroe?

It's because of l403, we only run the loop when the map isn't empty. (which
before was run it when remap callback is not null)

https://codereview.chromium.org/1152473006/diff/100001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1152473006/diff/100001/content/common/cc_mess...
content/common/cc_messages.cc:816: size_t to_reserve = sizeof(p.count) + p.count
* sizeof(p.ids[0]);
On 2015/05/28 23:00:15, danakj wrote:
> space for this is already reserved in ReserveSizeForRenderPassWrite no? the
size
> of the ids etc is all part of sizeof the largest draw quad?

Hmm makes sense. Done.

[vmpstr, 2015-05-29 18:25:43]

The perftest (IterateResources) improves by about 30%. In the grand scheme of
things, it probably makes little impact, but it's faster and imho nicer.

[vmpstr, 2015-05-29 18:28:51]

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

[vmpstr, 2015-05-29 18:29:30]

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

[vmpstr, 2015-05-29 18:30:55]

+jschuh for content/common/ (and security review)
+sky for mojo/converters/

Please take a look.

[sky, 2015-05-29 19:15:54]

mojo/converters LGTM

[vmpstr, 2015-06-01 22:28:52]

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

[vmpstr, 2015-06-01 22:28:52]

+mkwst as well for cc_messages

[Mike West, 2015-06-02 04:37:19]

IPC LGTM % comment.

https://codereview.chromium.org/1152473006/diff/140001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1152473006/diff/140001/content/common/cc_mess...
content/common/cc_messages.cc:839: for (size_t i = 0; i < p.count; ++i) {
Please check the size of `p.count` here as well.

[vmpstr, 2015-06-02 17:19:22]

https://codereview.chromium.org/1152473006/diff/140001/content/common/cc_mess...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/1152473006/diff/140001/content/common/cc_mess...
content/common/cc_messages.cc:839: for (size_t i = 0; i < p.count; ++i) {
On 2015/06/02 04:37:19, Mike West wrote:
> Please check the size of `p.count` here as well.

Done.

[vmpstr, 2015-06-02 20:11:26]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-02 20:11:27]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
mkwst@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1152473006/#ps160001
(title: "mkwst review")

[commit-bot: I haz the power, 2015-06-02 20:12:08]

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

[commit-bot: I haz the power, 2015-06-02 21:28:37]

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

[commit-bot: I haz the power, 2015-06-02 21:28:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[vmpstr, 2015-06-02 21:30:58]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-02 21:32:05]

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

[commit-bot: I haz the power, 2015-06-02 22:14:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-02 22:15:54]

Patchset 9 (id:??) landed as
https://crrev.com/0eca2e87110112c678c5f4c89b7f99e2de6026bb
Cr-Commit-Position: refs/heads/master@{#332476}

[Nico, 2015-07-23 18:04:40]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-23 18:04:41]

https://codereview.chromium.org/1152473006/diff/160001/cc/quads/draw_quad.h
File cc/quads/draw_quad.h (right):

https://codereview.chromium.org/1152473006/diff/160001/cc/quads/draw_quad.h#n...
cc/quads/draw_quad.h:128: enum : size_t { kMaxResourceIdCount = 4 };
(drive-by since I just saw this: explicit types on unnamed enums don't really
make sense. You're just using this line to tell the compiler "yo,
kMaxResourceIdCount is 4", the size_t bit doesn't add anything)

(also, all other fields in this class are at the beginning of this class; it's a
bit weird that resources is tacked on at the end)

[vmpstr, 2015-07-23 21:40:04]

On 2015/07/23 18:04:41, Nico (hiding) wrote:
> https://codereview.chromium.org/1152473006/diff/160001/cc/quads/draw_quad.h
> File cc/quads/draw_quad.h (right):
> 
>
https://codereview.chromium.org/1152473006/diff/160001/cc/quads/draw_quad.h#n...
> cc/quads/draw_quad.h:128: enum : size_t { kMaxResourceIdCount = 4 };
> (drive-by since I just saw this: explicit types on unnamed enums don't really
> make sense. You're just using this line to tell the compiler "yo,
> kMaxResourceIdCount is 4", the size_t bit doesn't add anything)

There's a DCHECK below that compares this to size_t, and without a type, there
would be a signed/unsigned comparison error (see trybots on
https://codereview.chromium.org/1248403002/). I can also do this as a cast, but
this seemed cleaner at the time (ie, it is an array size which is properly typed
as size_t...). 

> 
> (also, all other fields in this class are at the beginning of this class; it's
a
> bit weird that resources is tacked on at the end)

Because it's a type, I think it needs to be physically declared before it's
used, right? Maybe I confused this :P