[webkit-reviews] review denied: [Bug 3477] some US-centric date formats not parsed by JavaScript (clock at news8austin.com) : [Attachment 2444] Merged patch from KDE

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 3477: some US-centric date formats not parsed by JavaScript (clock at
news8austin.com)
http://bugzilla.opendarwin.org/show_bug.cgi?id=3477

Attachment 2444: Merged patch from KDE
http://bugzilla.opendarwin.org/attachment.cgi?id=2444&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.