Closed
Bug 1310078
Opened 8 years ago
Closed 7 years ago
Implement valueAsNumber and valueAsDate for <input type=datetime-local>
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jessica, Assigned: jessica)
References
Details
Attachments
(1 file, 2 obsolete files)
24.68 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jjong
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8822005 -
Attachment is obsolete: true
Attachment #8822112 -
Flags: review?(bugs)
Comment 3•7 years ago
|
||
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•7 years 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 5•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
added more tests and assertion, carrying r+ from smaug and waldo.
Attachment #8822112 -
Attachment is obsolete: true
Attachment #8823530 -
Flags: review+
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0d0837169d3cfc1f91999e235428f1195600dee&group_state=expanded
Keywords: checkin-needed
Comment 10•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/748fe8affdcf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
status-firefox52:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•