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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → jjong
(Assignee)

Comment 2

a year ago
Created attachment 8822112 [details] [diff] [review]
patch, v2.
Attachment #8822005 - Attachment is obsolete: true
Attachment #8822112 - Flags: review?(bugs)
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
Attachment #8822112 - Flags: review?(jwalden+bmo)
Attachment #8822112 - Flags: review?(bugs)
Attachment #8822112 - Flags: review+
(Assignee)

Comment 4

a year ago
(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 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.
Attachment #8822112 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Comment 7

a year ago
(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);'.
(Assignee)

Comment 8

a year ago
Created attachment 8823530 [details] [diff] [review]
patch, v3.

added more tests and assertion, carrying r+ from smaug and waldo.
Attachment #8822112 - Attachment is obsolete: true
Attachment #8823530 - Flags: review+

Comment 10

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/748fe8affdcf
Implement valueAsNumber and valueAsDate for <input type=datetime-local>. r=smaug,Waldo
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/748fe8affdcf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.