[webkit-reviews] review granted: [Bug 31340] Introduce a class representing HTML5 date&time values : [Attachment 43055] ISO 8601 class (rev.6)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted TAMURA, Kent
<tkent at chromium.org>'s request for review:
Bug 31340: Introduce a class representing HTML5 date&time values
https://bugs.webkit.org/show_bug.cgi?id=31340

Attachment 43055: ISO 8601 class (rev.6)
https://bugs.webkit.org/attachment.cgi?id=43055&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Thanks! Please consider addressing these final nits on landing.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

This is a much better changelog!


> diff --git a/WebCore/html/ISODateTime.cpp b/WebCore/html/ISODateTime.cpp
> +// The oldest day of Gregorian Calendar is 1582-10-15.  We don't support
dates older than it.

One space after periods.

> +static const int gregorianStartYear = 1582;
> +static const int gregorianStartMonth = 9; // 0-based October.

Use whole sentences in general. 0-based October doesn't make sense. It sound
like October is the 0th month. What you mean is that January is 0, so this
month 9 is really October.
Here's an idea for your comment  "// This is October, since months are 0
based."

>
> +static int dayOfWeek(int year, int month, int day)
> +{
> +    int shiftedMonth = month + 2;
> +    // 2:January, 3:Feburuary, 4:March, ...

Thanks! You had this before with the month++ and I missed it, but I find this
new form much easier to read.


> +// Very strict integer parser. Do not allow leading or trailing whitespaces
unlike charactersToIntStrict().

s/whitespaces/whitespace/