Change lower and higher limits of date/datetime/datetime-local/month/week types

[Kent Tamura]

Change lower and higher limits of date/datetime/datetime-local/month/week types

[Kent Tamura]

Created attachment 57094 [details]
Ppatch

[Kent Tamura]

Created attachment 57095 [details]
Patch 2 (removed TABs)

[David Levin]

Comment on attachment 57095 [details]
Patch 2 (removed TABs)

Just some initial comments. (I may have more once these are addressed.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Kent Tamura  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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...

[Kent Tamura]

Created attachment 58373 [details]
Patch 3

[Kent Tamura]

Thank you for reviewing.  I updated the patch.

(In reply to comment #3)
> Short per function comments are recommended that explain why you did a change there (or sometimes what but why is usually more useful, imo).

Done.

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

I changed so.  Actually I also didn't like "checkLimits" and had no other good idea.

[Kent Tamura]

Thank you for reviewing.
Landed as r66355.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66355 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66354
http://trac.webkit.org/changeset/66355