FTP: use ICU to parse month abbreviations.

FTP: use ICU to parse month abbreviations.

In theory we should be able to parse localized listings
in more languages now.

This also allows us to get rid of hardcoded special cases
for German.

BUG=65917
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78826

[eroman, 2011-03-16 00:46:44]

lgtm

http://codereview.chromium.org/6697021/diff/1001/net/ftp/ftp_util.h
File net/ftp/ftp_util.h (right):

http://codereview.chromium.org/6697021/diff/1001/net/ftp/ftp_util.h#newcode30
net/ftp/ftp_util.h:30: // Convert abbreviated month (like Nov) to its number (in
range 1-12).
nit: We should be using tense like "Converts" for these comments.
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

[Paweł Hajdan Jr., 2011-03-16 20:17:27]

jungshik: ping

http://codereview.chromium.org/6697021/diff/1001/net/ftp/ftp_util.h
File net/ftp/ftp_util.h (right):

http://codereview.chromium.org/6697021/diff/1001/net/ftp/ftp_util.h#newcode30
net/ftp/ftp_util.h:30: // Convert abbreviated month (like Nov) to its number (in
range 1-12).
On 2011/03/16 00:46:45, eroman wrote:
> nit: We should be using tense like "Converts" for these comments.
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

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

Hmm.. codereview is behaving in a strange manner. My inline comments are not
automatically included.  I'm copy'n'pasting them manually below: 

 116   const icu::Locale* locales
=icu::Locale::getAvailableLocales(locales_count);
 
This will return a lot more entries than we have datetime format symbols for. 

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

 120   for (int32_t locale = 0; locale < locales_count; locale++) {

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.

 134     // Loop over all abbreviated month names in given locale.
 135     // An alternative solution (to parse |text| in given locale) is more
 136     // lenient, and may accept more than we want even with
setLenient(false).

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.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode116
net/ftp/ftp_util.cc:116: const icu::Locale* locales =
icu::Locale::getAvailableLocales(locales_count);
This will return a lot more entries than we have datetime format symbols for. 

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

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode120
net/ftp/ftp_util.cc:120: for (int32_t locale = 0; locale < locales_count;
locale++) {
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.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode136
net/ftp/ftp_util.cc:136: // lenient, and may accept more than we want even with
setLenient(false).
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.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode152
net/ftp/ftp_util.cc:152: base::Time* result) {
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]

Thank you for review, please take another look.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode116
net/ftp/ftp_util.cc:116: const icu::Locale* locales =
icu::Locale::getAvailableLocales(locales_count);
On 2011/03/17 22:59:08, Jungshik Shin wrote:
> This will return a lot more entries than we have datetime format symbols for. 
> 
> You can use DateFormat::getAvailableLocale(), instead. 

Good idea, done.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode120
net/ftp/ftp_util.cc:120: for (int32_t locale = 0; locale < locales_count;
locale++) {
On 2011/03/17 22:59:08, Jungshik Shin wrote:
> 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. 

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 .

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

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.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode136
net/ftp/ftp_util.cc:136: // lenient, and may accept more than we want even with
setLenient(false).
On 2011/03/17 22:59:08, Jungshik Shin wrote:
> 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. 

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.

http://codereview.chromium.org/6697021/diff/5001/net/ftp/ftp_util.cc#newcode152
net/ftp/ftp_util.cc:152: base::Time* result) {
On 2011/03/17 22:59:08, Jungshik Shin wrote:
> Hmm... Do all the ftp servers use 'day month year' order? Callers of this
> function assume that. Perhaps, they do, but ...

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.

[jungshik at Google, 2011-03-19 01:11:20]

After writing my comment, it also occurred to me that displaying file names is
not a perf-critical part at all. So, I guess it's fine to do as you do now.

BTW, have you uploaded a new patch that uses DateFormat::getAvailableLocale?
With that change added, LGTM

[Paweł Hajdan Jr., 2011-03-19 09:30:10]

On 2011/03/19 01:11:20, Jungshik Shin wrote:
> BTW, have you uploaded a new patch that uses DateFormat::getAvailableLocale?

Oh, indeed I forgot to upload. Done that now.

> With that change added, LGTM

Thank you.