Comment on attachment 650607 [details] [diff] [review] patch Review of attachment 650607 [details] [diff] [review]: ----------------------------------------------------------------- r- because the test needs some fixing but also the parsing methods needs as a few errors I pointed for the date parsing method. Could you fix those and re-ask for a review? ::: content/html/content/src/nsHTMLInputElement.cpp @@ +2841,5 @@ > } > break; > + case NS_FORM_INPUT_TIME: > + { > + if (!IsValidTime(aValue)) { if (!aValue.IsEmpty() && !IsValid(aValue)) { @@ +2861,5 @@ > +nsHTMLInputElement::GetValueAsTime(nsAString &aValue, > + PRUint32 &aHour, > + PRUint32 &aMin, > + PRUint32 &aSec, > + PRUint32 &aMsec) const nit: coding style is "Type& var", not "Type &var" @@ +2864,5 @@ > + PRUint32 &aSec, > + PRUint32 &aMsec) const > +{ > + /* > + parse time string formatted as 'hh:mm:ss.ssss'. nit: "Parse [...]." ::: content/html/content/src/nsHTMLInputElement.h @@ +589,5 @@ > > /** > + * Parse a date string of the form hh:mm:ss.ssss > + * @param the string to be parsed. > + * @return wether the is a valid date. nit: whether @@ +595,5 @@ > + */ > + bool IsValidTime(nsAString &aValue) const; > + > + /** > + * Parse a date string of the form hh:mm:ss.ssss nit: .sss @@ +597,5 @@ > + > + /** > + * Parse a date string of the form hh:mm:ss.ssss > + * @param the string to be parsed. > + * @return wether the hour, min, sec and milliseconds values associated nit: whether ::: content/html/content/test/forms/test_input_sanitization.html @@ +181,5 @@ > + invalidData = invalidDates; > + } else if (element.type == 'time') { > + validData = validTimes; > + invalidData = invalidTimes; > + } Arf, I should have seen that before. You doesn't need a specific method for date/time. Do like 'number': parse the value if not valid according to the test, return "". That way, you can just add those values to the |testData| array.

Comment on attachment 700426 [details] [diff] [review] Patch >diff --git a/content/html/content/test/forms/test_input_sanitization.html b/content/html/content/test/forms/test_input_sanitization.html >--- a/content/html/content/test/forms/test_input_sanitization.html >+++ b/content/html/content/test/forms/test_input_sanitization.html >@@ -19,18 +19,18 @@ https://bugzilla.mozilla.org/show_bug.cg > <script type="application/javascript"> > > /** Test for Bug 549475 **/ > > // We are excluding "file" because it's too different from the other types. > // And it has no sanitizing algorithm. > var inputTypes = > [ >- "text", "password", "search", "tel", "hidden", "checkbox", "radio", >- "submit", "image", "reset", "button", "email", "url", "number", "date", >+// "text", "password", "search", "tel", "hidden", "checkbox", "radio", >+// "submit", "image", "reset", "button", "email", "url", "number", "date", > "time", Forget about that change. It was to speed up tests. This is reverted locally.

Comment on attachment 700426 [details] [diff] [review] Patch IsValidTime feels too complicated. If would add a helper, something like bool StringToNumber(const nsAString& aStr, uint32_t aStart, uint32_t aLen, uint32_t* aRetVal) { MOZ_ASSERT(aStr.Length() > (aStart + aLen)); for (uint32_t i = aStart; i < (aStart + aLen; ++i)) { if (!NS_IsAsciiDigit(aStr[i])) { return false; } } nsresult rv; *aRetVal = PromiseFlatString(Substring(aStr, aStart, aLen)).ToInteger(&ec); } Then something like uint32_t h, m, s, fs; uint32_t len = aValue.Length(); NS_ENSURE_TRUE(len >= 5, false); NS_ENSURE_TRUE(SubstringToNumber(aValue, 0, 2, &h) && h >= 0 && h <= 23, false); NS_ENSURE_TRUE(aValue[2] == ':'); NS_ENSURE_TRUE(SubstringToNumber(aValue, 3, 2, &m) && m >= 0 && m <= 59, false); if (len > 5) { NS_ENSURE_TRUE(aValue[5] == ':'); NS_ENSURE_TRUE(SubstringToNumber(aValue, 6, 2, &s) && s >= 0 && s <= 59, false); ... } Or without use of macros. But anyhow, I think without the for + switch-case.

(In reply to Olli Pettay [:smaug] from comment #5) [...] > Or without use of macros. But anyhow, I think without the for + switch-case. I like this idea. However, the macros are a bad idea here because we would warn the users because a string is wrongly formatted but the string is coming from the content so there is nothing we can do about it and this is an expected situation.

Comment on attachment 700565 [details] [diff] [review] Patch v2 remove the comments in the test before pushing. And could you add tests for overlong values and values with start with some letter.

See bug 866430 for documentation.