Add type-checked casts for TransformOperations, and give some C++11 love to the TransformOperation class and its subclasses.
Created attachment 231969 [details] Patch v1
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.
(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 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.
(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.
Committed r169320: <http://trac.webkit.org/changeset/169320>