[webkit-reviews] review denied: [Bug 39737] Change lower and higher limits of date/datetime/datetime-local/month/week types : [Attachment 57095] Patch 2 (removed TABs)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 39737: Change lower and higher limits of
date/datetime/datetime-local/month/week types
https://bugs.webkit.org/show_bug.cgi?id=39737

Attachment 57095: Patch 2 (removed TABs)
https://bugs.webkit.org/attachment.cgi?id=57095&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just some initial comments. (I may have more once these are addressed.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-05-26  Kent Tamura  <tkent at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Change lower and higher limits of
date/datetime/datetime-local/month/week types
> +	   https://bugs.webkit.org/show_bug.cgi?id=39737
> +
> +	   According to the latest draft of HTML5, ISO-8601 dates in HTML5
> +	   should support A.D.0001 in Gregorian calendar though Gregorian
> +	   calendar started in 1582. So, we change the lower limits of
> +	   date&time types to 0001-01-01T00:00.
> +
> +	   We also introduce the common higher limit, 275760-09-13T00:00. It
> +	   is the higher limit of Date type of ECMASCript.
> +
> +	   * html/DateComponents.cpp:

Short per function comments are recommended that explain why you did a change
there (or sometimes what but why is usually more useful, imo).

> +	   (WebCore::DateComponents::parseYear):
> +	   (WebCore::checkLimits):
> +	   (WebCore::DateComponents::addDay):
> +	   (WebCore::DateComponents::addMinute):
> +	   (WebCore::DateComponents::parseMonth):
> +	   (WebCore::DateComponents::parseDate):
> +	   (WebCore::DateComponents::parseWeek):
> +	   (WebCore::DateComponents::parseDateTimeLocal):
> +	   (WebCore::DateComponents::parseDateTime):
> +	   (WebCore::DateComponents::setMillisecondsSinceEpochForDate):
> +	   (WebCore::DateComponents::setMillisecondsSinceEpochForDateTime):
> +	   (WebCore::DateComponents::setMillisecondsSinceEpochForMonth):
> +	   (WebCore::DateComponents::setMonthsSinceEpoch):
> +	   (WebCore::DateComponents::setMillisecondsSinceEpochForWeek):
> +	   * html/DateComponents.h:
> +	   (WebCore::DateComponents::minimumDate):
> +	   (WebCore::DateComponents::minimumDateTime):
> +	   (WebCore::DateComponents::minimumMonth):
> +	   (WebCore::DateComponents::minimumWeek):
> +	   (WebCore::DateComponents::maximumDate):
> +	   (WebCore::DateComponents::maximumDateTime):
> +	   (WebCore::DateComponents::maximumMonth):
> +	   (WebCore::DateComponents::maximumWeek):
> +
>  2010-05-26  Xan Lopez  <xlopez at igalia.com>
>  
>	   Unreviewed GTK+ build fix.
> diff --git a/WebCore/html/DateComponents.cpp
b/WebCore/html/DateComponents.cpp

> -static bool beforeGregorianStartDate(int year, int month, int monthDay)
> +static bool checkLimits(int year, int month)

I don't like the name "checkLimits" because it doesn't read nicely in the code.


   if (!checkLimits...

Perhaps withinHtmlDateLimits because it produces more readable code:

   if (!withinHtmlDateLimits...