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

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 41 matching lines @@
 using JumpListData = JumpList::JumpListData;
 
 namespace {
 
 // Delay jumplist updates to allow collapsing of redundant update requests.
 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.
 void AppendCommonSwitches(ShellLinkItem* shell_link) {
   const char* kSwitchNames[] = { switches::kUserDataDir };
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   shell_link->GetCommandLine()->CopySwitchesFrom(command_line,
                                                  kSwitchNames,
                                                  arraysize(kSwitchNames));
@@ skipping 141 matching lines @@
 
   // Commit this transaction and send the updated JumpList to Windows.
   if (!jumplist_updater.CommitUpdate())
     return false;
 
   return true;
 }
 
 // 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
 // in it. This change will be reverted after user data gathered.
 bool MoveDebugWithUMA(const base::FilePath& from_path,
                       const base::FilePath& to_path,
                       int folder_delete_fail) {
   if (from_path.ReferencesParent() || to_path.ReferencesParent())
     return false;
 
   base::ThreadRestrictions::AssertIOAllowed();
 
   // 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(),
                  MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING) != 0) {
     RecordFolderMoveResult(move_operation_status);
     return true;
   }
 
   move_operation_status |= MoveOperationResult::MOVE_FILE_EX_FAILED;
 
   // Keep the last error value from MoveFileEx around in case the below
   // fails.
   bool ret = false;
   DWORD last_error = ::GetLastError();
 
   if (base::DirectoryExists(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);
 
   return ret;
 }
 
 // Updates the jumplist, once all the data has been fetched.
 void RunUpdateOnFileThread(
     IncognitoModePrefs::Availability incognito_availability,
@@ skipping 379 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);
 }