Truncate the download file name if it exceeds the filesystem limit.

chrome/browser/download/download_path_reservation_tracker.cc

Index: chrome/browser/download/download_path_reservation_tracker.cc
diff --git a/chrome/browser/download/download_path_reservation_tracker.cc b/chrome/browser/download/download_path_reservation_tracker.cc
index bb0deb2dc431aef5b62a16557344b87b58456cee..3f23f060d436fbc1b2790e892716ec0e4336ed8a 100644
--- a/chrome/browser/download/download_path_reservation_tracker.cc
+++ b/chrome/browser/download/download_path_reservation_tracker.cc
@@ -12,6 +12,9 @@
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
+#include "base/sys_info.h"
+#include "base/third_party/icu/icu_utf.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
@@ -26,6 +29,10 @@ namespace {
 
 typedef std::map<content::DownloadId, base::FilePath> ReservationMap;
 
+// The lower bound for file name truncation. If the truncation results in a name
+// shorter than this limit, we give up automatic truncation and prompt the user.
+static const int kTruncatedNameLengthLowerbound = 5;
+
 // Map of download path reservations. Each reserved path is associated with a
 // DownloadId. This object is destroyed in |Revoke()| when there are no more
 // reservations.
@@ -90,10 +97,34 @@ bool IsPathInUse(const base::FilePath& path) {
   return false;
 }
 
+// Truncate the given name so that its becomes .size() <= limit. Returns an
+// empty string if it is impossible due to unknown file-name encoding.
+FilePath::StringType TruncateFileName(const FilePath::StringType& name,
+                                      size_t limit) {
+  FilePath::StringType truncated;
+#if defined(OS_CHROMEOS) || defined(OS_MACOSX)
+  // Character encoding is UTF-8.
+  TruncateUTF8ToByteSize(name, limit, &truncated);
+#elif defined(OS_WIN)
+  // Character encoding is UTF-16.
+  if (name.size() <= limit) {
+    truncated = name;
+  } else {
+    truncated = name.substr(
+        0, limit > 0 && CBU16_IS_TRAIL(name[limit]) ? limit - 1 : limit);
+  }
+#else
+  // We cannot generally assume that the file name encoding is in UTF-8 (see
+  // the comment for FilePath::AsUTF8Unsafe), hence no safe way to truncate.
+#endif
+  return truncated;
+}
+
 // Called on the FILE thread to reserve a download path. This method:
 // - Creates directory |default_download_path| if it doesn't exist.
 // - Verifies that the parent directory of |suggested_path| exists and is
 //   writeable.
+// - Truncates the suggested name if it exceeds the filesystem's limit.
 // - Uniquifies |suggested_path| if |should_uniquify_path| is true.
 // - Schedules |callback| on the UI thread with the reserved path and a flag
 //   indicating whether the returned path has been successfully verified.
@@ -139,6 +170,33 @@ void CreateReservation(
     target_path = dir.Append(filename);
   }
 
+  // Check the limit of file name length if it could be obtained. When the
+  // suggested name exceeds the limit, truncate or prompt the user.
+  bool name_too_long = false;
+  if (is_path_writeable) {
+    int max_length = base::SysInfo::GetMaximumPathComponentLength(dir);

[asanka, 2013/02/12 20:20:12]

We should also limit the length of the full path to be shorter than MAX_PATH on
Windows. Some of the Windows APIs have this limitation.

[kinaba, 2013/02/14 04:52:47]

Done (inside the query function).

+    if (max_length > 0) {
+      // The extension should not be truncated, so we split it first.
+      FilePath::StringType ext = filename.Extension();
+      FilePath::StringType name = filename.RemoveExtension().value();
+
+      // Reserve several characters we may append later.
+      const int kExtraChars = sizeof(" (100).crdownload") - 1;

[asanka, 2013/02/12 20:20:12]

Feel free to push back on this one:

The intermediate path suffixes are implementation details. Ideally we shouldn't
truncate the download filename to accomodate them. I.e. the user should be able
to set a 255 character download filename if the file system supports it. This
might be even more of an issue when the limit is 143.

One approach we were discussing was to use random intermediate filenames if the
suffixes would cause the intermediate filename to exceed file system
limitations. This could conceivably be done by returning the path component
limit back to ChromeDownloadManagerDelegate where the limit would be used to
determine whether to use a .crdownload suffix or a random filename as the
intermediate.

[kinaba, 2013/02/14 04:52:47]

Sounds reasonable. Let me handle it in a separate CL to keep the current one
simpler.

In the latest patchset, I've at least removed the " (100)" reservation part so
that
by making uniquification respect the name length limit.

+      int limit = max_length - ext.size() - kExtraChars;
+      if (static_cast<int>(name.size()) > limit) {
+        name_too_long = true;
+        if (limit >= kTruncatedNameLengthLowerbound) {
+          // Truncate the name to fit in |limit|.
+          FilePath::StringType truncated = TruncateFileName(name, limit);
+          if (!truncated.empty()) {
+            target_path = dir.Append(truncated + ext);
+            name_too_long = false;
+          }
+        }
+      }
+    }
+  }
+
   if (is_path_writeable && should_uniquify && IsPathInUse(target_path)) {
     has_conflicts = true;
     for (int uniquifier = 1;
@@ -154,9 +212,9 @@ void CreateReservation(
     }
   }
   reservations[download_id] = target_path;
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(callback, target_path, (is_path_writeable && !has_conflicts)));
+  bool verified = (is_path_writeable && !has_conflicts && !name_too_long);
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(callback, target_path, verified));
 }
 
 // Called on the FILE thread to update the path of the reservation associated