Add CCDelegatingRenderer, and corresponding IPCs

Add CCDelegatingRenderer, and corresponding IPCs

This is work in progress and doesn't do anything aside of sending empty IPCs,
but hooks up the first pieces.

BUG=146080

[piman, 2012-09-15 02:45:38]

FYI, this is work in progress, but is where I'm going. With this I can run
Chrome with --enable-delegated-renderer, renderers don't draw anything anymore,
and I get NOTIMPLEMENTED warnings, but it triggers frames in the parent
compositor, which sends ACKs back.

From there, we need to serialize the quads, do the resource transfer magic, and
hook everything up to a WebDelegatedRendererLayer on the parent. The rest is
left as an exercise for the reader.

[danakj, 2012-09-21 20:51:41]

This is cool! I have some questions about it all for you.

https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp
File cc/CCDelegatingRenderer.cpp (right):

https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cp...
cc/CCDelegatingRenderer.cpp:77: m_capabilities.usingPartialSwap =
CCSettings::partialSwapEnabled();
I don't think we want to ever partial swap with delegating renderer. We want to
always send a full frame of quads. The browser can choose to only render a
subset of them for partial swap.

https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cp...
cc/CCDelegatingRenderer.cpp:139:
frame.render_passes.reserve(renderPassesInDrawOrder.size());
What about just storing a pointer/reference to the CCRenderPassList here in the
WCFrame? Instead of another vector holding a pointer to each CCRenderPass?

https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cp...
cc/CCDelegatingRenderer.cpp:144:
outputSurface->sendFrameToParentCompositor(frame);
Should this be in swapBuffers? Could it ever matter that it's not? Might we
possibly end up calling onSwapBuffersComplete before swapBuffers is called?

https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cp...
cc/CCDelegatingRenderer.cpp:154: // FIXME
Hm, in this case we're going to need a CCDirectRenderer of some sort to delegate
this to, I guess? Or are you imagining having the browser send back pixels?

https://codereview.chromium.org/10915298/diff/2001/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10915298/diff/2001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:703: renderer = CCRendererGL::create(this,
resourceProvider.get());
We've become inconsistent in our naming of renderers now. Are we going to use
PrefixRenderer or RendererSuffix?

https://codereview.chromium.org/10915298/diff/2001/cc/CCRenderPass.h
File cc/CCRenderPass.h (right):

https://codereview.chromium.org/10915298/diff/2001/cc/CCRenderPass.h#newcode91
cc/CCRenderPass.h:91: const CCSharedQuadStateList& sharedQuadStateList() const {
return m_sharedQuadStateList; }
nit: move this up by quadList()?

https://codereview.chromium.org/10915298/diff/2001/cc/web_compositor_frame.h
File cc/web_compositor_frame.h (right):

https://codereview.chromium.org/10915298/diff/2001/cc/web_compositor_frame.h#...
cc/web_compositor_frame.h:14: namespace WebKit {
How come this is WebKit::WebCompositorFrame and not cc::CompositorFrame?

[piman, 2012-09-21 22:02:12]

2 notes:
- this isn't intended for landing yet, just to show the direction for
end-to-end hookup. I'll separate into several CLs
- I have a follow up that does a bunch of other things such as doing the
texture transfers logic in CCDelegatingRenderer.

On Fri, Sep 21, 2012 at 1:51 PM, <danakj@chromium.org> wrote:

> This is cool! I have some questions about it all for you.
>
>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCDelegatingRenderer.cpp<https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp>
> File cc/CCDelegatingRenderer.cpp (right):
>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCDelegatingRenderer.cpp#**newcode77<https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp#newcode77>
> cc/CCDelegatingRenderer.cpp:**77: m_capabilities.**usingPartialSwap =
> CCSettings::**partialSwapEnabled();
> I don't think we want to ever partial swap with delegating renderer. We
> want to always send a full frame of quads. The browser can choose to
> only render a subset of them for partial swap.
>

We always want to send the full frame of quads indeed, but we do want to
track the dirty rect so that we can communicate it to the browser. Or do we
do that always anyway, and this only controls how we do the rendering?


>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCDelegatingRenderer.cpp#**newcode139<https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp#newcode139>
> cc/CCDelegatingRenderer.cpp:**139:
> frame.render_passes.reserve(**renderPassesInDrawOrder.size()**);
> What about just storing a pointer/reference to the CCRenderPassList here
> in the WCFrame? Instead of another vector holding a pointer to each
> CCRenderPass?
>

Trying to keep IPC in mind, it's easy to serialize std::vector, but not
webcore types. Also, we can't leak webcore types outside of cc/, where the
implementation of sendFrameToParentCompositor is, so WebCompositorFrame
can't use them.



>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCDelegatingRenderer.cpp#**newcode144<https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp#newcode144>
> cc/CCDelegatingRenderer.cpp:**144:
> outputSurface->**sendFrameToParentCompositor(**frame);
> Should this be in swapBuffers?


We probably should. This was done for expediency, because otherwise we need
to keep the WebCompositorFrame around, implying copying the CCRenderPasses.


> Could it ever matter that it's not?


It would matter if we started trying to do drawFrame without swapBuffers,
but currently they're called together.


> Might
> we possibly end up calling onSwapBuffersComplete before swapBuffers is
> called?
>

 Currently no, because both are called in the same task, and we won't
process returning IPCs until we go back to the top-level message loop.


> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCDelegatingRenderer.cpp#**newcode154<https://codereview.chromium.org/10915298/diff/2001/cc/CCDelegatingRenderer.cpp#newcode154>
> cc/CCDelegatingRenderer.cpp:**154: // FIXME
> Hm, in this case we're going to need a CCDirectRenderer of some sort to
> delegate this to, I guess? Or are you imagining having the browser send
> back pixels?
>

I'm thinking punting on it for now. IIRC it's only used for the in-renderer
thumbnailer that we don't use anymore (though we'll have to solve a similar
problem in the browser).
We can't locally use a CCDirectRenderer because we can't use the resources
that we sent to the browser. So we'd have to ask the browser.


>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCLayerTreeHostImpl.cpp<https://codereview.chromium.org/10915298/diff/2001/cc/CCLayerTreeHostImpl.cpp>
> File cc/CCLayerTreeHostImpl.cpp (right):
>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCLayerTreeHostImpl.cpp#**newcode703<https://codereview.chromium.org/10915298/diff/2001/cc/CCLayerTreeHostImpl.cpp#newcode703>
> cc/CCLayerTreeHostImpl.cpp:**703: renderer = CCRendererGL::create(this,
> resourceProvider.get());
> We've become inconsistent in our naming of renderers now. Are we going
> to use PrefixRenderer or RendererSuffix?
>

I don't have a strong opinion :) I'm happy with CCRendererDelegating, but
it doesn't sound as nice.


>
>
https://codereview.chromium.**org/10915298/diff/2001/cc/**CCRenderPass.h<http...
> File cc/CCRenderPass.h (right):
>
> https://codereview.chromium.**org/10915298/diff/2001/cc/**
>
CCRenderPass.h#newcode91<https://codereview.chromium.org/10915298/diff/2001/cc/CCRenderPass.h#newcode91>
> cc/CCRenderPass.h:91: const CCSharedQuadStateList& sharedQuadStateList()
> const { return m_sharedQuadStateList; }
> nit: move this up by quadList()?
>

sure.


>
> https://codereview.chromium.**org/10915298/diff/2001/cc/web_**
>
compositor_frame.h<https://codereview.chromium.org/10915298/diff/2001/cc/web_compositor_frame.h>
> File cc/web_compositor_frame.h (right):
>
> https://codereview.chromium.**org/10915298/diff/2001/cc/web_**
>
compositor_frame.h#newcode14<https://codereview.chromium.org/10915298/diff/2001/cc/web_compositor_frame.h#newcode14>
> cc/web_compositor_frame.h:14: namespace WebKit {
> How come this is WebKit::WebCompositorFrame and not cc::CompositorFrame?
>

WebCompositorFrame is what's used in WebCompositorOutputSurface which is
used in WebViewClient. WebKit API can only use WebKit API types, not cc
types - we have to draw the line somewhere :)

>
>
https://codereview.chromium.**org/10915298/<https://codereview.chromium.org/1...
>

[jam, 2012-09-24 18:25:36]

https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/...
File content/common/gpu/client/cc_param_traits.h (right):

https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/...
content/common/gpu/client/cc_param_traits.h:14: struct WebCompositorFrameAck;
I can't find these structs, so I don't know what their serialization will look
like. Can you point me to their declaration?

it's much more preferable to use the IPC_STRUCT_TRAITS_BEGIN macros if you can.
you can make content/common/content_message_generator.h include it which would
give it the multiple includes needed to use these macros.

[piman, 2012-09-24 19:59:29]

On Mon, Sep 24, 2012 at 11:25 AM, <jam@chromium.org> wrote:

>
> https://codereview.chromium.**org/10915298/diff/2001/**
>
content/common/gpu/client/cc_**param_traits.h<https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/cc_param_traits.h>
> File content/common/gpu/client/cc_**param_traits.h (right):
>
> https://codereview.chromium.**org/10915298/diff/2001/**
>
content/common/gpu/client/cc_**param_traits.h#newcode14<https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/cc_param_traits.h#newcode14>
> content/common/gpu/client/cc_**param_traits.h:14: struct
> WebCompositorFrameAck;
> I can't find these structs, so I don't know what their serialization
> will look like. Can you point me to their declaration?


> it's much more preferable to use the IPC_STRUCT_TRAITS_BEGIN macros if
> you can. you can make content/common/content_**message_generator.h include
> it which would give it the multiple includes needed to use these macros.
>

The last patch adds a definition, but as noted initially this is not for
review/landing. I noted your suggestion and agree, for the extent that it
is possible we should use the macros.


>
>
https://codereview.chromium.**org/10915298/<https://codereview.chromium.org/1...
>

[jam, 2012-09-24 20:45:01]

On 2012/09/24 19:59:29, piman wrote:
> On Mon, Sep 24, 2012 at 11:25 AM, <mailto:jam@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/10915298/diff/2001/**
> >
>
content/common/gpu/client/cc_**param_traits.h<https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/cc_param_traits.h>
> > File content/common/gpu/client/cc_**param_traits.h (right):
> >
> > https://codereview.chromium.**org/10915298/diff/2001/**
> >
>
content/common/gpu/client/cc_**param_traits.h#newcode14<https://codereview.chromium.org/10915298/diff/2001/content/common/gpu/client/cc_param_traits.h#newcode14>
> > content/common/gpu/client/cc_**param_traits.h:14: struct
> > WebCompositorFrameAck;
> > I can't find these structs, so I don't know what their serialization
> > will look like. Can you point me to their declaration?
> 
> 
> > it's much more preferable to use the IPC_STRUCT_TRAITS_BEGIN macros if
> > you can. you can make content/common/content_**message_generator.h include
> > it which would give it the multiple includes needed to use these macros.
> >
> 
> The last patch adds a definition,

i may not have been clear, I meant the definition of the structs. (I couldn't
find them in the webkit repo). I ask because I wanted to see if the traits can
be used. looking at your definition of the manual traits, it looks like they can
be automatic.

> but as noted initially this is not for
> review/landing. I noted your suggestion and agree, for the extent that it
> is possible we should use the macros.
> 
> 
> >
> >
>
https://codereview.chromium.**org/10915298/%3Chttps://codereview.chromium.org...>
> >

[aelias_OOO_until_Jul13, 2012-10-10 00:16:17]

https://codereview.chromium.org/10915298/diff/16001/cc/transferable_resource.h
File cc/transferable_resource.h (right):

https://codereview.chromium.org/10915298/diff/16001/cc/transferable_resource....
cc/transferable_resource.h:28: struct TransferableResourceList {
Clang builds fail, complaining this needs a constructor and destructor (which
also need to be non-inline).

[aelias_OOO_until_Jul13, 2012-10-10 00:47:25]

https://codereview.chromium.org/10915298/diff/16001/cc/web_compositor_frame.h
File cc/web_compositor_frame.h (right):

https://codereview.chromium.org/10915298/diff/16001/cc/web_compositor_frame.h...
cc/web_compositor_frame.h:19: struct WebCompositorFrame {
Let's define these as empty classes in WebKit tree (without a virtual destructor
either), and have these be a subclass.  Similarly for TransferableResource and
TransferableResourceList.  Traits would also work on the subclass only.

[piman, 2012-10-10 02:13:26]

On 2012/10/10 00:16:17, aelias wrote:
> https://codereview.chromium.org/10915298/diff/16001/cc/transferable_resource.h
> File cc/transferable_resource.h (right):
> 
>
https://codereview.chromium.org/10915298/diff/16001/cc/transferable_resource....
> cc/transferable_resource.h:28: struct TransferableResourceList {
> Clang builds fail, complaining this needs a constructor and destructor (which
> also need to be non-inline).

added in latest patch.

[piman, 2012-10-10 02:16:52]

On 2012/10/10 00:47:25, aelias wrote:
> https://codereview.chromium.org/10915298/diff/16001/cc/web_compositor_frame.h
> File cc/web_compositor_frame.h (right):
> 
>
https://codereview.chromium.org/10915298/diff/16001/cc/web_compositor_frame.h...
> cc/web_compositor_frame.h:19: struct WebCompositorFrame {
> Let's define these as empty classes in WebKit tree (without a virtual
destructor
> either), and have these be a subclass.  Similarly for TransferableResource and
> TransferableResourceList.  Traits would also work on the subclass only.

I did https://bugs.webkit.org/show_bug.cgi?id=98841 for that and take in latest
patch. Note that TransferableResource doesn't need to be in webkit (only
TransferableResourceList).