Not for review: Move the implementation of WebTransformOperations into chromium

Not for review: Move the implementation of WebTransformOperations into chromium

This will actually need to land as multiple CLs, but posting this since seeing the overall result makes it easier to discuss the intermediate steps.

The overall result is:
1) A new class WebTransformOperationsImpl, in webkit/compositor_bindings, implements WebTransformOperations using an instance of a new class, cc::TransformOperations.
2) The WebTransformOperations interface is reduced to just the subset that WebKit needs.
3) WebCompositorSupportImpl provides createTransformOperations methods.
4) cc no longer needs to know anything about WebTransformOperations. All usage of WebTransformOperations within cc is replaced by usage of cc::TransformOperations.
5) cc::TransformOperations still uses WebKit::WebTransformationMatrix, but this can be replaced with gfx::Transform in a follow-up CL.

The plan is to get to this overall result via the following steps:

1) (chromium) Port the existing WebKit::WebTransformOperations to a new class, cc::TransformOperations. For all existing usage of WebTransformOperations inside cc, add code to use cc::TransformOperations instead, but put this new code behind #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL. In webkit/compositor_bindings, define a new class WebTransformOperationsImpl that implements WebTransformOperations using an instance of cc::TransformOperations, but put this class behind #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL. Also, define WebCompositorSupportImpl::createTransformOperations, that returns an instance of WebTransformOperationsImpl. For all existing usage of WebTransformOperations inside webkit/compositor_bindings, add code (behind #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL) to use WebTransformOperationsImpl instead, extracting the underlying cc::TransformOperations before calling into cc.

2) (WebKit) Make WebTransformOperations pure virtual, and remove methods that are not used by WebKit. Define WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL. Add a createTransformOperations method to WebCompositorSupport.

3) (chromium) Remove code behind the #else branch of all the #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL checks.

BUG=166640

[ajuma, 2013-01-08 21:15:31]

jamesr@, what do you think about the overall approach?

[jamesr, 2013-01-08 21:59:22]

On 2013/01/08 21:15:31, ajuma wrote:
> jamesr@, what do you think about the overall approach?

If I understand correctly, the desired end state is to have
cc::TransformOperations have the the only concrete implementation and have
WebKit::WebTransformOperations be a virtual table that defers down to
cc::TransformOperations.  This sounds fine.  I would suggest an alternate
approach that's fewer patches+rolls.  The thing to keep in mind is what you're
doing to the WebKit API is actually replacing the interface with a completely
different one, even though you want to use the same name
(WebTransformOperations).  Here's what I would do:

(1) In chromium, add cc::TransformOperations (just as you have here) and port
all the chromium code in cc/ to use it.  Also add
webkit/compositor_bindings/web_transform_operations_impl.h/cc as you have, but
guard it by #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL (which will be set later). 
Also patch all entry points in webkit/compositor_bindings/ so that:
  *) if that #define is not set they convert from WebKit::WebTransformOperations
to cc::TransformOperations before calling into cc
  *) if the #define is not set they just extract the underlying
cc::TransformOperations from the WebKit::WebTransformOperations


(2) In WebKit (after the patch in (1) has rolled into
Source/WebKit/chromium/DEPS), convert the WebTransformOperation interface to
pure virtual, add #define WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL 1 to the top of
WebTransformOperation, and add createTransformOperations() to the
WebKit::WebCompositorSupport interface.  No create() functions!

(3) In chromium (after the patch in (2) has rolled into DEPS), remove all the
code behind the #else branch of your #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL
checks


Three patches and all done, and I think you've written pretty much all the
pieces for this already.  One other thing to keep in mind is you can subset the
WebTransformOperations interface to only the bits that WebKit needs once you
convert the cc/ code over to using cc::TransformOperations.  For instance, you
don't need WTO::blend() ::matchTypes() or ::apply(), you don't need to be able
to copy a WTO, etc.

[jamesr, 2013-01-08 22:11:00]

On 2013/01/08 21:59:22, jamesr wrote:
> On 2013/01/08 21:15:31, ajuma wrote:
> > jamesr@, what do you think about the overall approach?
> 
> If I understand correctly, the desired end state is to have
> cc::TransformOperations have the the only concrete implementation and have
> WebKit::WebTransformOperations be a virtual table that defers down to
> cc::TransformOperations.  This sounds fine.  I would suggest an alternate
> approach that's fewer patches+rolls.  The thing to keep in mind is what you're
> doing to the WebKit API is actually replacing the interface with a completely
> different one, even though you want to use the same name
> (WebTransformOperations).  Here's what I would do:
> 
> (1) In chromium, add cc::TransformOperations (just as you have here) and port
> all the chromium code in cc/ to use it.  Also add
> webkit/compositor_bindings/web_transform_operations_impl.h/cc as you have, but
> guard it by #if WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL (which will be set later).

> Also patch all entry points in webkit/compositor_bindings/ so that:
>   *) if that #define is not set they convert from
WebKit::WebTransformOperations
> to cc::TransformOperations before calling into cc
>   *) if the #define is not set they just extract the underlying
> cc::TransformOperations from the WebKit::WebTransformOperations

Note: Another way to handle this bit would be to leave two versions of the APIs
in cc/, one that accepts cc::TransformOperations and the other (guarded by
#ifndef WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL) that accepts a non-virtual
WebTransformOperations.  Once (2) lands you can remove these versions of things
from chromium in step (3).  It's a bit of duplication, but it doesn't appear the
current WebTransformOperations API supports copying into a
cc::TransformOperations type easily.

[ajuma, 2013-01-14 14:51:28]

On 2013/01/08 22:11:00, jamesr wrote:
 > Note: Another way to handle this bit would be to leave two versions of the
APIs
> in cc/, one that accepts cc::TransformOperations and the other (guarded by
> #ifndef WEB_TRANSFORM_OPERATIONS_IS_VIRTUAL) that accepts a non-virtual
> WebTransformOperations.  Once (2) lands you can remove these versions of
things
> from chromium in step (3).  It's a bit of duplication, but it doesn't appear
the
> current WebTransformOperations API supports copying into a
> cc::TransformOperations type easily.

Updated the patch and description to follow this approach.

[jamesr, 2013-01-14 22:03:00]

You're missing several #ifdefs, but this looks reasonable.  I assume you're
still working on making this compile?

https://chromiumcodereview.appspot.com/11745018/diff/5001/cc/transform_operat...
File cc/transform_operations.cc (right):

https://chromiumcodereview.appspot.com/11745018/diff/5001/cc/transform_operat...
cc/transform_operations.cc:15: const double EPSILON = 1e-4;
use chromium style kEpsilon

it's pretty weird for this file to declare its own epsilon - why?

https://chromiumcodereview.appspot.com/11745018/diff/5001/cc/transform_operat...
cc/transform_operations.cc:32: : type(TransformOperationIdentity) {
this should be indented 4 spaces from the previous line

[ajuma, 2013-01-15 00:45:06]

On 2013/01/14 22:03:00, jamesr wrote:
> You're missing several #ifdefs, but this looks reasonable.  I assume you're
> still working on making this compile?

Note that this is the combined patch (i.e. what the world will look like after
step 3), hence no #ifdefs. The patch for step 1 (with lots of #ifdefs!) is
https://chromiumcodereview.appspot.com/11876016

[jamesr, 2013-01-15 06:16:18]

On 2013/01/15 00:45:06, ajuma wrote:
> On 2013/01/14 22:03:00, jamesr wrote:
> > You're missing several #ifdefs, but this looks reasonable.  I assume you're
> > still working on making this compile?
> 
> Note that this is the combined patch (i.e. what the world will look like after
> step 3), hence no #ifdefs. The patch for step 1 (with lots of #ifdefs!) is
> https://chromiumcodereview.appspot.com/11876016

Of course! My mistake.

[Ian Vollick, 2013-01-15 15:02:38]

https://chromiumcodereview.appspot.com/11745018/diff/5001/cc/transform_operat...
File cc/transform_operations.cc (right):

https://chromiumcodereview.appspot.com/11745018/diff/5001/cc/transform_operat...
cc/transform_operations.cc:15: const double EPSILON = 1e-4;
On 2013/01/14 22:03:00, jamesr wrote:
> use chromium style kEpsilon
> 
> it's pretty weird for this file to declare its own epsilon - why?

I used this epsilon when checking if two rotations used the same axis. The
allowable error there is pretty big, much bigger than
numeric_limits<double>::epsilon, etc. Perhaps this constant should have a more
explicit name like kAngleEpsilon?