Bug 133217

Summary: Add type-checked casts for TransformOperations
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, bunhere, cmarcelo, commit-queue, darin, dino, gyuyoung.kim, kondapallykalyan, luiz, noam, sergio, simon.fraser, thorton, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 simon.fraser: review+, simon.fraser: commit-queue-

Description David Kilzer (:ddkilzer) 2014-05-23 09:05:04 PDT 
Add type-checked casts for TransformOperations, and give some C++11 love to the TransformOperation class and its subclasses.
Comment 1 David Kilzer (:ddkilzer) 2014-05-23 09:55:35 PDT 
Created attachment 231969 [details]
Patch v1
Comment 2 WebKit Commit Bot 2014-05-23 09:57:28 PDT 
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.
Comment 3 David Kilzer (:ddkilzer) 2014-05-24 10:14:26 PDT 
(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.
Comment 4 Simon Fraser (smfr) 2014-05-24 11:34:15 PDT 
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.
Comment 5 David Kilzer (:ddkilzer) 2014-05-25 10:13:24 PDT 
(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.
Comment 6 David Kilzer (:ddkilzer) 2014-05-25 11:10:14 PDT 
Committed r169320: <http://trac.webkit.org/changeset/169320>