[code health] merge implementations of transform matrix serialization

Refactor the two methods in ComputedStyleUtils for serializing
transformation matrix values (ValueforMatrixTransform and part of
ValueForTransformOperation) into a single method
ValueForTransformationMatrix which takes a parameter controlling
whether to force matrix3d serialization. This method uses the
implementation from ValueForTransformOperation verbatim.

Start a unit test suite for ComputedStyleUtils with tests for
ValueForTransformationMatrix testing the zoom and force_matrix3d
parameters. As such ComputedStyleUtils has been made CORE_EXPORT so
that unit tests can link to it.

This causes no behavioral change.

Change-Id: If1c0727dc80caff77b77d9611bdeb613a96601ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2133366Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Kevin Ellis <kevers@chromium.org>
Commit-Queue: George Steel <gtsteel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759278}

third_party/blink/renderer/core/css/BUILD.gn

@@ -654,6 +654,7 @@ blink_core_tests("unit_tests") {
     "parser/media_condition_test.cc",
     "parser/sizes_attribute_parser_test.cc",
     "parser/sizes_math_function_parser_test.cc",
+    "properties/computed_style_utils_test.cc",
     "properties/css_exposure_test.cc",
     "properties/css_parsing_utils_test.cc",
     "properties/css_property_ref_test.cc",

third_party/blink/renderer/core/css/properties/computed_style_utils.cc

@@ -1738,72 +1738,42 @@ CSSValue* ComputedStyleUtils::ValueForBorderRadiusCorner(
       CSSValuePair::kDropIdenticalValues);
 }
 
-CSSValue* ComputedStyleUtils::ValueForMatrixTransform(
-    const TransformationMatrix& transform_param,
-    const ComputedStyle& style) {
-  // Take TransformationMatrix by reference and then copy it because VC++
-  // doesn't guarantee alignment of function parameters.
-  TransformationMatrix transform = transform_param;
-  CSSFunctionValue* transform_value = nullptr;
-  transform.Zoom(1 / style.EffectiveZoom());
-  if (transform.IsAffine()) {
-    transform_value =
-        MakeGarbageCollected<CSSFunctionValue>(CSSValueID::kMatrix);
-
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.A(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.B(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.C(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.D(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.E(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.F(), CSSPrimitiveValue::UnitType::kNumber));
+CSSValue* ComputedStyleUtils::ValueForTransformationMatrix(
+    const TransformationMatrix& matrix,
+    float zoom,
+    bool force_matrix3d) {
+  if (matrix.IsAffine() && !force_matrix3d) {
+    auto* result = MakeGarbageCollected<CSSFunctionValue>(CSSValueID::kMatrix);
+    // CSS matrix values are returned in column-major order.
+    double values[6] = {matrix.A(), matrix.B(),  //
+                        matrix.C(), matrix.D(),  //
+                        // E and F are pixel lengths so unzoom
+                        matrix.E() / zoom, matrix.F() / zoom};
+    for (double value : values) {
+      result->Append(*CSSNumericLiteralValue::Create(
+          value, CSSPrimitiveValue::UnitType::kNumber));
+    }
+    return result;
   } else {
-    transform_value =
+    CSSFunctionValue* result =
         MakeGarbageCollected<CSSFunctionValue>(CSSValueID::kMatrix3d);
-
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M11(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M12(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M13(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M14(), CSSPrimitiveValue::UnitType::kNumber));
-
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M21(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M22(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M23(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M24(), CSSPrimitiveValue::UnitType::kNumber));
-
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M31(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M32(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M33(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M34(), CSSPrimitiveValue::UnitType::kNumber));
-
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M41(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M42(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M43(), CSSPrimitiveValue::UnitType::kNumber));
-    transform_value->Append(*CSSNumericLiteralValue::Create(
-        transform.M44(), CSSPrimitiveValue::UnitType::kNumber));
-  }
-
-  return transform_value;
+    // CSS matrix values are returned in column-major order.
+    double values[16] = {
+        // Note that the transformation matrix operates on (Length^3 * R).
+        // Each column contains 3 scalars followed by a reciprocal length
+        // (with a value in 1/px) which must be unzoomed accordingly.
+        matrix.M11(), matrix.M12(), matrix.M13(), matrix.M14() * zoom,
+        matrix.M21(), matrix.M22(), matrix.M23(), matrix.M24() * zoom,
+        matrix.M31(), matrix.M32(), matrix.M33(), matrix.M34() * zoom,
+        // Last column has 3 pixel lengths and a scalar
+        matrix.M41() / zoom, matrix.M42() / zoom, matrix.M43() / zoom,
+        matrix.M44()};
+    for (double value : values) {
+      result->Append(*CSSNumericLiteralValue::Create(
+          value, CSSPrimitiveValue::UnitType::kNumber));
+    }
+    return result;
+  }
 }
 
 // We collapse functions like translateX into translate, since we will reify
@@ -1908,39 +1878,14 @@ CSSValue* ComputedStyleUtils::ValueForTransformOperation(
     }
     case TransformOperation::kMatrix: {
       const auto& matrix = To<MatrixTransformOperation>(operation).Matrix();
-      auto* result =
-          MakeGarbageCollected<CSSFunctionValue>(CSSValueID::kMatrix);
-      // CSS matrix values are returned in column-major order.
-      double values[6] = {matrix.A(), matrix.B(),  //
-                          matrix.C(), matrix.D(),  //
-                          // E and F are pixel lengths so unzoom
-                          matrix.E() / zoom, matrix.F() / zoom};
-      for (double value : values) {
-        result->Append(*CSSNumericLiteralValue::Create(
-            value, CSSPrimitiveValue::UnitType::kNumber));
-      }
-      return result;
+      return ValueForTransformationMatrix(matrix, zoom,
+                                          /*force_matrix3d=*/false);
     }
     case TransformOperation::kMatrix3D: {
       const auto& matrix = To<Matrix3DTransformOperation>(operation).Matrix();
-      CSSFunctionValue* result =
-          MakeGarbageCollected<CSSFunctionValue>(CSSValueID::kMatrix3d);
-      // CSS matrix values are returned in column-major order.
-      double values[16] = {
-          // Note that the transformation matrix operates on (Length^3 * R).
-          // Each column contains 3 scalars followed by a reciprocal length
-          // (with a value in 1/px) which must be unzoomed accordingly.
-          matrix.M11(), matrix.M12(), matrix.M13(), matrix.M14() * zoom,
-          matrix.M21(), matrix.M22(), matrix.M23(), matrix.M24() * zoom,
-          matrix.M31(), matrix.M32(), matrix.M33(), matrix.M34() * zoom,
-          // Last column has 3 pixel lengths and a scalar
-          matrix.M41() / zoom, matrix.M42() / zoom, matrix.M43() / zoom,
-          matrix.M44()};
-      for (double value : values) {
-        result->Append(*CSSNumericLiteralValue::Create(
-            value, CSSPrimitiveValue::UnitType::kNumber));
-      }
-      return result;
+      // Force matrix3d serialization
+      return ValueForTransformationMatrix(matrix, zoom,
+                                          /*force_matrix3d=*/true);
     }
     case TransformOperation::kInterpolated:
       // TODO(816803): The computed value in this case is not fully spec'd
@@ -1984,7 +1929,8 @@ CSSValue* ComputedStyleUtils::ComputedTransform(
   // FIXME: Need to print out individual functions
   // (https://bugs.webkit.org/show_bug.cgi?id=23924)
   CSSValueList* list = CSSValueList::CreateSpaceSeparated();
-  list->Append(*ValueForMatrixTransform(transform, style));
+  list->Append(*ValueForTransformationMatrix(transform, style.EffectiveZoom(),
+                                             /*force_matrix3d=*/false));
 
   return list;
 }

third_party/blink/renderer/core/css/properties/computed_style_utils.h

@@ -24,7 +24,7 @@ class ComputedStyle;
 class StyleColor;
 class StylePropertyShorthand;
 
-class ComputedStyleUtils {
+class CORE_EXPORT ComputedStyleUtils {
   STATIC_ONLY(ComputedStyleUtils);
 
  public:
@@ -146,8 +146,15 @@ class ComputedStyleUtils {
     kDontUsePixelSnappedBox,
     kUsePixelSnappedBox,
   };
-  static CSSValue* ValueForMatrixTransform(const TransformationMatrix&,
-                                           const ComputedStyle&);
+
+  // Serializes a TransformationMatrix into a matrix() or matrix3d() transform
+  // function value. If force_matrix3d is true, it will always give a matrix3d
+  // value (for serializing a matrix3d in a transform list), otherwise it
+  // will give a matrix() where possible (for serializing matrix in transform
+  // lists or resolved transformation matrices).
+  static CSSValue* ValueForTransformationMatrix(const TransformationMatrix&,
+                                                float zoom,
+                                                bool force_matrix3d);
   static CSSValue* ValueForTransformOperation(const TransformOperation&,
                                               float zoom);
   static FloatRect ReferenceBoxForTransform(

third_party/blink/renderer/core/css/properties/computed_style_utils_test.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/css/properties/computed_style_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/googletest/src/googletest/include/gtest/gtest.h"
+
+namespace blink {
+
+TEST(ComputedStyleUtilsTest, MatrixForce3D) {
+  TransformationMatrix identity;
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(identity, 1, false)
+                ->CssText(),
+            "matrix(1, 0, 0, 1, 0, 0)");
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(identity, 1, true)
+                ->CssText(),
+            "matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)");
+}
+
+TEST(ComputedStyleUtilsTest, MatrixZoom2D) {
+  TransformationMatrix matrix(1, 2, 3, 4, 5, 6);
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(matrix, 1, false)
+                ->CssText(),
+            "matrix(1, 2, 3, 4, 5, 6)");
+  matrix.Zoom(2);
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(matrix, 2, false)
+                ->CssText(),
+            "matrix(1, 2, 3, 4, 5, 6)");
+}
+
+TEST(ComputedStyleUtilsTest, MatrixZoom3D) {
+  TransformationMatrix matrix(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+                              16);
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(matrix, 1, false)
+                ->CssText(),
+            "matrix3d(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16)");
+  matrix.Zoom(2);
+  EXPECT_EQ(ComputedStyleUtils::ValueForTransformationMatrix(matrix, 2, false)
+                ->CssText(),
+            "matrix3d(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16)");
+}
+
+}  // namespace blink

third_party/blink/web_tests/typedcssom/computedstyle/transform.html

@@ -87,6 +87,14 @@ test(function() {
   assert_equals(result.toString(), 'matrix3d(1, 0, 0, 0.01, 0, 2, 0, 0.02, 0, 0, 3, 0.03, 4, 5, 6, 1)');
 }, 'transform preserves a matrix3d value with perspective');
 
+test(function() {
+  testElement.style.transform = 'matrix3d(2, 0, 0, 0, 0, 3, 0, 0, 0, 0, 1, 0, 4, 5, 0, 1)';
+  var result = computedStyleMap.get('transform');
+  assert_equals(result.toString(), 'matrix3d(2, 0, 0, 0, 0, 3, 0, 0, 0, 0, 1, 0, 4, 5, 0, 1)',
+      'Computes as specified (matrix3d)');
+  assert_equals(getComputedStyle(testElement).transform, 'matrix(2, 0, 0, 3, 4, 5)', 'Resolves as 2D');
+}, 'transform preserved a matrix3d value which reperesents a 2d transform')
+
 </script>
 </body>
 </html>