Adding Blink-side 3d Box and Bounds calculations to TransformOperations

Adding Blink-side 3d Box and Bounds calculations to TransformOperations



BUG=280139
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176453

[Ian Vollick, 2014-06-16 15:23:38]

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
File Source/platform/geometry/FloatBox.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
Source/platform/geometry/FloatBox.h:40: 
nit: extra ws.

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
Source/platform/geometry/FloatBoxTestHelpers.h:45: if (::fabsf(a.x() - b.x()) >
TEST_EPSILON
Why not make an inline, "approximately equal" helper for floats? (And could that
helper use relative rather than absolute error and also maybe use a static
constant float rather than a #define?)

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/I...
File Source/platform/transforms/IdentityTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/I...
Source/platform/transforms/IdentityTransformOperation.h:39: virtual bool
isCompatibleType(const TransformOperation& other) const
bikeshed: "compatible" doesn't convey enough information.
CompatibleTypeForInterpolation is more meaningful, but also a mouthful. How
about canIterpolateWith(...)?

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
File Source/platform/transforms/RotateTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:35: static bool
isZeroAxis(const RotateTransformOperation& from)
Please expose this rather than duplicating in TransformOperations.cpp.
RotateTransformOperation::hasZeroLengthAxis or something.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:37: double length =
from.x() * from.x() + from.y() * from.y() + from.z() * from.z();
If you add a FloatPoint3D RotationTransformOperations::axis() fn, you can use
FloatPoint3D's lengthSquared here (as well as its 'dot' later).

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:41: static bool
shareSameAxis(const RotateTransformOperation* from, const
RotateTransformOperation* to, double& x, double& y, double& z, double&
fromAngle, double& toAngle)
Please expose this, too.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformOperations.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:37: static inline void
blendFloat(double& from, double to, double progress)
Please switch this to the pre-existing blend fn you told me about offline.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:137: 
A general comment here about how this method works would be helpful. I.e., finds
potential extrema along the x, y and z axes and checks which of these actually
lie along the arc.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:147: if
(axis.dot(FloatPoint3D(toTransform.x(), toTransform.y(), toTransform.z())) < 0)
{
FloatPoint3D RotationTransformOperation::axis() would be nice.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:148: toDegrees *= -1;
Nit: no braces on one-liners in blink. Here and elsewhere.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:226: 
Pls add comment that explains that this is where we check if the candidates are
actually on the arc.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:245: static bool
isEmptyAxis(const RotateTransformOperation& from)
nuke.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:251: static bool
shareSameAxis(const RotateTransformOperation* from, const
RotateTransformOperation* to, double& x, double& y, double& z)
ditto.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:406: corner += FloatPoint3D(i
& 1? fromBox.width() :0.f,
nit: very inconsistent use of spaces around here.

In retrospect, this code is probably too cute. A tiny triply-nested for might be
easier to read?

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformOperationsTest.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:113: // we can go outside
of these bounds, but will always at least contain [0,1]
nit: please add a period.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:115: { 0, 1 },
nit: you've got commas at the end of the lines here (which I like a little
better) and at the end above. Can you pls make consistent?

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:119: for (size_t i = 0; i
< sizeof(testTransforms) / sizeof(testTransforms[0]); ++i) {
WTF_ARRAY_LENGTH?

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:167: // All proressions
for animations start and end at 0, 1 respectively,
nit: typo. progressions

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:168: // we can go outside
of these bounds, but will always at least contain [0,1]
period, pls.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:263: // This would have
been best as anonymous structs, but |arraysize| does not get
Does WTF_ARRAY_SIZE have the same limitation? If not, please make these anon
structs.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:272: 
nit: extra line of ws.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformationMatrix.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformationMatrix.cpp:656: point +=
FloatPoint3D(corner & 1? box.width() : 0,
Same comment about this little snippet.

[awoloszyn, 2014-06-17 14:26:01]

General cleanup from last CR.

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
File Source/platform/geometry/FloatBox.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
Source/platform/geometry/FloatBox.h:40: 
On 2014/06/16 15:23:37, Ian Vollick wrote:
> nit: extra ws.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/geometry/Flo...
Source/platform/geometry/FloatBoxTestHelpers.h:45: if (::fabsf(a.x() - b.x()) >
TEST_EPSILON
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Why not make an inline, "approximately equal" helper for floats? (And could
that
> helper use relative rather than absolute error and also maybe use a static
> constant float rather than a #define?)

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/I...
File Source/platform/transforms/IdentityTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/I...
Source/platform/transforms/IdentityTransformOperation.h:39: virtual bool
isCompatibleType(const TransformOperation& other) const
On 2014/06/16 15:23:37, Ian Vollick wrote:
> bikeshed: "compatible" doesn't convey enough information.
> CompatibleTypeForInterpolation is more meaningful, but also a mouthful. How
> about canIterpolateWith(...)?
Agreed. Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
File Source/platform/transforms/RotateTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:35: static bool
isZeroAxis(const RotateTransformOperation& from)
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Please expose this rather than duplicating in TransformOperations.cpp.
> RotateTransformOperation::hasZeroLengthAxis or something.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:37: double length =
from.x() * from.x() + from.y() * from.y() + from.z() * from.z();
On 2014/06/16 15:23:37, Ian Vollick wrote:
> If you add a FloatPoint3D RotationTransformOperations::axis() fn, you can use
> FloatPoint3D's lengthSquared here (as well as its 'dot' later).

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/R...
Source/platform/transforms/RotateTransformOperation.cpp:41: static bool
shareSameAxis(const RotateTransformOperation* from, const
RotateTransformOperation* to, double& x, double& y, double& z, double&
fromAngle, double& toAngle)
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Please expose this, too.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformOperations.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:37: static inline void
blendFloat(double& from, double to, double progress)
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Please switch this to the pre-existing blend fn you told me about offline.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:137: 
On 2014/06/16 15:23:37, Ian Vollick wrote:
> A general comment here about how this method works would be helpful. I.e.,
finds
> potential extrema along the x, y and z axes and checks which of these actually
> lie along the arc.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:147: if
(axis.dot(FloatPoint3D(toTransform.x(), toTransform.y(), toTransform.z())) < 0)
{
On 2014/06/16 15:23:37, Ian Vollick wrote:
> FloatPoint3D RotationTransformOperation::axis() would be nice.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:148: toDegrees *= -1;
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Nit: no braces on one-liners in blink. Here and elsewhere.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:226: 
On 2014/06/16 15:23:37, Ian Vollick wrote:
> Pls add comment that explains that this is where we check if the candidates
are
> actually on the arc.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:245: static bool
isEmptyAxis(const RotateTransformOperation& from)
On 2014/06/16 15:23:37, Ian Vollick wrote:
> nuke.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:251: static bool
shareSameAxis(const RotateTransformOperation* from, const
RotateTransformOperation* to, double& x, double& y, double& z)
On 2014/06/16 15:23:37, Ian Vollick wrote:
> ditto.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperations.cpp:406: corner += FloatPoint3D(i
& 1? fromBox.width() :0.f,
On 2014/06/16 15:23:38, Ian Vollick wrote:
> nit: very inconsistent use of spaces around here.
> 
> In retrospect, this code is probably too cute. A tiny triply-nested for might
be
> easier to read?

I have no problems putting in the triply nested for-loop, my only concern is
that this will end up with us having 8 levels of indentation, which might cause
some confusion, although the last 4 levels will all be very localized to these
10 lines of code, so you are right it will probably be better.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformOperationsTest.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:113: // we can go outside
of these bounds, but will always at least contain [0,1]
On 2014/06/16 15:23:38, Ian Vollick wrote:
> nit: please add a period.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:115: { 0, 1 },
On 2014/06/16 15:23:38, Ian Vollick wrote:
> nit: you've got commas at the end of the lines here (which I like a little
> better) and at the end above. Can you pls make consistent?

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:119: for (size_t i = 0; i
< sizeof(testTransforms) / sizeof(testTransforms[0]); ++i) {
On 2014/06/16 15:23:38, Ian Vollick wrote:
> WTF_ARRAY_LENGTH?

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:167: // All proressions
for animations start and end at 0, 1 respectively,
On 2014/06/16 15:23:38, Ian Vollick wrote:
> nit: typo. progressions

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:168: // we can go outside
of these bounds, but will always at least contain [0,1]
On 2014/06/16 15:23:38, Ian Vollick wrote:
> period, pls.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:263: // This would have
been best as anonymous structs, but |arraysize| does not get
On 2014/06/16 15:23:38, Ian Vollick wrote:
> Does WTF_ARRAY_SIZE have the same limitation? If not, please make these anon
> structs.

Fixed the comment, but WTF_ARRAY_SIZE has the same problem with anonymous
structs. Also added into the comment that this will no longer be the  case with
C++11, and it should just work.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformOperationsTest.cpp:272: 
On 2014/06/16 15:23:38, Ian Vollick wrote:
> nit: extra line of ws.

Done.

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
File Source/platform/transforms/TransformationMatrix.cpp (right):

https://codereview.chromium.org/328333003/diff/1/Source/platform/transforms/T...
Source/platform/transforms/TransformationMatrix.cpp:656: point +=
FloatPoint3D(corner & 1? box.width() : 0,
On 2014/06/16 15:23:38, Ian Vollick wrote:
> Same comment about this little snippet.

Done.

[Ian Vollick, 2014-06-17 15:54:56]

A billion nits for you.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/geometry...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:42: const static float
kTestEpsilon = 1e-6;
Sorry for the churn, but I would really love to hide this away in a cpp file.
Can you please find this (and the fn's below) a new home?

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/RotateTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.cpp:34: static const double
kAngleEpsilon = 1e-4;
s/kAngleEpsilon/angleEpsilon/

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.cpp:43:
return(FloatPoint3D(x(), y(), z()));
No braces around the return value, pls. Here and above.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/RotateTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.h:49: bool hasZeroAxis()
const;
nit: now that you've got ::axis() below, maybe we could just use
axis().isZero()?

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperation.h:68: virtual bool
canInterpolateWith(const TransformOperation& other) const = 0;
Ack. I think I picked the wrong color for this bikeshed. canBlendWith seems
better since we use 'blend' all over the place in blink.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperations.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:132: // the ending point, and
any  of the extrema (in each dimension) found across
nit: extra space.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:140: FloatPoint3D
axis(fromTransform.x(), fromTransform.y(), fromTransform.z());
fromTransform.axis()

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:185: FloatPoint3D center =
origin + normal*toPoint.dot(normal);
http://dev.chromium.org/blink/coding-style says space around binary operators.
Here and elsewhere.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:250:
RefPtr<TransformOperation> fromOperation = (i < fromSize) ? from.operations()[i]
: static_cast<TransformOperation*>(0);
Can you use nullptr rather than static_cast<TransformOperation*>(0)?

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:251:
RefPtr<TransformOperation> toOperation = (i < toSize) ? operations()[i]:
static_cast<TransformOperation*>(0);
nit: space before colon

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:261: if (fromOperation &&
toOperation && !fromOperation->canInterpolateWith(*toOperation.get()))
This seems fishy. Why do we not return false here?

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:376: }
Nit: extra ws

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperationsTest.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:55: bool firstTime= true;
nit: ws

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:101: 
nit: extra ws.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:140: 
nit: extra ws.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:198: // 2 * sqrt(2)
should give the same result;
nit s/;/./

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformationMatrix.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformationMatrix.cpp:658: point += FloatPoint3D(i
* box.width(),
nit: I think this would look prettier on one line :) (Here and in the other
spot)

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformationMatrix.cpp:672: }
nit: extra ws.

[awoloszyn, 2014-06-17 20:06:03]

https://codereview.chromium.org/328333003/diff/20001/Source/platform/geometry...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:42: const static float
kTestEpsilon = 1e-6;
On 2014/06/17 15:54:55, Ian Vollick wrote:
> Sorry for the churn, but I would really love to hide this away in a cpp file.
> Can you please find this (and the fn's below) a new home?

Created a .cpp file and moved all of the functionality down.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/RotateTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.cpp:34: static const double
kAngleEpsilon = 1e-4;
On 2014/06/17 15:54:55, Ian Vollick wrote:
> s/kAngleEpsilon/angleEpsilon/

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.cpp:43:
return(FloatPoint3D(x(), y(), z()));
On 2014/06/17 15:54:55, Ian Vollick wrote:
> No braces around the return value, pls. Here and above.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/RotateTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/RotateTransformOperation.h:49: bool hasZeroAxis()
const;
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: now that you've got ::axis() below, maybe we could just use
> axis().isZero()?

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperation.h:68: virtual bool
canInterpolateWith(const TransformOperation& other) const = 0;
On 2014/06/17 15:54:56, Ian Vollick wrote:
> Ack. I think I picked the wrong color for this bikeshed. canBlendWith seems
> better since we use 'blend' all over the place in blink.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperations.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:132: // the ending point, and
any  of the extrema (in each dimension) found across
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: extra space.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:140: FloatPoint3D
axis(fromTransform.x(), fromTransform.y(), fromTransform.z());
On 2014/06/17 15:54:56, Ian Vollick wrote:
> fromTransform.axis()

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:185: FloatPoint3D center =
origin + normal*toPoint.dot(normal);
On 2014/06/17 15:54:56, Ian Vollick wrote:
> http://dev.chromium.org/blink/coding-style says space around binary operators.
> Here and elsewhere.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:250:
RefPtr<TransformOperation> fromOperation = (i < fromSize) ? from.operations()[i]
: static_cast<TransformOperation*>(0);
On 2014/06/17 15:54:56, Ian Vollick wrote:
> Can you use nullptr rather than static_cast<TransformOperation*>(0)?

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:251:
RefPtr<TransformOperation> toOperation = (i < toSize) ? operations()[i]:
static_cast<TransformOperation*>(0);
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: space before colon

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:261: if (fromOperation &&
toOperation && !fromOperation->canInterpolateWith(*toOperation.get()))
Thanks for the catch. This should almost certainly be return false.
On 2014/06/17 15:54:56, Ian Vollick wrote:
> This seems fishy. Why do we not return false here?

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperations.cpp:376: }
On 2014/06/17 15:54:56, Ian Vollick wrote:
> Nit: extra ws

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformOperationsTest.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:55: bool firstTime= true;
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: ws

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:101: 
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: extra ws.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:140: 
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: extra ws.

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformOperationsTest.cpp:198: // 2 * sqrt(2)
should give the same result;
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit s/;/./

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
File Source/platform/transforms/TransformationMatrix.cpp (right):

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformationMatrix.cpp:658: point += FloatPoint3D(i
* box.width(),
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: I think this would look prettier on one line :) (Here and in the other
> spot)

Done.

https://codereview.chromium.org/328333003/diff/20001/Source/platform/transfor...
Source/platform/transforms/TransformationMatrix.cpp:672: }
On 2014/06/17 15:54:56, Ian Vollick wrote:
> nit: extra ws.

Done.

[Ian Vollick, 2014-06-18 15:38:06]

lgtm with a few more nits.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:24: 
Include guards?

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:38: }
Nit: pls add a comment labeling which ns these braces are closing.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
File Source/platform/transforms/MatrixTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
Source/platform/transforms/MatrixTransformOperation.h:50: }
nit: pls add a line of ws.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
File Source/platform/transforms/SkewTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
Source/platform/transforms/SkewTransformOperation.cpp:45:
return(isSameType(other));
no braces around return type, pls.

[awoloszyn, 2014-06-18 18:31:09]

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
File Source/platform/geometry/FloatBoxTestHelpers.h (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:24: 
On 2014/06/18 15:38:06, Ian Vollick wrote:
> Include guards?

Done.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/geometry...
Source/platform/geometry/FloatBoxTestHelpers.h:38: }
On 2014/06/18 15:38:06, Ian Vollick wrote:
> Nit: pls add a comment labeling which ns these braces are closing.

Done.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
File Source/platform/transforms/MatrixTransformOperation.h (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
Source/platform/transforms/MatrixTransformOperation.h:50: }
On 2014/06/18 15:38:06, Ian Vollick wrote:
> nit: pls add a line of ws.

Done.

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
File Source/platform/transforms/SkewTransformOperation.cpp (right):

https://codereview.chromium.org/328333003/diff/40001/Source/platform/transfor...
Source/platform/transforms/SkewTransformOperation.cpp:45:
return(isSameType(other));
On 2014/06/18 15:38:06, Ian Vollick wrote:
> no braces around return type, pls.

Done.

[awoloszyn, 2014-06-18 20:51:39]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-06-18 20:52:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/awoloszyn@chromium.org/328333003/80001

[commit-bot: I haz the power, 2014-06-18 20:57:38]

Change committed as 176453