Summary: | Date object enhancements | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carsten Guenther <carsten> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Carsten Guenther <carsten> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ggaren | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 412 | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 3381, 3906 | ||||||||||||||||||
Attachments: |
|
Description
Carsten Guenther
2005-06-28 20:46:08 PDT
Working on that. Created attachment 2914 [details] Proposed fix This patch fixes 17 of the 20 failing Mozilla testcases. It fixes the setters setMilliseconds, setSeconds, setMinutes, setHours and their UTC-variants to deal correctly with out-of-bounds values. It also adds a constructor Date(bool) and sets invalidDate to LONG_MIN instead of -1. Patch contains: + date_object.cpp + date_object.h + expected.html + Changelog We should not enable the mozilla testcases by default until bug 3906 is fixed, they take an awful long time. I will look into that. Ignore last comment, wrong bug. Comment on attachment 2914 [details]
Proposed fix
This patch causes a regression when you use the form alert(new Date(new
String("1"))).
Generally, the default branch in the patch is incorrect (and was previously).
It would be good to fix it entirely to match the ecma spec:
15.9.3.2 new Date(value)
The [[Prototype]] property of the newly constructed object is set to the
original Date prototype object, the one that is the initial value of
Date.prototype (15.9.4.1).
The [[Class]] property of the newly constructed object is set to "Date".
The [[Value]] property of the newly constructed object is set as follows:
1. Call ToPrimitive(value).
2. If Type(Result(1)) is String, then go to step 5.
3. Let V be ToNumber(Result(1)).
4. Set the [[Value]] property of the newly constructed object to TimeClip(V)
and return.
5. Parse Result(1) as a date, in exactly the same manner as for the parse
method (15.9.4.2); let V be the time value for this date.
6. Go to step 4.
Created attachment 3009 [details]
Fixed default path in DateObjectImp::construct
I replaced
value = NaN;
with
value = args[0].toNumber(exec);
in DateObjectImp::construct.
Comment on attachment 3009 [details]
Fixed default path in DateObjectImp::construct
This should fix the regression, but the switch statement and the comment are
still misleading:
1. We *do* know what to do -- it's explained clearly in the spec.
2. There are only two possible cases, not four: (1) type == string: call parse;
(2) else: call x.toPrimitive().toNumber()
Created attachment 3020 [details]
Fixed above concerns
Fixed above concerns although personally I still had preferred the switch
statement since it more easily allows to alter the logic for a specific case
later, should that become necessary.
Why does this bug depend on 3381 and 3906? That doesn't make any sense to me. 3759 fixes a couple of problems and should be landed. Comment on attachment 3020 [details]
Fixed above concerns
Looks great, overall, but a few loose ends.
There should be a space after the comma in:
roundValue(exec,args[0]);
and the other places where roundValue calls were added.
The implementation of setSeconds sets the millseconds 0 when no milliseconds
are supplied. Instead it should preserve the existing milliseconds unless
milliseconds are passed explicitly. Same for setMinutes, setHours, etc. We need
test cases that would detect these problems and we should fix the patch too.
There are too many lines here with just whitespace changes; that's particularly
inconvenient since I have some global change patches outstanding, and any
whitespace-only change will create a merge conflict for my patch.
Do you have any ideas about the remaining test failures? For 15.9.5.28-1.js, I know that setMinutes succeeds w/numbers up to 166199 -- at 166200, it fails. I have two remaining test failures (a third one is due to a problem with the prototype object). Both seem to be related to daylight savings since the actual test result is always off by one hour from the expected result. I haven't looked into them in detail yet. ecma/Date/15.9.5.js (Date.prototype.valueOf()) I actually have fixed locally and will include in the next patch. (In reply to comment #12) The prototype problem should be fixed in TOT. I opened a separate bug for the daylight savings issue: http://bugzilla.opendarwin.org/show_bug.cgi?id=4142 Created attachment 3093 [details]
Final patch
This patch fixes the preservation of an existing milliseconds value when none
was passed in the setter (thanks for pointing that out Darin, good catch!). I
factored out that code into its own function (time_from_args).
Also added the spaces in the roundValue calls.
Comment on attachment 3093 [details]
Final patch
Looks great. Excellent work and a nice solution.
Formatting issues (minor):
1) Calls to and definition of time_from_args need the space removed between
the name of the function and the (.
2) Our usual naming scheme is timeFromArgs, numArgs, maxArgs.
3) There is still some stuff in this patch that is a whitespace change
only.
Test issue:
1) I'd like to see a test that tests the milliseconds behavior, since an
earlier version of the patch had that wrong.
I'm really tempted to set this to review+, since it's almost perfect, but I'm
going to be principled and set this to review-.
Created attachment 3101 [details]
Even more final patch
Fixed formatting issues.
Created attachment 3102 [details]
testcase
Comment on attachment 3102 [details]
testcase
Test would be better if the test output included the comment about what's
tested. The comment in the source is great, but it should be visible when
running the test.
Since this test output is all text, it should also say:
if (window.layoutTestController)
layoutTestController.dumpAsText;
Comment on attachment 3101 [details]
Even more final patch
r=me
Created attachment 3114 [details]
/fast/js/date-preserve-milliseconds.html
You've done good work here. For the future, please note some additions I've
made to your test before landing it:
(1) The description of what you're testing should appear in the page when you
open it (darin's comment).
(2) The description should explain what success will look like. (Since failure
is unpredictable, describing it isn't as important.)
(2) The test should print both failure messages and success messages.
(3) The test should print what value was right/wrong and how it was
right/wrong.
|