FTP: improve character encoding detection in cases where ICU's first guess is wrong.

FTP: improve character encoding detection in cases where ICU's first guess is wrong.

Instead of using ICU's first guessed encoding immediately,
we ask it for all possible encodings, try them in order,
and use the first one that works.

For some sites this still results in a gibberish being displayed,
but at least the links are clickable so the site navigation
is possible.

BUG=61073
TEST=see bug

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

[Paweł Hajdan Jr., 2010-11-13 11:14:52]

Please review:

eroman/wtc: network parts
jshin: i18n parts

I've removed the TODO(jungshik)s, because those functions are only used by FTP
code. I think that getting other clues like user's locale is likely to add to
the confusion (sites will open differently for different users), and the ICU's
documentation says the match's confidence value is not comparable between runs,
so it doesn't seem useful to use it, especially now that we have a variant that
returns all sniffed encodings.

[eroman, 2010-11-16 00:35:02]

Any unit-tests?

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
File base/i18n/icu_encoding_detection.cc (right):

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.cc:34: std::vector<std::string>* encodings) {
Please call:

encodings->clear();

At the start of the function. Otherwise change the name/documentation to make it
clear this appends rather than sets the list.

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.cc:60: return true;
In the case where we failed to get all the encoding's names (i.e. encodings is
empty), do we still want to return true?

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detection.h
File base/i18n/icu_encoding_detection.h (right):

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.h:21: bool DetectAllEncodings(const
std::string& text,
It doesn't look like the return value is used anywhere. Perhaps callers should
just expect that |encodings| may be empty and assume that is the "error" case.

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
net/ftp/ftp_directory_listing_buffer.cc:100: return
ERR_ENCODING_DETECTION_FAILED;
It seems prudent to clear converted_buffer_ before we return failures.

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
net/ftp/ftp_directory_listing_buffer.cc:104: if (!base::CodepageToUTF16(buffer_,
The structure of this function seems a little redundant. We already may have
converted successfully to UTF16 above, however we throw that work away and
re-convert down here. Can this be re-ordered a bit?

[Paweł Hajdan Jr., 2010-11-16 21:22:38]

jshin: ping!

eroman: Thanks for the review. I will add a test in next patchset in this CL.
For now please take a look at updated code.

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
File base/i18n/icu_encoding_detection.cc (right):

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.cc:34: std::vector<std::string>* encodings) {
On 2010/11/16 00:35:03, eroman wrote:
> Please call:
> 
> encodings->clear();
> 
> At the start of the function. Otherwise change the name/documentation to make
it
> clear this appends rather than sets the list.

Done.

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.cc:60: return true;
On 2010/11/16 00:35:03, eroman wrote:
> In the case where we failed to get all the encoding's names (i.e. encodings is
> empty), do we still want to return true?

Done.

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detection.h
File base/i18n/icu_encoding_detection.h (right):

http://codereview.chromium.org/4967001/diff/1/base/i18n/icu_encoding_detectio...
base/i18n/icu_encoding_detection.h:21: bool DetectAllEncodings(const
std::string& text,
On 2010/11/16 00:35:03, eroman wrote:
> It doesn't look like the return value is used anywhere. Perhaps callers should
> just expect that |encodings| may be empty and assume that is the "error" case.

That was actually my mistake. Added a check of the return value.

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
File net/ftp/ftp_directory_listing_buffer.cc (right):

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
net/ftp/ftp_directory_listing_buffer.cc:100: return
ERR_ENCODING_DETECTION_FAILED;
On 2010/11/16 00:35:03, eroman wrote:
> It seems prudent to clear converted_buffer_ before we return failures.

Skipped, as discussed offline.

http://codereview.chromium.org/4967001/diff/1/net/ftp/ftp_directory_listing_b...
net/ftp/ftp_directory_listing_buffer.cc:104: if (!base::CodepageToUTF16(buffer_,
On 2010/11/16 00:35:03, eroman wrote:
> The structure of this function seems a little redundant. We already may have
> converted successfully to UTF16 above, however we throw that work away and
> re-convert down here. Can this be re-ordered a bit?

Done.

[Paweł Hajdan Jr., 2010-11-17 13:36:11]

eroman: I think this is ready for review. I've added the tests. If you have more
ideas for tests, I'd be glad to implement them.

[cbentzel, 2010-11-18 16:39:43]

Overall this looks fine, but I have neither ICU nor FTP expertise.

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

http://codereview.chromium.org/4967001/diff/10001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:33: bool DetectAllEncodings(const
std::string& text,
Is a bool needed here? Could empty encodings be sufficient?

http://codereview.chromium.org/4967001/diff/10001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:36: UCharsetDetector* detector =
ucsdet_open(&status);
Should you check for failure after this call? 

http://icu-project.org/apiref/icu4c/ucsdet_8h.html#59f0ebad887fcf9d9259e60a51...

indicates that status is filled with any error conditions.

Main concern is if it does fail, is the return value guaranteed, and would a
ucsdet_close on it lead to issues?

http://codereview.chromium.org/4967001/diff/10001/base/i18n/icu_encoding_dete...
base/i18n/icu_encoding_detection.cc:37: ucsdet_setText(detector, text.data(),
static_cast<int32_t>(text.length()),
Should you check for failure after this call as well?

[eroman, 2010-11-18 18:07:25]

lgtm