Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete

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 <Shlwapi.h>
+#include <windows.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/trace_event/trace_event.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/metrics/jumplist_metrics_win.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/shell_integration_win.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 190 matching lines @@
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return false;
 
   // Commit this transaction and send the updated JumpList to Windows.
   if (!jumplist_updater.CommitUpdate())
     return false;
 
   return true;
 }
 
+// Renames the directory |from_dir| to |to_path|. This method fails if any
+// process has a handle open in |from_dir| or if |to_path| exists. Base::Move()
+// tries to rename a file and if this fails, it tries copy-n-delete; This
+// RenameDirectory method only does the rename part.
+bool RenameDirectory(const base::FilePath& from_path,
+                     const base::FilePath& to_path) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+    return false;
+  if (from_path.value().length() >= MAX_PATH ||
+      to_path.value().length() >= MAX_PATH) {
+    return false;
+  }
+  return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0;
+}
+
 // Updates the jumplist, once all the data has been fetched.
 void RunUpdateOnFileThread(
     IncognitoModePrefs::Availability incognito_availability,
     const std::wstring& app_id,
     const base::FilePath& icon_dir,
     base::RefCountedData<JumpListData>* ref_counted_data) {
   JumpListData* data = &ref_counted_data->data;
   ShellLinkItemList local_most_visited_pages;
   ShellLinkItemList local_recently_closed_pages;
 
   {
     base::AutoLock auto_lock(data->list_lock_);
     // Make sure we are not out of date: if icon_urls_ is not empty, then
     // another notification has been received since we processed this one
     if (!data->icon_urls_.empty())
       return;
 
     // Make local copies of lists so we can release the lock.
     local_most_visited_pages = data->most_visited_pages_;
     local_recently_closed_pages = data->recently_closed_pages_;
   }
 
   // Delete the directory which contains old icon files, rename the current
   // icon directory, and create a new directory which contains new JumpList
   // icon files.
-  base::FilePath icon_dir_old(icon_dir.value() + L"Old");
+  base::FilePath icon_dir_old = icon_dir.DirName().Append(
+      icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old"));
 
   enum FolderOperationResult {
     SUCCESS = 0,
     DELETE_DEST_FAILED = 1 << 0,
-    MOVE_FAILED = 1 << 1,
-    DELETE_SRC_FAILED = 1 << 2,
-    CREATE_SRC_FAILED = 1 << 3,
+    RENAME_FAILED = 1 << 1,
+    DELETE_SRC_CONTENT_FAILED = 1 << 2,
+    DELETE_SRC_DIR_FAILED = 1 << 3,
+    CREATE_SRC_FAILED = 1 << 4,
     // 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
+    END = 1 << 5
   };
 
   // This variable records the status of three folder operations.
   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);
+        UMA_HISTOGRAM_ENUMERATION(
+            "WinJumplist.DetailedFolderResultsDeleteUpdated",

[Ilya Sherman, 2017/03/10 23:30:16]

Optional nit: I'd suggest a simpler renaming, to
"WinJumplist.DetailedFolderResults2", but either name is fine.

[chengx, 2017/03/10 23:58:51]

I know it is a bit long, but I will stick with this name for now.

+            *folder_operation_status_ptr, FolderOperationResult::END);
       },
       base::Unretained(&folder_operation_status)));
 
-  // If deletion of |icon_dir_old| fails, do not move |icon_dir| to
+  // If deletion of |icon_dir_old| fails, do not rename |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_old, true)) {
     folder_operation_status |= FolderOperationResult::DELETE_DEST_FAILED;
-    // If deletion of |icon_dir| fails, exit early. This skips creating the same
-    // directory and updating jumplist icons to avoid bloating the JumplistIcons
-    // folder.
+    // If deletion of any item in |icon_dir| fails, exit early. If deletion of
+    // all the items succeeds while only deletion of the dir fails, it is okay
+    // to proceed. This skips creating the same directory and updating jumplist
+    // icons to avoid bloating the JumplistIcons folder.
     if (!base::DeleteFile(icon_dir, true)) {
-      folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
-      return;
+      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) {
+        folder_operation_status |=
+            FolderOperationResult::DELETE_SRC_CONTENT_FAILED;
+        return;
+      } else {
+        folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED;
+      }
     }
-  } 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.
+  } else if (!RenameDirectory(icon_dir, icon_dir_old)) {
+    // If RenameDirectory() fails, delete |icon_dir| to avoid file accumulation
+    // in this directory, which can eventually lead the folder to be huge.
+    folder_operation_status |= FolderOperationResult::RENAME_FAILED;
+    // If deletion of any item in |icon_dir| fails, exit early. If deletion of
+    // all the items succeeds while only deletion of the dir fails, it is okay
+    // to proceed. This skips creating the same directory and updating jumplist
+    // icons to avoid bloating the JumplistIcons folder.
     if (!base::DeleteFile(icon_dir, true)) {
-      folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
-      return;
+      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) {
+        folder_operation_status |=
+            FolderOperationResult::DELETE_SRC_CONTENT_FAILED;
+        return;
+      } else {
+        folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED;
+      }
     }
   }
 
   if (!base::CreateDirectory(icon_dir))
     folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED;
 
   // 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"
@@ skipping 327 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);
 }