Bug 5257 – setYear() does not match FireFox/IE behavior

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[George Staikos]

From the KJS testcases, setYear was failing in several cases: 
 
setYear(100) was not giving back 100 
setYear(1899) was giving back 3799 instead of 1899 
and more cases were broken.  Here is my testcase and patch will be attached. 
 
d2 = new Date(); d2.setYear(10); print(d2.getFullYear()); print("expected 
1910"); 
d2 = new Date(); d2.setYear(100); print(d2.getFullYear()); print("expected 
100"); 
d2 = new Date(); d2.setYear(105); print(d2.getFullYear()); print("expected 
105"); 
d2 = new Date(); d2.setYear(1899); print(d2.getFullYear()); print("expected 
1899"); 
d2 = new Date(); d2.setYear(1900); print(d2.getFullYear()); print("expected 
1900"); 
d2 = new Date(); d2.setYear(2000); print(d2.getFullYear()); print("expected 
2000"); 
d2 = new Date(); d2.setYear(0); print(d2.getFullYear()); print("expected 
1900"); 
d2 = new Date(); d2.setYear(-10); print(d2.getFullYear()); print("expected 
-10"); 
d2 = new Date(); d2.setYear(-0); print(d2.getFullYear()); print("expected 
1900");

[George Staikos]

Created attachment 4193 [details]
Suggested patch

[Geoffrey Garen]

Comment on attachment 4193 [details]
Suggested patch

I think it's a mistake to call fillStructures..., since that function parses
optional month and day arguments, which aren't valid in a setYear call. (That's
the cause for the r-.) 

I'm also not clear how this solves the 18XX case, since fillStructures... will
set tm_year to y - 1900 (a negative number).

Finally, it would be neat if you made this change in Date::construct() and
Date::callAsFunction, since they have the same problem.

[George Staikos]

Comment on attachment 4193 [details]
Suggested patch

> I think it's a mistake to call fillStructures..., since that function parses 
> optional month and day arguments, which aren't valid in a setYear call.  
> (That's the cause for the r-.)  
 
   Hm good point.  That should be fixed.  At least this is a case where we are 
more lenient than the spec says, as opposed to wrong. 
 
> I'm also not clear how this solves the 18XX case, since fillStructures...  
> will set tm_year to y - 1900 (a negative number). 
 
   mktime() at least handles that properly.  I tested with the testcase above 
on Linux. 
 
> Finally, it would be neat if you made this change in Date::construct() and 
> Date::callAsFunction, since they have the same problem. 
 
   Didn't try or investigate.  Better to open another bug report for this 
IMHO.

[Rob Buis]

Created attachment 9468 [details]
First attempt

The patch works for the tests mentioned in this bug report. Also playing with
http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_setyear showed no problems.
Let me know if this is the right approach, and what testcases are needed.
Cheers,

Rob.

[Geoffrey Garen]

Comment on attachment 9468 [details]
First attempt

This patch actually matches the logic in the spec (although it's inverted):

4. If Result(2) is not NaN and 0 <= ToInteger(Result(2)) <= 99 then Result(4) is ToInteger(Result(2)) + 1900. Otherwise, Result(4) is Result(2).

No testcase is needed if this fixes an existing testcase, but you should attach updated results for the testcases you've fixed. (r- for that)

[Rob Buis]

Hi Geoff,

(In reply to comment #5)
> (From update of attachment 9468 [details] [edit])
> This patch actually matches the logic in the spec (although it's inverted):
> 
> 4. If Result(2) is not NaN and 0 <= ToInteger(Result(2)) <= 99 then Result(4)
> is ToInteger(Result(2)) + 1900. Otherwise, Result(4) is Result(2).
> 
> No testcase is needed if this fixes an existing testcase, but you should attach
> updated results for the testcases you've fixed. (r- for that)

Well, I looked only at the JSC tests, no changes there. Now that I look at the layout tests, no
changes either :} But I noticed LayoutTests/fast/js/kde/Date.html-disabled, which actually tests
setYear well, just like in this bug report. I verified by reenabling it that all setYear tests worked,
these are the differences compared to the old situation:

< FAIL d2.setYear(-1), d2.getFullYear() should be -1 (of type number). Was 1899 (of type number).
---
> PASS d2.setYear(-1), d2.getFullYear() is -1
48c48
< FAIL d2.setYear(100), d2.getFullYear() should be 100 (of type number). Was 2000 (of type number).
---
> PASS d2.setYear(100), d2.getFullYear() is 100
50c50
< FAIL d2.setYear(1899), d2.getFullYear() should be 1899 (of type number). Was 3799 (of type number).
---
> PASS d2.setYear(1899), d2.getFullYear() is 1899

Unfortunately I guess LayoutTests/fast/js/kde/Date.html-disabled is disabled for a reason, so
there is no direct test at this point for my patch. Let me know what is the best action to take, I
could make a testcase based on Date.html-disabled that only tests setYear?
Cheers,

Rob.

[Rob Buis]

Created attachment 9519 [details]
Now with testcase

As suggested by ggaren, now we have a test for setYear, splitted from Date.js kde test.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9519 [details]
Now with testcase

Both the change logs mention the wrong bug number and the wrong bug description. Otherwise the fix looks great!

[Rob Buis]

Created attachment 9526 [details]
improved patch

Hi,

Hmm, I got two date fixes mixed up in a major way. Anyhoo, this one should be more accurate :)
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9526 [details]
improved patch

r=me