[Webkit-unassigned] [Bug 31340] Introduce a class representing HTML5 date&time values
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 12 03:24:51 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=31340
TAMURA, Kent <tkent at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #42943|0 |1
is obsolete| |
Attachment #43055| |review?
Flag| |
--- Comment #4 from TAMURA, Kent <tkent at chromium.org> 2009-11-12 03:24:50 PST ---
Created an attachment (id=43055)
--> (https://bugs.webkit.org/attachment.cgi?id=43055)
ISO 8601 class (rev.6)
(In reply to comment #3)
> > + https://bugs.webkit.org/show_bug.cgi?id=29004
>
> This is the wrong bug number.
Fixed.
> It is good to add per function comments in the change log in general or at
> least per file.
Added some comments.
> "Copyright (C) 2009 Google Inc. All rights reserved."
Fixed.
> > +static const int maxDayOfMonths[12] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
>
> It would be better to name this array daysInMonth which will help distinquish
> it from the function with a similar name.
Done.
> > +static int dayOfWeek(int year, int month, int day)
> > +{
> > + ++month;
> > +
> > + // Zeller's congruence
> > + if (month <= 2) {
>
> It looks like you're treating month as if it is 1 based (making Jan be 13 and
> Feb 14 which is what the algorithm calls for) but in other places you treat it
> as one based including this call for example "int day = dayOfWeek(m_year, 0,
> 1);"
>
> I don't understand how this is working. If this is a bug, please increase your
> test coverage to expose it.
It's not a bug.
- ISODateTime::m_month is 0-based to follow "struct tm".
- month values at other places are 0-based.
- The month parameter of this function is also 0-based.
I introduced a new local variable named `shiftedMonth' for readability, and
added some comments.
> Why not do
> int result = (day + 13 * (month + 1) / 5 + lowYear + lowYear / 4 + highYear
> / 4 + 5 * highYear + 6) % 7;
> and get rid of the if (result < 0)?
Done.
> > + return result; // Sunday origin
> One space before end of line comments but I feel like the Sunday origin comment
> should come before the equation above since the it the reason for the + 6.
Done.
> > +int ISODateTime::maxWeek() const
>
> I like maxWeekNumber as the name. (I don't know what the max week is?)
Done.
> > +static unsigned countDigits(const UChar* src, unsigned length, unsigned start)
> > +{
> > + unsigned count = start;
>
> count isn't a count. It is an index. (It get especially confusing when you see
> the return value for the count of digits is "count - start".
> I'd suggest changing the variable name to index.
Done.
> > +// Very strict integer parser. Not allow leading whitespaces unlike charactersToIntStrict().
>
> Please only use one space after periods in comments.
Done.
> "Not allow leading whitespaces unlike charactersToIntStrict()." -> "Do not
> allow leading or trailing whitespaces unlike charactersToIntStrict()."
Done.
> > +static bool toInt(const UChar* src, unsigned length, unsigned parseStart, unsigned parseLength, int* out)
>
> Because "out" may never be 0, use "int& out".
Done for this and all of "unsigned* end".
> > + if (value > (INT_MAX - digit) / 10) // overflow
>
> One space before end of line comments.
Done.
> Also, this line doesn't cause an overflow, so I'd change the comment: "// Check
> for overflow."
Done.
> > + for (; dayDiff > 0; --dayDiff) {
> > + ++day;
> > + if (day > maxDayOfMonth(year, month)) {
>
> Doing this look up repeatedly in a loop pains me (especially in Febuary in a
> leap year). Why not store the result and only change it when the month/year
> changes below?
Done.
> After looking over the code more, I take it back because I think this is never
> used to move very many days. Please correct me if I'm wrong.
Right. This function is used to adjust timezone offset for now.
However we'll use this function to implement HTMLInputElement::stepUp() and
steDown().
So it will be possible to move many days.
> > + if (month >= 12) { // month is 0-origin.
>
> Single space before end of line comments.
Done.
> > + if (year < 0) // Overflow.
>
> Single space before end of line comments.
Done.
> "// Check for overflow."
Done.
> > + // min can be negative or greater than 59
>
> Add "." to end of comment.
Done.
> > +// Parses a timezone part, and adjust year, month, monthDay, hour, minute, second, millesecond.
>
> "millesecond" should be "millisecond"
Done.
> > + if (src[i] == 'Z') {
> > + ++i;
> > + hour = 0;
> > + minute = 0;
>
> Why do you set the hour and minute in this case at all? Why not just move the
> addMinute into the else?
Removed them and addMinute() for 'Z' case.
> > + if (i >= length || src[i++] != ':')
> > + return false;
>
> It would be clear to move the i++ out of the src[i++] and put it here.
Done for all of src[i++] -> src[i] then i++.
> > + if (!addMinute( -(hour * 60 + minute)))
>
> No space after open paren.
Done.
> > + ASSERT(src);
> > + ASSERT(end);
> > + unsigned i;
>
> Consider using index instead of i.
Done for all of i -> index.
> > + // Optional second part
> > + if (i + 2 < length && src[i] == ':') {
> > + int second;
>
> > + if (toInt(src, length, i + 1, 2, &second) && second >= 0 && second <= 59) {
>
> Isn't it an error if the seconds are present but not in the proper range?
The policy of these parse*() functions is "consume only valid part of the
input".
Even If the second or fractional second part is invalid, hh:mm part should be
parsed successfully.
> > + unsigned digitsLength = countDigits(src, length, ++i);
> > + if (digitsLength > 0) {
>
> From my reading of http://www.w3.org/TR/NOTE-datetime, once there is a period,
> there should be one or more digits. It sounds like anything else is an error.
Ditto.
> > + bool ok;
> > + int millisecond;
> > + if (digitsLength == 1) {
> > + ok = toInt(src, length, i, 1, &millisecond);
> > + millisecond *= 100;
> > + } else if (digitsLength == 2) {
> > + ok = toInt(src, length, i, 2, &millisecond);
> > + millisecond *= 10;
> > + } else // digitsLength >= 3
> > + ok = toInt(src, length, i, 3, &millisecond);
> > + if (ok) {
> > + m_millisecond = millisecond;
> > + i += digitsLength;
> > + } else
> > + --i;
>
> I don't understand this --i. It is "unreading" the '.' but why isn't that done
> when digitsLength is 0 as well?
This "else" clause was attached to a wrong "if". It should be attached to "if
(digitsLength > 0)" to recover from an input like "23:59:59."
I found "if (ok)" made no sense because these toInt() calls parse number-only
strings.
I changed the code here.
> > diff --git a/WebCore/html/ISODateTime.h b/WebCore/html/ISODateTime.h
> > + * Copyright (c) 2009, Google Inc. All rights reserved.
>
> capital (C), no comma after 2009
Done.
> > + // The following six functions parse the input `src' of which length is
>
> "of which" -> "whose"
Done.
> > + // `src', `end' and `date' must be non-null. The `src' string doesn't needs to
>
> "needs" -> "need"
Done.
> > + // The functions return true if the parsing succeeds, and sets the next index
> > + // of the last consumed character to `*end'. The leading extra characters cause
> Consider using this:
> // The functions return true if the parsing succeeds and set 'end' to the
> index after
> // the last character consumed. Extra leading characters cause
> // parse failures, and trailing extra characters don't cause parse
> failures.
Done.
> > + enum {
> > + SUNDAY = 0,
> > + MONDAY,
> > + TUESDAY,
> > + WEDNESDAY,
> > + THURSDAY,
> > + FRIDAY,
> > + SATURDAY,
> > + };
>
> Enum members should user InterCaps with an initial capital letter.
> - http://webkit.org/coding/coding-style.html
Done.
Thank you very very much for loooong review!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list