cc: Receive and remap resources to the parent.

cc: Receive and remap resources to the parent.

The delegated renderer layer needs to give the resources it
receives for a frame to the resource provider. Then, the resources
are remapped into the parent's namespace so the quads need to be
updated to reflect this remapping. Finally, the resources no longer
in use by the layer should be returned to the child compositor.

The above is implemented in DelegatedRendererLayerImpl::SetFrameData()
however for now the resources to be returned are simply dropped with
a TODO.

Tests:
LayerTreeHostDelegatedTestMergeResources.runSingleThread
LayerTreeHostDelegatedTestMergeResources.runMultiThread
LayerTreeHostDelegatedTestRemapResourcesInQuads.runSingleThread
LayerTreeHostDelegatedTestRemapResourcesInQuads.runMultiThread

R=piman
BUG=123444
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185477

[danakj, 2013-02-27 22:44:31]

This needs tests which I will write tomorrow, so feel free to not review it yet.
However, wanted to put this up to show where I am trying to go. Feedback welcome
of course.

[piman, 2013-02-27 23:09:39]

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer.cc
File cc/delegated_renderer_layer.cc (right):

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer.c...
cc/delegated_renderer_layer.cc:67: frame_data_ = new_frame_data.Pass();
If we already had a frame_data_, we need to merge the old resource list with the
new one.

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.cc:64: frame_data->resource_list.empty());
What happens if that's not the case? It seems harmless for the purpose of this
function (the extra resources would be returned immediately).

In particular, in the case where we
SetFrameData(frame_data_with_passes_and_resources) and then
SetFrameData(frame_data_with_no_passes) without a commit in the middle, we'll
collect resources but end up with no passes

If we want to check that the renderer doesn't generate such a case, a better
place for the DCHECK would be in the DelegatingRenderer

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.cc:73:
DCHECK(!new_root_pass->output_rect.IsEmpty());
nit: this is tested above, so you can remove.

https://codereview.chromium.org/12340124/diff/1/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/12340124/diff/1/cc/draw_quad.h#newcode79
cc/draw_quad.h:79: virtual void RemapResources(const ResourceRemapCallback&
remap_callback);
Awesome, I was thinking of something similar.
Possibly taking it one step further, how about combining AppendResources and
RemapResources into a single IterateResources?

[piman, 2013-02-28 02:28:33]

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.cc:96: DrawQuad* quad = pass->quad_list[i];
s/quad_list[i]/quad_list[j]/

[danakj, 2013-02-28 02:51:05]

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
File cc/delegated_renderer_layer_impl.cc (right):

https://codereview.chromium.org/12340124/diff/1/cc/delegated_renderer_layer_i...
cc/delegated_renderer_layer_impl.cc:64: frame_data->resource_list.empty());
On 2013/02/27 23:09:39, piman wrote:
> What happens if that's not the case? It seems harmless for the purpose of this
> function (the extra resources would be returned immediately).
> 
> In particular, in the case where we
> SetFrameData(frame_data_with_passes_and_resources) and then
> SetFrameData(frame_data_with_no_passes) without a commit in the middle, we'll
> collect resources but end up with no passes
> 
> If we want to check that the renderer doesn't generate such a case, a better
> place for the DCHECK would be in the DelegatingRenderer

Right, with the merging you mentioned in DRLayer.cc this makes a lot of sense.

https://codereview.chromium.org/12340124/diff/1/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/12340124/diff/1/cc/draw_quad.h#newcode79
cc/draw_quad.h:79: virtual void RemapResources(const ResourceRemapCallback&
remap_callback);
On 2013/02/27 23:09:39, piman wrote:
> Awesome, I was thinking of something similar.
> Possibly taking it one step further, how about combining AppendResources and
> RemapResources into a single IterateResources?

Oh.. that is a cool idea.

[danakj, 2013-03-01 02:17:15]

Tests included now. PTAL.

[piman, 2013-03-01 02:46:34]

lgtm

https://codereview.chromium.org/12340124/diff/17002/cc/shared_quad_state.cc
File cc/shared_quad_state.cc (right):

https://codereview.chromium.org/12340124/diff/17002/cc/shared_quad_state.cc#n...
cc/shared_quad_state.cc:9: SharedQuadState::SharedQuadState() :
is_clipped(false), opacity(0) {}
nice catch.

[piman, 2013-03-01 02:51:17]

https://codereview.chromium.org/12340124/diff/17002/cc/test/fake_delegated_re...
File cc/test/fake_delegated_renderer_layer.h (right):

https://codereview.chromium.org/12340124/diff/17002/cc/test/fake_delegated_re...
cc/test/fake_delegated_renderer_layer.h:15: return make_scoped_refptr(new
FakeDelegatedRendererLayer());
FYI git said there's a trailing whitespace here.

[danakj, 2013-03-01 02:57:15]

https://codereview.chromium.org/12340124/diff/17002/cc/test/fake_delegated_re...
File cc/test/fake_delegated_renderer_layer.h (right):

https://codereview.chromium.org/12340124/diff/17002/cc/test/fake_delegated_re...
cc/test/fake_delegated_renderer_layer.h:15: return make_scoped_refptr(new
FakeDelegatedRendererLayer());
On 2013/03/01 02:51:17, piman wrote:
> FYI git said there's a trailing whitespace here.

Ah thanks!

[commit-bot: I haz the power, 2013-03-01 04:33:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12340124/12020

[commit-bot: I haz the power, 2013-03-01 07:32:02]

Change committed as 185477