1278186 - Implement valueAsNumber and valueAsDate for <input type=week>

Status: RESOLVED FIXED

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

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

Comment on attachment 8789730 [details] [diff] [review]
patch, v1.


>+        double year = JS::YearFromTime(aValue.toDouble());
>+        double month = JS::MonthFromTime(aValue.toDouble());
>+        double day = JS::DayFromTime(aValue.toDouble());
>+        double yearDay = JS::DayWithinYear(aValue.toDouble(), year) + 1;
 + 1 could use some explanation.

Variable names could perhaps indicate which date format they are using. ISO or something else.

>+
>+        uint32_t weekDay = DayOfWeek(year, month + 1, day);
>+        weekDay = (weekDay == SUNDAY) ? 7 : weekDay;
Also this, that JS starts weeks from sunday, but indexing them as 0.


>+        // Target on Wednesday since ISO 8601 states that week 1 is the week
>+        // with the first Thursday of that year.
>+        uint32_t week = (yearDay - weekDay + 10) / 7;
Took a bit time to understand how this works :)
Hopefully we have tests for all the cases when 1. day is any week day.
And when last day is any week day. Both including leap years.


(The Shire calendar system Hobbits have is so much easier. Every year every day of month is the say week day, and there are just couple of extra days which don't belong to any week. And on leap year one extra such day.  :)
 https://en.wikipedia.org/wiki/Middle-earth_calendar#Shire_calendar  )




>+HTMLInputElement::DaysSinceEpochFromWeek(uint32_t aYear, uint32_t aWeek) const
>+{
>+  double days = JS::DayFromYear(aYear) + (aWeek - 1) * 7;
>+  uint32_t dayOne = DayOfWeek(aYear, 1, 1);
>+  dayOne = (dayOne == SUNDAY) ? 7 : dayOne;
Hmm, would it make sense to pass some enum param to DayOfWeek to indicate what should be the first day and what should be the index of it
It is error prone to check everywhere == SUNDAY ... then do something special

>+function checkWeekGet()
...

>+function checkWeekSet()
Could you possibly add tests in both these functions testing years start and ending with all different week days and also in leap years.
So I guess totally 28 tests. (I assume writing such tests could be automated)

>+function checkWeekGet()
...
>+function checkWeekSet()
I wouldn't mind also these having such tests.

>+      ok(caught, "valueAsNumber should have trhown");
thrown


So, I'd like to see some more tests (and could you perhaps check that other browsers behave the same way) and possibly a new enum param to DayOfWeek indicating what kind of value should be returned (in that case WeekDay enum might become useless and confusing)




>+++ b/js/public/Date.h
>@@ -144,11 +144,21 @@ YearFromTime(double time);
> JS_PUBLIC_API(double)
> MonthFromTime(double time);
> 
> // Takes an integer number of milliseconds since the epoch and returns the
> // day (1-based).  Can return NaN, and will do so if NaN is passed in.
> JS_PUBLIC_API(double)
> DayFromTime(double time);
> 
>+// Takes an integer year and returns the number of days from epoch to the given
>+// year.
>+JS_PUBLIC_API(double)
>+DayFromYear(double year);
>+
>+// Takes an integer number of milliseconds since the epoch and an integer year,
>+// returns the number of days in that year.
>+JS_PUBLIC_API(double)
>+DayWithinYear(double time, double year);
>+
> } // namespace JS
> 
> #endif /* js_Date_h */
>diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp
>--- a/js/src/jsdate.cpp
>+++ b/js/src/jsdate.cpp
>@@ -370,16 +370,28 @@ JS::MonthFromTime(double time)
> }
> 
> JS_PUBLIC_API(double)
> JS::DayFromTime(double time)
> {
>     return DateFromTime(time);
> }
> 
>+JS_PUBLIC_API(double)
>+JS::DayFromYear(double year)
>+{
>+    return ::DayFromYear(year);
>+}
>+
>+JS_PUBLIC_API(double)
>+JS::DayWithinYear(double time, double year)
>+{
>+    return ::DayWithinYear(time, year);
>+}
>+
This needs a review from a JS peer. Perhaps waldo could review the js/* parts

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8789730 [details] [diff] [review]
patch, v1.

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

I mostly just looked at the JSAPI bits, plus a little around the edges to understand the need for them -- as apparently was desired.  Just an r+ on those bits, with the necessary changes made.

::: dom/html/HTMLInputElement.cpp
@@ +2161,5 @@
> +
> +        double year = JS::YearFromTime(aValue.toDouble());
> +        double month = JS::MonthFromTime(aValue.toDouble());
> +        double day = JS::DayFromTime(aValue.toDouble());
> +        double yearDay = JS::DayWithinYear(aValue.toDouble(), year) + 1;

Ugh, we need to give people better APIs for exploding a time into its constituent components.  Works for now, tho.

::: dom/html/HTMLInputElement.h
@@ +1505,5 @@
>    // Float value returned by GetStep() when the step attribute is set to 'any'.
>    static const Decimal kStepAny;
>  
> +  // Minimum year limited by HTML standard, year >= 1.
> +  static const double kMinimumYear;

I think you could consider using |static constexpr| for these so that you could define them inline, if you wanted.

::: dom/html/test/forms/test_valueasdate_attribute.html
@@ +527,5 @@
> +    [ "275760-W38", true ],
> +  ];
> +
> +  element.type = "week";
> +  for (data of validData) {

Do you mean for this |data| and in the other for-of loops -- and seemingly in all the rest of the code pre-existing in these two test files -- to be a global variable?  Should make them declarations -- any of var/let/const is equally good -- to reduce reader confusion.

::: dom/html/test/forms/test_valueasnumber_attribute.html
@@ +674,5 @@
> +      caught = true;
> +    }
> +
> +    if (data[2]) {
> +      ok(caught, "valueAsNumber should have trhown");

thrown

::: js/public/Date.h
@@ +149,5 @@
>  JS_PUBLIC_API(double)
>  DayFromTime(double time);
>  
> +// Takes an integer year and returns the number of days from epoch to the given
> +// year.

Add onto this, in a separate paragraph:

"""
NOTE: The calculation performed by this function is literally that given in the ECMAScript specification.  Nonfinite years, years containing fractional components, and years outside ECMAScript's date range are not handled with any particular intelligence.  Garbage in, garbage out.
"""

@@ +154,5 @@
> +JS_PUBLIC_API(double)
> +DayFromYear(double year);
> +
> +// Takes an integer number of milliseconds since the epoch and an integer year,
> +// returns the number of days in that year.

Add onto this (same paragraph)

"""
If |time| is nonfinite, returns NaN.  Otherwise |time| *must* correspond to a time within the valid year |year|.  This should usually be ensured by computing |year| as |JS::DayFromYear(time)|.
"""

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Thanks Olli and Waldo for the review.

Added an extra param @isoWeek for DayOfWeek(), but it's a boolean instead of an enum. And for variable names, I only added the 'iso' for week related variables, cause I think only week it's different from what we expect (starting on Monday and how it considers week 1).

Added tests for years starting and ending with all different week days and also in leap years.

Comment 5

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

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

>+    case NS_FORM_INPUT_WEEK:
>+      {
>+        aValue = aValue.floor();
>+
>+        // Based on ISO 8601 date.
>+        double year = JS::YearFromTime(aValue.toDouble());
>+        double month = JS::MonthFromTime(aValue.toDouble());
>+        double day = JS::DayFromTime(aValue.toDouble());
>+        // Adding 1 since day starts from 0.
>+        double dayInYear = JS::DayWithinYear(aValue.toDouble(), year) + 1;
>+
>+        uint32_t isoWeekday = DayOfWeek(year, month + 1, day, true);
Could use a comment why month + 1 here.

>+HTMLInputElement::DayOfWeek(uint32_t aYear, uint32_t aMonth, uint32_t aDay,
>+                            bool isoWeek) const
> {
>   // Tomohiko Sakamoto algorithm.
>   int monthTable[] = {0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4};
>   aYear -= aMonth < 3;
> 
>-  return (aYear + aYear / 4 - aYear / 100 + aYear / 400 +
>-          monthTable[aMonth - 1] + aDay) % 7;
>+  int day = (aYear + aYear / 4 - aYear / 100 + aYear / 400 +
Hmm, nit, int32_t, not int

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

addressed review comment 5, carrying r+.

Comment 7

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b1567fbaf0

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2614d75c2100