Not update jumplist icons when deletion of JumplistIcon folder fails

chrome/browser/win/jumplist.cc

 // Copyright 2012 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.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
@@ skipping 247 matching lines @@
     DELETE_SRC_FAILED = 1 << 2,
     CREATE_SRC_FAILED = 1 << 3,
     // This value is beyond the sum of all bit fields above and
     // should remain last (shifted by one more than the last value)
     END = 1 << 4
   };
 
   // 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);
+  // 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);
+    // 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);
+    // 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
 
 JumpList::JumpListData::JumpListData() {}
 
 JumpList::JumpListData::~JumpListData() {}
 
 JumpList::JumpList(Profile* profile)
     : RefcountedKeyedService(content::BrowserThread::GetTaskRunnerForThread(
@@ skipping 309 matching lines @@
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }