Summary: | some US-centric date formats not parsed by JavaScript (clock at news8austin.com) | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stuart Morgan <stuartmorgan> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Carsten Guenther <carsten> | ||||||||||||||||||||||
Status: | VERIFIED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | porten, vicki | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 412 | ||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||||||
URL: | http://www.news8austin.com/ | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 3296 | ||||||||||||||||||||||||
Attachments: |
|
Description
Stuart Morgan
2005-06-12 13:25:58 PDT
10/7/03 9:31 AM Darin Adler: The problem is that the site provides the date as a string, but it's a string our version of Date can't parse. var now = new Date("10/7/2003 11:30:29 AM") We'll have to add code to parse dates in this format to fix this. I will have a fix for that soon. Just needs some more testing. Created attachment 2337 [details]
Simple testcase
Created attachment 2338 [details]
Proposed fix
Instead of just taking this patch, we might want to take the KDE date_object.cpp, which I believe has improvements in handling of this and other similar date formats. I took a look at KHTML's date_object.cpp. They also support the YYYY/MM/DD format, other than that it looks not much different. Let me know what you prefer, I can prepare a patch based on the KHTML source if you want. I think it would be best to use the KDE version, esp. since they've switched KJS::KRFCDate_parseDate to return double, and solved the maximum year 2038 problem as outlined in Bug 3296 and Bug 3417. Created attachment 2444 [details]
Merged patch from KDE
Attached new patch from the KDE code. This fixes this bug and 3296.
Comment on attachment 2444 [details] Merged patch from KDE I think it's great to land this improved date code from KDE, presuming that all the existing date tests we have continue to work. For future improvements, it's probably worth checking out the FAQ at <http://www.tondering.dk/claus/calendar.html>, which can answer questions like the one in the yearFromTime function. In fact, I think <http://www.tondering.dk/claus/cal/node3.html#SECTION003161000000000000000> answers that question. There seems to be a missing "static" in front of timeFromYear, yearFromTime, and weekDay, which we should add. I don't understand the need for extrac checks in the part of code that says "if (utc)". As far as I can tell, ToGMTString and ToUTCString will always set utc to true, and ToString will always set utc to false, so there's no need for the additional checks. The code calls localtime_r inside the __APPLE__ ifdef. If it's no longer calling localtime() then we should get rid of the macro at the top of the file changing localtime() calls to localtimeUsingCF() calls. The original reason we called localtimeUsingCF() instead of localtime() is that localtime() hit the disk every time; we'll need to test to see that is no longer the case. The simplest thing to do may be to extend the macro so there's one for localtime_r too. I see some tabs in this patch. Please fix that and use only spaces for indenting. All the loops that say: + while(*dateString && isspace(*dateString)) could leave out the 0 check. isspace will return false for the 0 character. The calls to strtol should check the error; I don't think the patch should remove our errno checking and any call to strtol should check errno after the fact. I'm not sure that calling isspace is right -- do we really want characters like '\n' or '\t' to be allowed in dates, but not Unicode whitespace? I think our isSpaceOrTab might have been better. We'd need test cases to be sure. Seems close, but not quite ready to land. Here comes the improved version of the patch. > There seems to be a missing "static" in front of timeFromYear, yearFromTime, > and weekDay, which we should add. Added the missing statics. > I don't understand the need for extrac checks in the part of code that says "if > (utc)". As far as I can tell, ToGMTString and ToUTCString will always set utc > to true, and ToString will always set utc to false, so there's no need for the > additional checks. Not excactly sure what tests you mean. Can you give me the line numbers? > The code calls localtime_r inside the __APPLE__ ifdef. If it's no longer > calling localtime() then we should get rid of the macro at the top of the file > changing localtime() calls to localtimeUsingCF() calls. The original reason we > called localtimeUsingCF() instead of localtime() is that localtime() hit the > disk every time; we'll need to test to see that is no longer the case. The > simplest thing to do may be to extend the macro so there's one for localtime_r > too. I ran some tests with fs_usage and could not find any disk access when calling localtime(). However, I vote for keeping the macros and the core foundation code for now since according to a comment in the code it provides us with larger date ranges and I am not sure of all the consequences of removing them. This could be addressed in a separate bug. I also rewrote the one localtime_r to be localtime. > I see some tabs in this patch. Please fix that and use only spaces for > indenting. Cleaned up indendation. > All the loops that say: > > + while(*dateString && isspace(*dateString)) > > could leave out the 0 check. isspace will return false for the 0 character. Removed the *dateString test from those loops. > The calls to strtol should check the error; I don't think the patch should > remove our errno checking and any call to strtol should check errno after the > fact. Put back in the errno checks after calls to strtol. > I'm not sure that calling isspace is right -- do we really want characters like > '\n' or '\t' to be allowed in dates, but not Unicode whitespace? I think our > isSpaceOrTab might have been better. We'd need test cases to be sure. isSpaceOrTab does not check for more than spaces and tabs either. In addition we should also allow '\r' what isspace() does. I ran a quick test with Firefox and it allows both tabs and newlines in dates. So I think using isspace() is the right thing until we decide to support all unicode whitespace characters. And I am not sure if we should. Created attachment 2533 [details]
Improved patch
Comment on attachment 2533 [details]
Improved patch
Setting the review flag on Carsten's behalf.
Comment on attachment 2533 [details]
Improved patch
I'm not so happy with all the whitespace and indenting changes. There is lots
of code here that hasn't changed at all except that you seem to have run some
program's indenting on it (emacs, perhaps?).
Maybe we should do that in a separate pass, as it makes it harder to spot the
actual changes.
To me, some of the whitespace changes look wrong, too. Like the
strangely-indented body of DateProtoFuncImp::DateProtoFuncImp for example.
My (utc) comment above is about this code:
+ if ( (id == DateProtoFuncImp::ToGMTString) ||
+ (id == DateProtoFuncImp::ToUTCString) )
+ t = gmtime(&tv);
+ else if (id == DateProtoFuncImp::ToString)
+ t = localtime(&tv);
+ else if (utc)
+ t = gmtime(&tv);
+ else
+ t = localtime(&tv);
I think the first two if statements are entirely unnecessary.
Because there were so many whitespace changes, I wasn't able to do a thorough
review of the new patch.
Created attachment 2552 [details]
Changes without indentation cleanup
Added another patch with the above changes but without the indentation cleanup.
I think you are right about the if(utc) check. I removed the first two if
statements since they are redundant, utc got already set depending on the id
value.
Comment on attachment 2552 [details]
Changes without indentation cleanup
Patch looks great now.
I think we need a better test case now, that covers a lot more cases that the
single test case attached to this bug.
Perhaps we can find one the KDE project is already using? Or maybe with these
changes we can enable the Mozilla date tests.
In any case, the only remaining issue here is that the test case is not good
enough. I will mark this review+, but we should get a much more thorough test
case that covers the new code paths before we land this patch.
In particular, my insistence that we check errno may have resulted in a
stricter function that behaves different than KDE's KJS on various date
strings, so we might end up deciding we need to change that.
There is still a problem with dates < 1970, the mozilla test results in 2 regressions. I will look into that. Also reassigning to myself. Created attachment 2635 [details]
Next interation of proposed fix
Here comes the (hopefully) final patch. It fixes all outstanding issues I could
find with dates that were not in the portable range.
This patch also fixes one of the date tests (ecma_3/Date/15.9.5.5.js), I will
also append the updated expected.html file.
In regards to testcases, I would like to enable all of the mozilla test cases
for dates (the ones in ecma/dates). I did a first run locally and it does not
look too bad. But to fully support those I need to add some code, for example
to create a date object from a bool value. I will open a separate bug for this.
Created attachment 2636 [details]
Updated expected.html
Comment on attachment 2635 [details]
Next interation of proposed fix
This line:
+ return seconds == -1 ? NaN : seconds * 1000.0;
should use the constant "invalidDate" rather than a hard-wired -1.
Otherwise, r=me, assuming that our testing is super-thorough. Ideally I'd want
a test date that exercises each branch within the date-parsing function.
Enabling the date tests in the Mozilla JavaScript test suite is a great idea.
I'm so happy this change will allow us to do that!
Harri Porten also showed me a pretty complete date test page the other day. I
suggest contacting him, because I'd like to land that as a layout test.
Created attachment 2637 [details]
Final patch
Replaced -1 with invalidDate.
Comment on attachment 2637 [details]
Final patch
r=me
This patch breaks: fast/js/date-parse-test Changing status to reopened and rolling out patch. Here are the KDE tests: http://websvn.kde.org/trunk/tests/khtmltests/regression/tests/js/Date.js?rev=424394&view=markup Not as complete as the Mozilla tests but a) they test the features currently supported by the implementation and b) test some special, non-standard cases found on the Web. As I commented on in another br I think the date-parse-test contains some questionnable tests. (My) Firefox and Konqueror have 10 failures, IE 9 (partially different ones). Could be that both browsers have their own bugs but I'd be careful with the interpretation of the results from this test. The failures are probably due to items in the test that are incorrect. If so, we need to update expected results. Sorry about that, I did not run the layout tests after my latest changes. I will look into that later this week. Created attachment 2723 [details]
New patch including updated layout tests
Here comes a new patch that contains date_object.cpp|h, expected.html and a
modified fast/js/date-parse-test.
Let's go through the failed layout tests in detail since they fall into four
groups:
1. Five of the tests were failing because we are checking for errno after every
strtol operation. In one location this was wrong: when we have the year follow
by the timezone, without a time. I removed that check for errno and added a
comment.
2. Six of the tests contained invalid timezone identifiers. Those are invalid
dates and NaN should be returned so the testcases were wrong. In addition to
that our code was wrong too since it produces a number. I added that check for
an unknown timezone and also modified the testcases to have NaN as the expected
result. I also cross-checked with what Firefox does.
3. Two other testcases were wrong in that they assume that "Dec 25 1995 1:30AM"
and "Dec 25 1995 1:30PM" are valid dates. Both are invalid date descriptions,
note the missing space between the time and AM/PM. I modified the testcases to
expect NaN and also cross-checked with Firefox.
4. The remaining two testcases expected NaN althought they are valid: "Dec 25
1995 0:30 AM GMT" and "Dec 25 1995 0:30 PM GMT". Our code produced the right
numbers, again I cross-checked with the results from Firefox. I modified those
two testcases to expect the right results.
Comment on attachment 2723 [details]
New patch including updated layout tests
r=me
Created attachment 2724 [details]
Copyright years update
A one-liner that might make the patch work even better ;)
Comment on attachment 2724 [details]
Copyright years update
Patch accidentally reversed.
reporter could you veriufy this one? thx. |