We would like to migrate the compositor code to use a new WebTransformationMatrix object instead of WebCore TransformationMatrix. This patch is step (1), submitting the actual code for WebTransformationMatrix, but it will be un-used for now, and it will still use webcore rect/quad/point/size types until step (4). After this patch: (2) Follow up patch: CCKeyframedAnimationCurve contains usage of other transform related webcore classes. We should add functionality to WebTransformationMatrix (ideally static helpers, or in worst case more wrapper objects) so that there is no direct usage of transform functionality. And then when we're ready to do the actual migration in minimum amount of time: (3) In the compositor, replace all instances of "TransformationMatrix" with "WebTransformationMatrix". (While making this patch, I'm already doing that for proof of concept) (3a) Where possible, reduce the size of the WebTransformationMatrix API (e.g. operator *= is equivalent to .multiply()) (4) Remove the WebCore rect/quad/point/size types from WebTransformationMatrix, and get that to compile with whatever explicit conversions are needed. (5) In the compositor, replace all instances of WebCore rect/quad/point/size types with the analogous WebXXX versions. Its possible (4) and (5) are just the same step, not sure yet. Finally, after that, explicit conversions should be limited to the public compositor API, including WebLayer and GraphicsLayerChromium, I think. That should pretty much give us the isolation we want from these data types. James, please correct me if that plan is flawed or not what you intended =)
Spoke with vollick@ offline, and it became clear that we will have to also migrate the additional TransformOperation stuff, too. So that will replace step (2) in the plan described above =)
Created attachment 141277 [details] Just for reference This first patch is just for reference, it contains the entire change needed to make the switch from WebCore::TransformationMatrix to WebKit::WebTransformationMatrix. Passes all unit tests on osx. I'll submit another patch that only adds the WebTransformationMatrix class, and I will make a separate bug to add unit tests to cover this new class
Created attachment 141278 [details] Patch for review There will be style errors, but personally I think these errors should be overruled? Specifically, the single variable symbols are actually a bit useful here, and the alphabetical sorting seems to contradict the preferred WebCommon.h include at the top.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Attachment 141278 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/Platform/chromium/public/WebTransformationMatrix.h:35: Alphabetical sorting problem. [build/include_order] [4] Source/Platform/chromium/public/WebTransformationMatrix.h:37: Alphabetical sorting problem. [build/include_order] [4] Source/Platform/chromium/public/WebTransformationMatrix.h:55: The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:55: The parameter name "d" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:55: The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:139: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:141: The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:143: The parameter name "c" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:145: The parameter name "d" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:147: The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:149: The parameter name "f" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 141278 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=141278&action=review > Source/Platform/chromium/public/WebTransformationMatrix.h:38 > +#include "FloatPoint.h" you will have to modify Source/Platform/Platform.gyp/Platform.gyp and add FloatQuad and FloatPoint3D.h to the webcore_headers section to do this (reasons are a bit complex, I can explain if you are interested). it looks like these are temporary so that should be OK > Source/Platform/chromium/public/WebTransformationMatrix.h:45 > + > +class TransformationMatrix; > + nitty nit nit: probably don't need the empty lines between the namespace and class name declarations > Source/Platform/chromium/public/WebTransformationMatrix.h:58 > + // Conversions between WebKit::WebTransformationMatrix and WebCore::TransformationMatrix I think these should be inside a #if WEBKIT_IMPLEMENTATION guard and down at the end of the public section since they aren't part of the public (used by things other than WebKit implementation) interface > Source/Platform/chromium/public/WebTransformationMatrix.h:94 > + // FIXME: these map functions should not exist, should be using CCMathUtil can you move these down and put them in #if WEBKIT_IMPLEMENTATION as well? stuff that's not WEBKIT_IMPLEMENTATION will not be able to use these WebCore types, but I agree it's OK to have these here to ease the transition > Source/Platform/chromium/public/WebTransformationMatrix.h:107 > + void setM11(double f); probably don't need to name these parameters for these setters in the header >> Source/Platform/chromium/public/WebTransformationMatrix.h:139 >> + void setA(double a); > > The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] I kind of agree with these style nits - it's pretty obvious what the parameter is without the name. mind putting a newline between the m[1-4][1-4]()/setM[1-4][1-4]() block and the [a-f]()/set[A-F]() block? > Source/WebKit/chromium/public/platform/WebTransformationMatrix.h:26 > +#include "../../../../Platform/chromium/public/WebTransformationMatrix.h" We don't actually need this forwarding header, I think? > Source/WebKit/chromium/src/WebTransformationMatrix.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2012 this file would fit slightly better in Source/WebCore/platform/chromium/support/ - it depends on WebCore but it doesn't depend on anything else that's in Source/WebKit/chromium/src > Source/WebKit/chromium/src/WebTransformationMatrix.cpp:27 > +#include "platform/WebTransformationMatrix.h" <platform/WebTransformationMatrix.h> please > Source/WebKit/chromium/src/WebTransformationMatrix.cpp:30 > +#include "WTF/PassOwnPtr.h" <wtf/PassOwnPtr.h> please > Source/WebKit/chromium/src/WebTransformationMatrix.cpp:45 > + // FIXME: The overhead of allocating a new TransformationMatrix needs to go away as soon as possible. just one of these FIXME's near is probably sufficient
Cool thanks for the super quick review. Will make all those changes soon.
Comment on attachment 141278 [details] Patch for review Attachment 141278 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12672197
Created attachment 141458 [details] Addressed reviewer comments
Attachment 141458 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/platform/chromium/support/WebTransformationMatrix.cpp:29: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/Platform/chromium/public/WebTransformationMatrix.h:52: The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:52: The parameter name "d" adds no information, so it should be removed. [readability/parameter_name] [5] Source/Platform/chromium/public/WebTransformationMatrix.h:52: The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 141458 [details] Addressed reviewer comments View in context: https://bugs.webkit.org/attachment.cgi?id=141458&action=review Looks great! >>>> Source/Platform/chromium/public/WebTransformationMatrix.h:52 >>>> + WebTransformationMatrix(double a, double b, double c, double d, double e, double f); >>> >>> The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] >> >> The parameter name "d" adds no information, so it should be removed. [readability/parameter_name] [5] > > The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] I think the style guide is wrong on this, FWIW > Source/WebCore/platform/chromium/support/WebTransformationMatrix.cpp:30 > +#include <wtf/PassOwnPtr.h> is this #include being used?
(In reply to comment #11) > (From update of attachment 141458 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141458&action=review > > Looks great! > > >>>> Source/Platform/chromium/public/WebTransformationMatrix.h:52 > >>>> + WebTransformationMatrix(double a, double b, double c, double d, double e, double f); > >>> > >>> The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] > >> > >> The parameter name "d" adds no information, so it should be removed. [readability/parameter_name] [5] > > > > The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] > > I think the style guide is wrong on this, FWIW > > > Source/WebCore/platform/chromium/support/WebTransformationMatrix.cpp:30 > > +#include <wtf/PassOwnPtr.h> > > is this #include being used? Nope, its not necessary. Now I removed it for the final patch. Thanks!
Committed r116797: <http://trac.webkit.org/changeset/116797>