1310078 - Implement valueAsNumber and valueAsDate for <input type=datetime-local>

Status: RESOLVED FIXED

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Comment 3

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

Comment on attachment 8822112 [details] [diff] [review]
patch, v2.

Are there tests around leap days/years. Might be worth to add, that parsing datetime works correctly Feb 29 on a leap year but doesn't on non-leap year. Also test the first and last day of leap year or so.


>diff --git a/js/public/Date.h b/js/public/Date.h
>--- a/js/public/Date.h
>+++ b/js/public/Date.h
>@@ -129,16 +129,24 @@ NewDateObject(JSContext* cx, ClippedTime
...
>+// Year is a year, month is 0-11, day is 1-based, and time is in milliseconds.
So time is time in ms since the beginning of the day, I assume

>+// The return value is a number of milliseconds since the epoch.
>+//
>+// Consistent with the MakeDate algorithm defined in ECMAScript, this value is
>+// *not* clipped!  Use JS::TimeClip if you need a clipped date.
>+JS_PUBLIC_API(double)
>+MakeDate(double year, unsigned month, unsigned day, double time);
>+
...
 
> JS_PUBLIC_API(double)
>+JS::MakeDate(double year, unsigned month, unsigned day, double time)
>+{
>+    return ::MakeDate(MakeDay(year, month, day), time);
>+}

Some js engine peer should review this, perhaps Waldo

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8822112 [details] [diff] [review]
> patch, v2.
> 
> Are there tests around leap days/years. Might be worth to add, that parsing
> datetime works correctly Feb 29 on a leap year but doesn't on non-leap year.
> Also test the first and last day of leap year or so.
> 

Sure, will add more tests.

> 
> >diff --git a/js/public/Date.h b/js/public/Date.h
> >--- a/js/public/Date.h
> >+++ b/js/public/Date.h
> >@@ -129,16 +129,24 @@ NewDateObject(JSContext* cx, ClippedTime
> ...
> >+// Year is a year, month is 0-11, day is 1-based, and time is in milliseconds.
> So time is time in ms since the beginning of the day, I assume

Actually, the time in ms is just added to the ms from year, month and day.

> 
> >+// The return value is a number of milliseconds since the epoch.
> >+//
> >+// Consistent with the MakeDate algorithm defined in ECMAScript, this value is
> >+// *not* clipped!  Use JS::TimeClip if you need a clipped date.
> >+JS_PUBLIC_API(double)
> >+MakeDate(double year, unsigned month, unsigned day, double time);
> >+
> ...
>  
> > JS_PUBLIC_API(double)
> >+JS::MakeDate(double year, unsigned month, unsigned day, double time)
> >+{
> >+    return ::MakeDate(MakeDay(year, month, day), time);
> >+}
> 
> Some js engine peer should review this, perhaps Waldo

Thanks for adding the review.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8822112 [details] [diff] [review]
patch, v2.

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

::: js/public/Date.h
@@ +139,5 @@
> +//
> +// Consistent with the MakeDate algorithm defined in ECMAScript, this value is
> +// *not* clipped!  Use JS::TimeClip if you need a clipped date.
> +JS_PUBLIC_API(double)
> +MakeDate(double year, unsigned month, unsigned day, double time);

Hmm.  MakeDate in the spec has (day, time) arguments, not (year, month, day) *or* (year, month, day, time) arguments.  So "Consistent with the MakeDate algorithm in ECMAScript" isn't really correct for this function, or for the existing one.

Given there's a wrong signature already, and introducing the right one would require also adding JS::MakeDay as well, just go with what you have.  Rather than dump that hassle on you, I'll fix JS::MakeDate and add JS::MakeDay in a separate bug.

::: js/src/jsdate.cpp
@@ +358,5 @@
>  }
>  
>  JS_PUBLIC_API(double)
> +JS::MakeDate(double year, unsigned month, unsigned day, double time)
> +{

Add

  MOZ_ASSERT(0 <= month && month <= 11);
  MOZ_ASSERT(day <= 1 && day <= 31);

to cover the two preconditions this function is documented to have.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 8822112 [details] [diff] [review]
> patch, v2.
> 
> Review of attachment 8822112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/Date.h
> @@ +139,5 @@
> > +//
> > +// Consistent with the MakeDate algorithm defined in ECMAScript, this value is
> > +// *not* clipped!  Use JS::TimeClip if you need a clipped date.
> > +JS_PUBLIC_API(double)
> > +MakeDate(double year, unsigned month, unsigned day, double time);
> 
> Hmm.  MakeDate in the spec has (day, time) arguments, not (year, month, day)
> *or* (year, month, day, time) arguments.  So "Consistent with the MakeDate
> algorithm in ECMAScript" isn't really correct for this function, or for the
> existing one.
> 
> Given there's a wrong signature already, and introducing the right one would
> require also adding JS::MakeDay as well, just go with what you have.  Rather
> than dump that hassle on you, I'll fix JS::MakeDate and add JS::MakeDay in a
> separate bug.

Doing it on a separate bug sounds good to me, thanks for it.

> 
> ::: js/src/jsdate.cpp
> @@ +358,5 @@
> >  }
> >  
> >  JS_PUBLIC_API(double)
> > +JS::MakeDate(double year, unsigned month, unsigned day, double time)
> > +{
> 
> Add
> 
>   MOZ_ASSERT(0 <= month && month <= 11);
>   MOZ_ASSERT(day <= 1 && day <= 31);
> 
> to cover the two preconditions this function is documented to have.

Sure, will do.

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #6)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> > 
> > ::: js/src/jsdate.cpp
> > @@ +358,5 @@
> > >  }
> > >  
> > >  JS_PUBLIC_API(double)
> > > +JS::MakeDate(double year, unsigned month, unsigned day, double time)
> > > +{
> > 
> > Add
> > 
> >   MOZ_ASSERT(0 <= month && month <= 11);
> >   MOZ_ASSERT(day <= 1 && day <= 31);
> > 
> > to cover the two preconditions this function is documented to have.
> 
> Sure, will do.

month is unsigned, so I'll change it to 'MOZ_ASSERT(month <= 11);'.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

added more tests and assertion, carrying r+ from smaug and waldo.

Comment 9

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0d0837169d3cfc1f91999e235428f1195600dee&group_state=expanded

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748fe8affdcf
Implement valueAsNumber and valueAsDate for <input type=datetime-local>. r=smaug,Waldo

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/748fe8affdcf