Fix matrix3d transform under page zoom

third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp

Index: third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
diff --git a/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp b/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
index af0f0c3cf40f1fca8abf92558a84da81dabc8e3c..be0cbbd9c681dc19ab1c2e9b7eafe6a02f8569e3 100644
--- a/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
+++ b/third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
@@ -90,13 +90,6 @@ inline static CSSValue* zoomAdjustedPixelValueOrAuto(
   return zoomAdjustedPixelValue(length.value(), style);
 }
 
-inline static CSSPrimitiveValue* zoomAdjustedNumberValue(
-    double value,
-    const ComputedStyle& style) {
-  return CSSPrimitiveValue::create(value / style.effectiveZoom(),
-                                   CSSPrimitiveValue::UnitType::Number);
-}
-
 static CSSValue* zoomAdjustedPixelValueForLength(const Length& length,
                                                  const ComputedStyle& style) {
   if (length.isFixed())
@@ -1356,6 +1349,7 @@ static CSSFunctionValue* valueForMatrixTransform(
     const TransformationMatrix& transform,
     const ComputedStyle& style) {
   CSSFunctionValue* transformValue = nullptr;
+  double zoomFactor = style.effectiveZoom();

[pdr., 2016/11/08 07:04:08]

I wonder if we could simplify this by moving it up into computedTransform?

Something like:

computedTransform(...) {
  ...
  TransformationMatrix matrix;
  ...
  if (style.effectiveZoom() != 1) {
    if (affine) {
      ...
    } else {
      ...
    }
  }
  then return the list
}

[Franklin Ta, 2016/11/08 07:52:18]

There are a lot of other functions in this file in the same style (valueForBlah)
that could be inlined but aren't. I think we should just stick with it? Let me
know if you feel strongly about this.

[Franklin Ta, 2016/11/08 16:47:02]

Terminology note: I just noticed you guys define TransformationMatrix::isAffine
as the affine form for 2d points but in my description when I say affine I meant
the form for 3d points (i.e., the bottom row has to be [0, 0, 0, 1]). Hope that
wasn't confusing.

   if (transform.isAffine()) {
     transformValue = CSSFunctionValue::create(CSSValueMatrix);
 
@@ -1367,8 +1361,10 @@ static CSSFunctionValue* valueForMatrixTransform(
         transform.c(), CSSPrimitiveValue::UnitType::Number));
     transformValue->append(*CSSPrimitiveValue::create(
         transform.d(), CSSPrimitiveValue::UnitType::Number));
-    transformValue->append(*zoomAdjustedNumberValue(transform.e(), style));
-    transformValue->append(*zoomAdjustedNumberValue(transform.f(), style));
+    transformValue->append(*CSSPrimitiveValue::create(
+        transform.e() / zoomFactor, CSSPrimitiveValue::UnitType::Number));
+    transformValue->append(*CSSPrimitiveValue::create(
+        transform.f() / zoomFactor, CSSPrimitiveValue::UnitType::Number));
   } else {
     transformValue = CSSFunctionValue::create(CSSValueMatrix3d);
 
@@ -1379,7 +1375,7 @@ static CSSFunctionValue* valueForMatrixTransform(
     transformValue->append(*CSSPrimitiveValue::create(
         transform.m13(), CSSPrimitiveValue::UnitType::Number));
     transformValue->append(*CSSPrimitiveValue::create(
-        transform.m14(), CSSPrimitiveValue::UnitType::Number));
+        transform.m14() * zoomFactor, CSSPrimitiveValue::UnitType::Number));
 
     transformValue->append(*CSSPrimitiveValue::create(
         transform.m21(), CSSPrimitiveValue::UnitType::Number));
@@ -1388,7 +1384,7 @@ static CSSFunctionValue* valueForMatrixTransform(
     transformValue->append(*CSSPrimitiveValue::create(
         transform.m23(), CSSPrimitiveValue::UnitType::Number));
     transformValue->append(*CSSPrimitiveValue::create(
-        transform.m24(), CSSPrimitiveValue::UnitType::Number));
+        transform.m24() * zoomFactor, CSSPrimitiveValue::UnitType::Number));
 
     transformValue->append(*CSSPrimitiveValue::create(
         transform.m31(), CSSPrimitiveValue::UnitType::Number));
@@ -1397,11 +1393,14 @@ static CSSFunctionValue* valueForMatrixTransform(
     transformValue->append(*CSSPrimitiveValue::create(
         transform.m33(), CSSPrimitiveValue::UnitType::Number));
     transformValue->append(*CSSPrimitiveValue::create(
-        transform.m34(), CSSPrimitiveValue::UnitType::Number));
+        transform.m34() * zoomFactor, CSSPrimitiveValue::UnitType::Number));
 
-    transformValue->append(*zoomAdjustedNumberValue(transform.m41(), style));
-    transformValue->append(*zoomAdjustedNumberValue(transform.m42(), style));
-    transformValue->append(*zoomAdjustedNumberValue(transform.m43(), style));
+    transformValue->append(*CSSPrimitiveValue::create(
+        transform.m41() / zoomFactor, CSSPrimitiveValue::UnitType::Number));
+    transformValue->append(*CSSPrimitiveValue::create(
+        transform.m42() / zoomFactor, CSSPrimitiveValue::UnitType::Number));
+    transformValue->append(*CSSPrimitiveValue::create(
+        transform.m43() / zoomFactor, CSSPrimitiveValue::UnitType::Number));

[alancutter (OOO until 2018), 2016/11/09 02:35:59]

I'd push the Matrix3DTransformOperation::zoom() logic up into
TransformationMatrix and then you can just call transform.zoom(1 / zoomFactor)
here instead of duplicating how zooming works in the reverse direction.

[Franklin Ta, 2016/11/09 07:12:06]

This is a great idea and tidies up everything! Done.

     transformValue->append(*CSSPrimitiveValue::create(
         transform.m44(), CSSPrimitiveValue::UnitType::Number));
   }