[webkit-reviews] review denied: [Bug 130123] Incorrect Date returned between March 1, 2034 and February 28, 2100. : [Attachment 226728] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Byungseon Shin <xingri at gmail.com>'s
request for review:
Bug 130123: Incorrect Date returned between March 1, 2034 and  February 28,
2100.
https://bugs.webkit.org/show_bug.cgi?id=130123

Attachment 226728: Patch
https://bugs.webkit.org/attachment.cgi?id=226728&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226728&action=review


> LayoutTests/js/script-tests/date-constructor.js:73
> +var monthNames = ["January", "February", "March", "April", "May", "June",
"July", "August", "September", "October", "November", "December" ];

Please remove the space between “December” and “];”.

> LayoutTests/js/script-tests/date-constructor.js:81
> +    if (dateObj.getFullYear() != year) {
> +	   success = false;

Please add:
    shouldBe(“new Date(“ + dateString + “).getFullYear()”, year);

That way, if it fails, we’ll know what values it failed on, and lessen the
effort to get a reproduction case for debugging the issue.

> LayoutTests/js/script-tests/date-constructor.js:83
> +    } if (dateObj.getMonth() != month) {
> +	   success = false;

Ditto.	Please add:
    shouldBe(“new Date(“ + dateString + “).getMonth()”, month);

> LayoutTests/js/script-tests/date-constructor.js:85
> +    } if (dateObj.getDate() != date) {
> +	   success = false;

Ditto.	Please add:
    shouldBe(“new Date(“ + dateString + “).getDate()”, date);

> LayoutTests/js/script-tests/date-constructor.js:100
> +var leapTestResult = true;
> +var year = 1970;
> +var month = 2;
> +var date = 1;
> +
> +// Check value of March 1 day since 1970 till 9999
> +while (year < 10000) {
> +    leapTestResult = testDate(year, month, date); 
> +    if(!leapTestResult) break;
> +    year += 1; 
> +} 

In my original suggestion, I had the month and date incrementing by a prime
number so that we can test different months and dates as well:	
     month = (month + 7) % 12;
     date = (date + 13) % 28;

Is there a reason you opted to go only with "March 1”?	I feel that this
actually lessens the coverage of the test.  If you don’t have a good reason for
this, please add back the date and month increments so that we can test
different values.  If there’s something special about “March 1”, then please
add a line to test it explicitly (in addition to the one that tests the varying
date values) with a comment to describe why it is :
     leapTestResult ||= testDate(year, 2, 1); // <reason why you’re explicitly
testing this month and date …>

In my original suggestion, I also incremented the year by a prime number
greater than 1.  The reason was so that the test will take less time to run. 
But I supposed any modern computer should be able to blow thru 10000 iterations
in no time.  So, incrementing the year by 1 is fine.

According to
http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects
/Date/getFullYear, the full year ranges from 1000 to 9999.  So, we might as
well test Date ranges in the past as well.  A quick test on FF, Chrome, and
Safari all reveals that they can take years going back to year 100.  On Safari
and Chrome, years less than that get aliases e.g. 99 become 1999, 10 becomes
2010.  On FF, 99 becomes 1999, 10 returns a NaN.  How about starting the year
with year 100?