Bug 3759 - Date object enhancements
Summary: Date object enhancements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Carsten Guenther
URL:
Keywords:
Depends on:
Blocks: 3381 3906
  Show dependency treegraph
 
Reported: 2005-06-28 20:46 PDT by Carsten Guenther
Modified: 2005-07-27 11:38 PDT (History)
1 user (show)

See Also:


Attachments
Proposed fix (116.22 KB, patch)
2005-07-11 21:32 PDT, Carsten Guenther
ggaren: review-
Details | Formatted Diff | Diff
Fixed default path in DateObjectImp::construct (116.24 KB, patch)
2005-07-18 15:44 PDT, Carsten Guenther
ggaren: review-
Details | Formatted Diff | Diff
Fixed above concerns (104.87 KB, patch)
2005-07-19 12:26 PDT, Carsten Guenther
darin: review-
Details | Formatted Diff | Diff
Final patch (8.87 KB, patch)
2005-07-26 15:14 PDT, Carsten Guenther
darin: review-
Details | Formatted Diff | Diff
Even more final patch (8.23 KB, patch)
2005-07-26 19:38 PDT, Carsten Guenther
darin: review+
Details | Formatted Diff | Diff
testcase (1.87 KB, text/html)
2005-07-26 19:38 PDT, Carsten Guenther
no flags Details
/fast/js/date-preserve-milliseconds.html (2.39 KB, text/html)
2005-07-27 11:15 PDT, Geoffrey Garen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carsten Guenther 2005-06-28 20:46:08 PDT
This bug tracks the work necessary to make the Date object pass all the ECMA autotests (the ones that are 
currently skipped).

Among the things that need to be fixed:
- ability to create a date object from a boolean value
- fix the setters (setMilliseconds, setSeconds, ...) to do the right thing with out-of-range values
- implement Date.UTC

Assigning to myself.
Comment 1 Carsten Guenther 2005-07-08 10:38:44 PDT
Working on that.
Comment 2 Carsten Guenther 2005-07-11 21:32:01 PDT
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.
Comment 3 Carsten Guenther 2005-07-11 21:34:35 PDT
I will look into that.
Comment 4 Carsten Guenther 2005-07-11 21:54:51 PDT
Ignore last comment, wrong bug.
Comment 5 Geoffrey Garen 2005-07-18 14:50:35 PDT
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.
Comment 6 Carsten Guenther 2005-07-18 15:44:37 PDT
Created attachment 3009 [details]
Fixed default path in DateObjectImp::construct

I replaced

    value = NaN;

with 

    value = args[0].toNumber(exec);

in DateObjectImp::construct.
Comment 7 Geoffrey Garen 2005-07-19 11:56:58 PDT
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()
Comment 8 Carsten Guenther 2005-07-19 12:26:19 PDT
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.
Comment 9 Carsten Guenther 2005-07-24 13:06:44 PDT
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 10 Darin Adler 2005-07-25 11:53:00 PDT
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.
Comment 11 Geoffrey Garen 2005-07-25 22:08:12 PDT
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.
Comment 12 Carsten Guenther 2005-07-26 08:28:30 PDT
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.
Comment 13 Carsten Guenther 2005-07-26 09:07:29 PDT
ecma/Date/15.9.5.js (Date.prototype.valueOf()) I actually have fixed locally and will include in the next 
patch.
Comment 14 Geoffrey Garen 2005-07-26 11:01:05 PDT
(In reply to comment #12)
The prototype problem should be fixed in TOT.
Comment 15 Carsten Guenther 2005-07-26 11:08:56 PDT
I opened a separate bug for the daylight savings issue:
    http://bugzilla.opendarwin.org/show_bug.cgi?id=4142
Comment 16 Carsten Guenther 2005-07-26 15:14:18 PDT
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 17 Darin Adler 2005-07-26 16:13:30 PDT
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-.
Comment 18 Carsten Guenther 2005-07-26 19:38:13 PDT
Created attachment 3101 [details]
Even more final patch

Fixed formatting issues.
Comment 19 Carsten Guenther 2005-07-26 19:38:58 PDT
Created attachment 3102 [details]
testcase
Comment 20 Darin Adler 2005-07-27 10:25:07 PDT
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 21 Darin Adler 2005-07-27 10:25:22 PDT
Comment on attachment 3101 [details]
Even more final patch

r=me
Comment 22 Geoffrey Garen 2005-07-27 11:15:46 PDT
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.