Bug 84935

Created attachment 138968 [details] Patch

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

Created attachment 139136 [details] Patch 2 Add tests

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

Created attachment 139583 [details] Patch 3

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

Committed r115713: <http://trac.webkit.org/changeset/115713>