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");
Created attachment 4193 [details] Suggested patch
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 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.
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 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)
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.
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 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!
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 on attachment 9526 [details] improved patch r=me