Implementation of WebCompositorSupport when use_libcc_for_compositor=1

Implementation of WebCompositorSupport when use_libcc_for_compositor=1

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

[jamesr, 2012-09-01 01:31:11]

Here's a start of support for WebCompositorSupport.  I'd like to use this to
start migrating WebKit code away from calling the WEBKIT_EXPORT static
...Create() functions since they are really annoying to get working in the
windows component build.

Also, Nat suggested renaming webkit/compositor/ to webkit/compositor_bindings/
which I think is a great idea.  I'll rebase this patch when I do that.

[tfarina, 2012-09-01 01:50:02]

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
File webkit/compositor/web_compositor_support_impl.h (right):

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
webkit/compositor/web_compositor_support_impl.h:5: #ifndef
WEB_COMPOSITOR_SUPPORT_IMPL_H_
does this needs to follow our header guards convention?
if so, it should be WEBKIT_COMPOSITOR_WEB_COMPOSITOR_SUPPORT_IMPL_H_

[nduca, 2012-09-01 01:50:08]

ltm :)

[nduca, 2012-09-01 01:50:15]

lgtm even

[tfarina, 2012-09-01 01:52:14]

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
File webkit/compositor/web_compositor_support_impl.h (right):

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
webkit/compositor/web_compositor_support_impl.h:18: virtual
WebKit::WebLayerTreeView* createLayerTreeView(
Are these virtual functions overrides from WebCompositorSupport? If so, can you
mark them with OVERRIDE?

[tfarina, 2012-09-01 01:55:09]

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/glue/webkitplat...
File webkit/glue/webkitplatformsupport_impl.h (right):

https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/glue/webkitplat...
webkit/glue/webkitplatformsupport_impl.h:176: webkit::WebCompositorSupportImpl
compositor_support_;
can this be a scoped_ptr? up to you though. At least that way we avoid including
web_compositor_support_impl.h here.

[darin (slow to review), 2012-09-01 03:31:53]

LGTM ... I don't have strong feelings about the suggestions by tfarina.

[jamesr, 2012-09-01 19:45:33]

On 2012/09/01 01:50:02, tfarina wrote:
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
> File webkit/compositor/web_compositor_support_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
> webkit/compositor/web_compositor_support_impl.h:5: #ifndef
> WEB_COMPOSITOR_SUPPORT_IMPL_H_
> does this needs to follow our header guards convention?
> if so, it should be WEBKIT_COMPOSITOR_WEB_COMPOSITOR_SUPPORT_IMPL_H_

Yup, fixed

[jamesr, 2012-09-01 19:46:11]

On 2012/09/01 01:55:09, tfarina wrote:
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/glue/webkitplat...
> File webkit/glue/webkitplatformsupport_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/glue/webkitplat...
> webkit/glue/webkitplatformsupport_impl.h:176: webkit::WebCompositorSupportImpl
> compositor_support_;
> can this be a scoped_ptr? up to you though. At least that way we avoid
including
> web_compositor_support_impl.h here.

Don't think it's needed here - no need for a heap allocation for an empty class
(vtable only). web_compositor_support_impl.h is not a big header

[jamesr, 2012-09-01 19:47:26]

On 2012/09/01 01:52:14, tfarina wrote:
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
> File webkit/compositor/web_compositor_support_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/10918037/diff/1/webkit/compositor/web_...
> webkit/compositor/web_compositor_support_impl.h:18: virtual
> WebKit::WebLayerTreeView* createLayerTreeView(
> Are these virtual functions overrides from WebCompositorSupport? If so, can
you
> mark them with OVERRIDE?

They are but they should *not* have OVERRIDE - WebCompositorSupport is in a
different repo (WebKit) and having OVERRIDE makes refactoring much too difficult

[tfarina, 2012-09-01 23:30:02]

http://codereview.chromium.org/10918037/diff/13001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_compositor_support_impl.h (right):

http://codereview.chromium.org/10918037/diff/13001/webkit/compositor_bindings...
webkit/compositor_bindings/web_compositor_support_impl.h:19:
WebKit::WebLayerTreeViewClient* client, const WebKit::WebLayer& root,
nit: write one arg per line?

http://codereview.chromium.org/10918037/diff/13001/webkit/compositor_bindings...
webkit/compositor_bindings/web_compositor_support_impl.h:26:
createExternalTextureLayer(WebKit::WebExternalTextureLayerClient* client);
indent 4 spaces. same for the other cases below.

http://codereview.chromium.org/10918037/diff/13001/webkit/compositor_bindings...
webkit/compositor_bindings/web_compositor_support_impl.h:41: int animationId);
does this needs to follow our naming convention? unix_hacker style?!

http://codereview.chromium.org/10918037/diff/13001/webkit/compositor_bindings...
webkit/compositor_bindings/web_compositor_support_impl.h:46: };
do we want to disallow copy and assign of this class?

[nduca, 2012-09-02 20:11:46]

@tfarina, these are webkit bindings changes so we typically inherit the webkit
style for bits of this folder. I wouldn't worry too much about nits here, quite
yet.

[tfarina, 2012-09-02 20:21:44]

On Sun, Sep 2, 2012 at 5:11 PM,  <nduca@chromium.org> wrote:
> @tfarina, these are webkit bindings changes so we typically inherit the
> webkit
> style for bits of this folder. I wouldn't worry too much about nits here,
> quite
> yet.
>
OK. Stepping back here. I wasn't sure if they should follow the
chromium code style.

I'll let the nits for cc so :)

-- 
Thiago