FTP: Multiple fixes for localized directory listings:

FTP: Multiple fixes for localized directory listings:

- fix detection of KOI8-R and possibly other encodings
- fix parsing Russian month names

When detecting the listing encoding, we need to not only
check whether the data can be converted using given encoding,
but also whether the result can be parsed as a valid directory listing.

Also, we only need to compare the first three characters of the
abbreviated month name, because that's how they're abbreviated
in FTP directory listings.

Finally, the Russian directory listings have swapped the "month" and "day of month" columns.

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

[Paweł Hajdan Jr., 2011-03-29 17:58:49]

+jshin for ICU parts

[Paweł Hajdan Jr., 2011-03-30 15:50:02]

ping

[gavinp, 2011-03-30 17:14:23]

Good stuff.  I just found the tests difficult to understand, so we might need
some renaming there?

As well, do we test russian month names with standard month/dayofmonth ordering,
and english month names with inverse month/dayofmonth ordering?

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_ls_unittest.cc (right):

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_ls_unittest.cc:47: { "drwxrwxrwx   1 owner 
  group               1024 Sep 13  0:30 audio",
What's going on with 0 length files?

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_unittest.cc (right):

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:49: "dir-listing-ls-25",
These filenames should include at least the name of the character set and/or
language we're testing for: so "dir-listing-ls-25-russian", etc...

Even better would be to capture information in the test like the
inversion/noninversion of months in the test...

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

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_util.cc#newcode143
net/ftp/ftp_util.cc:143: if (months[month].caseCompare(0, 3, unicode_text, 0) ==
0) {
NIT: maybe worth noting that caseCompare works when months[month] is zero, one
or two chars long too.  It took a bit of digging to convince myself of this.

[Paweł Hajdan Jr., 2011-04-01 15:55:27]

Thank you for review, please take another look.

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_ls_unittest.cc (right):

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_ls_unittest.cc:47: { "drwxrwxrwx   1 owner 
  group               1024 Sep 13  0:30 audio",
On 2011/03/30 17:14:23, gavinp wrote:
> What's going on with 0 length files?

I've changed the 0 to 1024 because 0 could be confused with some other column (I
don't remember which one, can check if you want).

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
File net/ftp/ftp_directory_listing_parser_unittest.cc (right):

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_directory_listin...
net/ftp/ftp_directory_listing_parser_unittest.cc:49: "dir-listing-ls-25",
On 2011/03/30 17:14:23, gavinp wrote:
> These filenames should include at least the name of the character set and/or
> language we're testing for: so "dir-listing-ls-25-russian", etc...
> 
> Even better would be to capture information in the test like the
> inversion/noninversion of months in the test...

I've added some comments here, not sure how much they're helping. Generally the
listings here test parsing of the _entire_ listing, not any of its "features"
like swapped date columns.

On the other hand, encoding is really one of the things being tested here, and
it indeed may be difficult to recover later (for example, the "file" command
failed to recognize the KOI8-R encoding, saying it's ISO-8859 family; they're
just very similar).

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

http://codereview.chromium.org/6718043/diff/2001/net/ftp/ftp_util.cc#newcode143
net/ftp/ftp_util.cc:143: if (months[month].caseCompare(0, 3, unicode_text, 0) ==
0) {
On 2011/03/30 17:14:23, gavinp wrote:
> NIT: maybe worth noting that caseCompare works when months[month] is zero, one
> or two chars long too.  It took a bit of digging to convince myself of this.

Done.

[jungshik at Google, 2011-04-01 18:28:41]

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
File base/i18n/icu_encoding_detection.cc (right):

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:21: UCharsetDetector* detector =
ucsdet_open(&status);
You might consider using scoped_ptr_malloc. Well, the overhead for that seems
larger than savings you'd get here... perhaps, it's not worth it.

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:54: // first phase.
hmm... if ucsdet_detectAll does not return all the detectable encodings that are
compatible with an input text, that's a bug. Could you send me a couple of
sample input strings to me by email?

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:70: // Append all encodings not included
earlier, in arbitrary order.
This behavior should be documented in the header file. Adding all the detectable
encodings (regardless of whether or not they're compatible with the input text)
is not what customers of this function would expect. 

Or, instead of adding all the detectable encodings in the loop below, each
encoding to add should be tested to see if they can be used to convert the input
before being added.

Anyway, the above ICU 'bug' is fixed. This should not be necessary once it's
fixed. So, please send me details about cases where 'ucsdet_detectAll' does not
behave as advertised.

http://codereview.chromium.org/6718043/diff/11001/net/ftp/ftp_directory_listi...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/6718043/diff/11001/net/ftp/ftp_directory_listi...
net/ftp/ftp_directory_listing_parser_ls.cc:103: // sure no column offset works
above (this is a more strict way).
the other day, the order would be always the same when I asked about this. Sorry
I can't help :-)

http://codereview.chromium.org/6718043/diff/11001/net/ftp/ftp_directory_listi...
File net/ftp/ftp_directory_listing_parser_unittest.cc (right):

http://codereview.chromium.org/6718043/diff/11001/net/ftp/ftp_directory_listi...
net/ftp/ftp_directory_listing_parser_unittest.cc:46: "dir-listing-ls-22",  //
TODO(phajdan.jr): should use windows-1251 encoding.
Are these TODO still valid given that you're adding windows-1251 in this CL?

[Paweł Hajdan Jr., 2011-04-01 18:45:42]

Thank you for mentioning fixing the underlying problem in ICU. Please let me
know if you'd still rather get an e-mail than look for some bug attachments, I
can just send the file to you.

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
File base/i18n/icu_encoding_detection.cc (right):

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:21: UCharsetDetector* detector =
ucsdet_open(&status);
On 2011/04/01 18:28:41, Jungshik Shin wrote:
> You might consider using scoped_ptr_malloc. Well, the overhead for that seems
> larger than savings you'd get here... perhaps, it's not worth it.

Yeah, I also don't think it's worth it, especially that ucsdet_close is probably
intended to be opaque, even if it just calls free.

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:54: // first phase.
On 2011/04/01 18:28:41, Jungshik Shin wrote:
> hmm... if ucsdet_detectAll does not return all the detectable encodings that
are
> compatible with an input text, that's a bug. Could you send me a couple of
> sample input strings to me by email? 

See the raw listing attached to the bug. That was the test case I used.

http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:70: // Append all encodings not included
earlier, in arbitrary order.
On 2011/04/01 18:28:41, Jungshik Shin wrote:
> This behavior should be documented in the header file. Adding all the
detectable
> encodings (regardless of whether or not they're compatible with the input
text)
> is not what customers of this function would expect. 

Well, even before this change that was true, i.e. ucsdet_detectAll can return
encodings that fail to decode the text and it is documented by ICU.

> Or, instead of adding all the detectable encodings in the loop below, each
> encoding to add should be tested to see if they can be used to convert the
input
> before being added.

The FTP code has to do that anyway, so for efficiency I didn't include that code
here. However, it could indeed be a better API if only "valid" encodings were
returned by this function. If you think it's worth to change, I can do that.

> Anyway, the above ICU 'bug' is fixed. This should not be necessary once it's
> fixed. So, please send me details about cases where 'ucsdet_detectAll' does
not
> behave as advertised. 

Right, this shouldn't be necessary. Do you want me to add a TODO(jshin)?

[jungshik at Google, 2011-04-04 10:26:02]

On Fri, Apr 1, 2011 at 11:45 AM, <phajdan.jr@chromium.org> wrote:

> Thank you for mentioning fixing the underlying problem in ICU. Please let
> me
> know if you'd still rather get an e-mail than look for some bug
> attachments, I
> can just send the file to you.
>
>
>
>
>
http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
> File base/i18n/icu_encoding_detection.cc (right):
>
>
>
http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
> base/i18n/icu_encoding_detection.cc:21: UCharsetDetector* detector =
> ucsdet_open(&status);
> On 2011/04/01 18:28:41, Jungshik Shin wrote:
>
>> You might consider using scoped_ptr_malloc. Well, the overhead for
>>
> that seems
>
>> larger than savings you'd get here... perhaps, it's not worth it.
>>
>
> Yeah, I also don't think it's worth it, especially that ucsdet_close is
> probably intended to be opaque, even if it just calls free.
>
>
>
>
http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
> base/i18n/icu_encoding_detection.cc:54: // first phase.
> On 2011/04/01 18:28:41, Jungshik Shin wrote:
>
>> hmm... if ucsdet_detectAll does not return all the detectable
>>
> encodings that are
>
>> compatible with an input text, that's a bug. Could you send me a
>>
> couple of
>
>> sample input strings to me by email?
>>
>
> See the raw listing attached to the bug. That was the test case I used.


Ok. I'll take a look.

>
>
>
>
http://codereview.chromium.org/6718043/diff/11001/base/i18n/icu_encoding_dete...
> base/i18n/icu_encoding_detection.cc:70: // Append all encodings not
> included earlier, in arbitrary order.
> On 2011/04/01 18:28:41, Jungshik Shin wrote:
>
>> This behavior should be documented in the header file. Adding all the
>>
> detectable
>
>> encodings (regardless of whether or not they're compatible with the
>>
> input text)
>
>> is not what customers of this function would expect.
>>
>
> Well, even before this change that was true, i.e. ucsdet_detectAll can
> return encodings that fail to decode the text and it is documented by
> ICU.


Sorry I forgot to mention the confidence level. If you include only
encodings with a confidence score >= 10, you'd only get encodings that are
consistent with the input text.



>
>
>  Or, instead of adding all the detectable encodings in the loop below,
>>
> each
>
>> encoding to add should be tested to see if they can be used to convert
>>
> the input
>
>> before being added.
>>
>
> The FTP code has to do that anyway, so for efficiency I didn't include
> that code here. However, it could indeed be a better API if only "valid"
> encodings were returned by this function. If you think it's worth to
> change, I can do that.
>
>
>  Anyway, the above ICU 'bug' is fixed. This should not be necessary
>>
> once it's
>
>> fixed. So, please send me details about cases where 'ucsdet_detectAll'
>>
> does not
>
>> behave as advertised.
>>
>
> Right, this shouldn't be necessary. Do you want me to add a TODO(jshin)?


Yup !


>
>
> http://codereview.chromium.org/6718043/
>

[Paweł Hajdan Jr., 2011-04-04 18:35:22]

Patch updated, please take another look.

[Paweł Hajdan Jr., 2011-04-05 17:11:21]

ping

[gavinp, 2011-04-05 18:55:23]

LGTM.

Nit: the date/month reversing logic is not russian specific, and a separate test
for that without the russian charset present would make this more clear.

On 2011/04/05 17:11:21, Paweł Hajdan Jr. wrote:
> ping

[jungshik at Google, 2011-04-06 06:04:37]

LGTM

On Tue, Apr 5, 2011 at 11:55 AM, <gavinp@chromium.org> wrote:

> LGTM.
>
> Nit: the date/month reversing logic is not russian specific, and a separate
> test
> for that without the russian charset present would make this more clear.
>
>
> On 2011/04/05 17:11:21, Paweł Hajdan Jr. wrote:
>
>> ping
>>
>
>
>
> http://codereview.chromium.org/6718043/
>

[Paweł Hajdan Jr., 2011-04-06 06:27:09]

On 2011/04/05 18:55:23, gavinp wrote:
> Nit: the date/month reversing logic is not russian specific, and a separate
test
> for that without the russian charset present would make this more clear.

I'm not sure about the above. Other locales might also swap the columns, but so
far Russian is the only one I know does that. Adding a test with swapped columns
and a locale which normally doesn't do that would be even more confusing IMHO.
If you think it's important to have some case for that, I can do a follow-up.

[eroman, 2011-04-07 01:37:10]

LGTM

[commit-bot: I haz the power, 2011-04-09 14:25:04]

Presubmit check for 6718043-19001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  base/i18n/icu_encoding_detection.h

Presubmit checks took 1.8s to calculate.

[commit-bot: I haz the power, 2011-04-09 14:52:43]

Try job failure for 6718043-29001 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...