Bug 84935

Summary: [Chromium/Windows] Add LocalizedDateWin
Product: WebKit Reporter: Kent Tamura <tkent>
Component: PlatformAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, morrita
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 84388    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3 haraken: review+

Description Kent Tamura 2012-04-26 01:47:43 PDT 
[Chromium/Windows] Add LocalizedDateWin
Comment 1 Kent Tamura 2012-04-26 03:52:00 PDT 
Created attachment 138968 [details]
Patch
Comment 2 Kent Tamura 2012-04-26 03:53:17 PDT 
Morita-san, Haraken-san,
This patch is for Windows, but the majority of the code is string manipulation.  So I hope you can review this.
Comment 3 Kentaro Hara 2012-04-26 10:22:44 PDT 
Comment on attachment 138968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138968&action=review

> Source/WebCore/ChangeLog:17
> +        No new tests. The behavior depends on system settings.

Landing this patch without tests seems a bit dangerous. Isn't there any way to make it testable?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:67
> +    buffer.shrink(bufferSizeWithNUL - 1);

Is this needed?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:87
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

Shall we put labels->reserveCapacity(WTF_ARRAY_LENGTH(types))?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:91
> +            labels->shrink(0);

Nit: I would prefer labels->clear() for such cases.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:103
> +    static Vector<String>* labels = createShortMonthLabels().leakPtr();

- Please use DEFINE_STATIC_LOCAL() for a static variable declaration.

- I know this code is correct, but using adoptPtr() => release() => leakPtr() seems a bit dirty. How about writing like this?

static const Vector<String>& shortMonthLabels()
{
    DEFINE_STATIC_LOCAL(Vector<String>, labels, ());
    if (!labels.isEmpty())
        return labels;

    ...; /* The code in createShortMonthLabels() */
}

> Source/WebCore/platform/text/LocalizedDateWin.cpp:152
> +static unsigned countContinuousLetters(const String& format, unsigned i)

Nit: 'i' => 'index', for clarification? You are using 'index' in parseNumberXXX().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:184
> +                if (format[i - 1] == '\'')

Maybe I am missing something, why do we need to handle '' specially? In other words, why do we need a different logic for '' and 'foo'?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:193
> +            if (i > 0 && format[i-1] == '\'')

When can this happen?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:227
> +            // HTML5 date supports only A.D.

Nit: It would be interesting if HTML5 supported B.C.:)

> Source/WebCore/platform/text/LocalizedDateWin.cpp:261
> +        if (input.substring(index, shortLabels[i].length()) == shortLabels[i]) {

What happens if input="Mayyyyyy" and shortLabels[i]="May". This will return 4, but is it expected?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:268
> +        if (input.substring(index, labels[i].length()) == labels[i]) {

Ditto.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:293
> +            if (day < 1 || day > 31)

Just an out-of-curiosity question: Where is the difference of the number of monthly dates treated? e.g. There are only 30 days in April.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:355
> +static void appendTwoDigitNumber(int value, StringBuilder& buffer)

appendTwoDigit*s*Number()? Dunno.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:362
> +static void appendFourDigitNumber(int value, StringBuilder& buffer)

appendFourDigit*s*Number?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:405
> +            appendFourDigitNumber(year, buffer);

Maybe I am missing something, but isn't it 'else { appendFourDigitNumber(year, buffer); }'?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:413
> +            appendFourDigitNumber(year, buffer);

Ditto.

> Source/WebCore/platform/text/LocalizedDateWin.cpp:508
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

You can put labels->reserveCapacity(WTF_ARRAY_LENGTH(types));

> Source/WebCore/platform/text/LocalizedDateWin.cpp:512
> +            labels->shrink(0);

labels->clear() is better?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:525
> +            labels->append("January");
> +            labels->append("February");
> +            labels->append("March");
> +            labels->append("April");
> +            labels->append("May");
> +            labels->append("June");
> +            labels->append("July");
> +            labels->append("August");
> +            labels->append("September");
> +            labels->append("October");
> +            labels->append("November");
> +            labels->append("December");

Why didn't you use WTF::monthName[i]?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:534
> +    static Vector<String>* labels = createMonthLabels().leakPtr();

The same comment for shortMonthLabels().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:549
> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);

You can put labels.reserveCapacity(WTF_ARRAY_LENGTH(types));

> Source/WebCore/platform/text/LocalizedDateWin.cpp:553
> +            labels->shrink(0);

labels->clear() is better?

> Source/WebCore/platform/text/LocalizedDateWin.cpp:567
> +    static Vector<String>* labels = createWeekDayShortLabels().leakPtr();

The same comment for shortMonthLabels().

> Source/WebCore/platform/text/LocalizedDateWin.cpp:582
> +    static unsigned firstDayOfWeek = createFirstDayOfWeek();

Please use DEFINE_STATIC_LOCAL().
Comment 4 Kent Tamura 2012-04-26 22:30:55 PDT 
Comment on attachment 138968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138968&action=review

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:67
>> +    buffer.shrink(bufferSizeWithNUL - 1);
> 
> Is this needed?

Yes.  GetLocaleInfo adds a terminating '\0'.  We don't need it.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:87
>> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);
> 
> Shall we put labels->reserveCapacity(WTF_ARRAY_LENGTH(types))?

done

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:91
>> +            labels->shrink(0);
> 
> Nit: I would prefer labels->clear() for such cases.

clear() is inefficient in this case because it shrink the capacity.  We'd like to keep the capacity.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:152
>> +static unsigned countContinuousLetters(const String& format, unsigned i)
> 
> Nit: 'i' => 'index', for clarification? You are using 'index' in parseNumberXXX().

done.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:184
>> +                if (format[i - 1] == '\'')
> 
> Maybe I am missing something, why do we need to handle '' specially? In other words, why do we need a different logic for '' and 'foo'?

According to MSDN,  '' produces one single quote though 'foo' produces foo.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:193
>> +            if (i > 0 && format[i-1] == '\'')
> 
> When can this happen?

According to MSDn, 'foo''bar' produces foo'bar
That is to say, '' always produces one single quote regardless of inQuote.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:261
>> +        if (input.substring(index, shortLabels[i].length()) == shortLabels[i]) {
> 
> What happens if input="Mayyyyyy" and shortLabels[i]="May". This will return 4, but is it expected?

Yes, parsing the month succeeds, then probably remaining "yyyy" causes an error.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:293
>> +            if (day < 1 || day > 31)
> 
> Just an out-of-curiosity question: Where is the difference of the number of monthly dates treated? e.g. There are only 30 days in April.

April 31 will be recognized as May 1 by dateToDaysFrom1970().  I don't have strong preference to this behavior.  It's easy to implement.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:355
>> +static void appendTwoDigitNumber(int value, StringBuilder& buffer)
> 
> appendTwoDigit*s*Number()? Dunno.

done

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:405
>> +            appendFourDigitNumber(year, buffer);
> 
> Maybe I am missing something, but isn't it 'else { appendFourDigitNumber(year, buffer); }'?

You're right!

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:508
>> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);
> 
> You can put labels->reserveCapacity(WTF_ARRAY_LENGTH(types));

done

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:525
>> +            labels->append("December");
> 
> Why didn't you use WTF::monthName[i]?

WTF::monthName is a set of short names such as "Jan". We need full names.

>> Source/WebCore/platform/text/LocalizedDateWin.cpp:549
>> +    OwnPtr<Vector<String> > labels = adoptPtr(new Vector<String>);
> 
> You can put labels.reserveCapacity(WTF_ARRAY_LENGTH(types));

done.
Comment 5 Kent Tamura 2012-04-26 23:11:27 PDT 
Created attachment 139136 [details]
Patch 2

Add tests
Comment 6 Kentaro Hara 2012-04-27 10:54:05 PDT 
Comment on attachment 139136 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=139136&action=review

> Source/WebCore/platform/text/LocaleWin.cpp:55
> +    : m_lcid(lcid)

Shall we initialize m_shortMonthLabels etc? WebKit requires to initialize all member variables unless there is a performance concern.
See http://www.webkit.org/coding/coding-style.html (Other punctuations - 1.)

> Source/WebCore/platform/text/LocaleWin.cpp:81
> +PassOwnPtr<LocaleWin> LocaleWin::create(LCID lcid)
> +{
> +    return adoptPtr(new LocaleWin(lcid));
> +}
> +
> +LocaleWin* LocaleWin::currentLocale()
> +{
> +    // Ideally we should make LCID from defaultLanguage(). But
> +    // ::LocaleNameToLCID() is available since Windows Vista.
> +    static LocaleWin* currentLocale = LocaleWin::create(LOCALE_USER_DEFAULT).leakPtr();
> +    return currentLocale;
> +}

Shall we eliminate adoptPtr() and leakPtr(), in the same way as initializeShortMonthLabels()?

> Source/WebCore/platform/text/LocaleWin.cpp:98
> +void LocaleWin::initializeShortMonthLabels()

ensureShortMonthLabels() might be a better name?

> Source/WebCore/platform/text/LocaleWin.cpp:101
> +    if (m_shortMonthLabels.size())
> +        return;

Do we need to call .size()? 'if (m_shortMonthLabels)' is not enough?

I like the approach of using m_shortMonthLabels, but how many LocalWin objects will be generated? If it can be many, using static variables would be a better approach.

> Source/WebCore/platform/text/LocaleWin.cpp:194
> +        UChar ch = format[i];

Let's add ASSERT(i < format.length()).

> Source/WebCore/platform/text/LocaleWin.cpp:277
> +        if (equalIgnoringCase(input.substring(index, m_monthLabels[i].length()), m_monthLabels[i])) {
> +            index += m_monthLabels[i].length();

Please use 'unsigned length = m_monthLabels[i].length();' in order to avoid accessing .length() twice.

> Source/WebCore/platform/text/LocaleWin.cpp:283
> +        if (equalIgnoringCase(input.substring(index, m_shortMonthLabels[i].length()), m_shortMonthLabels[i])) {
> +            index += m_shortMonthLabels[i].length();

Ditto.

> Source/WebCore/platform/text/LocaleWin.cpp:313
> +            if (input.substring(inputIndex, data.length()) == data)
> +                inputIndex += data.length();

Ditto.

> Source/WebCore/platform/text/LocaleWin.cpp:465
> +void LocaleWin::initializeShortDateTokens()

ensureShortDateTokens() would be a better name?

> Source/WebCore/platform/text/LocaleWin.cpp:468
> +    if (m_shortDateTokens.size())
> +        return;

Same comment for m_shortMonthLabels.

> Source/WebCore/platform/text/LocaleWin.cpp:482
> +            buffer.append(dayText.isEmpty() ? "Day" : dayText);

Let's write

    String dayTextString = dayText.isEmpty() ? "Day" : dayText;

outside of the for loop, and then use dayTextString here. (I hope the complier will do the optimization though.)

> Source/WebCore/platform/text/LocaleWin.cpp:488
> +            buffer.append(monthText.isEmpty() ? "Month" : monthText);

Ditto.

> Source/WebCore/platform/text/LocaleWin.cpp:493
> +            buffer.append(yearText.isEmpty() ? "Year" : yearText);

Ditto.

> Source/WebCore/platform/text/LocaleWin.cpp:511
> +void LocaleWin::initializeMonthLabels()

ensureMonthLabels() would be a better name?

> Source/WebCore/platform/text/LocaleWin.cpp:514
> +    if (m_monthLabels.size())
> +        return;

Same comment for m_monthShortLabels.

> Source/WebCore/platform/text/LocaleWin.cpp:546
> +            m_monthLabels.reserveCapacity(12);
> +            m_monthLabels.append("January");
> +            m_monthLabels.append("February");
> +            m_monthLabels.append("March");
> +            m_monthLabels.append("April");
> +            m_monthLabels.append("May");
> +            m_monthLabels.append("June");
> +            m_monthLabels.append("July");
> +            m_monthLabels.append("August");
> +            m_monthLabels.append("September");
> +            m_monthLabels.append("October");
> +            m_monthLabels.append("November");
> +            m_monthLabels.append("December");

It is OK, but it might be better to implement WTF::monthFullName[] and use it here.

> Source/WebCore/platform/text/LocaleWin.cpp:552
> +void LocaleWin::initializeWeekDayShortLabels()

ensureWeekDayShortLabels()?

> Source/WebCore/platform/text/LocaleWin.cpp:555
> +    if (m_weekDayShortLabels.size())
> +        return;

Same comment for m_monthShortLabels.

> Source/WebCore/platform/text/LocaleWin.h:63
> +    LocaleWin(LCID);

'explicit' please.

> Source/WebCore/platform/text/LocaleWin.h:79
> +    Vector<String> m_monthLabels;

Shouldn't this be in #if ENABLE(CALENDAR_PICKER)? I am not sure why only m_weekDayShortLabels and m_firstDayOfWeek are in the enable flag.

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:58
> +    EXPECT_STREQ("year/month/day", LocaleWin::dateFormatText("yyyy/MMMM/dddd", "year", "month", "day").utf8().data());

+ EXPECT_STREQ("year/month/day/year/month/day", LocaleWin::dateFormatText("yyyy/MMMM/dddd/yyyy/MMMM/dddd", "year", "month", "day").utf8().data());
+ EXPECT_STREQ("YY/mm/DD", LocaleWin::dateFormatText("YY/mm/DD", "year", "month", "day").utf8().data());

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:71
> +    EXPECT_STREQ("4/7/8", locale->formatDate("M/d/y", 2012, 2008, April, 7).utf8().data());

Please add tests for the "both-side" of edge cases.
+ EXPECT_STREQ("4/7/2007", locale->formatDate("M/d/y", 2012, 2007, April, 7).utf8().data());

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:72
> +    EXPECT_STREQ("4/7/7", locale->formatDate("M/d/y", 2012, 2017, April, 7).utf8().data());

+ EXPECT_STREQ("4/7/2018", locale->formatDate("M/d/y", 2012, 2018, April, 7).utf8().data());

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:78
> +    EXPECT_STREQ("04/07/63", locale->formatDate("MM/dd/yy", 2012, 1963, April, 7).utf8().data());

+ EXPECT_STREQ("04/07/1962", locale->formatDate("MM/dd/yy", 2012, 1962, April, 7).utf8().data());

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:87
> +    EXPECT_STREQ("Jan/7/2012", locale->formatDate("MMM/d/yyyy", 2012, 2012, January, 7).utf8().data());
> +    EXPECT_STREQ("Feb/7/2008", locale->formatDate("MMM/d/yyyy", 2012, 2008, February, 7).utf8().data());
> +    EXPECT_STREQ("Mar/7/2017", locale->formatDate("MMM/d/yyyy", 2012, 2017, March, 7).utf8().data());
> +    EXPECT_STREQ("Dec/31/2062", locale->formatDate("MMM/d/yyyy", 2012, 2062, December, 31).utf8().data());
> +    EXPECT_STREQ("May/7/0002", locale->formatDate("MMM/d/yyyy", 2012, 2, May, 7).utf8().data());

Let's add test cases for other months.

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:93
> +    EXPECT_STREQ("June-7-22012", locale->formatDate("MMMM-d-yyyy", 2012, 22012, June, 7).utf8().data());
> +    EXPECT_STREQ("July-7-12008", locale->formatDate("MMMM-d-yyyy", 2012, 12008, July, 7).utf8().data());
> +    EXPECT_STREQ("August-7-2017", locale->formatDate("MMMM-d-yyyy", 2012, 2017, August, 7).utf8().data());
> +    EXPECT_STREQ("September-31-2062", locale->formatDate("MMMM-d-yyyy", 2012, 2062, September, 31).utf8().data());
> +    EXPECT_STREQ("October-7-0002", locale->formatDate("MMMM-d-yyyy", 2012, 2, October, 7).utf8().data());

Let's add test cases for other months.

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:94
> +}

Please add test cases for invalid baseyear, year, month and date. baseyear=-1, year=-1, month=12, date=32 etc.

> Source/WebKit/chromium/tests/LocaleWinTest.cpp:122
> +}

Please add test cases for invalid baseyear, year, month and date. e.g. "Mayyyy/4/2012", "May/0/2012", "May/32/2012", "May/4/-1" etc

Please add test cases that include spaces. e.g. " May / 4 / 2012 " etc.
Comment 7 Kent Tamura 2012-04-30 21:54:45 PDT 
Comment on attachment 139136 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=139136&action=review

>> Source/WebCore/platform/text/LocaleWin.cpp:55
>> +    : m_lcid(lcid)
> 
> Shall we initialize m_shortMonthLabels etc? WebKit requires to initialize all member variables unless there is a performance concern.
> See http://www.webkit.org/coding/coding-style.html (Other punctuations - 1.)

We don't need to initialize m_shortMonthLabels, etc. because they are Vector<> and initialized automatically.

BTW, you can refer to a specific style rule by a URL; e.g. http://www.webkit.org/coding/coding-style.html#punctuation-member-init

>> Source/WebCore/platform/text/LocaleWin.cpp:81
>> +}
> 
> Shall we eliminate adoptPtr() and leakPtr(), in the same way as initializeShortMonthLabels()?

We shouldn't.  We have "PassOwnPtr<LocaleWin> create()", and always use it.

>> Source/WebCore/platform/text/LocaleWin.cpp:98
>> +void LocaleWin::initializeShortMonthLabels()
> 
> ensureShortMonthLabels() might be a better name?

Renamed.

>> Source/WebCore/platform/text/LocaleWin.cpp:101
>> +        return;
> 
> Do we need to call .size()? 'if (m_shortMonthLabels)' is not enough?
> 
> I like the approach of using m_shortMonthLabels, but how many LocalWin objects will be generated? If it can be many, using static variables would be a better approach.

AFAIK, Vector->bool conversion is not defined.

It is possible to have multiple LocaleWin objects for various locales at the same time in the future.  So we should make this a data member.

>> Source/WebCore/platform/text/LocaleWin.cpp:194
>> +        UChar ch = format[i];
> 
> Let's add ASSERT(i < format.length()).

I don't think it is helpful.  The above 'for' ensures it.

>> Source/WebCore/platform/text/LocaleWin.cpp:277
>> +            index += m_monthLabels[i].length();
> 
> Please use 'unsigned length = m_monthLabels[i].length();' in order to avoid accessing .length() twice.

done.

>> Source/WebCore/platform/text/LocaleWin.cpp:482
>> +            buffer.append(dayText.isEmpty() ? "Day" : dayText);
> 
> Let's write
> 
>     String dayTextString = dayText.isEmpty() ? "Day" : dayText;
> 
> outside of the for loop, and then use dayTextString here. (I hope the complier will do the optimization though.)

done.

>> Source/WebCore/platform/text/LocaleWin.cpp:546
>> +            m_monthLabels.append("December");
> 
> It is OK, but it might be better to implement WTF::monthFullName[] and use it here.

done.

>> Source/WebCore/platform/text/LocaleWin.h:63
>> +    LocaleWin(LCID);
> 
> 'explicit' please.

done.

>> Source/WebCore/platform/text/LocaleWin.h:79
>> +    Vector<String> m_monthLabels;
> 
> Shouldn't this be in #if ENABLE(CALENDAR_PICKER)? I am not sure why only m_weekDayShortLabels and m_firstDayOfWeek are in the enable flag.

m_monthLabels is used by parseDate() and monthLabels().  parseDate() is used even if CALENDAR_PICKER is disabled. monthLabels() is called only if CALENDAR_PICKER is enabled.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:58
>> +    EXPECT_STREQ("year/month/day", LocaleWin::dateFormatText("yyyy/MMMM/dddd", "year", "month", "day").utf8().data());
> 
> + EXPECT_STREQ("year/month/day/year/month/day", LocaleWin::dateFormatText("yyyy/MMMM/dddd/yyyy/MMMM/dddd", "year", "month", "day").utf8().data());
> + EXPECT_STREQ("YY/mm/DD", LocaleWin::dateFormatText("YY/mm/DD", "year", "month", "day").utf8().data());

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:71
>> +    EXPECT_STREQ("4/7/8", locale->formatDate("M/d/y", 2012, 2008, April, 7).utf8().data());
> 
> Please add tests for the "both-side" of edge cases.
> + EXPECT_STREQ("4/7/2007", locale->formatDate("M/d/y", 2012, 2007, April, 7).utf8().data());

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:72
>> +    EXPECT_STREQ("4/7/7", locale->formatDate("M/d/y", 2012, 2017, April, 7).utf8().data());
> 
> + EXPECT_STREQ("4/7/2018", locale->formatDate("M/d/y", 2012, 2018, April, 7).utf8().data());

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:78
>> +    EXPECT_STREQ("04/07/63", locale->formatDate("MM/dd/yy", 2012, 1963, April, 7).utf8().data());
> 
> + EXPECT_STREQ("04/07/1962", locale->formatDate("MM/dd/yy", 2012, 1962, April, 7).utf8().data());

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:87
>> +    EXPECT_STREQ("May/7/0002", locale->formatDate("MMM/d/yyyy", 2012, 2, May, 7).utf8().data());
> 
> Let's add test cases for other months.

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:93
>> +    EXPECT_STREQ("October-7-0002", locale->formatDate("MMMM-d-yyyy", 2012, 2, October, 7).utf8().data());
> 
> Let's add test cases for other months.

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:94
>> +}
> 
> Please add test cases for invalid baseyear, year, month and date. baseyear=-1, year=-1, month=12, date=32 etc.

done.

>> Source/WebKit/chromium/tests/LocaleWinTest.cpp:122
>> +}
> 
> Please add test cases for invalid baseyear, year, month and date. e.g. "Mayyyy/4/2012", "May/0/2012", "May/32/2012", "May/4/-1" etc
> 
> Please add test cases that include spaces. e.g. " May / 4 / 2012 " etc.

done.
Comment 8 Kent Tamura 2012-04-30 22:20:51 PDT 
Created attachment 139583 [details]
Patch 3
Comment 9 Kentaro Hara 2012-05-01 00:20:52 PDT 
Comment on attachment 139583 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=139583&action=review

Looks good to me

> Source/WebCore/ChangeLog:72
> +        Added. The follwoing functions simply delegate to LocaleWin::currentLocale().

Nit: follwoing => following

> Source/WebCore/platform/text/LocaleWin.cpp:79
> +    static LocaleWin *currentLocale = LocaleWin::create(LOCALE_USER_DEFAULT).leakPtr();

Can't we use DEFINE_STATIC_LOCAL()?
Comment 10 Kent Tamura 2012-05-01 00:50:29 PDT 
Comment on attachment 139583 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=139583&action=review

Thank you for the review!



>> Source/WebCore/ChangeLog:72
>> +        Added. The follwoing functions simply delegate to LocaleWin::currentLocale().
> 
> Nit: follwoing => following

will fix.

>> Source/WebCore/platform/text/LocaleWin.cpp:79
>> +    static LocaleWin *currentLocale = LocaleWin::create(LOCALE_USER_DEFAULT).leakPtr();
> 
> Can't we use DEFINE_STATIC_LOCAL()?

We can't. DEFINE_STATIC_LOCAL always uses "new" in it.
Comment 11 Kent Tamura 2012-05-01 00:53:43 PDT 
Committed r115713: <http://trac.webkit.org/changeset/115713>