[webkit-reviews] review denied: [Bug 84935] [Chromium/Windows] Add LocalizedDateWin : [Attachment 139136] Patch 2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 27 10:54:05 PDT 2012
Kentaro Hara <haraken at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 84935: [Chromium/Windows] Add LocalizedDateWin
https://bugs.webkit.org/show_bug.cgi?id=84935
Attachment 139136: Patch 2
https://bugs.webkit.org/attachment.cgi?id=139136&action=review
------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.
More information about the webkit-reviews
mailing list