FTP: use ICU to parse month abbreviations.

net/ftp/ftp_util.cc

Index: net/ftp/ftp_util.cc
diff --git a/net/ftp/ftp_util.cc b/net/ftp/ftp_util.cc
index 2633e9d5dfa4d016c353d948101f3e8a53fd9388..948fe0c528964a6d78a8bc451f1833101902436b 100644
--- a/net/ftp/ftp_util.cc
+++ b/net/ftp/ftp_util.cc
@@ -12,6 +12,7 @@
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
+#include "unicode/dtfmtsym.h"
 
 // For examples of Unix<->VMS path conversions, see the unit test file. On VMS
 // a path looks differently depending on whether it's a file or directory.
@@ -108,41 +109,39 @@ std::string FtpUtil::VMSPathToUnix(const std::string& vms_path) {
 }
 
 // static
-bool FtpUtil::ThreeLetterMonthToNumber(const string16& text, int* number) {
-  const static char* months[] = { "jan", "feb", "mar", "apr", "may", "jun",
-                                  "jul", "aug", "sep", "oct", "nov", "dec" };
-
-  for (size_t i = 0; i < arraysize(months); i++) {
-    if (LowerCaseEqualsASCII(text, months[i])) {
-      *number = i + 1;
-      return true;
+bool FtpUtil::AbbreviatedMonthToNumber(const string16& text, int* number) {
+  icu::UnicodeString unicode_text(text.data(), text.size());
+
+  int32_t locales_count;
+  const icu::Locale* locales = icu::Locale::getAvailableLocales(locales_count);

[jungshik at Google, 2011/03/17 22:59:08]

This will return a lot more entries than we have datetime format symbols for. 

You can use DateFormat::getAvailableLocale(), instead. 

[Paweł Hajdan Jr., 2011/03/18 09:04:41]

Good idea, done.

+
+  // Some FTP servers localize the date listings. To guess the locale,
+  // we loop over all available ones.
+  for (int32_t locale = 0; locale < locales_count; locale++) {

[jungshik at Google, 2011/03/17 22:59:08]

Every time this function is called, we have to go through this loop. How about
splitting this function into two functions? The first one will be used to
determine the locale. The second one will convert an abbreviated month name to a
month index in the locale determined in the first. 

In addition, we can try 'en' first before trying other locales in the first
function.

[Paweł Hajdan Jr., 2011/03/18 09:04:41]

I was considering some ways to make it faster, but I don't consider this code
performance-critical. Downloading the listing from FTP will take much more time,
especially because of multiple TCP connections.

Also, if we "lock into" a locale too easily, we can run into a variation of
http://code.google.com/p/chromium/issues/detail?id=76171 .
I was also thinking about that, but I'm not sure whether a special case is
really worth it. Also, I'd like the ICU code to be exercised a lot here, at
least for a few dev channel releases after this change lands.

+    UErrorCode status(U_ZERO_ERROR);
+
+    icu::DateFormatSymbols format_symbols(locales[locale], status);
+
+    // If we cannot get format symbols for some locale, it's not a fatal error.
+    // Just try another one.
+    if (U_FAILURE(status))
+      continue;
+
+    int32_t months_count;
+    const icu::UnicodeString* months =
+        format_symbols.getShortMonths(months_count);
+
+    // Loop over all abbreviated month names in given locale.
+    // An alternative solution (to parse |text| in given locale) is more
+    // lenient, and may accept more than we want even with setLenient(false).

[jungshik at Google, 2011/03/17 22:59:08]

Have you tried parsing a whole 'date string' instead of just a month name? Hmm,
in that case, you have to detect the language first. Perhaps, it's slower
because you need to go through supported locales. 

[Paweł Hajdan Jr., 2011/03/18 09:04:41]

Parsing a whole date string may accept more than what's valid in an "ls -l"
style listing. Moreover, there are no guarantees it _will_ accept the date
format that is valid in an "ls -l" style listing.

Because of lack of those guarantees, I'd like to not go that route even if it
would work for some version of ICU.

+    for (int32_t month = 0; month < months_count; month++) {
+      if (months[month].caseCompare(unicode_text, 0) == 0) {
+        *number = month + 1;
+        return true;
+      }
     }
   }
 
-  // Special cases for directory listings in German (other three-letter month
-  // abbreviations are the same as in English). Note that we don't need to do
-  // a case-insensitive compare here. Only "ls -l" style listings may use
-  // localized month names, and they will always start capitalized. Also,
-  // converting non-ASCII characters to lowercase would be more complicated.
-  if (text == UTF8ToUTF16("M\xc3\xa4r")) {
-    // The full month name is M-(a-umlaut)-rz (March), which is M-(a-umlaut)r
-    // when abbreviated.
-    *number = 3;
-    return true;
-  }
-  if (text == ASCIIToUTF16("Mai")) {
-    *number = 5;
-    return true;
-  }
-  if (text == ASCIIToUTF16("Okt")) {
-    *number = 10;
-    return true;
-  }
-  if (text == ASCIIToUTF16("Dez")) {
-    *number = 12;
-    return true;
-  }
-
   return false;
 }
 
@@ -153,7 +152,7 @@ bool FtpUtil::LsDateListingToTime(const string16& month, const string16& day,
                                   base::Time* result) {

[jungshik at Google, 2011/03/17 22:59:08]

Hmm... Do all the ftp servers use 'day month year' order? Callers of this
function assume that. Perhaps, they do, but ...

[Paweł Hajdan Jr., 2011/03/18 09:04:41]

They do. I consider that a part of the listing format definition. We actually
rely on this ordering to detect multiple variants of the "ls -l" listing. I've
found that to be the most reliable method.

   base::Time::Exploded time_exploded = { 0 };
 
-  if (!ThreeLetterMonthToNumber(month, &time_exploded.month))
+  if (!AbbreviatedMonthToNumber(month, &time_exploded.month))
     return false;
 
   if (!base::StringToInt(day, &time_exploded.day_of_month))