[webkit-reviews] review granted: [Bug 133217] Add type-checked casts for TransformOperations : [Attachment 231969] Patch v1

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted David Kilzer
(:ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 133217: Add type-checked casts for TransformOperations
https://bugs.webkit.org/show_bug.cgi?id=133217

Attachment 231969: Patch v1
https://bugs.webkit.org/attachment.cgi?id=231969&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231969&action=review


> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:180
> +	   value.setX(transformOp ?
narrowPrecisionToFloat(toScaleTransformOperation(transformOp)->x()) : 1);
> +	   value.setY(transformOp ?
narrowPrecisionToFloat(toScaleTransformOperation(transformOp)->y()) : 1);
> +	   value.setZ(transformOp ?
narrowPrecisionToFloat(toScaleTransformOperation(transformOp)->z()) : 1);

Would be better to have a local var for the casted transform op.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:186
> +	   value.setX(transformOp ?
narrowPrecisionToFloat(toTranslateTransformOperation(transformOp)->x(size)) :
0);
> +	   value.setY(transformOp ?
narrowPrecisionToFloat(toTranslateTransformOperation(transformOp)->y(size)) :
0);
> +	   value.setZ(transformOp ?
narrowPrecisionToFloat(toTranslateTransformOperation(transformOp)->z(size)) :
0);

Ditto.

> Source/WebCore/platform/graphics/transforms/Matrix3DTransformOperation.h:74
> +inline bool Matrix3DTransformOperation::operator==(const TransformOperation&
o) const
> +{
> +    if (!isSameType(o))
> +	   return false;
> +    const Matrix3DTransformOperation& m = toMatrix3DTransformOperation(o);
> +    return m_matrix == m.m_matrix;
> +}

This is virtual so I'm not sure there's any point making it inline.

>
Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp
:191
> +	       encoder << toScaleTransformOperation(operation)->x();
> +	       encoder << toScaleTransformOperation(operation)->y();
> +	       encoder << toScaleTransformOperation(operation)->z();

Would be bit cleaner to have encoders for each subclass here I think, but this
is OK for now.

>
Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp
:205
> +	   case TransformOperation::ROTATE_Z:

ROTATE==ROTATE_Z if you look at TransformOperation.h Maybe just add a comment
next to ROTATE.

>
Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp
:281
> +	   case TransformOperation::ROTATE_Z:

ROTATE==ROTATE_Z if you look at TransformOperation.h Maybe just add a comment
next to ROTATE.