Bug 16753

This is the spec: 15.9.5.29 Date.prototype.setUTCMilliseconds(ms) 1. Let t be this time value. 2. Call ToNumber(ms). 3. Compute MakeTime(HourFromTime(t), MinFromTime(t), SecFromTime(t), Result(2)). 4. Compute MakeDate(Day(t), Result(3)). 5. Set the [[Value]] property of the this value to TimeClip(Result(4)). 6. Return the value of the [[Value]] property of the this value. I'm not sure I fully understand why this turns date into NaN yet.

Never mind. It's quite clear from the spec we should be setting the Date internal value to be NaN. Our implementation works differently than the spec does, which is our problem.

I'll fix this one.

Created attachment 18308 [details] Abstract all dateObject.set* functions into shared static functions JavaScriptCore/ChangeLog | 26 +++ JavaScriptCore/kjs/date_object.cpp | 293 ++++++------------------------------ 2 files changed, 74 insertions(+), 245 deletions(-)

Created attachment 18309 [details] Add fast/js/date-set-to-nan test LayoutTests/fast/js/date-set-to-nan.html | 13 +++++ LayoutTests/fast/js/resources/date-set-to-nan.js | 53 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 0 deletions(-)

Comment on attachment 18308 [details] Abstract all dateObject.set* functions into shared static functions This can stand for review by itself. Then when it's OK'd I'll land it and clear the flag.

(In reply to comment #4) > Created an attachment (id=18308) [edit] > Abstract all dateObject.set* functions into shared static functions Sam, I think this patch may address <rdar://problem/5595552>.

Comment on attachment 18308 [details] Abstract all dateObject.set* functions into shared static functions Thanks, Eric! r=me

At this point, the difficult part is not the fix, it's the test case. I'm not certain the test case I've posted is enough. I think for functions which take more arguments, I may need to test correct handling of NaN/null/undefined/inf, at different argument numbers to make sure NaN is produced in appropriate cases.

Comment on attachment 18308 [details] Abstract all dateObject.set* functions into shared static functions This landed as: http://trac.webkit.org/projects/webkit/changeset/29217 Clearing review flag.

I'm distracted by other things at the moment. Returning this back to unassigned.

Created attachment 18897 [details] patch, good enough It'd be even better to fix cases like undefined and null. But to fix enough to pass Acid3, this patch is all it will take.

Comment on attachment 18897 [details] patch, good enough Looking at the original Ecma algorithm, there are other ways to get a NaN during processing. I don't think we support any of those either.

Created attachment 19197 [details] Patch to handle NaN, Inf and missing arguments The fast/js/date-set-to-nan test runs successfully after applying this patch. But there seems to be an additional problem with the DST correction for years smaller 1917 (at least when using UTC-1)

Created attachment 19206 [details] Patch to handle NaN, Inf and missing arguments There was still a bug with recovering from NaN, by using setMilliseconds() which lead to a buggy date.

Comment on attachment 19206 [details] Patch to handle NaN, Inf and missing arguments Thanks for the patch! A few comments. One, you should familiarize yourself with the WebKit coding style guidelines: http://webkit.org/coding/coding-style.html A few "violations" I see in this patch include: if (foo) { (The { should be on the same line as the if) and: if (0 != args.size()) should be: if (args.size()) (we avoid == 0 and != 0 when the meaning is otherwise clear from context. e.g. comparing pointers to NULL or using size() for isEmpty()... in this case, there migth even be an isEmpty() which could be more clear). Single line ifs should not have { } Wouldn't: ok = !(isinf(millis) || isnan(millis)); be equivalent to: ok = isfinite(mills); ? Also, the above if (ok) block, generally we prefer an early-return style in webkit. In this instance you would use: if (!ok) return false; instead of wrapping that section in an if (ok) block. Likewise, it would be more clear (or at least more inline with webkit style) to do: if (!args.size()) { result = jsNaN(); date->setInternalValue(result); return result; } as an early return, instead of adding a long if block. We'll need to of course run SunSpider with this change to make sure there is not speed regression due to added branching in these functions. r- for the style issues. Ideallly your patch would include the diff for the test case, since someone else will be landing this for you (unless you have commit-bit and I simply don't recognize your name). Thanks again for the patch!

Created attachment 19214 [details] Patch to handle NaN, Inf and missing arguments I changed the code to match the code style guidelines and I added a modified the testcase which also checks for NaN "recovery" using the setTime, setYear, setFullYear and setUTCFullYear methods. For all other methods it is not possible to recover from an NaN date

Comment on attachment 19214 [details] Patch to handle NaN, Inf and missing arguments + if (3 == numArgsToUse && isnan(milli)) I know that in some projects people think that putting the constant on the left side of an "==" is good style because it helps you avoid the mistake of using "=" by mistake, but we really don't believe in that, and I think it's strange to have this once instance of it mixed in with the other code. The test is great. It's thorough. I'd prefer it if a lot more of the test cases and results were written out; it's better if the results file shows a lot more of what's being tested rather than just the conclusions. If you look at other tests that make heavy use of shouldBe you'll see how that can work. +function makeIEHappy (functionNameRoot, value) We don't put spaces after function names. +function testDateFunctionWithValueExpectingNaN1(functionNameRoot) { We put the braces on the next line, not the same line as the function. r=me as-is, but please consider making improvements as well.

Created attachment 19240 [details] revised version of the patch Changed the testcase based on darin comments.

Comment on attachment 19240 [details] revised version of the patch r=me

Landed in r30453.