Bug 3759 – Date object enhancements

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Carsten Guenther]

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.

[Carsten Guenther]

Working on that.

[Carsten Guenther]

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.

[Carsten Guenther]

I will look into that.

[Carsten Guenther]

Ignore last comment, wrong bug.

[Geoffrey Garen]

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.

[Carsten Guenther]

Created attachment 3009 [details]
Fixed default path in DateObjectImp::construct

I replaced

    value = NaN;

with 

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

in DateObjectImp::construct.

[Geoffrey Garen]

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()

[Carsten Guenther]

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.

[Carsten Guenther]

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.

[Darin Adler]

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.

[Geoffrey Garen]

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.

[Carsten Guenther]

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.

[Carsten Guenther]

ecma/Date/15.9.5.js (Date.prototype.valueOf()) I actually have fixed locally and will include in the next 
patch.

[Geoffrey Garen]

(In reply to comment #12)
The prototype problem should be fixed in TOT.

[Carsten Guenther]

I opened a separate bug for the daylight savings issue:
    http://bugzilla.opendarwin.org/show_bug.cgi?id=4142

[Carsten Guenther]

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.

[Darin Adler]

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-.

[Carsten Guenther]

Created attachment 3101 [details]
Even more final patch

Fixed formatting issues.

[Carsten Guenther]

Created attachment 3102 [details]
testcase

[Darin Adler]

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;

[Darin Adler]

Comment on attachment 3101 [details]
Even more final patch

r=me

[Geoffrey Garen]

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.