Retire some metrics and update file util methods for JumpList

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
index 9dd5b11e3041070d2fad91ac470742862438e25f..f2c8393c5cd3707ac8238f0d2e32d91a9888e90b 100644
--- a/chrome/browser/win/jumplist_file_util.cc
+++ b/chrome/browser/win/jumplist_file_util.cc
@@ -11,12 +11,11 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_restrictions.h"
 
-FolderDeleteResult DeleteFiles(const base::FilePath& path,
-                               const base::FilePath::StringType& pattern,
-                               int max_file_deleted) {
+void DeleteFiles(const base::FilePath& path,
+                 const base::FilePath::StringType& pattern,
+                 int max_file_deleted) {
   int success_count = 0;
   int failure_count = 0;
-  FolderDeleteResult delete_status = SUCCEED;
 
   base::FileEnumerator traversal(
       path, false,
@@ -31,119 +30,67 @@ FolderDeleteResult DeleteFiles(const base::FilePath& path,
           info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
     }
 
-    if (info.IsDirectory()) {
-      // JumpListIcons{,Old} directories shouldn't have sub-directories.
-      // If any of them does for unknown reasons, don't delete them. Instead,
-      // increment the failure count and record this information.
-      delete_status = FAIL_SUBDIRECTORY_EXISTS;
-      failure_count++;
-    } else if (!::DeleteFile(current.value().c_str())) {
+    // JumpListIcons{,Old} directories shouldn't have sub-directories.
+    // If any of them does for unknown reasons, don't delete them. Instead,
+    // increment the failure count.
+    if (info.IsDirectory() || !::DeleteFile(current.value().c_str())) {

[grt (UTC plus 2), 2017/04/28 07:37:08]

nit: omit braces

[chengx, 2017/04/28 22:29:29]

Done. However, I think braces are allowed here. 
https://google.github.io/styleguide/cppguide.html#Conditionals

[grt (UTC plus 2), 2017/05/01 09:45:36]

you're correct. one of the more important goals stated in the style guide is to
be consistent with surrounding code
(https://google.github.io/styleguide/cppguide.html#Goals). in general, the
windows code in chromium (and possible much of chromium) omits braces when the
conditional and its body are each one line. a quick survey of this directory
shows that this holds here. there may be other parts of the codebase that always
include braces, so you may need to include them when working there.

[chengx, 2017/05/01 18:04:30]

Cool. Thanks for this reminder!

       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.
-      return failure_count ? FAIL_DELETE_MAX_FILES_WITH_ERRORS : delete_status;
-    }
-    if (failure_count >= max_file_deleted) {
-      // The desired max number of failures have been hit.
-      return FAIL_MAX_DELETE_FAILURES;
-    }
+
+    // The desired max number of files have been deleted, or The desired max
+    // number of failures have been hit.
+    if (success_count >= max_file_deleted || failure_count >= max_file_deleted)
+      return;

[grt (UTC plus 2), 2017/04/28 07:37:08]

nit: i slightly prefer breaking out of the loop here so that there's only one
exit path to the function.

[chengx, 2017/04/28 22:29:29]

Done.

   }
-  return delete_status;
 }
 
-FolderDeleteResult DeleteDirectoryContent(const base::FilePath& path,
-                                          int max_file_deleted) {
+void DeleteDirectoryContent(const base::FilePath& path, int max_file_deleted) {
   base::ThreadRestrictions::AssertIOAllowed();
 
-  if (path.empty())
-    return SUCCEED;
-
-  // For JumpListIcons{,Old} directories, since their names are shorter than
-  // MAX_PATH, hitting the code in the if-block below is unexpected.
-  if (path.value().length() >= MAX_PATH)
-    return FAIL_INVALID_FILE_PATH;
+  if (path.empty() || path.value().length() >= MAX_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 SUCCEED;
+    return;
   // Try to clear the read-only bit if we find it.
   if ((attr & FILE_ATTRIBUTE_READONLY) &&
       !SetFileAttributes(path.value().c_str(),
                          attr & ~FILE_ATTRIBUTE_READONLY)) {
-    return FAIL_READ_ONLY_DIRECTORY;
+    return;
   }
 
   // If |path| is a file, simply delete it. However, since JumpListIcons{,Old}
   // are directories, hitting the code inside the if-block below is unexpected.
   if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
     ::DeleteFile(path.value().c_str());
-    return FAIL_DELETE_SINGLE_FILE;
+    return;
   }
 
   // If |path| is a directory, delete at most |max_file_deleted| files in it.
-  return DeleteFiles(path, L"*", max_file_deleted);
+  DeleteFiles(path, L"*", max_file_deleted);
 }
 
-FolderDeleteResult DeleteDirectory(const base::FilePath& path,
-                                   int max_file_deleted) {
+void DeleteDirectory(const base::FilePath& path, int max_file_deleted) {
   base::ThreadRestrictions::AssertIOAllowed();
+
   // Delete at most |max_file_deleted| files in |path|.
-  FolderDeleteResult delete_status =
-      DeleteDirectoryContent(path, max_file_deleted);
+  DeleteDirectoryContent(path, max_file_deleted);
+
   // Since DeleteDirectoryContent() can only delete at most |max_file_deleted|
   // files, its return value cannot indicate if |path| is empty or not.
-  // Instead, use PathIsDirectoryEmpty to check if |path| is empty and remove it
+  // Instead, use IsDirectoryEmpty to check if |path| is empty and remove it
   // if it is.
-  if (base::IsDirectoryEmpty(path) &&
-      !::RemoveDirectory(path.value().c_str())) {
-    delete_status = FAIL_REMOVE_RAW_DIRECTORY;
-  }
-  return delete_status;
-}
-
-void DeleteDirectoryAndLogResults(const base::FilePath& path,
-                                  int max_file_deleted) {
-  DirectoryStatus dir_status = NON_EXIST;
-
-  if (base::DirectoryExists(path)) {
-    FolderDeleteResult delete_status = DeleteDirectory(path, max_file_deleted);
-    UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
-                              delete_status, FolderDeleteResult::END);
-    if (base::DirectoryExists(path))
-      dir_status = base::IsDirectoryEmpty(path) ? EMPTY : NON_EMPTY;
-  }
-
-  UMA_HISTOGRAM_ENUMERATION("WinJumplist.DirectoryStatusJumpListIconsOld",
-                            dir_status, DIRECTORY_STATUS_END);
+  if (base::IsDirectoryEmpty(path))

[grt (UTC plus 2), 2017/04/28 07:37:08]

if you no longer care whether RemoveDirectory succeeds or fails, remove the call
to IsDirectoryEmpty. RD won't do anything if the directory isn't empty.

[chengx, 2017/04/28 22:29:29]

Done.

+    ::RemoveDirectory(path.value().c_str());
 }
 
-void DeleteDirectoryContentAndLogResults(const base::FilePath& path,
+void DeleteDirectoryContentAndLogRuntime(const base::FilePath& path,
                                          int max_file_deleted) {
-  // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.DeleteDirectoryContentDuration");
 
-  DirectoryStatus dir_status = NON_EXIST;
-
-  // Delete the content in |path|. If |path| doesn't exist, create one.
-  if (base::DirectoryExists(path)) {
-    FolderDeleteResult delete_status =
-        DeleteDirectoryContent(path, kFileDeleteLimit);
-
-    UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIcons",
-                              delete_status, FolderDeleteResult::END);
-
-    if (base::DirectoryExists(path))
-      dir_status = base::IsDirectoryEmpty(path) ? EMPTY : NON_EMPTY;
-  } else if (base::CreateDirectory(path)) {
-    dir_status = EMPTY;
-  }
-
-  UMA_HISTOGRAM_ENUMERATION("WinJumplist.DirectoryStatusJumpListIcons",
-                            dir_status, DIRECTORY_STATUS_END);
+  DeleteDirectoryContent(path, kFileDeleteLimit);
 }