Closed Bug 781570 Opened 7 years ago Closed 7 years ago

implement valueAsNumber and valueAsDate for input <input type=time>

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
relnote-firefox --- -

People

(Reporter: raphc, Assigned: mounir)

References

Details

(Keywords: doc-bug-filed)

Attachments

(2 files, 1 obsolete file)

No description provided.
Blocks: 781572
Attached patch patch (obsolete) — Splinter Review
Attachment #650609 - Flags: feedback?(mounir)
Attachment #650609 - Flags: feedback?(mounir) → review?(mounir)
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?
Attachment #650609 - Flags: review?(mounir)
Keywords: dev-doc-needed
Attached patch .valueAsNumberSplinter Review
Assignee: raphael.catolino → mounir
Status: NEW → ASSIGNED
Attachment #703196 - Flags: review?(bugs)
Attached patch .valueAsDateSplinter Review
Attachment #650609 - Attachment is obsolete: true
Attachment #703197 - Flags: review?(bugs)
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.
Attachment #703196 - Flags: review?(bugs) → review+
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 -
Attachment #703197 - Flags: review?(bugs) → review+
(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.
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
Jean-Yves, shouldn't we put the entire <input type='time'> supported on Mobile in the relnotes?
We'll track bug 777279 for relnotes.
See bug 866430 for documentation.
You need to log in before you can comment on or make changes to this bug.