Bug 133217 – Add type-checked casts for TransformOperations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Add type-checked casts for TransformOperations, and give some C++11 love to the TransformOperation class and its subclasses.

[David Kilzer (:ddkilzer)]

Created attachment 231969 [details]
Patch v1

[WebKit Commit Bot]

Attachment 231969 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/transforms/SkewTransformOperation.h:43:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/graphics/transforms/RotateTransformOperation.h:50:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/platform/graphics/transforms/MatrixTransformOperation.h:48:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Attachment 231969 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/graphics/transforms/SkewTransformOperation.h:43:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> ERROR: Source/WebCore/platform/graphics/transforms/RotateTransformOperation.h:50:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> ERROR: Source/WebCore/platform/graphics/transforms/MatrixTransformOperation.h:48:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> Total errors found: 3 in 20 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I think the code is more readable with the explicit use of '0 in these cases.

[Simon Fraser (smfr)]

Comment on attachment 231969 [details]
Patch v1

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.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (From update of attachment 231969 [details])
> 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.

Fixed.

> > 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.

Will move this to the implementation source file (and all the other subclasses).

> > 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.

This feels like a separate patch, but I agree this would be cleaner.

> > 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.

I covered why I did this in the ChangeLog, but I don't feel strongly about it, so I'll just remove these.

[David Kilzer (:ddkilzer)]

Committed r169320: <http://trac.webkit.org/changeset/169320>