Bug 1222461 - fix how rewriteDeclarations reports unrelated changes. r=pbrosset, a=ritu
authorTom Tromey <tromey@mozilla.com>
Fri, 06 Nov 2015 10:43:00 +0100
changeset 305490 d4159c66b1835d2c8e33c53b545bc33228608c3f
parent 305489 dd19fd9498a2bc8081113e400548cc3eeda69432
child 305491 680d8c1b67166e25be9fa539b5a3b9940433d28a
push id1001
push userraliiev@mozilla.com
push dateMon, 18 Jan 2016 19:06:03 +0000
treeherdermozilla-release@8b89261f3ac4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset, ritu
bugs1222461
milestone44.0a2
Bug 1222461 - fix how rewriteDeclarations reports unrelated changes. r=pbrosset, a=ritu
devtools/client/shared/css-parsing-utils.js
devtools/client/shared/test/unit/test_rewriteDeclarations.js
--- a/devtools/client/shared/css-parsing-utils.js
+++ b/devtools/client/shared/css-parsing-utils.js
@@ -562,24 +562,29 @@ RuleRewriter.prototype = {
    * Modify a property value to ensure it is "lexically safe" for
    * insertion into a style sheet.  This function doesn't attempt to
    * ensure that the resulting text is a valid value for the given
    * property; but rather just that inserting the text into the style
    * sheet will not cause unwanted changes to other rules or
    * declarations.
    *
    * @param {String} text The input text.  This should include the trailing ";".
-   * @return {String} Text that has been rewritten to be "lexically safe".
+   * @return {Array} An array of the form [anySanitized, text], where
+   *                 |anySanitized| is a boolean that indicates
+   *                  whether anything substantive has changed; and
+   *                  where |text| is the text that has been rewritten
+   *                  to be "lexically safe".
    */
   sanitizePropertyValue: function(text) {
     let lexer = DOMUtils.getCSSLexer(text);
 
     let result = "";
     let previousOffset = 0;
     let braceDepth = 0;
+    let anySanitized = false;
     while (true) {
       let token = lexer.nextToken();
       if (!token) {
         break;
       }
 
       if (token.tokenType === "symbol") {
         switch (token.text) {
@@ -600,26 +605,31 @@ RuleRewriter.prototype = {
             if (braceDepth < 0) {
               // Found an unmatched close bracket.
               braceDepth = 0;
               // Copy out text from |previousOffset|.
               result += text.substring(previousOffset, token.startOffset);
               // Quote the offending symbol.
               result += "\\" + token.text;
               previousOffset = token.endOffset;
+              anySanitized = true;
             }
             break;
         }
       }
     }
 
     // Copy out any remaining text, then any needed terminators.
-    result += text.substring(previousOffset, text.length) +
-      lexer.performEOFFixup("", true);
-    return result;
+    result += text.substring(previousOffset, text.length);
+    let eofFixup = lexer.performEOFFixup("", true);
+    if (eofFixup) {
+      anySanitized = true;
+      result += eofFixup;
+    }
+    return [anySanitized, result];
   },
 
   /**
    * Start at |index| and skip whitespace
    * backward in |string|.  Return the index of the first
    * non-whitespace character, or -1 if the entire string was
    * whitespace.
    * @param {String} string the input string
@@ -657,20 +667,26 @@ RuleRewriter.prototype = {
     endIndex = this.skipWhitespaceBackward(this.result, endIndex) + 1;
 
     let trailingText = this.result.substring(endIndex);
     if (termDecl.terminator) {
       // Insert the terminator just at the end of the declaration,
       // before any trailing whitespace.
       this.result = this.result.substring(0, endIndex) + termDecl.terminator +
         trailingText;
-      // The terminator includes the ";", but we don't want it in
-      // the changed value.
-      this.changedDeclarations[index] =
-        termDecl.value + termDecl.terminator.slice(0, -1);
+      // In a couple of cases, we may have had to add something to
+      // terminate the declaration, but the termination did not
+      // actually affect the property's value -- and at this spot, we
+      // only care about reporting value changes.  In particular, we
+      // might have added a plain ";", or we might have terminated a
+      // comment with "*/;".  Neither of these affect the value.
+      if (termDecl.terminator !== ";" && termDecl.terminator !== "*/;") {
+        this.changedDeclarations[index] =
+          termDecl.value + termDecl.terminator.slice(0, -1);
+      }
     }
     // If the rule generally has newlines, but this particular
     // declaration doesn't have a trailing newline, insert one now.
     // Maybe this style is too weird to bother with.
     if (this.hasNewLine && !NEWLINE_RX.test(trailingText)) {
       this.result += "\n";
     }
   },
@@ -680,18 +696,18 @@ RuleRewriter.prototype = {
    * If the property is rewritten during sanitization, make a note in
    * |changedDeclarations|.
    *
    * @param {String} text The property text.
    * @param {Number} index The index of the property.
    * @return {String} The sanitized text.
    */
   sanitizeText: function(text, index) {
-    let sanitizedText = this.sanitizePropertyValue(text);
-    if (sanitizedText !== text) {
+    let [anySanitized, sanitizedText] = this.sanitizePropertyValue(text);
+    if (anySanitized) {
       this.changedDeclarations[index] = sanitizedText;
     }
     return sanitizedText;
   },
 
   /**
    * Rename a declaration.
    *
--- a/devtools/client/shared/test/unit/test_rewriteDeclarations.js
+++ b/devtools/client/shared/test/unit/test_rewriteDeclarations.js
@@ -264,117 +264,132 @@ const TEST_DATA = [
     expected: "  c:d;  \n    e:f;"
   },
 
   // Termination insertion corner case.
   {
     desc: "enable single quote termination",
     input: "/* content: 'hi */ color: red;",
     instruction: {type: "enable", name: "content", value: true, index: 0},
-    expected: "content: 'hi'; color: red;"
+    expected: "content: 'hi'; color: red;",
+    changed: {0: "'hi'"}
   },
   // Termination insertion corner case.
   {
     desc: "create single quote termination",
     input: "content: 'hi",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "content: 'hi';color: red;"
+    expected: "content: 'hi';color: red;",
+    changed: {0: "'hi'"}
   },
 
   // Termination insertion corner case.
   {
     desc: "enable double quote termination",
     input: "/* content: \"hi */ color: red;",
     instruction: {type: "enable", name: "content", value: true, index: 0},
-    expected: "content: \"hi\"; color: red;"
+    expected: "content: \"hi\"; color: red;",
+    changed: {0: "\"hi\""}
   },
   // Termination insertion corner case.
   {
     desc: "create double quote termination",
     input: "content: \"hi",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "content: \"hi\";color: red;"
+    expected: "content: \"hi\";color: red;",
+    changed: {0: "\"hi\""}
   },
 
   // Termination insertion corner case.
   {
     desc: "enable url termination",
     input: "/* background-image: url(something.jpg */ color: red;",
     instruction: {type: "enable", name: "background-image", value: true,
                   index: 0},
-    expected: "background-image: url(something.jpg); color: red;"
+    expected: "background-image: url(something.jpg); color: red;",
+    changed: {0: "url(something.jpg)"}
   },
   // Termination insertion corner case.
   {
     desc: "create url termination",
     input: "background-image: url(something.jpg",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "background-image: url(something.jpg);color: red;"
+    expected: "background-image: url(something.jpg);color: red;",
+    changed: {0: "url(something.jpg)"}
   },
 
   // Termination insertion corner case.
   {
     desc: "enable url single quote termination",
     input: "/* background-image: url('something.jpg */ color: red;",
     instruction: {type: "enable", name: "background-image", value: true,
                   index: 0},
-    expected: "background-image: url('something.jpg'); color: red;"
+    expected: "background-image: url('something.jpg'); color: red;",
+    changed: {0: "url('something.jpg')"}
   },
   // Termination insertion corner case.
   {
     desc: "create url single quote termination",
     input: "background-image: url('something.jpg",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "background-image: url('something.jpg');color: red;"
+    expected: "background-image: url('something.jpg');color: red;",
+    changed: {0: "url('something.jpg')"}
   },
 
   // Termination insertion corner case.
   {
     desc: "create url double quote termination",
     input: "/* background-image: url(\"something.jpg */ color: red;",
     instruction: {type: "enable", name: "background-image", value: true,
                   index: 0},
-    expected: "background-image: url(\"something.jpg\"); color: red;"
+    expected: "background-image: url(\"something.jpg\"); color: red;",
+    changed: {0: "url(\"something.jpg\")"}
   },
   // Termination insertion corner case.
   {
     desc: "enable url double quote termination",
     input: "background-image: url(\"something.jpg",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "background-image: url(\"something.jpg\");color: red;"
+    expected: "background-image: url(\"something.jpg\");color: red;",
+    changed: {0: "url(\"something.jpg\")"}
   },
 
   // Termination insertion corner case.
   {
     desc: "create backslash termination",
     input: "something: \\",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "something: \\\\;color: red;"
+    expected: "something: \\\\;color: red;",
+    // The lexer rewrites the token before we see it.  However this is
+    // so obscure as to be inconsequential.
+    changed: {0: "\uFFFD\\"}
   },
 
   // Termination insertion corner case.
   {
     desc: "enable backslash single quote termination",
     input: "something: '\\",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "something: '\\\\';color: red;"
+    expected: "something: '\\\\';color: red;",
+    changed: {0: "'\\\\'"}
   },
   {
     desc: "enable backslash double quote termination",
     input: "something: \"\\",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
-    expected: "something: \"\\\\\";color: red;"
+    expected: "something: \"\\\\\";color: red;",
+    changed: {0: "\"\\\\\""}
   },
 
   // Termination insertion corner case.
   {
     desc: "enable comment termination",
     input: "something: blah /* comment ",
     instruction: {type: "create", name: "color", value: "red", priority: "",
                   index: 1},
@@ -466,13 +481,18 @@ function rewriteDeclarations(inputString
 
 function run_test() {
   let i = 0;
   for (let test of TEST_DATA) {
     ++i;
     let {changed, text} = rewriteDeclarations(test.input, test.instruction,
                                               "\t");
     equal(text, test.expected, "output for " + test.desc);
+
+    let expectChanged;
     if ("changed" in test) {
-      deepEqual(changed, test.changed, "changed result for " + test.desc);
+      expectChanged = test.changed;
+    } else {
+      expectChanged = {};
     }
+    deepEqual(changed, expectChanged, "changed result for " + test.desc);
   }
 }