Downloads: After 100 numerically deduplicated files, try timestamp

After deduplicating 100 filenames,
i.e. foo.jpg, foo (1).jpg, ... foo (100).jpg, Chrome will stop trying.

On Desktop this is acceptable, as the user gets a prompt to choose
a filename.

On Android, this is really bad, because there is no prompt to choose
an alternate filename, and the download just silently fails.

This CL starts using a timestamp after 100, and creates filenames in
the ISO 8601 format, like: foo - 2018-10-11T21:55:47.392Z.jpg

Bug: 135428
Change-Id: I13748110171f799f58c8ddf27f22eebc97149c09
Reviewed-on: https://chromium-review.googlesource.com/c/1278137Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599802}

chrome/browser/download/download_path_reservation_tracker.cc

@@ -25,6 +25,8 @@
 #include "base/task/post_task.h"
 #include "base/task_runner_util.h"
 #include "base/third_party/icu/icu_utf.h"
+#include "base/time/time.h"
+#include "base/time/time_to_iso8601.h"
 #include "build/build_config.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/common/chrome_paths.h"
@@ -125,12 +127,23 @@ bool IsPathInUse(const base::FilePath& path) {
 // Create a unique filename by appending a uniquifier. Modifies |path| in place
 // if successful and returns true. Otherwise |path| is left unmodified and
 // returns false.
-bool CreateUniqueFilename(int max_path_component_length, base::FilePath* path) {
+bool CreateUniqueFilename(int max_path_component_length,
+                          const base::Time& download_start_time,
+                          base::FilePath* path) {
+  // Try every numeric uniquifier. Then make one attempt with the timestamp.
   for (int uniquifier = 1;
-       uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles;
+       uniquifier <= DownloadPathReservationTracker::kMaxUniqueFiles + 1;
        ++uniquifier) {
     // Append uniquifier.
     std::string suffix(base::StringPrintf(" (%d)", uniquifier));
+
+    // After we've tried all the unique numeric indices, make one attempt using
+    // the timestamp.
+    if (uniquifier > DownloadPathReservationTracker::kMaxUniqueFiles) {
+      suffix = base::StringPrintf(
+          " - %s", base::TimeToISO8601(download_start_time).c_str());
+    }
+
     base::FilePath path_to_check(*path);
     // If the name length limit is available (max_length != -1), and the
     // the current name exceeds the limit, truncate.
@@ -166,6 +179,7 @@ struct CreateReservationInfo {
   base::FilePath default_download_path;
   base::FilePath temporary_path;
   bool create_target_directory;
+  base::Time start_time;
   DownloadPathReservationTracker::FilenameConflictAction conflict_action;
   DownloadPathReservationTracker::ReservedPathCallback completion_callback;
 };
@@ -227,7 +241,8 @@ PathValidationResult ValidatePathAndResolveConflicts(
 
   switch (info.conflict_action) {
     case DownloadPathReservationTracker::UNIQUIFY:
-      return CreateUniqueFilename(max_path_component_length, target_path)
+      return CreateUniqueFilename(max_path_component_length, info.start_time,
+                                  target_path)
                  ? PathValidationResult::SUCCESS
                  : PathValidationResult::CONFLICT;
 
@@ -416,6 +431,7 @@ void DownloadPathReservationTracker::GetReservedPath(
                                 default_path,
                                 download_item->GetTemporaryFilePath(),
                                 create_directory,
+                                download_item->GetStartTime(),
                                 conflict_action,
                                 callback};
 

chrome/browser/download/download_path_reservation_tracker_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/test_file_util.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/download/download_path_reservation_tracker.h"
 #include "chrome/browser/download/download_target_determiner.h"
@@ -101,6 +102,8 @@ MockDownloadItem* DownloadPathReservationTrackerTest::CreateDownloadItem(
   EXPECT_CALL(*item, GetState())
       .WillRepeatedly(Return(DownloadItem::IN_PROGRESS));
   EXPECT_CALL(*item, GetURL()).WillRepeatedly(ReturnRefOfCopy(GURL()));
+  EXPECT_CALL(*item, GetStartTime())
+      .WillRepeatedly(Return(base::Time::UnixEpoch()));
   return item;
 }
 
@@ -434,23 +437,32 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingCaseReservations) {
 TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
   base::FilePath path(
       GetPathInDownloadsDirectory(FILE_PATH_LITERAL("foo.txt")));
+  // Make room for the path with no uniquifier, the |kMaxUniqueFiles|
+  // numerically uniquified paths, and then one more for the timestamp
+  // uniquified path.
   std::unique_ptr<MockDownloadItem>
-      items[DownloadPathReservationTracker::kMaxUniqueFiles + 1];
+      items[DownloadPathReservationTracker::kMaxUniqueFiles + 2];
   DownloadPathReservationTracker::FilenameConflictAction conflict_action =
     DownloadPathReservationTracker::UNIQUIFY;
   bool create_directory = false;
 
-  // Create |kMaxUniqueFiles + 1| reservations for |path|. The first reservation
-  // will have no uniquifier. The |kMaxUniqueFiles| remaining reservations do.
-  for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles; i++) {
+  // Create |kMaxUniqueFiles + 2| reservations for |path|. The first reservation
+  // will have no uniquifier. Then |kMaxUniqueFiles| paths have numeric
+  // uniquifiers. Then one more will have a timestamp uniquifier.
+  for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles + 1;
+       i++) {
+    SCOPED_TRACE(testing::Message() << "i = " << i);
     base::FilePath reserved_path;
     base::FilePath expected_path;
     PathValidationResult result = PathValidationResult::NAME_TOO_LONG;
-    if (i > 0) {
+    if (i == 0) {
+      expected_path = path;
+    } else if (i > 0 && i <= DownloadPathReservationTracker::kMaxUniqueFiles) {
       expected_path =
           path.InsertBeforeExtensionASCII(base::StringPrintf(" (%d)", i));
     } else {
-      expected_path = path;
+      expected_path =
+          path.InsertBeforeExtensionASCII(" - 1970-01-01T00:00:00.000Z");
     }
     items[i].reset(CreateDownloadItem(i));
     EXPECT_FALSE(IsPathInUse(expected_path));
@@ -462,7 +474,7 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
   }
   // The next reservation for |path| will fail to be unique.
   std::unique_ptr<MockDownloadItem> item(
-      CreateDownloadItem(DownloadPathReservationTracker::kMaxUniqueFiles + 1));
+      CreateDownloadItem(DownloadPathReservationTracker::kMaxUniqueFiles + 2));
   base::FilePath reserved_path;
   PathValidationResult result = PathValidationResult::NAME_TOO_LONG;
   CallGetReservedPath(item.get(), path, create_directory, conflict_action,
@@ -471,8 +483,8 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) {
   EXPECT_EQ(path.value(), reserved_path.value());
 
   SetDownloadItemState(item.get(), DownloadItem::COMPLETE);
-  for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles; i++)
-    SetDownloadItemState(items[i].get(), DownloadItem::COMPLETE);
+  for (auto& item : items)
+    SetDownloadItemState(item.get(), DownloadItem::COMPLETE);
 }
 
 // If the target directory is unwriteable, then callback should be notified that