Bug 5257 - setYear() does not match FireFox/IE behavior
Summary: setYear() does not match FireFox/IE behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Other All
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-04 10:29 PDT by George Staikos
Modified: 2006-07-24 06:55 PDT (History)
1 user (show)

See Also:


Attachments
Suggested patch (843 bytes, patch)
2005-10-04 10:30 PDT, George Staikos
ggaren: review-
Details | Formatted Diff | Diff
First attempt (1.21 KB, patch)
2006-07-15 13:36 PDT, Rob Buis
ggaren: review-
Details | Formatted Diff | Diff
Now with testcase (4.29 KB, patch)
2006-07-17 00:05 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
improved patch (4.25 KB, patch)
2006-07-17 11:01 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Staikos 2005-10-04 10:29:28 PDT
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");
Comment 1 George Staikos 2005-10-04 10:30:07 PDT
Created attachment 4193 [details]
Suggested patch
Comment 2 Geoffrey Garen 2005-10-09 14:06:09 PDT
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.
Comment 3 George Staikos 2005-10-10 15:45:02 PDT
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.
Comment 4 Rob Buis 2006-07-15 13:36:37 PDT
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.
Comment 5 Geoffrey Garen 2006-07-16 02:25:43 PDT
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)
Comment 6 Rob Buis 2006-07-16 03:51:19 PDT
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.
Comment 7 Rob Buis 2006-07-17 00:05:34 PDT
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.
Comment 8 Darin Adler 2006-07-17 07:51:11 PDT
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!
Comment 9 Rob Buis 2006-07-17 11:01:28 PDT
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.
Comment 10 Darin Adler 2006-07-17 11:40:22 PDT
Comment on attachment 9526 [details]
improved patch

r=me