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

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 3c114eeab9aa26465d4946a0390a3ba229b22062..80a4cfb0ccdb5009fda9d27d053c9b96d0f1814f 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/win/jumplist.h"
 
+#include <windows.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
@@ -15,6 +17,7 @@
 #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"
@@ -225,6 +228,20 @@ bool UpdateJumpList(const wchar_t* app_id,
   return true;
 }
 
+// Base::Move() tries to rename a file and if this fails, it tries copy-n-delete

[grt (UTC plus 2), 2017/03/09 10:11:48]

nit: document what this function does before explaining why; see
https://google.github.io/styleguide/cppguide.html#Function_Comments for details.

i would also be inclined to make this RenameDirectory and document that this is
all it should be used for with something like:

// Renames the directory |from_dir| to |to_path|. Fails if any process has a
handle open in |from_dir| or if |to_path| exists.

[chengx, 2017/03/09 23:57:36]

Done.

+// This Rename method has the same functionality of its "rename" part.
+bool Rename(const base::FilePath& from_path, const base::FilePath& to_path) {
+  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+    return false;
+  base::ThreadRestrictions::AssertIOAllowed();

[grt (UTC plus 2), 2017/03/09 10:11:48]

nit: move the assert to the top of the function

[chengx, 2017/03/09 23:57:36]

Done.

+  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(),
+                    MOVEFILE_REPLACE_EXISTING) != 0;

[grt (UTC plus 2), 2017/03/09 10:11:48]

MOVEFILE_REPLACE_EXISTING -> 0 since you're moving a directory (msdn says "This
value cannot be used if lpNewFileName or lpExistingFileName names a
directory.").

[chengx, 2017/03/09 23:57:36]

Ah, yes you are right.

+}
+
 // Updates the jumplist, once all the data has been fetched.
 void RunUpdateOnFileThread(
     IncognitoModePrefs::Availability incognito_availability,
@@ -286,7 +303,7 @@ void RunUpdateOnFileThread(
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
       return;
     }
-  } else if (!base::Move(icon_dir, icon_dir_old)) {
+  } else if (!Rename(icon_dir, icon_dir_old)) {
     folder_operation_status |= FolderOperationResult::MOVE_FAILED;
     // If Move() fails, delete |icon_dir| to avoid file accumulation in this

[grt (UTC plus 2), 2017/03/09 10:11:48]

"If Rename() fails..." (or RenameDirectory if you accept my comment above)

[chengx, 2017/03/09 23:57:36]

Done.

     // directory, which can eventually lead the folder to be huge.