Raphael Catolino (:raphc) Reporter
	Description • 12 years ago

      No description provided.

	Mounir Lamouri (:mounir) Assignee
	Comment 2 • 12 years ago

Comment on attachment 650609 [details] [diff] [review]
patch

Review of attachment 650609 [details] [diff] [review]:
-----------------------------------------------------------------

Looks generally good but not r+'ing because I want to figure out some details.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1058,5 @@
> +        if (!GetValueAsTime(aValue, hour, min, sec, msec)) {
> +          return false;
> +        }
> +
> +        aResultValue = msec + sec * 1000 + min * 60000 + hour * 3600000;

I believe this is more readable:
aResultValue = msec + 1000 * (sec + 60 * (min + 60 * hour));

@@ +1185,5 @@
> +        if (MOZ_DOUBLE_IS_NaN(aValue)) {
> +          break;
> +        }
> +
> +        PRInt64 timestamp = NS_floorModulo((PRInt64)aValue, 86400000);

Do we want to do the double -> PRInt64 conversion with 'round' or 'floor'? Maybe that should be more explicit.

@@ +1207,5 @@
> +        } else if (optionalField && msec % 10 == 0) {
> +          aResultString.AppendPrintf(":%05.2f", optionalField);
> +        } else if (optionalField) {
> +          aResultString.AppendPrintf(":%06.3f", optionalField);
> +        }

That entire block could be simpler and only check if msec > 99, > 9 and > 0.

@@ +1255,5 @@
> +          return NS_OK;
> +        }
> +
> +        jsval time;
> +        time.setInt32(msec + sec * 1000 + min * 60000 + hour * 3600000);

msec + 1000 * (sec + 60 * (min + 60 * hour) ?

@@ +4325,5 @@
>    switch (mType)
>    {
>      case NS_FORM_INPUT_NUMBER:
>      case NS_FORM_INPUT_DATE:
> +    case NS_FORM_INPUT_TIME:

Why is that added in that patch?

::: content/html/content/test/forms/test_valueasnumber_attribute.html
@@ +275,5 @@
> +    [ "02:12:25.006", 7945006 ],
> +    [ "02:12:25.000", 7945000 ],
> +    [ "02:12:00.000", 7920000 ],
> +    [ "00:00:00.000", 0 ],
> +    [ "00:00:00.0", 0 ],

[ "00:00:00.001", 1 ],
[ "00:00:00.01", 10 ],
[ "00:00:00.1", 100 ],

@@ +329,5 @@
> +    "84:05",
> +    "14:60",
> +    "24:60",
> +    "23:59:60",
> +    "23:59:59:1000",

"00:00:00.0000"

@@ +360,5 @@
> +    [ -27847212,          "16:15:52.788" ],
> +    [ 86399999,          "23:59:59.999" ],
> +    // Only the time part of the date is taken into account
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],

I wonder if those two values should return "00:00" or "". The specs are not clear IMO.

@@ +362,5 @@
> +    // Only the time part of the date is taken into account
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],
> +    [ -62167219200000,   "00:00" ],
> +    [ 1330473600000,     "00:00" ],

Same here.

@@ +363,5 @@
> +    [ -86400000,         "00:00" ],
> +    [ 86400000,          "00:00" ],
> +    [ -62167219200000,   "00:00" ],
> +    [ 1330473600000,     "00:00" ],
> +    [ 7243748535100,     "16:22:15.1" ],

And here.

@@ +368,5 @@
> +    // "Values must be truncated to valid times"
> +    [ 42.1234,           "00:00:00.042" ],
> +    [ 12345.1234567891,  "00:00:12.345" ],
> +    [ 1e-1,              "00:00" ],
> +    [ 1298851745010,     "00:09:05.01" ],

Here maybe too?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 11 years ago

Comment on attachment 703196 [details] [diff] [review]
.valueAsNumber


>+    case NS_FORM_INPUT_TIME:
>+      {
>+        uint32_t value = NS_floorModulo(floor(aValue), 86400000);
This could use some comment that what 86400000 is.

>+    { value: -1, result: "23:59:59.999" },
>+    { value: -86400000, result: "00:00" },
>+    { value: -86400001, result: "23:59:59.999" },
>+    { value: -56789, result: "23:59:03.211" },
Hmm, really? Could you tell where the spec say this is ok?
(I think the behavior is ok, I just couldn't find it in the spec.)

r=me assuming the spec really allows negative values.

Hmm, or do allow negative values because 'new Date(negative_value)' is ok... I guess so.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 11 years ago

Comment on attachment 703197 [details] [diff] [review]
.valueAsDate


>+      jsval rval;
>+      jsval fullYear[3];
>+      fullYear[0].setInt32(year);
>+      fullYear[1].setInt32(month-1);
You're just moving this code, but there should be a space before and after -

	Mounir Lamouri (:mounir) Assignee
	Comment 7 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #5)
> >+    { value: -1, result: "23:59:59.999" },
> >+    { value: -86400000, result: "00:00" },
> >+    { value: -86400001, result: "23:59:59.999" },
> >+    { value: -56789, result: "23:59:03.211" },
> Hmm, really? Could you tell where the spec say this is ok?
> (I think the behavior is ok, I just couldn't find it in the spec.)
> 
> r=me assuming the spec really allows negative values.
> 
> Hmm, or do allow negative values because 'new Date(negative_value)' is ok...
> I guess so.

I explicitly asked Hixie about that on IRC because I believe the spec is hard to read. He confirmed that negative values are allowed and indeed, this is because we use Date(). Though, honestly, I think the specs are a pain to read and I will send an email to have them specified more clearly. I think specifying the algorithm I used would be way more understandable for human beings.

	Mounir Lamouri (:mounir) Assignee
	Comment 8 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bc74ed9256
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f8673cfafa

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 11 years ago

Backed out for Windows and b2g build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2f0c12218b

	Mounir Lamouri (:mounir) Assignee
	Comment 10 • 11 years ago

Damn compilers that can't just do the right thing... Hopefully, this will work better.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ffc20b4545
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c62ff59f9c54

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f7ffc20b4545
https://hg.mozilla.org/mozilla-central/rev/c62ff59f9c54

	Mounir Lamouri (:mounir) Assignee
	Comment 12 • 11 years ago

Jean-Yves, shouldn't we put the entire <input type='time'> supported on Mobile in the relnotes?

	Alex Keybl [:akeybl]
	Comment 13 • 11 years ago

We'll track bug 777279 for relnotes.

	Florian Scholz (Open Web Docs)
	Comment 14 • 11 years ago

See bug 866430 for documentation.