Only create instances of WebTransformOperations using cc::TransformOperations::Create

Only create instances of WebTransformOperations using cc::TransformOperations::Create

This is a precursor to making WebTransformOperations pure 
virtual and moving the implementation into chromium.

BUG=166640

[ajuma, 2012-12-20 16:48:11]

Rebased, and added a couple changes to WebTransformAnimationCurveImpl that are
also needed.

https://codereview.chromium.org/11644004/diff/1011/webkit/compositor_bindings...
File webkit/compositor_bindings/web_transform_animation_curve_impl.cc (right):

https://codereview.chromium.org/11644004/diff/1011/webkit/compositor_bindings...
webkit/compositor_bindings/web_transform_animation_curve_impl.cc:35:
m_curve->addKeyframe(cc::TransformKeyframe::create(keyframe.time(),
keyframe.value(), createTimingFunction(type)));
WebTransformKeyframe will no longer be able to directly expose an instance of
WebTransformOperations, so we'll need to use an accessor method.

[Ian Vollick, 2012-12-20 16:56:41]

lgtm

+enne for OWNERS

[enne (OOO), 2012-12-20 19:10:18]

CCing GTFO expert jamesr.

I think this is definitely a positive move forward, in that you can pull the
implementation into Chromium, but this can't be a long term solution.  We're
looking to remove all includes from WebKit out of cc and only having them from
the webkit/compositor_bindings files.

I think you'll need to do something similar to what Dana did for
ScrollbarThemePainter (see: https://codereview.chromium.org/11609002), by having
the abstract virtual class in cc, and having some WebTransformOperationsImpl
that   implements this cc::TransformOperations interface and bridges between
WebTransformationOperations and cc::TransformOperations.

[Ian Vollick, 2012-12-20 19:21:36]

On 2012/12/20 19:10:18, enne wrote:
> CCing GTFO expert jamesr.
> 
> I think this is definitely a positive move forward, in that you can pull the
> implementation into Chromium, but this can't be a long term solution.  We're
> looking to remove all includes from WebKit out of cc and only having them from
> the webkit/compositor_bindings files.
> 
> I think you'll need to do something similar to what Dana did for
> ScrollbarThemePainter (see: https://codereview.chromium.org/11609002), by
having
> the abstract virtual class in cc, and having some WebTransformOperationsImpl
> that   implements this cc::TransformOperations interface and bridges between
> WebTransformationOperations and cc::TransformOperations.

This is definitely an intermediate step.

The plan is to do the following (please let me know if there's a better way)
1. Get cc/ prepared for creating transform operations via WTO::create (i.e.,
step 2)
2. Make WTO pure virtual, (with the implementation living in WebKit)
3. Make a WTO implementation in compositor bindings (at this point, we can
switch cc/transform_operations to use that instead of wk.
4. Get WK to create its WTO's via platform support rather than the static
function, and get rid of the WTO implementation in wk.

[jamesr, 2012-12-20 19:52:25]

Seems like a reasonable plan.

https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc
File cc/transform_operations.cc (right):

https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc...
cc/transform_operations.cc:6: #include <public/WebTransformOperations.h>
"third_party/WebKit/.....", please

https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc...
cc/transform_operations.cc:12: #ifdef WEBKIT_WEB_TRANSFORM_OPERATIONS_USE_CREATE
no more ::create()s, please!  Just call the constructor

[ajuma, 2012-12-20 20:40:59]

On 2012/12/20 19:52:25, jamesr wrote:

https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc...
> cc/transform_operations.cc:12: #ifdef
WEBKIT_WEB_TRANSFORM_OPERATIONS_USE_CREATE
> no more ::create()s, please!  Just call the constructor

Once WTO is pure virtual it won't have a constructor, so 
to avoid calling create(), we'd need to do one of:
1) Temporarily expose the new implementation class WTOI in the WebKit public API
so that we can directly instantiate it here. Then once we move the
implementation into compositor_bindings, we can instantiate that instead.
2) Make cc::TransformOperations be a subclass of either WTO or WTOI (depending
on the #ifdef), and then instantiate cc::TransformOperations rather than having
a cc::TransformOperations::Create. But this seems to be going in the wrong
direction wrt making cc not depend on includes from WebKit.
3) something else?

What did you have in mind here?

[jamesr, 2012-12-20 20:47:07]

On 2012/12/20 20:40:59, ajuma wrote:
> On 2012/12/20 19:52:25, jamesr wrote:
> 
>
https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc...
> > cc/transform_operations.cc:12: #ifdef
> WEBKIT_WEB_TRANSFORM_OPERATIONS_USE_CREATE
> > no more ::create()s, please!  Just call the constructor
> 
> Once WTO is pure virtual it won't have a constructor, so 
> to avoid calling create(), we'd need to do one of:
> 1) Temporarily expose the new implementation class WTOI in the WebKit public
API
> so that we can directly instantiate it here. Then once we move the
> implementation into compositor_bindings, we can instantiate that instead.
> 2) Make cc::TransformOperations be a subclass of either WTO or WTOI (depending
> on the #ifdef), and then instantiate cc::TransformOperations rather than
having
> a cc::TransformOperations::Create. But this seems to be going in the wrong
> direction wrt making cc not depend on includes from WebKit.
> 3) something else?
> 
> What did you have in mind here?

Sorry, I didn't read comment #5 carefully enough.  That plan's a bit suboptimal.
 Two choices - one more work, one less:

(1) Don't make WTO pure virtual.  Leave it alone, but add a new virtual
interface in cc:: that is implemented in webkit/compositor_bindings/ on top of
the (non-virtual) WTO.  Port cc to depend on this new virtual interface.

(2) Make WTO pure virtual and provide a factory function off of
WebCompositorSupport that provides implementations to WebKit

If you do the plan in comment #5 you'll be really stuck trying to provide the
::create().  If you define a ::create() in the WebKit API, you have to link the
implementation of the ::create into WebKit.dll or you'll never get it compiling
on windows.  This means the implementation is also stuck inside WebKit.dll 
(meaning you can't put it in webkit/compositor_bindings/).  You can get around
this by providing a createTransformOperations(), since that's provided to
WebKit.dll via dependency injection.

[Ian Vollick, 2012-12-20 20:54:14]

On 2012/12/20 20:47:07, jamesr wrote:
> On 2012/12/20 20:40:59, ajuma wrote:
> > On 2012/12/20 19:52:25, jamesr wrote:
> > 
> >
>
https://codereview.chromium.org/11644004/diff/1011/cc/transform_operations.cc...
> > > cc/transform_operations.cc:12: #ifdef
> > WEBKIT_WEB_TRANSFORM_OPERATIONS_USE_CREATE
> > > no more ::create()s, please!  Just call the constructor
> > 
> > Once WTO is pure virtual it won't have a constructor, so 
> > to avoid calling create(), we'd need to do one of:
> > 1) Temporarily expose the new implementation class WTOI in the WebKit public
> API
> > so that we can directly instantiate it here. Then once we move the
> > implementation into compositor_bindings, we can instantiate that instead.
> > 2) Make cc::TransformOperations be a subclass of either WTO or WTOI
(depending
> > on the #ifdef), and then instantiate cc::TransformOperations rather than
> having
> > a cc::TransformOperations::Create. But this seems to be going in the wrong
> > direction wrt making cc not depend on includes from WebKit.
> > 3) something else?
> > 
> > What did you have in mind here?
> 
> Sorry, I didn't read comment #5 carefully enough.  That plan's a bit
suboptimal.
>  Two choices - one more work, one less:
> 
> (1) Don't make WTO pure virtual.  Leave it alone, but add a new virtual
> interface in cc:: that is implemented in webkit/compositor_bindings/ on top of
> the (non-virtual) WTO.  Port cc to depend on this new virtual interface.
> 
> (2) Make WTO pure virtual and provide a factory function off of
> WebCompositorSupport that provides implementations to WebKit
> 
> If you do the plan in comment #5 you'll be really stuck trying to provide the
> ::create().  If you define a ::create() in the WebKit API, you have to link
the
> implementation of the ::create into WebKit.dll or you'll never get it
compiling
> on windows.  This means the implementation is also stuck inside WebKit.dll 
> (meaning you can't put it in webkit/compositor_bindings/).  You can get around
> this by providing a createTransformOperations(), since that's provided to
> WebKit.dll via dependency injection.

I may not have described it properly, but the plan was really #2 as you describe
it here. Step 3 of the plan was to create the factory function (and then
TransformOperations could use it), and step 4 was to get WK to use the factory
function, too.

[jamesr, 2012-12-20 22:01:00]

On 2012/12/20 20:54:14, vollick wrote:
> I may not have described it properly, but the plan was really #2 as you
describe
> it here. Step 3 of the plan was to create the factory function (and then
> TransformOperations could use it), and step 4 was to get WK to use the factory
> function, too.

Very important that the factory not be WebKit::XXX::create(), or you won't be
able to get it to link at all in the middle.

Personally I don't see what the advantage of (2) is over (1).