Bug 1646336 - Don't unnecessarily lose precision in nsComputedDOMStyle. r=hiro
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 18 Jun 2020 00:42:47 +0000
changeset 536234 2069d33fce0318e442afb5111ccd2a96a2b87bf8
parent 536233 a8706acb5a4ca68ca49f5310f8d28f8d7d92f953
child 536235 d0f9d98b428c437b3fe913b4fc099c60eaf64e52
push id37517
push usermalexandru@mozilla.com
push dateThu, 18 Jun 2020 04:43:29 +0000
treeherdermozilla-central@7f0b0cbecd94 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1646336
milestone79.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1646336 - Don't unnecessarily lose precision in nsComputedDOMStyle. r=hiro We're converting to nscoord in some places unnecessarily, reducing the precision of the computed value we return. This makes some tests unnecessarily fail if we change the base of nscoord. Differential Revision: https://phabricator.services.mozilla.com/D79996
accessible/base/StyleInfo.cpp
devtools/client/accessibility/test/browser/browser_accessibility_print_to_json.js
layout/style/nsComputedDOMStyle.cpp
layout/style/nsComputedDOMStyle.h
layout/style/nsROCSSPrimitiveValue.cpp
layout/style/nsROCSSPrimitiveValue.h
layout/style/test/test_animations.html
layout/style/test/test_bug399349.html
layout/style/test/test_units_length.html
--- a/accessible/base/StyleInfo.cpp
+++ b/accessible/base/StyleInfo.cpp
@@ -48,18 +48,28 @@ void StyleInfo::TextIndent(nsAString& aV
   aValue.AppendLiteral("0px");
 }
 
 void StyleInfo::Margin(Side aSide, nsAString& aValue) {
   MOZ_ASSERT(mElement->GetPrimaryFrame(),
              " mElement->GetPrimaryFrame() needs to be valid pointer");
   aValue.Truncate();
 
-  nscoord coordVal = mElement->GetPrimaryFrame()->GetUsedMargin().Side(aSide);
-  aValue.AppendFloat(nsPresContext::AppUnitsToFloatCSSPixels(coordVal));
+  nsIFrame* frame = mElement->GetPrimaryFrame();
+
+  // This is here only to guarantee that we do the same as getComputedStyle
+  // does, so that we don't hit precision errors in tests.
+  auto& margin = frame->StyleMargin()->mMargin.Get(aSide);
+  if (margin.ConvertsToLength()) {
+    aValue.AppendFloat(margin.AsLengthPercentage().ToLengthInCSSPixels());
+  } else {
+    nscoord coordVal = frame->GetUsedMargin().Side(aSide);
+    aValue.AppendFloat(CSSPixel::FromAppUnits(coordVal));
+  }
+
   aValue.AppendLiteral("px");
 }
 
 void StyleInfo::FormatColor(const nscolor& aValue, nsString& aFormattedValue) {
   // Combine the string like rgb(R, G, B) from nscolor.
   aFormattedValue.AppendLiteral("rgb(");
   aFormattedValue.AppendInt(NS_GET_R(aValue));
   aFormattedValue.AppendLiteral(", ");
--- a/devtools/client/accessibility/test/browser/browser_accessibility_print_to_json.js
+++ b/devtools/client/accessibility/test/browser/browser_accessibility_print_to_json.js
@@ -82,17 +82,17 @@ const OOP_FRAME_DOCUMENT_SNAPSHOT = {
       nodeType: 1,
       role: "heading",
       value: "",
       actions: [],
       attributes: {
         display: "block",
         formatting: "block",
         level: "1",
-        "margin-bottom": "21.4333px",
+        "margin-bottom": "21.44px",
         "margin-left": "0px",
         "margin-right": "0px",
         "margin-top": "0px",
         tag: "h1",
         "text-align": "start",
         "text-indent": "0px",
       },
       states: ["selectable text", "opaque", "enabled", "sensitive"],
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -1226,17 +1226,17 @@ already_AddRefed<CSSValue> nsComputedDOM
   const auto& origin = StyleDisplay()->mTransformOrigin;
 
   RefPtr<nsROCSSPrimitiveValue> width = new nsROCSSPrimitiveValue;
   auto position = MaybeResolvePositionForTransform(
       origin.horizontal, origin.vertical, mInnerFrame);
   SetValueToPosition(position, valueList);
   if (!origin.depth.IsZero()) {
     RefPtr<nsROCSSPrimitiveValue> depth = new nsROCSSPrimitiveValue;
-    depth->SetAppUnits(origin.depth.ToAppUnits());
+    depth->SetPixels(origin.depth.ToCSSPixels());
     valueList->AppendCSSValue(depth.forget());
   }
   return valueList.forget();
 }
 
 /* Convert the stored representation into a list of two values and then hand
  * it back.
  */
@@ -1823,17 +1823,17 @@ already_AddRefed<CSSValue> nsComputedDOM
     if (GetLineHeightCoord(lineHeight)) {
       val->SetAppUnits(lineHeight);
       return val.forget();
     }
   }
 
   auto& lh = StyleText()->mLineHeight;
   if (lh.IsLength()) {
-    val->SetAppUnits(lh.AsLength().ToAppUnits());
+    val->SetPixels(lh.AsLength().ToCSSPixels());
   } else if (lh.IsNumber()) {
     val->SetNumber(lh.AsNumber());
   } else if (lh.IsMozBlockHeight()) {
     val->SetString("-moz-block-height");
   } else {
     MOZ_ASSERT(lh.IsNormal());
     val->SetString("normal");
   }
@@ -2020,22 +2020,37 @@ already_AddRefed<CSSValue> nsComputedDOM
   if (coord.IsAuto()) {
     if (!aResolveAuto) {
       val->SetString("auto");
       return val.forget();
     }
     coord = positionData->mOffset.Get(NS_OPPOSITE_SIDE(aSide));
     sign = -1;
   }
+  if (!coord.IsLengthPercentage()) {
+    val->SetPixels(0.0f);
+    return val.forget();
+  }
+
+  auto& lp = coord.AsLengthPercentage();
+  if (lp.ConvertsToLength()) {
+    val->SetPixels(sign * lp.ToLengthInCSSPixels());
+    return val.forget();
+  }
 
   PercentageBaseGetter baseGetter = (aSide == eSideLeft || aSide == eSideRight)
                                         ? aWidthGetter
                                         : aHeightGetter;
-
-  val->SetAppUnits(sign * StyleCoordToNSCoord(coord, baseGetter, 0, false));
+  nscoord percentageBase;
+  if (!(this->*baseGetter)(percentageBase)) {
+    val->SetPixels(0.0f);
+    return val.forget();
+  }
+  nscoord result = lp.Resolve(percentageBase);
+  val->SetAppUnits(sign * result);
   return val.forget();
 }
 
 already_AddRefed<CSSValue> nsComputedDOMStyle::GetAbsoluteOffset(
     mozilla::Side aSide) {
   const auto& offset = StylePosition()->mOffset;
   const auto& coord = offset.Get(aSide);
   const auto& oppositeCoord = offset.Get(NS_OPPOSITE_SIDE(aSide));
@@ -2268,62 +2283,35 @@ void nsComputedDOMStyle::SetValueToLengt
   SetValueToLengthPercentage(aValue, aSize.AsLengthPercentage(),
                              aClampNegativeCalc);
 }
 
 void nsComputedDOMStyle::SetValueToLengthPercentage(
     nsROCSSPrimitiveValue* aValue, const mozilla::LengthPercentage& aLength,
     bool aClampNegativeCalc) {
   if (aLength.ConvertsToLength()) {
-    nscoord result = aLength.ToLength();
+    CSSCoord length = aLength.ToLengthInCSSPixels();
     if (aClampNegativeCalc) {
-      result = std::max(result, 0);
+      length = std::max(float(length), 0.0f);
     }
-    return aValue->SetAppUnits(result);
+    return aValue->SetPixels(length);
   }
   if (aLength.ConvertsToPercentage()) {
     float result = aLength.ToPercentage();
     if (aClampNegativeCalc) {
       result = std::max(result, 0.0f);
     }
     return aValue->SetPercent(result);
   }
 
   nsAutoString result;
   Servo_LengthPercentage_ToCss(&aLength, &result);
   aValue->SetString(result);
 }
 
-nscoord nsComputedDOMStyle::StyleCoordToNSCoord(
-    const LengthPercentage& aCoord, PercentageBaseGetter aPercentageBaseGetter,
-    nscoord aDefaultValue, bool aClampNegativeCalc) {
-  MOZ_ASSERT(aPercentageBaseGetter, "Must have a percentage base getter");
-  if (aCoord.ConvertsToLength()) {
-    return aCoord.ToLength();
-  }
-  nscoord percentageBase;
-  if ((this->*aPercentageBaseGetter)(percentageBase)) {
-    nscoord result = aCoord.Resolve(percentageBase);
-    if (aClampNegativeCalc && result < 0) {
-      // It's expected that we can get a negative value here with calc().
-      // We can also get a negative value with a percentage value if
-      // percentageBase is negative; this isn't expected, but can happen
-      // when large length values overflow.
-      NS_WARNING_ASSERTION(percentageBase >= 0,
-                           "percentage base value overflowed to become "
-                           "negative for a property "
-                           "that disallows negative values");
-      result = 0;
-    }
-    return result;
-  }
-
-  return aDefaultValue;
-}
-
 bool nsComputedDOMStyle::GetCBContentWidth(nscoord& aWidth) {
   if (!mOuterFrame) {
     return false;
   }
 
   AssertFlushedPendingReflows();
 
   aWidth = mOuterFrame->GetContainingBlock()->GetContentRect().width;
--- a/layout/style/nsComputedDOMStyle.h
+++ b/layout/style/nsComputedDOMStyle.h
@@ -286,37 +286,16 @@ class nsComputedDOMStyle final : public 
                                   const LengthPercentage&,
                                   bool aClampNegativeCalc);
 
   void SetValueToMaxSize(nsROCSSPrimitiveValue* aValue, const StyleMaxSize&);
 
   void SetValueToExtremumLength(nsROCSSPrimitiveValue* aValue,
                                 StyleExtremumLength);
 
-  /**
-   * If aCoord is a eStyleUnit_Coord returns the nscoord.  If it's
-   * eStyleUnit_Percent, attempts to resolve the percentage base and returns
-   * the resulting nscoord.  If it's some other unit or a percentage base can't
-   * be determined, returns aDefaultValue.
-   */
-  nscoord StyleCoordToNSCoord(const LengthPercentage& aCoord,
-                              PercentageBaseGetter aPercentageBaseGetter,
-                              nscoord aDefaultValue, bool aClampNegativeCalc);
-  template <typename LengthPercentageLike>
-  nscoord StyleCoordToNSCoord(const LengthPercentageLike& aCoord,
-                              PercentageBaseGetter aPercentageBaseGetter,
-                              nscoord aDefaultValue, bool aClampNegativeCalc) {
-    if (aCoord.IsLengthPercentage()) {
-      return StyleCoordToNSCoord(aCoord.AsLengthPercentage(),
-                                 aPercentageBaseGetter, aDefaultValue,
-                                 aClampNegativeCalc);
-    }
-    return aDefaultValue;
-  }
-
   bool GetCBContentWidth(nscoord& aWidth);
   bool GetCBContentHeight(nscoord& aHeight);
   bool GetCBPaddingRectWidth(nscoord& aWidth);
   bool GetCBPaddingRectHeight(nscoord& aHeight);
   bool GetScrollFrameContentWidth(nscoord& aWidth);
   bool GetScrollFrameContentHeight(nscoord& aHeight);
   bool GetFrameBorderRectWidth(nscoord& aWidth);
   bool GetFrameBorderRectHeight(nscoord& aHeight);
--- a/layout/style/nsROCSSPrimitiveValue.cpp
+++ b/layout/style/nsROCSSPrimitiveValue.cpp
@@ -12,29 +12,28 @@
 #include "nsStyleUtil.h"
 #include "nsIURI.h"
 #include "nsError.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 nsROCSSPrimitiveValue::nsROCSSPrimitiveValue() : CSSValue(), mType(CSS_PX) {
-  mValue.mAppUnits = 0;
+  mValue.mFloat = 0.0f;
 }
 
 nsROCSSPrimitiveValue::~nsROCSSPrimitiveValue() { Reset(); }
 
 void nsROCSSPrimitiveValue::GetCssText(nsString& aCssText, ErrorResult& aRv) {
   nsAutoString tmpStr;
   aCssText.Truncate();
 
   switch (mType) {
     case CSS_PX: {
-      float val = nsPresContext::AppUnitsToFloatCSSPixels(mValue.mAppUnits);
-      nsStyleUtil::AppendCSSNumber(val, tmpStr);
+      nsStyleUtil::AppendCSSNumber(mValue.mFloat, tmpStr);
       tmpStr.AppendLiteral("px");
       break;
     }
     case CSS_STRING: {
       tmpStr.Append(mValue.mString);
       break;
     }
     case CSS_URI: {
@@ -136,18 +135,22 @@ void nsROCSSPrimitiveValue::SetPercent(f
 
 void nsROCSSPrimitiveValue::SetDegree(float aValue) {
   Reset();
   mValue.mFloat = aValue;
   mType = CSS_DEG;
 }
 
 void nsROCSSPrimitiveValue::SetAppUnits(nscoord aValue) {
+  SetPixels(nsPresContext::AppUnitsToFloatCSSPixels(aValue));
+}
+
+void nsROCSSPrimitiveValue::SetPixels(float aValue) {
   Reset();
-  mValue.mAppUnits = aValue;
+  mValue.mFloat = aValue;
   mType = CSS_PX;
 }
 
 void nsROCSSPrimitiveValue::SetAppUnits(float aValue) {
   SetAppUnits(NSToCoordRound(aValue));
 }
 
 void nsROCSSPrimitiveValue::SetString(const nsACString& aString) {
--- a/layout/style/nsROCSSPrimitiveValue.h
+++ b/layout/style/nsROCSSPrimitiveValue.h
@@ -56,16 +56,17 @@ class nsROCSSPrimitiveValue final : publ
   // nsROCSSPrimitiveValue
   nsROCSSPrimitiveValue();
 
   void SetNumber(float aValue);
   void SetNumber(int32_t aValue);
   void SetNumber(uint32_t aValue);
   void SetPercent(float aValue);
   void SetDegree(float aValue);
+  void SetPixels(float aValue);
   void SetAppUnits(nscoord aValue);
   void SetAppUnits(float aValue);
   void SetString(const nsACString& aString);
   void SetString(const nsAString& aString);
 
   template <size_t N>
   void SetString(const char (&aString)[N]) {
     SetString(nsLiteralCString(aString));
@@ -76,17 +77,16 @@ class nsROCSSPrimitiveValue final : publ
   void Reset();
 
   virtual ~nsROCSSPrimitiveValue();
 
  protected:
   uint16_t mType;
 
   union {
-    nscoord mAppUnits;
     float mFloat;
     int32_t mInt32;
     uint32_t mUint32;
     char16_t* mString;
     nsIURI* MOZ_OWNING_REF mURI;
   } mValue;
 };
 
--- a/layout/style/test/test_animations.html
+++ b/layout/style/test/test_animations.html
@@ -641,17 +641,17 @@ advance_clock(2000);
 is(cs.paddingTop, "20px", "kf_cascade1 at 7s");
 advance_clock(500);
 is(cs.paddingTop, "20px", "kf_cascade1 at 7.5s");
 advance_clock(500);
 is(cs.paddingTop, "25px", "kf_cascade1 at 8s");
 advance_clock(500);
 is(cs.paddingTop, "30px", "kf_cascade1 at 8.5s");
 advance_clock(10);
-is(cs.paddingTop, "60px", "kf_cascade1 at 8.51s");
+is_approx(px_to_num(cs.paddingTop), 60, 0.001, "kf_cascade1 at 8.51s");
 advance_clock(745);
 is(cs.paddingTop, "65px", "kf_cascade1 at 9.2505s");
 done_div();
 
 // Test cascading of the @keyframes rules themselves.
 new_div("animation: kf_cascade2 linear 10s");
 is(cs.marginTop, "0px", "@keyframes rule with margin-top should be ignored");
 is(cs.marginLeft, "300px", "last @keyframes rule with margin-left should win");
--- a/layout/style/test/test_bug399349.html
+++ b/layout/style/test/test_bug399349.html
@@ -52,29 +52,29 @@ var a3 = window.getComputedStyle(documen
 is(a3.width, "0.1px");
 is(a3.height, "0.3px");
 is(a3.top, "-0.1px");
 is(a3.left, "-0.3px");
 
 var a4 = window.getComputedStyle(document.getElementById("Afour"));
 is(a4.width, "100.017px");
 is(a4.height, "400.017px");
-is(a4.top, "-0.116667px");
-is(a4.left, "-0.216667px");
+is(a4.top, "-0.117px");
+is(a4.left, "-0.217px");
 
 var a5 = window.getComputedStyle(document.getElementById("Afive"));
 is(a5.width, "2345px");
 is(a5.height, "456px");
 is(a5.top, "-2123px");
 is(a5.left, "-6544px");
 
 var a6 = window.getComputedStyle(document.getElementById("Asix"));
 is(a6.width, "12px");
 is(a6.height, "37.45px");
 is(a6.top, "-23px");
-is(a6.left, "-44.45px");
+is(a6.left, "-44.4568px");
 
 </script>
 
 </script>
 </pre>
 </body>
 </html>
--- a/layout/style/test/test_units_length.html
+++ b/layout/style/test/test_units_length.html
@@ -39,17 +39,18 @@ for (var test in tests) {
   p.setAttribute("style", "margin-left: " + test);
   is(p.style.getPropertyValue("margin-left"), test,
      test + " serializes to exactly itself");
   var equiv = tests[test];
   if (equiv) {
     var cm1 = getComputedStyle(p, "").marginLeft;
     p.style.marginLeft = equiv;
     var cm2 = getComputedStyle(p, "").marginLeft;
-    is(cm1, cm2, test + " should compute to the same as " + equiv);
+
+    ok(Math.abs(parseFloat(cm1, 10) - parseFloat(cm2, 10)) <= 0.0001, test + " should compute to the same as " + equiv + ", modulo floating point math error");
   }
 }
 
 // Bug 393910
 p.setAttribute("style", "margin-left: 0");
 is(p.style.getPropertyValue("margin-left"), "0px",
   "0 serializes to 0px");