Bug 1330172 part 2 - Fix serialization of property declaration with variable reference. r=heycam
authorXidorn Quan <me@upsuper.org>
Fri, 20 Jan 2017 22:35:12 +1100
changeset 377869 434b0b5d7780ec784b359c57811a741d7cfc7480
parent 377868 de759b957d75c0c681c0d02c388e9b7b1f4da63e
child 377870 c847fb6dba8eff5cb59607aa38693a745d6b3a97
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1330172
milestone53.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 1330172 part 2 - Fix serialization of property declaration with variable reference. r=heycam MozReview-Commit-ID: ATDj4lHrtR1
layout/style/Declaration.cpp
layout/style/Declaration.h
layout/style/test/test_value_storage.html
layout/style/test/test_variables.html
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -246,28 +246,32 @@ Declaration::HasProperty(nsCSSPropertyID
                                       ? mImportantData : mData;
   const nsCSSValue *val = data->ValueFor(aProperty);
   return !!val;
 }
 
 bool
 Declaration::AppendValueToString(nsCSSPropertyID aProperty,
                                  nsAString& aResult,
-                                 nsCSSValue::Serialization aSerialization) const
+                                 nsCSSValue::Serialization aSerialization,
+                                 bool* aIsTokenStream) const
 {
   MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT_no_shorthands,
              "property ID out of range");
 
   nsCSSCompressedDataBlock *data = GetPropertyIsImportantByID(aProperty)
                                       ? mImportantData : mData;
   const nsCSSValue *val = data->ValueFor(aProperty);
   if (!val) {
     return false;
   }
 
+  if (aIsTokenStream) {
+    *aIsTokenStream = val->GetUnit() == eCSSUnit_TokenStream;
+  }
   val->AppendToString(aProperty, aResult, aSerialization);
   return true;
 }
 
 static void
 AppendSingleImageLayerPositionValue(const nsCSSValue& aPositionX,
                                     const nsCSSValue& aPositionY,
                                     const nsCSSPropertyID aTable[],
@@ -531,23 +535,26 @@ Declaration::GetImageLayerPositionValue(
     aValue.Append(char16_t(','));
     aValue.Append(char16_t(' '));
   }
 }
 
 void
 Declaration::GetPropertyValueInternal(
     nsCSSPropertyID aProperty, nsAString& aValue,
-    nsCSSValue::Serialization aSerialization) const
+    nsCSSValue::Serialization aSerialization, bool* aIsTokenStream) const
 {
   aValue.Truncate(0);
+  if (aIsTokenStream) {
+    *aIsTokenStream = false;
+  }
 
   // simple properties are easy.
   if (!nsCSSProps::IsShorthand(aProperty)) {
-    AppendValueToString(aProperty, aValue, aSerialization);
+    AppendValueToString(aProperty, aValue, aSerialization, aIsTokenStream);
     return;
   }
 
   // DOM Level 2 Style says (when describing CSS2Properties, although
   // not CSSStyleDeclaration.getPropertyValue):
   //   However, if there is no shorthand declaration that could be added
   //   to the ruleset without changing in any way the rules already
   //   declared in the ruleset (i.e., by adding longhand rules that were
@@ -635,16 +642,19 @@ Declaration::GetPropertyValueInternal(
       unsetCount != 0 || nonMatchingTokenStreamCount != 0) {
     // Case (2): partially initial, inherit, unset or token stream.
     return;
   }
   if (tokenStream) {
     if (matchingTokenStreamCount == totalCount) {
       // Shorthand was specified using variable references and all of its
       // longhand components were set by the shorthand.
+      if (aIsTokenStream) {
+        *aIsTokenStream = true;
+      }
       aValue.Append(tokenStream->GetTokenStreamValue()->mTokenStream);
     } else {
       // In all other cases, serialize to the empty string.
     }
     return;
   }
 
   nsCSSCompressedDataBlock *data = importantCount ? mImportantData : mData;
@@ -1600,31 +1610,39 @@ Declaration::GetPropertyIsImportantByID(
       return false;
     }
   }
   return true;
 }
 
 void
 Declaration::AppendPropertyAndValueToString(nsCSSPropertyID aProperty,
+                                            nsAString& aResult,
                                             nsAutoString& aValue,
-                                            nsAString& aResult) const
+                                            bool aValueIsTokenStream) const
 {
   MOZ_ASSERT(0 <= aProperty && aProperty < eCSSProperty_COUNT,
              "property enum out of range");
   MOZ_ASSERT((aProperty < eCSSProperty_COUNT_no_shorthands) == aValue.IsEmpty(),
              "aValue should be given for shorthands but not longhands");
   AppendASCIItoUTF16(nsCSSProps::GetStringValue(aProperty), aResult);
-  aResult.AppendLiteral(": ");
-  if (aValue.IsEmpty())
-    AppendValueToString(aProperty, aResult, nsCSSValue::eNormalized);
-  else
-    aResult.Append(aValue);
+  if (aValue.IsEmpty()) {
+    AppendValueToString(aProperty, aValue,
+                        nsCSSValue::eNormalized, &aValueIsTokenStream);
+  }
+  aResult.Append(':');
+  if (!aValueIsTokenStream) {
+    aResult.Append(' ');
+  }
+  aResult.Append(aValue);
   if (GetPropertyIsImportantByID(aProperty)) {
-    aResult.AppendLiteral(" !important");
+    if (!aValueIsTokenStream) {
+      aResult.Append(' ');
+    }
+    aResult.AppendLiteral("!important");
   }
   aResult.AppendLiteral("; ");
 }
 
 void
 Declaration::AppendVariableAndValueToString(const nsAString& aName,
                                             nsAString& aResult) const
 {
@@ -1733,42 +1751,47 @@ Declaration::ToString(nsAString& aString
     for (const nsCSSPropertyID *shorthands =
            nsCSSProps::ShorthandsContaining(property);
          *shorthands != eCSSProperty_UNKNOWN; ++shorthands) {
       // ShorthandsContaining returns the shorthands in order from those
       // that contain the most subproperties to those that contain the
       // least, which is exactly the order we want to test them.
       nsCSSPropertyID shorthand = *shorthands;
 
-      GetPropertyValueByID(shorthand, value);
+      bool isTokenStream;
+      GetPropertyValueInternal(shorthand, value,
+                               nsCSSValue::eNormalized, &isTokenStream);
 
       // in the system font case, skip over font-variant shorthand, since all
       // subproperties are already dealt with via the font shorthand
       if (shorthand == eCSSProperty_font_variant &&
           value.EqualsLiteral("-moz-use-system-font")) {
         continue;
       }
 
-      // If GetPropertyValueByID gives us a non-empty string back, we can
+      // If GetPropertyValueInternal gives us a non-empty string back, we can
       // use that value; otherwise it's not possible to use this shorthand.
       if (!value.IsEmpty()) {
-        AppendPropertyAndValueToString(shorthand, value, aString);
+        AppendPropertyAndValueToString(shorthand, aString,
+                                       value, isTokenStream);
         shorthandsUsed.AppendElement(shorthand);
         doneProperty = true;
         break;
       }
 
       if (shorthand == eCSSProperty_font) {
         if (haveSystemFont && !didSystemFont) {
           // Output the shorthand font declaration that we will
           // partially override later.  But don't add it to
           // |shorthandsUsed|, since we will have to override it.
           systemFont->AppendToString(eCSSProperty__x_system_font, value,
                                      nsCSSValue::eNormalized);
-          AppendPropertyAndValueToString(eCSSProperty_font, value, aString);
+          isTokenStream = systemFont->GetUnit() == eCSSUnit_TokenStream;
+          AppendPropertyAndValueToString(eCSSProperty_font, aString,
+                                         value, isTokenStream);
           value.Truncate();
           didSystemFont = true;
         }
 
         // That we output the system font is enough for this property if:
         //   (1) it's the hidden system font subproperty (which either
         //       means we output it or we don't have it), or
         //   (2) its value is the hidden system font value and it matches
@@ -1781,17 +1804,17 @@ Declaration::ToString(nsAString& aString
           break;
         }
       }
     }
     if (doneProperty)
       continue;
 
     MOZ_ASSERT(value.IsEmpty(), "value should be empty now");
-    AppendPropertyAndValueToString(property, value, aString);
+    AppendPropertyAndValueToString(property, aString, value, false);
   }
   if (! aString.IsEmpty()) {
     // if the string is not empty, we have trailing whitespace we
     // should remove
     aString.Truncate(aString.Length() - 1);
   }
 }
 
--- a/layout/style/Declaration.h
+++ b/layout/style/Declaration.h
@@ -307,29 +307,31 @@ public:
     return nullptr;
   }
 
 private:
   Declaration& operator=(const Declaration& aCopy) = delete;
   bool operator==(const Declaration& aCopy) const = delete;
 
   void GetPropertyValueInternal(nsCSSPropertyID aProperty, nsAString& aValue,
-                                nsCSSValue::Serialization aValueSerialization)
-    const;
+                                nsCSSValue::Serialization aValueSerialization,
+                                bool* aIsTokenStream = nullptr) const;
   bool GetPropertyIsImportantByID(nsCSSPropertyID aProperty) const;
 
   static void AppendImportanceToString(bool aIsImportant, nsAString& aString);
   // return whether there was a value in |aValue| (i.e., it had a non-null unit)
   bool AppendValueToString(nsCSSPropertyID aProperty, nsAString& aResult) const;
   bool AppendValueToString(nsCSSPropertyID aProperty, nsAString& aResult,
-                           nsCSSValue::Serialization aValueSerialization) const;
+                           nsCSSValue::Serialization aValueSerialization,
+                           bool* aIsTokenStream = nullptr) const;
   // Helper for ToString with strange semantics regarding aValue.
   void AppendPropertyAndValueToString(nsCSSPropertyID aProperty,
+                                      nsAString& aResult,
                                       nsAutoString& aValue,
-                                      nsAString& aResult) const;
+                                      bool aValueIsTokenStream) const;
   // helper for ToString that serializes a custom property declaration for
   // a variable with the specified name
   void AppendVariableAndValueToString(const nsAString& aName,
                                       nsAString& aResult) const;
 
   void GetImageLayerValue(nsCSSCompressedDataBlock *data,
                           nsAString& aValue,
                           nsCSSValue::Serialization aSerialization,
--- a/layout/style/test/test_value_storage.html
+++ b/layout/style/test/test_value_storage.html
@@ -150,16 +150,17 @@ function test_property(property)
       is(gDeclaration.getPropertyValue(sh), "",
          "setting '" + value + "' on '" + property + "' (for shorthand '" + sh + "')");
     }
   }
 
   function test_value(value, resolved_value) {
     var value_has_variable_reference = resolved_value != null;
 
+    var colon = value == "var(--a)" ? ":" : ": ";
     gDeclaration.setProperty(property, value, "");
 
     var idx;
 
     var step1val = gDeclaration.getPropertyValue(property);
     var step1vals = [];
     var step1ser = gDeclaration.cssText;
     if ("subproperties" in info)
@@ -188,30 +189,30 @@ function test_property(property)
         }
       }
 
     // We don't care particularly about the whitespace or the placement of
     // semicolons, but for simplicity we'll test the current behavior.
     var expected_serialization = "";
     if (step1val != "") {
       if ("alias_for" in info) {
-        expected_serialization = info.alias_for + ": " + step1val + ";";
+        expected_serialization = info.alias_for + colon + step1val + ";";
       } else {
-        expected_serialization = property + ": " + step1val + ";";
+        expected_serialization = property + colon + step1val + ";";
       }
     }
     is(step1ser, expected_serialization,
        "serialization should match property value");
 
     gDeclaration.removeProperty(property);
     gDeclaration.setProperty(property, step1val, "");
 
     is(gDeclaration.getPropertyValue(property), step1val,
        "parse+serialize should be idempotent for '" +
-         property + ": " + value + "'");
+         property + colon + value + "'");
     if (info.type != CSS_TYPE_TRUE_SHORTHAND) {
       is(gComputedStyle.getPropertyValue(property), step1comp,
          "serialize+parse should be identity transform for '" +
          property + ": " + value + "'");
     }
 
     if ("subproperties" in info &&
         // Using setProperty over subproperties is not sufficient for
@@ -232,17 +233,17 @@ function test_property(property)
       for (idx in info.subproperties) {
         var subprop = info.subproperties[idx];
         is(gComputedStyle.getPropertyValue(subprop), step1comps[idx],
            "serialize(" + subprop + ")+parse should be the identity " +
            "transform for '" + property + ": " + value + "'");
       }
       is(gDeclaration.getPropertyValue(property), step1val,
          "parse+split+serialize should be idempotent for '" +
-         property + ": " + value + "'");
+         property + colon + value + "'");
     }
 
     if (info.type != CSS_TYPE_TRUE_SHORTHAND &&
           property != "mask") {
       gDeclaration.removeProperty(property);
       gDeclaration.setProperty(property, step1comp, "");
       var func = xfail_compute(property, value) ? todo_is : is;
       func(gComputedStyle.getPropertyValue(property), step1comp,
--- a/layout/style/test/test_variables.html
+++ b/layout/style/test/test_variables.html
@@ -97,19 +97,19 @@ var tests = [
     test6.style.color = "white";
     is(declaration.getPropertyValue("-x-system-font"), " var(--var6) hangul mongolian");
   },
 
   function() {
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1154356
     var test7 = document.getElementById("test7");
     test7.textContent = "p { --weird\\;name: green; }";
-    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name:  green;");
+    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name: green;");
     test7.textContent = "p { --0: green; }";
-    is(test7.sheet.cssRules[0].style.cssText, "--0:  green;");
+    is(test7.sheet.cssRules[0].style.cssText, "--0: green;");
   },
 
   function() {
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1330172
     var test8 = document.getElementById("test8");
     test8.textContent = "p { --a:inHerit; }";
     is(test8.sheet.cssRules[0].style.cssText, "--a: inherit;");
     test8.textContent = "p { --b: initial!important; }";