FTP: Multiple fixes for localized directory listings:

base/i18n/icu_encoding_detection.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/i18n/icu_encoding_detection.h"
 
+#include <set>
+
 #include "base/string_util.h"
 #include "unicode/ucsdet.h"
 
 namespace base {
 
 bool DetectEncoding(const std::string& text, std::string* encoding) {
   if (IsStringASCII(text)) {
     *encoding = std::string();
     return true;
   }
 
   UErrorCode status = U_ZERO_ERROR;
   UCharsetDetector* detector = ucsdet_open(&status);

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

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.

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

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.

   ucsdet_setText(detector, text.data(), static_cast<int32_t>(text.length()),
                  &status);
   const UCharsetMatch* match = ucsdet_detect(detector, &status);
   const char* detected_encoding = ucsdet_getName(match, &status);
   ucsdet_close(detector);
 
   if (U_FAILURE(status))
     return false;
 
   *encoding = detected_encoding;
   return true;
 }
 
 bool DetectAllEncodings(const std::string& text,
                         std::vector<std::string>* encodings) {
   UErrorCode status = U_ZERO_ERROR;
   UCharsetDetector* detector = ucsdet_open(&status);
   ucsdet_setText(detector, text.data(), static_cast<int32_t>(text.length()),
                  &status);
   int matches_count = 0;
   const UCharsetMatch** matches = ucsdet_detectAll(detector,
                                                    &matches_count,
                                                    &status);
   if (U_FAILURE(status)) {
     ucsdet_close(detector);
     return false;
   }
 
+  // ICU has some heuristics for encoding detection, such that the more likely
+  // encodings should be returned first. However, it doesn't always return
+  // all encodings that properly decode |text|, so we'll append more encodings
+  // later. To make that efficient, keep track of encodings sniffed in this
+  // first phase.

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

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? 

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

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

+  std::set<std::string> sniffed_encodings;
+
   encodings->clear();
   for (int i = 0; i < matches_count; i++) {
     UErrorCode get_name_status = U_ZERO_ERROR;
     const char* encoding_name = ucsdet_getName(matches[i], &get_name_status);
 
     // If we failed to get the encoding's name, ignore the error.
     if (U_FAILURE(get_name_status))
       continue;
 
     encodings->push_back(encoding_name);
+    sniffed_encodings.insert(encoding_name);
   }
 
+  // Append all encodings not included earlier, in arbitrary order.

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

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. 

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

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.
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.
Right, this shouldn't be necessary. Do you want me to add a TODO(jshin)?

+  UEnumeration* detectable_encodings = ucsdet_getAllDetectableCharsets(detector,
+                                                                       &status);
+  int detectable_count = uenum_count(detectable_encodings, &status);
+  for (int i = 0; i < detectable_count; i++) {
+    int name_length;
+    const char* name_raw = uenum_next(detectable_encodings,
+                                      &name_length,
+                                      &status);
+    std::string name(name_raw, name_length);
+    if (sniffed_encodings.find(name) == sniffed_encodings.end())
+      encodings->push_back(name);
+  }
+  uenum_close(detectable_encodings);
+
   ucsdet_close(detector);
   return !encodings->empty();
 }
 
 }  // namespace base