Quote and escape contents of url() when serializing. (Bug 478160) r+sr=bzbarsky
authorL. David Baron <dbaron@dbaron.org>
Fri, 06 Mar 2009 13:05:01 +0900
changeset 25794 1ff736fd5e41a34b244d224a2e596a058150ac0b
parent 25793 5a21d2b181e9ba1b540138ad25fc5459f8b63df7
child 25795 d5df3e584fdb9e1cee4d2b5cd0c388fa83fca4a4
push idunknown
push userunknown
push dateunknown
bugs478160
milestone1.9.2a1pre
Quote and escape contents of url() when serializing. (Bug 478160) r+sr=bzbarsky
layout/style/nsCSSDeclaration.cpp
layout/style/nsCSSRules.cpp
layout/style/nsROCSSPrimitiveValue.cpp
layout/style/nsROCSSPrimitiveValue.h
layout/style/test/Makefile.in
layout/style/test/property_database.js
layout/style/test/test_at_rule_parse_serialize.html
layout/style/test/test_bug379440.html
layout/style/test/test_bug397427.html
layout/style/test/test_parse_url.html
--- a/layout/style/nsCSSDeclaration.cpp
+++ b/layout/style/nsCSSDeclaration.cpp
@@ -392,19 +392,20 @@ nsCSSDeclaration::AppendCSSValueToString
         tmpStr.AppendFloat(nsStyleUtil::ColorComponentToFloat(a));
       }
       tmpStr.Append(PRUnichar(')'));
 
       aResult.Append(tmpStr);
     }
   }
   else if (eCSSUnit_URL == unit || eCSSUnit_Image == unit) {
-    aResult.Append(NS_LITERAL_STRING("url(") +
-                   nsDependentString(aValue.GetOriginalURLValue()) +
-                   NS_LITERAL_STRING(")"));
+    aResult.Append(NS_LITERAL_STRING("url("));
+    nsStyleUtil::AppendEscapedCSSString(
+      nsDependentString(aValue.GetOriginalURLValue()), aResult);
+    aResult.Append(NS_LITERAL_STRING(")"));
   }
   else if (eCSSUnit_Percent == unit) {
     nsAutoString tmpStr;
     tmpStr.AppendFloat(aValue.GetPercentValue() * 100.0f);
     aResult.Append(tmpStr);
   }
   else if (eCSSUnit_Percent < unit) {  // length unit
     nsAutoString tmpStr;
--- a/layout/style/nsCSSRules.cpp
+++ b/layout/style/nsCSSRules.cpp
@@ -566,17 +566,17 @@ CSSImportRuleImpl::GetType(PRUint16* aTy
   *aType = nsIDOMCSSRule::IMPORT_RULE;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 CSSImportRuleImpl::GetCssText(nsAString& aCssText)
 {
   aCssText.AssignLiteral("@import url(");
-  aCssText.Append(mURLSpec);
+  nsStyleUtil::AppendEscapedCSSString(mURLSpec, aCssText);
   aCssText.Append(NS_LITERAL_STRING(")"));
   if (mMedia) {
     nsAutoString mediaText;
     mMedia->GetText(mediaText);
     if (!mediaText.IsEmpty()) {
       aCssText.AppendLiteral(" ");
       aCssText.Append(mediaText);
     }
@@ -1157,29 +1157,28 @@ nsCSSDocumentRule::GetType(PRUint16* aTy
 
 NS_IMETHODIMP
 nsCSSDocumentRule::GetCssText(nsAString& aCssText)
 {
   aCssText.AssignLiteral("@-moz-document ");
   for (URL *url = mURLs; url; url = url->next) {
     switch (url->func) {
       case eURL:
-        aCssText.AppendLiteral("url(\"");
+        aCssText.AppendLiteral("url(");
         break;
       case eURLPrefix:
-        aCssText.AppendLiteral("url-prefix(\"");
+        aCssText.AppendLiteral("url-prefix(");
         break;
       case eDomain:
-        aCssText.AppendLiteral("domain(\"");
+        aCssText.AppendLiteral("domain(");
         break;
     }
-    nsCAutoString escapedURL(url->url);
-    escapedURL.ReplaceSubstring("\"", "\\\""); // escape quotes
-    AppendUTF8toUTF16(escapedURL, aCssText);
-    aCssText.AppendLiteral("\"), ");
+    nsStyleUtil::AppendEscapedCSSString(NS_ConvertUTF8toUTF16(url->url),
+                                        aCssText);
+    aCssText.AppendLiteral("), ");
   }
   aCssText.Cut(aCssText.Length() - 2, 1); // remove last ,
 
   return nsCSSGroupRule::AppendRulesToCssText(aCssText);
 }
 
 NS_IMETHODIMP
 nsCSSDocumentRule::SetCssText(const nsAString& aCssText)
@@ -1442,17 +1441,17 @@ CSSNameSpaceRuleImpl::GetCssText(nsAStri
   aCssText.AssignLiteral("@namespace ");
   if (mPrefix) {
     nsString atomStr;
     mPrefix->ToString(atomStr);
     aCssText.Append(atomStr);
     aCssText.AppendLiteral(" ");
   }
   aCssText.AppendLiteral("url(");
-  aCssText.Append(mURLSpec);
+  nsStyleUtil::AppendEscapedCSSString(mURLSpec, aCssText);
   aCssText.Append(NS_LITERAL_STRING(");"));
   return NS_OK;
 }
 
 NS_IMETHODIMP
 CSSNameSpaceRuleImpl::SetCssText(const nsAString& aCssText)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
--- a/layout/style/nsROCSSPrimitiveValue.cpp
+++ b/layout/style/nsROCSSPrimitiveValue.cpp
@@ -41,67 +41,30 @@
 
 #include "nsCOMPtr.h"
 #include "nsDOMError.h"
 #include "prprf.h"
 #include "nsContentUtils.h"
 #include "nsXPIDLString.h"
 #include "nsCRT.h"
 #include "nsPresContext.h"
+#include "nsStyleUtil.h"
 
 nsROCSSPrimitiveValue::nsROCSSPrimitiveValue(PRInt32 aAppUnitsPerInch)
   : mType(CSS_PX), mAppUnitsPerInch(aAppUnitsPerInch)
 {
   mValue.mAppUnits = 0;
 }
 
 
 nsROCSSPrimitiveValue::~nsROCSSPrimitiveValue()
 {
   Reset();
 }
 
-void
-nsROCSSPrimitiveValue::GetEscapedURI(nsIURI *aURI, PRUnichar **aReturn)
-{
-  nsCAutoString specUTF8;
-  aURI->GetSpec(specUTF8);
-  NS_ConvertUTF8toUTF16 spec(specUTF8);
-
-  PRUint16 length = spec.Length();
-  PRUnichar *escaped = (PRUnichar *)nsMemory::Alloc(length * 2 * sizeof(PRUnichar) + sizeof(PRUnichar('\0')));
-
-  if (escaped) {
-    PRUnichar *ptr = escaped;
-
-    for (PRUint16 i = 0; i < length; ++i) {
-      switch (spec[i]) {
-        case ' ' : // space
-        case '\t': // tab
-        case '(' : // opening parenthesis
-        case ')' : // closing parenthesis
-        case '\'': // single quote
-        case '"' : // double quote
-        case ',' : // comma
-        case '\\': // backslash
-          // We have one of the above special characters.
-          // Prepend it with a backslash.
-          *ptr++ = '\\';
-          break;
-        default:
-          break;
-      }
-      *ptr++ = spec[i];
-    }
-    *ptr = 0;
-  }
-  *aReturn = escaped;
-}
-
-
 NS_IMPL_ADDREF(nsROCSSPrimitiveValue)
 NS_IMPL_RELEASE(nsROCSSPrimitiveValue)
 
 
 // QueryInterface implementation for nsROCSSPrimitiveValue
 NS_INTERFACE_MAP_BEGIN(nsROCSSPrimitiveValue)
   NS_INTERFACE_MAP_ENTRY(nsIDOMCSSPrimitiveValue)
   NS_INTERFACE_MAP_ENTRY(nsIDOMCSSValue)
@@ -137,22 +100,24 @@ nsROCSSPrimitiveValue::GetCssText(nsAStr
     case CSS_STRING :
     case CSS_COUNTER : /* FIXME: COUNTER should use an object */
       {
         tmpStr.Append(mValue.mString);
         break;
       }
     case CSS_URI :
       {
-        nsXPIDLString uri;
         if (mValue.mURI) {
-          GetEscapedURI(mValue.mURI, getter_Copies(uri));
-          tmpStr.Assign(NS_LITERAL_STRING("url(") +
-                        uri +
-                        NS_LITERAL_STRING(")"));
+          nsCAutoString specUTF8;
+          mValue.mURI->GetSpec(specUTF8);
+
+          tmpStr.AssignLiteral("url(");
+          nsStyleUtil::AppendEscapedCSSString(NS_ConvertUTF8toUTF16(specUTF8),
+                                              tmpStr);
+          tmpStr.AppendLiteral(")");
         } else {
           // XXXldb Any better ideas?  It's good to have something that
           // doesn't parse so that things round-trip "correctly".
           tmpStr.Assign(NS_LITERAL_STRING("url(invalid-url:)"));
         }
         break;
       }
     case CSS_ATTR :
--- a/layout/style/nsROCSSPrimitiveValue.h
+++ b/layout/style/nsROCSSPrimitiveValue.h
@@ -204,18 +204,16 @@ public:
       case CSS_RGBCOLOR:
         NS_ASSERTION(mValue.mColor, "Null RGBColor should never happen");
         NS_RELEASE(mValue.mColor);
         break;
     }
   }
 
 private:
-  void GetEscapedURI(nsIURI *aURI, PRUnichar **aReturn);
-
   PRUint16 mType;
 
   union {
     nscoord         mAppUnits;
     float           mFloat;
     nsDOMCSSRGBColor* mColor;
     nsIDOMRect*     mRect;
     PRUnichar*      mString;
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -64,16 +64,17 @@ include $(topsrcdir)/config/rules.mk
 css_properties.js: host_ListCSSProperties$(HOST_BIN_SUFFIX) css_properties_like_longhand.js Makefile
 	$(RM) $@
 	./host_ListCSSProperties$(HOST_BIN_SUFFIX) > $@
 	cat $(srcdir)/css_properties_like_longhand.js >> $@
 
 GARBAGE += css_properties.js
 
 _TEST_FILES =	test_acid3_test46.html \
+		test_at_rule_parse_serialize.html \
 		test_bug73586.html \
 		test_bug74880.html \
 		test_bug98997.html \
 		test_bug160403.html \
 		test_bug221428.html \
 		test_bug229915.html \
 		test_bug302186.html \
 		test_bug319381.html \
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@@ -1189,17 +1189,26 @@ var gCSSProperties = {
 		],
 		invalid_values: [ "outside outside", "disc disc", "unknown value", "none none none", "none disc url(404.png)", "none url(404.png) disc", "disc none url(404.png)", "disc url(404.png) none", "url(404.png) none disc", "url(404.png) disc none", "none disc outside url(404.png)" ]
 	},
 	"list-style-image": {
 		domProp: "listStyleImage",
 		inherited: true,
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "none" ],
-		other_values: [ 'url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAAKElEQVR42u3NQQ0AAAgEoNP+nTWFDzcoQE1udQQCgUAgEAgEAsGTYAGjxAE/G/Q2tQAAAABJRU5ErkJggg==")' ],
+		other_values: [ 'url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAAKElEQVR42u3NQQ0AAAgEoNP+nTWFDzcoQE1udQQCgUAgEAgEAsGTYAGjxAE/G/Q2tQAAAABJRU5ErkJggg==")',
+			// Add some tests for interesting url() values here to test serialization, etc.
+			"url(\'data:text/plain,\"\')",
+			"url(\"data:text/plain,\'\")",
+			"url(\'data:text/plain,\\\'\')",
+			"url(\"data:text/plain,\\\"\")",
+			"url(\'data:text/plain,\\\"\')",
+			"url(\"data:text/plain,\\\'\")",
+			"url(data:text/plain,\\\\)",
+		],
 		invalid_values: []
 	},
 	"list-style-position": {
 		domProp: "listStylePosition",
 		inherited: true,
 		type: CSS_TYPE_LONGHAND,
 		initial_values: [ "outside" ],
 		other_values: [ "inside" ],
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_at_rule_parse_serialize.html
@@ -0,0 +1,44 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=478160
+-->
+<head>
+  <title>Test for Bug 478160</title>
+  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <style id="style" type="text/css"></style>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=478160">Mozilla Bug 478160</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 478160 **/
+
+var style_element = document.getElementById("style");
+var style_text = document.createTextNode("");
+style_element.appendChild(style_text);
+
+function test_at_rule(str) {
+    style_text.data = str;
+    is(style_element.sheet.cssRules.length, 1,
+       "should have one rule from " + str);
+    var ser1 = style_element.sheet.cssRules[0].cssText;
+    isnot(ser1, "", "should have non-empty rule from " + str);
+    style_text.data = ser1;
+    var ser2 = style_element.sheet.cssRules[0].cssText;
+    is(ser2, ser1, "parse+serialize should be idempotent for " + str);
+}
+
+test_at_rule("@namespace 'a b'");
+
+</script>
+</pre>
+</body>
+</html>
--- a/layout/style/test/test_bug379440.html
+++ b/layout/style/test/test_bug379440.html
@@ -52,20 +52,20 @@ https://bugzilla.mozilla.org/show_bug.cg
 <script class="testbody" type="text/javascript">
 
 /** Test for Bug 379440 **/
 
 function cur(id) {
   return document.defaultView.getComputedStyle($(id), "").cursor;
 }
 
-is(cur("t1"), "url(http://example.com/), crosshair",
+is(cur("t1"), "url(\"http://example.com/\"), crosshair",
    "Drop unloadable URIs");
 is(cur("t2"), "crosshair", "Drop unloadable URIs again");
-is(cur("t3"), "url(http://example.com/), crosshair", "URI + fallback");
+is(cur("t3"), "url(\"http://example.com/\"), crosshair", "URI + fallback");
 is(cur("t4"), "auto", "Must have a fallback");
 is(cur("t5"), "auto", "Fallback must be recognized");
 is(cur("t6"), "crosshair", "Just a fallback");
 is(cur("t7"), "auto", "Invalid fallback means ignore");
 
 </script>
 </pre>
 </body>
--- a/layout/style/test/test_bug397427.html
+++ b/layout/style/test/test_bug397427.html
@@ -36,27 +36,27 @@ SimpleTest.waitForExplicitFinish();
 addLoadEvent(function() {
   is($("a").sheet.href, null, "href should be null");
   is(typeof($("a").sheet.href), "object", "should be actual null");
 
   // Make sure the redirected sheets are loaded and have the right base URI
   is(document.defaultView.getComputedStyle($("one"), "").color,
      "rgb(0, 128, 0)", "Redirect 1 did not work");
   is(document.defaultView.getComputedStyle($("one"), "").backgroundImage,
-     "url(http://example.org/tests/layout/style/test/post-redirect-1.css#)",
+     "url(\"http://example.org/tests/layout/style/test/post-redirect-1.css#\")",
       "Redirect 1 did not get right base URI");
   is(document.defaultView.getComputedStyle($("two"), "").color,
      "rgb(0, 128, 0)", "Redirect 2 did not work");
   is(document.defaultView.getComputedStyle($("two"), "").backgroundImage,
-     "url(http://example.org/tests/layout/style/test/post-redirect-2.css#)",
+     "url(\"http://example.org/tests/layout/style/test/post-redirect-2.css#\")",
       "Redirect 2 did not get right base URI");
   is(document.defaultView.getComputedStyle($("three"), "").color,
      "rgb(0, 128, 0)", "Redirect 3 did not work");
   is(document.defaultView.getComputedStyle($("three"), "").backgroundImage,
-     "url(http://example.org/tests/layout/style/test/post-redirect-3.css#)",
+     "url(\"http://example.org/tests/layout/style/test/post-redirect-3.css#\")",
       "Redirect 3 did not get right base URI");
 
   var ruleList = $("a").sheet.cssRules;
   
   is(ruleList[0].styleSheet.href,
      window.location.href.replace(/test_bug397427.html$/, "redirect-1.css"),
      "Unexpected href for imported sheet");
   todo(ruleList[0].href == window.location.href.replace(/test_bug397427.html$/, "redirect-1.css"),
--- a/layout/style/test/test_parse_url.html
+++ b/layout/style/test/test_parse_url.html
@@ -17,34 +17,33 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 <pre id="test">
 <script type="application/javascript">
 
 /** Test for Bug 473914 **/
 
 var div = document.getElementById("content");
 
-// This test relies on normalization (removal of quote marks) that we're not
-// really guaranteed to continue doing in the future (and we should probably
-// actually change to add the quote marks).
+// This test relies on normalization (insertion of quote marks) that
+// we're not really guaranteed to continue doing in the future.
 div.style.listStyleImage = 'url(http://example.org/**/)';
-is(div.style.listStyleImage, 'url(http://example.org/**/)',
+is(div.style.listStyleImage, 'url("http://example.org/**/")',
    "not treated as comment");
 div.style.listStyleImage = 'url("http://example.org/**/")';
-is(div.style.listStyleImage, 'url(http://example.org/**/)',
+is(div.style.listStyleImage, 'url("http://example.org/**/")',
    "not treated as comment");
 div.style.listStyleImage = 'url(/**/foo)';
-is(div.style.listStyleImage, 'url(/**/foo)',
+is(div.style.listStyleImage, 'url("/**/foo")',
    "not treated as comment");
 div.style.listStyleImage = 'url("/**/foo")';
-is(div.style.listStyleImage, 'url(/**/foo)',
+is(div.style.listStyleImage, 'url("/**/foo")',
    "not treated as comment");
 div.style.listStyleImage = 'url(/**/)';
-is(div.style.listStyleImage, 'url(/**/)',
+is(div.style.listStyleImage, 'url("/**/")',
    "not treated as comment");
 div.style.listStyleImage = 'url("/**/")';
-is(div.style.listStyleImage, 'url(/**/)',
+is(div.style.listStyleImage, 'url("/**/")',
    "not treated as comment");
 
 </script>
 </pre>
 </body>
 </html>