Remove JumpListIconsOld directory and set upper limit for delete attempts

chrome/browser/win/jumplist_file_util.cc

Index: chrome/browser/win/jumplist_file_util.cc
diff --git a/chrome/browser/win/jumplist_file_util.cc b/chrome/browser/win/jumplist_file_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ebbe8389398a8ad2e1cc324428023d5462079014
--- /dev/null
+++ b/chrome/browser/win/jumplist_file_util.cc
@@ -0,0 +1,120 @@
+// Copyright (c) 2017 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 "chrome/browser/win/jumplist_file_util.h"
+
+#include <Shlwapi.h>
+#include <windows.h>
+
+#include "base/files/file_enumerator.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_restrictions.h"
+
+const FolderDeleteResult DeleteFiles(const base::FilePath& path,
+                                     const base::FilePath::StringType& pattern,
+                                     const int max_file_deleted) {
+  int success_count = 0;
+  int failure_count = 0;
+  FolderDeleteResult delete_status = SUCCEED;
+
+  base::FileEnumerator traversal(
+      path, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+  for (base::FilePath current = traversal.Next(); !current.empty();
+       current = traversal.Next()) {
+    // Try to clear the read-only bit if we find it.
+    base::FileEnumerator::FileInfo info = traversal.GetInfo();
+    if (info.find_data().dwFileAttributes & FILE_ATTRIBUTE_READONLY) {
+      SetFileAttributes(
+          current.value().c_str(),
+          info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+    }
+
+    if (info.IsDirectory()) {
+      // JumpListIcons{,Old} directories shouldn't have sub-directories.
+      // If it does for unknown reasons, don't try to delete it. Instead,
+      // increment failure count and record this information in |delete_status|.
+      delete_status = FAIL_SUBDIRECTORY_EXISTS;
+      failure_count++;
+    } else if (!::DeleteFile(current.value().c_str())) {
+      failure_count++;
+    } else {
+      success_count++;
+    }
+    // If it deletes max_file_deleted files with any attempt failures, record
+    // this information in |delete_status|.
+    if (success_count >= max_file_deleted) {
+      // The desired max number of files have been deleted.
+      if (failure_count)
+        delete_status = FAIL_DELETE_MAX_FILES_WITH_ERRORS;
+      return delete_status;
+    }
+    if (failure_count >= max_file_deleted) {
+      // The desired max number of failures have been hit.
+      return FAIL_MAX_DELETE_FAILURES;
+    }
+  }
+  return delete_status;
+}
+
+const FolderDeleteResult DeleteDirectoryContent(const base::FilePath& path,
+                                                const int max_file_deleted) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  FolderDeleteResult delete_status = SUCCEED;
+
+  if (path.empty())
+    return delete_status;
+
+  // For JumpListIcons{,Old} directories, since their names are shorter than
+  // MAX_PATH, hitting the code block below is unexpected.
+  if (path.value().length() >= MAX_PATH) {
+    return FAIL_INVALID_FILE_PATH;
+  }
+
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return delete_status;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),
+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    return FAIL_READ_ONLY_DIRECTORY;
+  }
+
+  // If |path| is a file, simply delete it.
+  // Since JumpListIcons{,Old} are directories, hitting the code block below is
+  // unexpected.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
+    ::DeleteFile(path.value().c_str());
+    return FAIL_DELETE_SINGLE_FILE;
+  }
+
+  // If |path| is a directory, delete all its contents but save the raw
+  // directory.
+  return DeleteFiles(path, L"*", max_file_deleted);
+}
+
+const FolderDeleteResult DeleteDirectory(const base::FilePath& path,
+                                         const int max_file_deleted) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  // Delete the contents in the directory first.
+  FolderDeleteResult delete_status =
+      DeleteDirectoryContent(path, max_file_deleted);
+  // Since DeleteDirectoryContent can only delete maximum files allowed, its
+  // return cannot be used to indicate if the directory is empty now. Therefore,
+  // check if the directory is empty using ::PathIsDirectoryEmpty directly.
+  if (::PathIsDirectoryEmpty(path.value().c_str()) &&
+      !::RemoveDirectory(path.value().c_str()))
+    delete_status = FAIL_REMOVE_RAW_DIRECTORY;
+  return delete_status;
+}
+
+void DeleteDirectoryAndLogResults(const base::FilePath& path,
+                                  const int max_file_deleted) {
+  FolderDeleteResult delete_status = DeleteDirectory(path, max_file_deleted);
+  UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
+                            delete_status, END);

[Ilya Sherman, 2017/03/22 22:24:00]

FWIW, you might want to add histogram test coverage along with other test
coverage, using base::HistogramTester.  (Note: There's probably not much point
in testing just this method in isolation, since it's pretty much just internal
API; but if you can test the code that wraps this code, that might be
appropriate.)

[chengx, 2017/03/23 21:16:11]

I would probably not add histogram test for this internal API now. Thanks for
your suggestion though.

+}