781569 - implement the value sanitizing algorithm for <input type=time>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 2

[Mounir Lamouri (:mounir)]

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 3

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 4

[Mounir Lamouri (:mounir)]

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 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Mounir Lamouri (:mounir)]

(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 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6de45a6a33df

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6de45a6a33df

Comment 11

[Florian Scholz (Open Web Docs)]

See bug 866430 for documentation.