Not update jumplist icons when deletion of JumplistIcon folder fails

chrome/browser/win/jumplist.cc

Index: chrome/browser/win/jumplist.cc
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc
index 14b6f870ca397a92d57d6b15c899bc5272e6ceb2..992b86c8316827da9af12f33df2c6b96a92d2141 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -265,41 +265,52 @@ void RunUpdateOnFileThread(
   // This variable records the status of three folder operations.
   uint32_t folder_operation_status = FolderOperationResult::SUCCESS;
 
+  // This variable records the status of |icon_dir| deletion.
+  bool icon_dir_delete_success = true;
+
   if (!base::DeleteFile(icon_dir_old, true)) {
     folder_operation_status |= FolderOperationResult::DELETE_DEST_FAILED;
     // If deletion of |icon_dir_old| fails, do not move |icon_dir| to
     // |icon_dir_old|, instead, delete |icon_dir| directly to avoid bloating
     // |icon_dir_old| by moving more things to it.
-    if (!base::DeleteFile(icon_dir, true))
+    if (!base::DeleteFile(icon_dir, true)) {
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
+      icon_dir_delete_success = false;
+    }
   } else if (!base::Move(icon_dir, icon_dir_old)) {
     folder_operation_status |= FolderOperationResult::MOVE_FAILED;
     // If Move() fails, delete |icon_dir| to avoid file accumulation in this
     // directory, which can eventually lead the folder to be huge.
-    if (!base::DeleteFile(icon_dir, true))
+    if (!base::DeleteFile(icon_dir, true)) {
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
+      icon_dir_delete_success = false;
+    }
   }
-  if (!base::CreateDirectory(icon_dir))
+

[gab, 2017/03/03 07:35:01]

Prefer an early exit (return;) above instead of capturing a bool and cancelling
work below.

To make sure you still invoke UMA, here's a neat trick:

On line 266:

  uint32_t folder_operation_status = FolderOperationResult::SUCCESS;
  base::ScopedClosureRunner log_operation_status_when_done(
      base::Bind([](uint32_t* folder_operation_status_ptr) -> {
        UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderResults",
                                  *folder_operation_status_ptr,
                                  FolderOperationResult::END);
      }, base::Unretained(&folder_operation_status)));

[chengx, 2017/03/03 21:07:34]

Sounds good. I have updated accordingly.

Btw, I think 
base::Bind([](uint32_t* folder_operation_status_ptr) -> {
should be
base::Bind([](uint32_t* folder_operation_status_ptr) {

+  // If deletion of |icon_dir| fails, no need to create the same directory.
+  if (icon_dir_delete_success && !base::CreateDirectory(icon_dir))
     folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED;
 
   UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderResults",
                             folder_operation_status,
                             FolderOperationResult::END);
 
-  // Create temporary icon files for shortcuts in the "Most Visited" category.
-  CreateIconFiles(icon_dir, local_most_visited_pages);
-
-  // Create temporary icon files for shortcuts in the "Recently Closed"
-  // category.
-  CreateIconFiles(icon_dir, local_recently_closed_pages);
-
-  // We finished collecting all resources needed for updating an application
-  // JumpList. So, create a new JumpList and replace the current JumpList
-  // with it.
-  UpdateJumpList(app_id.c_str(),
-                 local_most_visited_pages,
-                 local_recently_closed_pages,
-                 incognito_availability);
+  // If deletion of |icon_dir| fails, skip updating jumplist icons to avoid
+  // bloating the JumplistIcons folder.
+  if (icon_dir_delete_success) {
+    // Create temporary icon files for shortcuts in the "Most Visited" category.
+    CreateIconFiles(icon_dir, local_most_visited_pages);
+
+    // Create temporary icon files for shortcuts in the "Recently Closed"
+    // category.
+    CreateIconFiles(icon_dir, local_recently_closed_pages);
+
+    // We finished collecting all resources needed for updating an application
+    // JumpList. So, create a new JumpList and replace the current JumpList
+    // with it.
+    UpdateJumpList(app_id.c_str(), local_most_visited_pages,
+                   local_recently_closed_pages, incognito_availability);
+  }
 }
 
 }  // namespace