Add more detailed JumpListIcons folder's move operation metric to UMA

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 1b9d92eb395eb0659484e7ed99f6aad01923d9bb..a9d67ed1b7a4a9e215edae0e0cb4fc651a529b98 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -59,11 +59,12 @@ const int kDelayForJumplistUpdateInMS = 3500;
 // JumpList folder's move operation status.
 enum MoveOperationResult {
   SUCCESS = 0,
-  MAX_PATH_CHECK_FAILED = 1 << 0,
-  MOVE_FILE_EX_FAILED = 1 << 1,
-  COPY_AND_DELETE_DIR_FAILED = 1 << 2,
-  DELETE_FAILED = 1 << 3,
-  END = 1 << 4
+  MOVE_FILE_EX_FAILED = 1 << 0,
+  COPY_FILE_FAILED = 1 << 1,
+  DELETE_FILE_RECURSIVE_FAILED = 1 << 2,
+  DELETE_SOURCE_FOLDER_FAILED = 1 << 3,
+  DELETE_TARGET_FOLDER_FAILED = 1 << 4,
+  END = 1 << 5
 };
 
 // Append the common switches to each shell link.
@@ -225,10 +226,88 @@ bool UpdateJumpList(const wchar_t* app_id,
 
 // A wrapper function for recording UMA histogram.
 void RecordFolderMoveResult(int move_operation_status) {
-  UMA_HISTOGRAM_ENUMERATION("WinJumplist.FolderMoveResults",
+  UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderMoveResults",
                             move_operation_status, MoveOperationResult::END);
 }
 
+// This function is mostly copied from //base for UMA debugging purpose.

[gab, 2016/12/09 19:12:12]

// This function is an exact copy of the one in //base used for UMA debugging of
DeleteFile() below.

[chengx, 2016/12/09 21:36:30]

Yes, you are right. It is an exact copy. I copy it because it is in anonymous
namespace in //base, making it inaccessible from outside.

+// Deletes all files and directories in a path.
+// Returns false on the first failure it encounters.
+bool DeleteFileRecursive(const base::FilePath& path,
+                         const base::FilePath::StringType& pattern,
+                         bool recursive) {
+  base::FileEnumerator traversal(
+      path, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+  for (base::FilePath current = traversal.Next(); !current.empty();
+       current = traversal.Next()) {
+    // Try to clear the read-only bit if we find it.
+    base::FileEnumerator::FileInfo info = traversal.GetInfo();
+    if ((info.find_data().dwFileAttributes & FILE_ATTRIBUTE_READONLY) &&
+        (recursive || !info.IsDirectory())) {
+      SetFileAttributes(
+          current.value().c_str(),
+          info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+    }
+
+    if (info.IsDirectory()) {
+      if (recursive && (!DeleteFileRecursive(current, pattern, true) ||
+                        !RemoveDirectory(current.value().c_str())))
+        return false;
+    } else if (!::DeleteFile(current.value().c_str())) {
+      return false;
+    }
+  }
+  return true;
+}
+
+// This function is mostly copied from //base for UMA debugging purpose.

[gab, 2016/12/09 19:12:12]

// This function is a copy of the one in //base augmented for UMA debugging
purposes.

(i.e. "mostly" copied leaves it unclear whether logic was modified or not)

[chengx, 2016/12/09 21:36:31]

Done.

+// Deletes all files and directories in a path.
+// Returns false on the first failure it encounters.
+bool DeleteFileTemp(const base::FilePath& path,

[gab, 2016/12/09 19:12:12]

naming this "DeleteFile" is fine IMO, it won't be in base:: so it's easy to know
you're referring to the right one.

[chengx, 2016/12/09 21:36:30]

Done.

+                    bool recursive,
+                    int* move_operation_status) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  if (path.empty())
+    return true;
+
+  if (path.value().length() >= MAX_PATH)
+    return false;
+
+  // Handle any path with wildcards.
+  if (path.BaseName().value().find_first_of(L"*?") !=
+      base::FilePath::StringType::npos) {
+    return DeleteFileRecursive(path.DirName(), path.BaseName().value(),

[gab, 2016/12/09 19:12:12]

Not worried about this failing?

[chengx, 2016/12/09 21:36:31]

I don't think it will be hit in my case. But I add UMA here for full coverage in
the new patch. Thanks for reminder.

+                               recursive);
+  }
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return true;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),

[gab, 2016/12/09 19:12:12]

Since we now have methods in file scope, some in base: and some from Windows,
let's make it clear which methods are from MSDN by prefixing with ::

::GetFileAttributes()
::SetFileAttributes()
::RemoveDirectory()

[chengx, 2016/12/09 21:36:31]

Done.

+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    return false;

[gab, 2016/12/09 19:12:12]

Not worried about this failing?

[chengx, 2016/12/09 21:36:30]

Just added UMA for this. Thanks for reminder.

+  }
+  // Directories are handled differently if they're recursive.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY))
+    return !!::DeleteFile(path.value().c_str());

[gab, 2016/12/09 19:12:12]

#include <windows.h> for ::DeleteFile() and other constants/methods above.

[chengx, 2016/12/09 21:36:31]

Done.

+  // Handle a simple, single file delete.
+  if (!recursive || DeleteFileRecursive(path, L"*", true)) {
+    bool ret = !!RemoveDirectory(path.value().c_str());
+    if (!ret)
+      *move_operation_status |=
+          MoveOperationResult::DELETE_SOURCE_FOLDER_FAILED;
+    return ret;
+  }
+
+  *move_operation_status |= MoveOperationResult::DELETE_FILE_RECURSIVE_FAILED;
+  *move_operation_status |= MoveOperationResult::DELETE_SOURCE_FOLDER_FAILED;
+  return false;
+}
+
 // This function is a temporary fork of Move() and MoveUnsafe() in
 // base/files/file_util.h, where UMA functions are added to gather user data.
 // As //base is highly mature, we tend not to add this temporary function
@@ -244,14 +323,12 @@ bool MoveDebugWithUMA(const base::FilePath& from_path,
   // This variable records the status of move operations.
   int move_operation_status = MoveOperationResult::SUCCESS;
   if (folder_delete_fail)
-    move_operation_status |= MoveOperationResult::DELETE_FAILED;
+    move_operation_status |= MoveOperationResult::DELETE_TARGET_FOLDER_FAILED;
 
   // NOTE: I suspect we could support longer paths, but that would involve
   // analyzing all our usage of files.
   if (from_path.value().length() >= MAX_PATH ||
       to_path.value().length() >= MAX_PATH) {
-    move_operation_status |= MoveOperationResult::MAX_PATH_CHECK_FAILED;
-    RecordFolderMoveResult(move_operation_status);
     return false;
   }
   if (MoveFileEx(from_path.value().c_str(), to_path.value().c_str(),
@@ -271,14 +348,21 @@ bool MoveDebugWithUMA(const base::FilePath& from_path,
     // MoveFileEx fails if moving directory across volumes. We will simulate
     // the move by using Copy and Delete. Ideally we could check whether
     // from_path and to_path are indeed in different volumes.
-    ret = base::internal::CopyAndDeleteDirectory(from_path, to_path);
+    if (base::CopyDirectory(from_path, to_path, true)) {
+      if (DeleteFileTemp(from_path, true, &move_operation_status))
+        ret = true;
+    } else {
+      move_operation_status |= MoveOperationResult::COPY_FILE_FAILED;

[gab, 2016/12/09 19:12:13]

This is copying a directory, not a "file", right?

s/COPY_FILE_FAILED/COPY_DIRECTORY_FAILED/ ?

[chengx, 2016/12/09 21:36:31]

Yes, you are right. 

+      move_operation_status |=
+          MoveOperationResult::DELETE_FILE_RECURSIVE_FAILED;
+      move_operation_status |= MoveOperationResult::DELETE_SOURCE_FOLDER_FAILED;
+    }
   }
 
   if (!ret) {
     // Leave a clue about what went wrong so that it can be (at least) picked
     // up by a PLOG entry.
     ::SetLastError(last_error);
-    move_operation_status |= MoveOperationResult::COPY_AND_DELETE_DIR_FAILED;
   }
 
   RecordFolderMoveResult(move_operation_status);