Define cc::TransformOperations and webkit::WebTransformOperationsImpl

Define cc::TransformOperations and webkit::WebTransformOperationsImpl

This ports the implementation of WebTransformOperations into cc::TransformOperations, Also, behind #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL, this makes cc use TransformOperations instead of WebTransformOperations; defines a new class WebTransformOperationsImpl that implements WebTransformOperations using an instance of cc::TransformOperations; defines WebCompositorSupportImpl::createTransformOperations; and makes compositor_bindings use WebTransformOperationsImpl instead of WebTransformOperations (extracting the underlying cc::TransformOperations before calling into cc). 

This is step 1 of the 3-step plan outlined at https://chromiumcodereview.appspot.com/11745018 for moving the implementation of WebTransformOperations into chromium.

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

[jamesr, 2013-01-15 06:23:00]

Looking pretty good, but I don't fully understand what's going on with
cc::TransformOperations. If there are expected to be subclasses, then I'd think
the copy c'tor would have to be smarter.

If it makes life easier, you can give cc::TransformOperations WebKit-style
member function names at first (i.e. lowercase) - I think that might cut down on
some of the #ifdefs you have currently.  This means you or someone else would
have to do a follow-up to chromify them, so only do this if you think it's worth
it to make this transition easier.  I think what you have here is also OK.

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operations.h
File cc/transform_operations.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:29: virtual ~TransformOperations();
why virtual?

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:32: virtual WebKit::WebTransformationMatrix Apply()
const;
why virtual? (same comment for the rest of the interface)

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:69: scoped_ptr<TransformOperationsPrivate> private_;
why the Private indirection?

In the WebKit API version of this, this pattern was used since there isn't a
growable array type that can be exposed in a public WebKit API type.  here, I
can't see any issue with forward-declaring a TransformOperation type and having
this be a std::vector<TransformOperation> operations_;

https://chromiumcodereview.appspot.com/11876016/diff/1/webkit/compositor_bind...
File webkit/compositor_bindings/web_transform_operations_impl.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/1/webkit/compositor_bind...
webkit/compositor_bindings/web_transform_operations_impl.h:16: namespace WebKit
{
you should put this in namespace 'webkit' - 'WebKit' is supposed to be for
things in the WebKit repository, 'webkit' is the things we implement in chromium
to support our embedding.  this means you'll have to write WebKit:: in front of
some of the types below, but there aren't all that many and it looks like some
of these types already have WebKit:: in front of them

[ajuma, 2013-01-15 19:23:53]

On 2013/01/15 06:23:00, jamesr wrote:
> Looking pretty good, but I don't fully understand what's going on with
> cc::TransformOperations. If there are expected to be subclasses, then I'd
think
> the copy c'tor would have to be smarter.

There aren't expected to be subclasses -- all the virtuals were leftovers from a
previous version of this.

[ajuma, 2013-01-15 19:24:29]

Addressed nits.

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operations.h
File cc/transform_operations.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:29: virtual ~TransformOperations();
On 2013/01/15 06:23:01, jamesr wrote:
> why virtual?

Fixed.

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:32: virtual WebKit::WebTransformationMatrix Apply()
const;
On 2013/01/15 06:23:01, jamesr wrote:
> why virtual? (same comment for the rest of the interface)

Fixed.

https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
cc/transform_operations.h:69: scoped_ptr<TransformOperationsPrivate> private_;
On 2013/01/15 06:23:01, jamesr wrote:
> why the Private indirection?
> 
> In the WebKit API version of this, this pattern was used since there isn't a
> growable array type that can be exposed in a public WebKit API type.  here, I
> can't see any issue with forward-declaring a TransformOperation type and
having
> this be a std::vector<TransformOperation> operations_;

Done.

https://chromiumcodereview.appspot.com/11876016/diff/1/webkit/compositor_bind...
File webkit/compositor_bindings/web_transform_operations_impl.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/1/webkit/compositor_bind...
webkit/compositor_bindings/web_transform_operations_impl.h:16: namespace WebKit
{
On 2013/01/15 06:23:01, jamesr wrote:
> you should put this in namespace 'webkit' - 'WebKit' is supposed to be for
> things in the WebKit repository, 'webkit' is the things we implement in
chromium
> to support our embedding.  this means you'll have to write WebKit:: in front
of
> some of the types below, but there aren't all that many and it looks like some
> of these types already have WebKit:: in front of them

Done.

[jamesr, 2013-01-16 00:58:49]

lgtm!

https://chromiumcodereview.appspot.com/11876016/diff/7001/cc/transform_operat...
File cc/transform_operations_unittest.cc (right):

https://chromiumcodereview.appspot.com/11876016/diff/7001/cc/transform_operat...
cc/transform_operations_unittest.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
2013

https://chromiumcodereview.appspot.com/11876016/diff/7001/webkit/compositor_b...
File webkit/compositor_bindings/web_transform_operations_impl.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/7001/webkit/compositor_b...
webkit/compositor_bindings/web_transform_operations_impl.h:42:
scoped_ptr<cc::TransformOperations> transform_operations_;
it's not a big deal, but is there a reason this is heap-allocated instead of
just being a direct member variable?

[ajuma, 2013-01-16 15:36:54]

https://chromiumcodereview.appspot.com/11876016/diff/7001/cc/transform_operat...
File cc/transform_operations_unittest.cc (right):

https://chromiumcodereview.appspot.com/11876016/diff/7001/cc/transform_operat...
cc/transform_operations_unittest.cc:1: // Copyright 2012 The Chromium Authors.
All rights reserved.
On 2013/01/16 00:58:49, jamesr wrote:
> 2013

Done.

https://chromiumcodereview.appspot.com/11876016/diff/7001/webkit/compositor_b...
File webkit/compositor_bindings/web_transform_operations_impl.h (right):

https://chromiumcodereview.appspot.com/11876016/diff/7001/webkit/compositor_b...
webkit/compositor_bindings/web_transform_operations_impl.h:42:
scoped_ptr<cc::TransformOperations> transform_operations_;
On 2013/01/16 00:58:49, jamesr wrote:
> it's not a big deal, but is there a reason this is heap-allocated instead of
> just being a direct member variable?

Good point. Changed to a direct member variable now.

[commit-bot: I haz the power, 2013-01-16 15:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/11876016/24001

[commit-bot: I haz the power, 2013-01-16 15:40:52]

Failed to apply patch for cc/cc_tests.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/cc_tests.gyp
  Hunk #1 FAILED at 61.
  1 out of 1 hunk FAILED -- saving rejects to file cc/cc_tests.gyp.rej

Patch:       cc/cc_tests.gyp
Index: cc/cc_tests.gyp
diff --git a/cc/cc_tests.gyp b/cc/cc_tests.gyp
index
7b0ae4cc60ab19424e0ab7e195a948552f9d0e56..78b7dd6e524c6a67b4f0efc82cbf58fdc9f36742
100644
--- a/cc/cc_tests.gyp
+++ b/cc/cc_tests.gyp
@@ -61,6 +61,7 @@
       'tiled_layer_impl_unittest.cc',
       'tiled_layer_unittest.cc',
       'tiling_data_unittest.cc',
+      'transform_operations_unittest.cc',
       'tree_synchronizer_unittest.cc',
       'timing_function_unittest.cc',
       'test/fake_web_graphics_context_3d_unittest.cc',

[commit-bot: I haz the power, 2013-01-16 15:56:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/11876016/21002

[ajuma, 2013-01-16 20:52:52]

On 2013/01/15 06:23:00, jamesr wrote:
>
https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
> cc/transform_operations.h:69: scoped_ptr<TransformOperationsPrivate> private_;
> why the Private indirection?
> 
> In the WebKit API version of this, this pattern was used since there isn't a
> growable array type that can be exposed in a public WebKit API type.  here, I
> can't see any issue with forward-declaring a TransformOperation type and
having
> this be a std::vector<TransformOperation> operations_;

It turns out that forward-declaring TransformOperation and having an instance
variable of type std::vector<TransformOperation> breaks Windows debug builds
(the compiler is unhappy that it doesn't know the size of TransformOperation).
I've posted an updated patch where TransformOperation is now a private nested
class that is declared in transform_operations.h. Alternatively (to avoid
exposing these implementation details in the .h file), we could go back to the
TransformOperationsPrivate approach from the first version.

jamesr, what do you think?

[jamesr, 2013-01-16 21:05:02]

On 2013/01/16 20:52:52, ajuma wrote:
> On 2013/01/15 06:23:00, jamesr wrote:
> >
>
https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
> > cc/transform_operations.h:69: scoped_ptr<TransformOperationsPrivate>
private_;
> > why the Private indirection?
> > 
> > In the WebKit API version of this, this pattern was used since there isn't a
> > growable array type that can be exposed in a public WebKit API type.  here,
I
> > can't see any issue with forward-declaring a TransformOperation type and
> having
> > this be a std::vector<TransformOperation> operations_;
> 
> It turns out that forward-declaring TransformOperation and having an instance
> variable of type std::vector<TransformOperation> breaks Windows debug builds
> (the compiler is unhappy that it doesn't know the size of TransformOperation).
> I've posted an updated patch where TransformOperation is now a private nested
> class that is declared in transform_operations.h. Alternatively (to avoid
> exposing these implementation details in the .h file), we could go back to the
> TransformOperationsPrivate approach from the first version.
> 
> jamesr, what do you think?

Or just put the type in its own header with its own cc.

[jamesr, 2013-01-16 21:05:48]

On 2013/01/16 21:05:02, jamesr wrote:
> On 2013/01/16 20:52:52, ajuma wrote:
> > On 2013/01/15 06:23:00, jamesr wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/11876016/diff/1/cc/transform_operation...
> > > cc/transform_operations.h:69: scoped_ptr<TransformOperationsPrivate>
> private_;
> > > why the Private indirection?
> > > 
> > > In the WebKit API version of this, this pattern was used since there isn't
a
> > > growable array type that can be exposed in a public WebKit API type. 
here,
> I
> > > can't see any issue with forward-declaring a TransformOperation type and
> > having
> > > this be a std::vector<TransformOperation> operations_;
> > 
> > It turns out that forward-declaring TransformOperation and having an
instance
> > variable of type std::vector<TransformOperation> breaks Windows debug builds
> > (the compiler is unhappy that it doesn't know the size of
TransformOperation).
> > I've posted an updated patch where TransformOperation is now a private
nested
> > class that is declared in transform_operations.h. Alternatively (to avoid
> > exposing these implementation details in the .h file), we could go back to
the
> > TransformOperationsPrivate approach from the first version.
> > 
> > jamesr, what do you think?
> 
> Or just put the type in its own header with its own cc.

By "the type" I mean TransformOperation

[ajuma, 2013-01-16 21:34:09]

On 2013/01/16 21:05:48, jamesr wrote:
> On 2013/01/16 21:05:02, jamesr wrote:
> > Or just put the type in its own header with its own cc.
> 
> By "the type" I mean TransformOperation

Done. PTAL.

[jamesr, 2013-01-16 21:38:02]

lgtm

[commit-bot: I haz the power, 2013-01-16 21:42:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/11876016/34001

[commit-bot: I haz the power, 2013-01-17 00:40:29]

Change committed as 177290