Bug 673820 - Rewriting StringToInteger function to match behavior of Rules for parsing Integers specified in spec. r=jonas f=mounir,Ms2ger,dbaron
authorAtul Aggarwal <atulagrwl@gmail.com>
Sat, 01 Oct 2011 19:30:27 +0530
changeset 77959 423728a5c37a28f980426b4d65e72a0b86dc2b11
parent 77958 164fd1bbd06f7e1e0f501469043d53f703e3039b
child 77960 5f0f1b44e73d2e6caab5eb39ed99ea71f61634cd
push idunknown
push userunknown
push dateunknown
reviewersjonas
bugs673820
milestone10.0a1
Bug 673820 - Rewriting StringToInteger function to match behavior of Rules for parsing Integers specified in spec. r=jonas f=mounir,Ms2ger,dbaron
content/base/src/nsAttrValue.cpp
content/canvas/test/test_canvas.html
content/html/content/test/reflect.js
layout/reftests/bugs/539880-1-ref.html
--- a/content/base/src/nsAttrValue.cpp
+++ b/content/base/src/nsAttrValue.cpp
@@ -1351,80 +1351,80 @@ nsAttrValue::GetStringBuffer(const nsASt
 }
 
 PRInt32
 nsAttrValue::StringToInteger(const nsAString& aValue, bool* aStrict,
                              PRInt32* aErrorCode,
                              bool aCanBePercent,
                              bool* aIsPercent) const
 {
-  *aStrict = PR_FALSE;
+  *aStrict = true;
   *aErrorCode = NS_ERROR_ILLEGAL_VALUE;
   if (aCanBePercent) {
-    *aIsPercent = PR_FALSE;
+    *aIsPercent = false;
   }
 
   nsAString::const_iterator iter, end;
   aValue.BeginReading(iter);
   aValue.EndReading(end);
+
+  while (iter != end && nsContentUtils::IsHTMLWhitespace(*iter)) {
+    *aStrict = false;
+    ++iter;
+  }
+
+  if (iter == end) {
+    return 0;
+  }
+
   bool negate = false;
+  if (*iter == PRUnichar('-')) {
+    negate = true;
+    ++iter;
+  } else if (*iter == PRUnichar('+')) {
+    *aStrict = false;
+    ++iter;
+  }
+
   PRInt32 value = 0;
-  if (iter != end) {
-    if (*iter == PRUnichar('-')) {
-      negate = PR_TRUE;
+  PRInt32 pValue = 0; // Previous value, used to check integer overflow
+  while (iter != end) {
+    if (*iter >= PRUnichar('0') && *iter <= PRUnichar('9')) {
+      value = (value * 10) + (*iter - PRUnichar('0'));
       ++iter;
+      // Checking for integer overflow.
+      if (pValue > value) {
+        *aStrict = false;
+        *aErrorCode = NS_ERROR_ILLEGAL_VALUE;
+        break;
+      } else {
+        pValue = value;
+        *aErrorCode = NS_OK;
+      }
+    } else if (aCanBePercent && *iter == PRUnichar('%')) {
+      ++iter;
+      *aIsPercent = true;
+      if (iter != end) {
+        *aStrict = false;
+        break;
+      }
+    } else {
+      *aStrict = false;
+      break;
     }
-    if (iter != end) {
-      if ((*iter >= PRUnichar('1') || (*iter == PRUnichar('0') && !negate)) &&
-          *iter <= PRUnichar('9')) {
-        value = *iter - PRUnichar('0');
-        ++iter;
-        *aStrict = (value != 0 || iter == end ||
-                    (aCanBePercent && *iter == PRUnichar('%')));
-        while (iter != end && *aStrict) {
-          if (*iter >= PRUnichar('0') && *iter <= PRUnichar('9')) {
-            value = (value * 10) + (*iter - PRUnichar('0'));
-            ++iter;
-            if (iter != end && value > ((PR_INT32_MAX / 10) - 9)) {
-              *aStrict = PR_FALSE;
-            }
-          } else if (aCanBePercent && *iter == PRUnichar('%')) {
-            ++iter;
-            if (iter == end) {
-              *aIsPercent = PR_TRUE;
-            } else {
-              *aStrict = PR_FALSE;
-            }
-          } else {
-            *aStrict = PR_FALSE;
-          }
-        }
-        if (*aStrict) {
-          if (negate) {
-            value = -value;
-          }
-          if (!aCanBePercent || !*aIsPercent) {
-            *aErrorCode = NS_OK;
-#ifdef DEBUG
-            nsAutoString stringValue;
-            stringValue.AppendInt(value);
-            if (aCanBePercent && *aIsPercent) {
-              stringValue.AppendLiteral("%");
-            }
-            NS_ASSERTION(stringValue.Equals(aValue), "Wrong conversion!");
-#endif
-            return value;
-          }
-        }
-      }
+  }
+  if (negate) {
+    value = -value;
+    // Checking the special case of -0.
+    if (!value) {
+      *aStrict = false;
     }
   }
 
-  nsAutoString tmp(aValue);
-  return tmp.ToInteger(aErrorCode);
+  return value;
 }
 
 PRInt64
 nsAttrValue::SizeOf() const
 {
   PRInt64 size = sizeof(*this);
 
   switch (BaseType()) {
--- a/content/canvas/test/test_canvas.html
+++ b/content/canvas/test/test_canvas.html
@@ -20185,17 +20185,17 @@ ok(canvas.getAttribute('height') == 60, 
 <canvas id="c637" width="100foo" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
 <script>
 
 function test_size_attributes_parse_badsuffix() {
 
 var canvas = document.getElementById('c637');
 var ctx = canvas.getContext('2d');
 
-todo(canvas.width == 100, "canvas.width == 100");
+is(canvas.width, 100, "canvas.width == 100");
 
 
 }
 </script>
 
 <!-- [[[ test_size.attributes.parse.floatsuffix.html ]]] -->
 
 <p>Canvas test: size.attributes.parse.floatsuffix</p>
@@ -20395,17 +20395,17 @@ ok(canvas.width == 300, "canvas.width ==
 <script>
 
 function test_size_attributes_setAttribute_badsuffix() {
 
 var canvas = document.getElementById('c648');
 var ctx = canvas.getContext('2d');
 
 canvas.setAttribute('width', '100foo');
-todo(canvas.width == 100, "canvas.width == 100");
+is(canvas.width, 100, "canvas.width == 100");
 
 
 }
 </script>
 
 <!-- [[[ test_size.attributes.setAttribute.floatsuffix.html ]]] -->
 
 <p>Canvas test: size.attributes.setAttribute.floatsuffix</p>
--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
@@ -457,21 +457,16 @@ function reflectBoolean(aParameters)
  * @param aParameters   Object    object containing the parameters, which are:
  *  - element           Element   node to test on
  *  - attribute         String    name of the attribute
  *  - nonNegative       Boolean   true if the attribute is limited to 'non-negative numbers', false otherwise
  *  - defaultValue      Integer   [optional] default value, if one exists
  */
 function reflectInt(aParameters)
 {
-  //TBD: Bug 673820: .setAttribute(exponential) -> incorrect reflection for element[attr]
-  function testExponential(value) {
-    return !!/^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);
-  }
-
   // Expected value returned by .getAttribute() when |value| has been previously passed to .setAttribute().
   function expectedGetAttributeResult(value) {
     return (value !== null) ? String(value) : "";
   }
 
   function stringToInteger(value, nonNegative, defaultValue) {
     if (nonNegative === false) {
       // Parse: Ignore leading whitespace, find [+/-][numbers]
@@ -551,44 +546,20 @@ function reflectInt(aParameters)
 
     is(element.getAttribute(attr), expectedGetAttributeResult(v), element.localName + ".setAttribute(" +
       attr + ", " + v + "), " + element.localName + ".getAttribute(" + attr + ") ");
 
     if (intValue == -2147483648 && element[attr] == defaultValue) {
       //TBD: Bug 586761: .setAttribute(attr, -2147483648) --> element[attr] == defaultValue instead of -2147483648
       todo_is(element[attr], intValue, "Bug 586761: " + element.localName +
         ".setAttribute(value, " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (testExponential(v)) {
-      //TBD: Bug 673820: .setAttribute(exponential) -> incorrect reflection for element[attr]
-      todo_is(element[attr], intValue, "Bug 673820: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (v == "why 567 what") {
-      //TBD: Bug 679672: .setAttribute() is somehow able to parse "why 567 what" into "567"
-      todo_is(element[attr], intValue, "Bug 679672: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (v === "-0" && nonNegative) {
+    } else if ((v === "-0" || v == "-0xABCDEF") && nonNegative) {
       //TBD: Bug 688093: Non-negative integers should return defaultValue when attempting to reflect "-0"
       todo_is(element[attr], intValue, "Bug 688093: " + element.localName +
         ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (v == "+42foo") {
-      //TBD: Bug: Unable to correctly parse "+" character in front of string
-      todo_is(element[attr], intValue, "Bug: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (v == "0x10FFFF" && defaultValue != 0) {
-      //TBD: Bug: Integer attributes should parse "0x10FFFF" as 0, but instead incorrectly return defaultValue
-      todo_is(element[attr], intValue, "Bug: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if (v == "-0xABCDEF" && !nonNegative && defaultValue != 0) {
-      //TBD: Bug: Signed integer attributes should parse "-0xABCDEF" as -0, but instead incorrectly return defaultValue
-      todo_is(element[attr], intValue, "Bug: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
-    } else if ((v == "++2" || v == "+-2" || v == "--2" || v == "-+2") && element[attr] != defaultValue)  {
-      //TBD: Bug: Should not be able to parse strings with multiple sign characters, should return defaultValue
-      todo_is(element[attr], intValue, "Bug: " + element.localName +
-        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
     } else {
       is(element[attr], intValue, element.localName +
         ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
     }
     element.removeAttribute(attr);
 
     if (nonNegative && expectedIdlAttributeResult(v) < 0) {
       try {
--- a/layout/reftests/bugs/539880-1-ref.html
+++ b/layout/reftests/bugs/539880-1-ref.html
@@ -1,38 +1,38 @@
 <html>
 <body>
 
 <table cellpadding="10">
 	<tr>
 		<th>0</th>
 		<td><table cellpadding="4" border="0"><tr><td>border="0"</td></tr></table></td>
 		<td><table cellpadding="4" border="0"><tr><td>border="0px"</td></tr></table></td>
-		<td><table cellpadding="4" border="1"><tr><td>border="0em"</td></tr></table></td>
+		<td><table cellpadding="4" border="0"><tr><td>border="0em"</td></tr></table></td>
 	</tr>
 	<tr>
 		<th>1</th>
 		<td><table cellpadding="4" border="1"><tr><td>border="1"</td></tr></table></td>
 		<td><table cellpadding="4" border="1"><tr><td>border="1px"</td></tr></table></td>
 		<td><table cellpadding="4" border="1"><tr><td>border="1em"</td></tr></table></td>
 	</tr>
 	<tr>
 		<th>2</th>
 		<td><table cellpadding="4" border="2"><tr><td>border="2"</td></tr></table></td>
 		<td><table cellpadding="4" border="2"><tr><td>border="2px"</td></tr></table></td>
-		<td><table cellpadding="4" border="1"><tr><td>border="2em"</td></tr></table></td>
+		<td><table cellpadding="4" border="2"><tr><td>border="2em"</td></tr></table></td>
 	</tr>
 	<tr>
 		<th>3</th>
 		<td><table cellpadding="4" border="3"><tr><td>border="3"</td></tr></table></td>
 		<td><table cellpadding="4" border="3"><tr><td>border="3px"</td></tr></table></td>
-		<td><table cellpadding="4" border="1"><tr><td>border="3em"</td></tr></table></td>
+		<td><table cellpadding="4" border="3"><tr><td>border="3em"</td></tr></table></td>
 	</tr>
 	<tr>
 		<th>10</th>
 		<td><table cellpadding="4" border="10"><tr><td>border="10"</td></tr></table></td>
 		<td><table cellpadding="4" border="10"><tr><td>border="10px"</td></tr></table></td>
-		<td><table cellpadding="4" border="1"><tr><td>border="10em"</td></tr></table></td>
+		<td><table cellpadding="4" border="10"><tr><td>border="10em"</td></tr></table></td>
 	</tr>
 </table>
 
 </body>
 </html>