Last Comment Bug 781570 - implement valueAsNumber and valueAsDate for input <input type=time>
: implement valueAsNumber and valueAsDate for input <input type=time>
Status: RESOLVED FIXED
: doc-bug-filed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 781569
Blocks: 777279 781572
  Show dependency treegraph
 
Reported: 2012-08-09 10:14 PDT by Raphael Catolino (:raphc)
Modified: 2013-04-27 11:58 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch (16.02 KB, patch)
2012-08-09 10:30 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
.valueAsNumber (15.57 KB, patch)
2013-01-17 00:20 PST, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review
.valueAsDate (16.91 KB, patch)
2013-01-17 00:21 PST, Mounir Lamouri (:mounir)
bugs: review+
Details | Diff | Splinter Review

Description Raphael Catolino (:raphc) 2012-08-09 10:14:46 PDT

    
Comment 1 Raphael Catolino (:raphc) 2012-08-09 10:30:04 PDT
Created attachment 650609 [details] [diff] [review]
patch
Comment 2 Mounir Lamouri (:mounir) 2012-08-10 03:28:09 PDT
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?
Comment 3 Mounir Lamouri (:mounir) 2013-01-17 00:20:43 PST
Created attachment 703196 [details] [diff] [review]
.valueAsNumber
Comment 4 Mounir Lamouri (:mounir) 2013-01-17 00:21:10 PST
Created attachment 703197 [details] [diff] [review]
.valueAsDate
Comment 5 Olli Pettay [:smaug] (reviewing overload) 2013-01-17 03:13:17 PST
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.
Comment 6 Olli Pettay [:smaug] (reviewing overload) 2013-01-17 03:46:17 PST
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 -
Comment 7 Mounir Lamouri (:mounir) 2013-01-21 07:54:44 PST
(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.
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-01-23 04:10:05 PST
Backed out for Windows and b2g build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2f0c12218b
Comment 10 Mounir Lamouri (:mounir) 2013-01-23 09:58:58 PST
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
Comment 12 Mounir Lamouri (:mounir) 2013-02-15 10:51:34 PST
Jean-Yves, shouldn't we put the entire <input type='time'> supported on Mobile in the relnotes?
Comment 13 Alex Keybl [:akeybl] 2013-02-15 13:16:30 PST
We'll track bug 777279 for relnotes.
Comment 14 Florian Scholz [:fscholz] (MDN) 2013-04-27 11:58:16 PDT
See bug 866430 for documentation.

Note You need to log in before you can comment on or make changes to this bug.