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..44a58793e47e1def30a5cea221ab3f7ffff66569 100644
--- a/chrome/browser/download/download_path_reservation_tracker.cc
+++ b/chrome/browser/download/download_path_reservation_tracker.cc
@@ -12,6 +12,8 @@
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
+#include "base/string_util.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 +28,16 @@ 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 size_t kTruncatedNameLengthLowerbound = 5;
+
+// The length of the suffix string we append for an intermediate file name.
+// In the file name truncation, we keep the margin to append the suffix.
+// TODO(kinaba): remove the margin. The user should be able to set maximum
+// possible filename.
+static const size_t kIntermediateNameSuffixLength = sizeof(".crdownload") - 1;
+
 // 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 +102,51 @@ bool IsPathInUse(const base::FilePath& path) {
   return false;
 }
 
+// Truncates path->BaseName() to satisfy the condition .value().size() <= limit.

[asanka, 2013/02/19 17:59:15]

Nit: path->BaseName().value().size() <= limit.

|limit| only applies to the BaseName().

[kinaba, 2013/02/20 13:57:41]

Done.

+// - It keeps the extension as is. Only truncates the body part.

[asanka, 2013/02/19 17:59:15]

Nit: "base filename" would, I think, be clearer than "body part."

[kinaba, 2013/02/20 13:57:41]

Done.

+// - It secures the body part length to be >= kTruncatedNameLengthLowerbound.
+// If it was unable to shorten the name, returns false.
+bool TruncateFileName(base::FilePath* path, size_t limit) {
+  base::FilePath basename(path->BaseName());
+  // It is already short enough.
+  if (basename.value().size() <= limit)
+    return true;
+
+  base::FilePath dir(path->DirName());
+  base::FilePath::StringType ext(basename.Extension());
+  base::FilePath::StringType name(basename.RemoveExtension().value());
+
+  // Impossible to satisfy the limit.
+  if (limit < kTruncatedNameLengthLowerbound + ext.size())
+    return false;
+  limit -= ext.size();
+
+  // Encoding specific truncation logic.
+  base::FilePath::StringType truncated;
+#if defined(OS_CHROMEOS) || defined(OS_MACOSX)
+  // UTF-8.
+  TruncateUTF8ToByteSize(name, limit, &truncated);
+#elif defined(OS_WIN)
+  // UTF-16.
+  DCHECK(name.size() > limit);
+  truncated = name.substr(
+      0, limit > 0 && CBU16_IS_TRAIL(name[limit]) ? limit - 1 : limit);

[asanka, 2013/02/19 17:59:15]

Nit: limit > 0 is always true at this point.

[kinaba, 2013/02/20 13:57:41]

Done.

+#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
+
+  if (truncated.size() < kTruncatedNameLengthLowerbound)
+    return false;
+  *path = dir.Append(truncated + ext);
+  return true;
+}
+
 // 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.
@@ -118,6 +171,7 @@ void CreateReservation(
   base::FilePath target_path(suggested_path.NormalizePathSeparators());
   bool is_path_writeable = true;
   bool has_conflicts = false;
+  bool name_too_long = false;
 
   // Create the default download path if it doesn't already exist and is where
   // we are going to create the downloaded file. |target_path| might point
@@ -139,24 +193,43 @@ void CreateReservation(
     target_path = dir.Append(filename);
   }
 
-  if (is_path_writeable && should_uniquify && IsPathInUse(target_path)) {
-    has_conflicts = true;
-    for (int uniquifier = 1;
-         uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles;
-         ++uniquifier) {
-      base::FilePath path_to_check(target_path.InsertBeforeExtensionASCII(
-          StringPrintf(" (%d)", uniquifier)));
-      if (!IsPathInUse(path_to_check)) {
-        target_path = path_to_check;
-        has_conflicts = false;
-        break;
+  if (is_path_writeable) {
+    // Check the limit of file name length if it could be obtained. When the
+    // suggested name exceeds the limit, truncate or prompt the user.
+    int max_length = file_util::GetMaximumPathComponentLength(dir);
+    if (max_length != -1) {
+      int limit = max_length - kIntermediateNameSuffixLength;
+      if (limit <= 0 || !TruncateFileName(&target_path, limit))
+        name_too_long = true;
+    }
+
+    // Uniquify the name, if it already exists.
+    if (!name_too_long && should_uniquify && IsPathInUse(target_path)) {
+      has_conflicts = true;
+      for (int uniquifier = 1;
+           uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles;
+           ++uniquifier) {
+        // Append uniquifier. If necessary, truncate the name.
+        std::string suffix(StringPrintf(" (%d)", uniquifier));

[asanka, 2013/02/19 17:59:15]

Nit: Use base::StringPrintf() and include "base/stringprintf.h"

[kinaba, 2013/02/20 13:57:41]

Done.

+        base::FilePath path_to_check(target_path);
+        int limit = max_length - kIntermediateNameSuffixLength - suffix.size();
+        if (limit <= 0 || !TruncateFileName(&path_to_check, limit))
+          break;

[asanka, 2013/02/19 17:59:15]

Nit: We could get here if |max_length| is -1 and there is a path conflict. If
that case was handled explicitly then we wouldn't be doing arithmetic on a
signal value and the code would be easier to follow.

[kinaba, 2013/02/20 13:57:41]

Good catch. Thanks.
Explicitly dispatched the |max_length| = -1 case.

+        path_to_check = path_to_check.InsertBeforeExtensionASCII(suffix);
+
+        if (!IsPathInUse(path_to_check)) {
+          target_path = path_to_check;
+          has_conflicts = false;
+          break;
+        }
       }
     }
   }
+
   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