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..9b970ba78e13cad7096f2a55c25e99e97fcd1a9d
--- /dev/null
+++ b/chrome/browser/win/jumplist_file_util.cc
@@ -0,0 +1,158 @@
+// 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 <windows.h>
+
+#include "base/files/file_enumerator.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_restrictions.h"
+
+bool DeleteFiles(const base::FilePath& path,
+                 const base::FilePath::StringType& pattern,
+                 bool has_upper_limit,
+                 FolderDeleteResult* delete_status) {
+  base::FileEnumerator traversal(
+      path, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+  int succees_count = 0;
+  int failure_count = 0;
+  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 if |has_upper_limit| is true and record this
+      // information in |delete_status|.
+      *delete_status = FAIL_SUBDIRECTORY_EXISTS;
+      if (!has_upper_limit)
+        return false;
+      failure_count++;
+    } else if (!::DeleteFile(current.value().c_str())) {
+      // If there is no upper limit, return false intermediately. Otherwise,
+      // increment |failure_count| until it hits the upper limit.
+      if (!has_upper_limit)
+        return false;
+      failure_count++;
+    } else {
+      succees_count++;
+    }
+    // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files without

[grt (UTC plus 2), 2017/03/22 12:07:21]

i think this is more clear with both limit checks in one block:
    if (has_upper_limit) {
      if (success_count >= kMaxIconFilesDeletedPerUpdate) {
        // The desired max number of files have been deleted.
        if (!failure_count)
          return true;
        *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY;
        return false;
      }
      if (failure_count >= kMaxIconFilesDeletedPerUpdate) {
        // The desired max number of failures have been hit.
        *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY;
        return false;
      }
    }

[chengx, 2017/03/22 21:16:36]

Agreed and updated accordingly.

+    // any failures, return true. If some files fail to be delete, record this
+    // information in |delete_status| and returns false.
+    if (has_upper_limit && succees_count >= kMaxIconFilesDeletedPerUpdate) {
+      if (failure_count > 0) {
+        *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY;

[grt (UTC plus 2), 2017/03/22 12:07:22]

this reads to me like "we failed because we perfectly deleted the max # of
files. wdyt of FAIL_DELETE_MAX_FILES_WITH_ERRORS, which sounds more to me like
"failed because we delete the max # of files but had some errors mixed in".

[chengx, 2017/03/22 21:16:36]

Done.

+      }
+      return failure_count == 0;
+    }
+    // Return false when failing to delete |kMaxIconFilesDeletedPerUpdate| files
+    // comes before successfully deleting |kMaxIconFilesDeletedPerUpdate| files.
+    if (has_upper_limit && failure_count >= kMaxIconFilesDeletedPerUpdate) {
+      *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY;

[grt (UTC plus 2), 2017/03/22 12:07:21]

FAIL_MAX_DELETE_FAILURES -> "we failed because we hit the max # of tolerable
failures"?

[chengx, 2017/03/22 21:16:36]

Sounds good.

+      return false;
+    }
+  }
+  return true;
+}
+
+bool DeleteDirectoryContent(const base::FilePath& path,

[grt (UTC plus 2), 2017/03/22 12:07:21]

return FolderDeleteResult rather than bool (which seems to be ignored). callers
can compare it with SUCCEED if they care.

[chengx, 2017/03/22 21:16:36]

Done.

+                            bool has_upper_limit,
+                            FolderDeleteResult* delete_status) {
+  base::ThreadRestrictions::AssertIOAllowed();

[grt (UTC plus 2), 2017/03/22 12:07:21]

this function seems to assume that |delete_status| is SUCCEED on entry. please
either DCHECK that this is true, or explicitly set it.

[chengx, 2017/03/22 21:16:36]

Changed to explicitly set it.

+
+  if (path.empty())
+    return true;
+
+  // 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) {
+    *delete_status = FAIL_INVALID_FILE_PATH;
+    return false;
+  }
+
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return true;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),
+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    *delete_status = FAIL_READ_ONLY_DIRECTORY;
+    return false;
+  }
+
+  // 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)) {
+    bool delete_result = !!::DeleteFile(path.value().c_str());
+    if (!delete_result)
+      *delete_status = FAIL_DELETE_SINGLE_FILE;
+    return delete_result;
+  }
+
+  // If |path| is a directory, delete all its content but save the raw
+  // directory.
+  return DeleteFiles(path, L"*", has_upper_limit, delete_status);
+}
+
+void DeleteDirectory(const base::FilePath& path,

[grt (UTC plus 2), 2017/03/22 12:07:21]

return FolderDeleteResult rather than void w/ out param

[chengx, 2017/03/22 21:16:36]

Done.

+                     bool has_upper_limit,
+                     FolderDeleteResult* delete_status) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  if (path.empty())
+    return;
+
+  // 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) {
+    *delete_status = FAIL_INVALID_FILE_PATH;
+    return;
+  }
+
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),
+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    *delete_status = FAIL_READ_ONLY_DIRECTORY;
+    return;
+  }
+  // If |path| is a file, delete it.
+  // Since JumpListIcons{,Old} are directories, hitting the code block below is
+  // unexpected.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
+    ::DeleteFile(path.value().c_str());
+    *delete_status = FAIL_DELETE_SINGLE_FILE;
+    return;
+  }
+  // If |path| is a directory, delete its contents and if it succeeds, remove
+  // the raw directory.
+  if (DeleteFiles(path, L"*", has_upper_limit, delete_status))
+    ::RemoveDirectory(path.value().c_str());
+  return;
+}
+
+void DeleteDirectoryAndLogResults(const base::FilePath& path,
+                                  bool has_upper_limit) {
+  FolderDeleteResult delete_status = SUCCEED;
+  DeleteDirectory(path, has_upper_limit, &delete_status);
+  UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
+                            static_cast<int>(delete_status), END);

[grt (UTC plus 2), 2017/03/22 12:07:21]

since FolderDeleteResult is an old-school enum and not an enum class, is this
static_cast needed?

[chengx, 2017/03/22 21:16:36]

You are right. It is not needed.

+}