Index: chrome/browser/win/jumplist.cc |
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc |
index 680db16a6a490256bf10dd17a699b6806db5152a..c6c53d7197a2d63a5567f4d7534c70e7068b07d1 100644 |
--- a/chrome/browser/win/jumplist.cc |
+++ b/chrome/browser/win/jumplist.cc |
@@ -11,12 +11,14 @@ |
#include "base/bind_helpers.h" |
#include "base/callback_helpers.h" |
#include "base/command_line.h" |
+#include "base/files/file_enumerator.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/task_scheduler/post_task.h" |
#include "base/threading/thread.h" |
#include "base/threading/thread_restrictions.h" |
#include "base/trace_event/trace_event.h" |
@@ -63,6 +65,9 @@ namespace { |
// Delay jumplist updates to allow collapsing of redundant update requests. |
const int kDelayForJumplistUpdateInMS = 3500; |
+// Maximum number of icon files allowed to delete per update |
+const int kMaxIconFilesDeletedPerUpdate = 100; |
+ |
// Append the common switches to each shell link. |
void AppendCommonSwitches(ShellLinkItem* shell_link) { |
const char* kSwitchNames[] = { switches::kUserDataDir }; |
@@ -229,20 +234,192 @@ bool UpdateJumpList(const wchar_t* app_id, |
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) { |
+// Folder delete status enumeration, used in Delete* methods below. |
+// This is used for UMA. Do not delete entries, and keep in sync with |
+// histograms.xml. |
+enum FolderDeleteResult { |
+ SUCCEED = 0, |
+ // File name's length exceeds MAX_PATH. This won't happen. |
+ FAIL_INVALID_FILE_PATH, |
+ // JumpListIcons{,Old} directories are read-only. This may heppen. |
+ FAIL_READ_ONLY_DIRECTORY, |
+ // Since JumpListIcons{,Old} are directories, this won't happen. |
+ FAIL_DELETE_SINGLE_FILE, |
+ // Delete maximum files allowed succeeds. However, in the process of deleting |
+ // these files, it fails to delete some other files. This may happen. |
+ FAIL_DELETE_MAX_FILE_PERFECTLY, |
+ // Fail to delete maximum files allowed when the maximum attempts allowed |
+ // have been used. This may heppen. |
+ FAIL_DELETE_MAX_FILES_ANYWAY, |
+ // Add new items before this one, always keep this one at the end. |
+ END |
+}; |
+ |
+// This method is similar to base::DeleteFileRecursive in |
+// file_util_win.cc with the following differences. |
+// 1) It forces deleting files recursively. |
+// 2) It has a boolean input parameter |has_upper_limit|. When it is set to |
+// true, this method returns false when the files deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| or the files failed to be deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| per directory (e.g., |path| if it is |
+// a directory or any of its sub-directories). When it is set to false, all |
+// files under |path| are deleted. |
+// 3) Failure cause is recored in |delete_status|. |
+bool DeleteFileRecursive(const base::FilePath& path, |
grt (UTC plus 2)
2017/03/21 08:40:26
if this is only called to do recursive deletes on
chengx
2017/03/22 06:46:28
Done. Renamed to "DeleteDirectoryRecursive".
|
+ const base::FilePath::StringType& pattern, |
+ bool has_upper_limit, |
+ int* delete_status) { |
grt (UTC plus 2)
2017/03/21 08:40:27
int* -> FolderDeleteResult*
chengx
2017/03/22 06:46:27
Done.
|
+ base::FileEnumerator traversal( |
+ path, false, |
+ base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern); |
+ int file_deleted = 0, file_fail_to_deted = 0; |
grt (UTC plus 2)
2017/03/21 08:40:26
file_fail_to_deted -> failure_count
grt (UTC plus 2)
2017/03/21 08:40:27
file_deleted -> attempt_count
grt (UTC plus 2)
2017/03/21 08:40:27
nit: one definition per line, please
chengx
2017/03/22 06:46:28
I will rename it to |success_count|. I think if we
chengx
2017/03/22 06:46:28
Done.
chengx
2017/03/22 06:46:28
Done.
|
+ for (base::FilePath current = traversal.Next(); !current.empty(); |
+ current = traversal.Next(), file_deleted++) { |
+ // 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) { |
+ SetFileAttributes( |
+ current.value().c_str(), |
+ info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY); |
+ } |
+ |
+ if (info.IsDirectory()) { |
+ // If DeleteFileRecursive or RemoveDirectory fails, return false |
+ // intermediately. |
+ if (!DeleteFileRecursive(current, pattern, has_upper_limit, |
+ delete_status) || |
+ !::RemoveDirectory(current.value().c_str())) { |
+ return false; |
+ } |
+ } else if (!::DeleteFile(current.value().c_str())) { |
+ // If there is no upper limit, return false intermediately. Otherwise, |
+ // increment |file_fail_to_deted| until it hits the upper limit. |
+ if (!has_upper_limit) |
+ return false; |
+ file_fail_to_deted++; |
grt (UTC plus 2)
2017/03/21 08:40:27
since you don't decrement file_deleted here, it's
chengx
2017/03/22 06:46:27
You are right. It's actually a count of the # of a
|
+ } |
+ // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files, return |
+ // true. If some files fail to be delete, record this information in |
+ // |delete_status|. |
+ if (has_upper_limit && file_deleted > kMaxIconFilesDeletedPerUpdate) { |
grt (UTC plus 2)
2017/03/21 08:40:26
? file_deleted >= kMaxIconFilesDeletedPerUpdate ?
chengx
2017/03/22 06:46:28
Done.
|
+ if (file_fail_to_deted > 0) |
+ *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY; |
+ return true; |
+ } |
+ // Return false when failing to delete |kMaxIconFilesDeletedPerUpdate| files |
+ // comes before successfully deleting |kMaxIconFilesDeletedPerUpdate| files. |
+ if (has_upper_limit && file_fail_to_deted > kMaxIconFilesDeletedPerUpdate) { |
+ *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY; |
+ return false; |
+ } |
+ } |
+ return true; |
+} |
+ |
+// This method is similar to base::DeleteFile in file_util_win.cc |
+// with the following differences. |
+// 1) It forces deleting files recursively. |
+// 2) If |path| is a directory, it won't be deleted even if all its contents are |
+// deleted successfully. |
+// 3) It has a boolean input parameter |has_upper_limit|. When it is set to |
+// true, this method returns false when the files deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| or the files fail to be deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| per directory (e.g., under path if it is |
+// a directory or any of its sub-directory). When it is set to false, all |
+// files under path are trying to be deleted. |
+// 4) Failure cause is recored in |delete_status|. |
+bool DeleteDirectoryContent(const base::FilePath& path, |
+ bool has_upper_limit, |
+ int* delete_status) { |
grt (UTC plus 2)
2017/03/21 08:40:26
int* -> FolderDeleteResult*
chengx
2017/03/22 06:46:27
Done.
|
base::ThreadRestrictions::AssertIOAllowed(); |
- if (from_path.ReferencesParent() || to_path.ReferencesParent()) |
+ |
+ if (path.empty()) |
+ return true; |
+ |
+ if (path.value().length() >= MAX_PATH) { |
+ *delete_status = FAIL_INVALID_FILE_PATH; |
return false; |
- if (from_path.value().length() >= MAX_PATH || |
- to_path.value().length() >= MAX_PATH) { |
+ } |
+ |
+ 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(), |
+ attr & ~FILE_ATTRIBUTE_READONLY)) { |
+ *delete_status = FAIL_READ_ONLY_DIRECTORY; |
return false; |
} |
- return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0; |
+ // If |path| is a file, simply delete it. |
grt (UTC plus 2)
2017/03/21 08:40:26
this is never expected to be hit, but i agree that
chengx
2017/03/22 06:46:27
Done.
|
+ if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) { |
+ bool delete_result = !!::DeleteFile(path.value().c_str()); |
+ if (!delete_result) |
+ *delete_status = FAIL_DELETE_SINGLE_FILE; |
+ return delete_result; |
+ } |
+ // If |path| is a directory, delete all its content but save the raw |
+ // directory. |
+ return DeleteFileRecursive(path, L"*", has_upper_limit, delete_status); |
+} |
+ |
+// This method is similar to base::DeleteFile in file_util_win.cc |
+// with the following differences. |
+// 1) It forces deleting files recursively. |
+// 2) It runs in the asynchronous manner, so there is no return value. |
+// 3) It has a boolean input parameter |has_upper_limit|. When it is set to |
+// true, this method returns false when the files deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| or the files fail to be deleted exceeds |
+// |kMaxIconFilesDeletedPerUpdate| per directory (e.g., under path if it is |
+// a directory or any of its sub-directory). When it is set to false, all |
+// files under path are trying to be deleted. |
+// 4) Failure cause is recored in |delete_status|. |
+void DeleteDirectory(const base::FilePath& path, |
+ bool has_upper_limit, |
+ int* delete_status) { |
grt (UTC plus 2)
2017/03/21 08:40:26
int* -> FolderDeleteResult*
chengx
2017/03/22 06:46:28
Done.
|
+ base::ThreadRestrictions::AssertIOAllowed(); |
+ |
+ if (path.empty()) |
+ return; |
+ |
+ if (path.value().length() >= MAX_PATH) { |
+ *delete_status = FAIL_INVALID_FILE_PATH; |
+ return; |
+ } |
+ |
+ DWORD attr = GetFileAttributes(path.value().c_str()); |
+ // We're done if we can't find the path. |
+ if (attr == INVALID_FILE_ATTRIBUTES) |
+ return; |
+ // We may need to clear the read-only bit. |
+ if ((attr & FILE_ATTRIBUTE_READONLY) && |
+ !SetFileAttributes(path.value().c_str(), |
+ attr & ~FILE_ATTRIBUTE_READONLY)) { |
+ *delete_status = FAIL_READ_ONLY_DIRECTORY; |
+ return; |
+ } |
+ // If |path| is a file, delete it. |
+ if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) { |
+ ::DeleteFile(path.value().c_str()); |
+ *delete_status = FAIL_DELETE_SINGLE_FILE; |
+ return; |
+ } |
+ // If |path| is a directory, delete its contents and if it succeeds, remove |
+ // the raw directory. |
+ if (DeleteFileRecursive(path, L"*", has_upper_limit, delete_status)) |
+ ::RemoveDirectory(path.value().c_str()); |
+ return; |
+} |
+ |
+// Call DeleteDirectory method and then log the delete status using |
+// UMA_HISTOGRAM_ENUMERATION. |
Ilya Sherman
2017/03/21 20:56:44
This comment literally just repeats the code, whic
chengx
2017/03/22 06:46:27
Done.
|
+void DeleteDirectoryAndLogResults(const base::FilePath& path, |
+ bool has_upper_limit, |
+ int* delete_status, |
+ std::string histogram_name) { |
Ilya Sherman
2017/03/21 20:56:45
Why are you passing this string by copy?
chengx
2017/03/22 06:46:28
This parameter is removed in the new patch set.
|
+ DeleteDirectory(path, has_upper_limit, delete_status); |
+ UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END); |
grt (UTC plus 2)
2017/03/21 08:40:26
the UMA macros cannot be used with a generated his
Ilya Sherman
2017/03/21 20:56:44
You can also use UmaHistogramEnumeration() from hi
chengx
2017/03/22 06:46:27
The name parameter is removed.
chengx
2017/03/22 06:46:28
I have changed to use "WinJumplist.DeleteStatusJum
|
} |
// Updates the jumplist, once all the data has been fetched. |
@@ -267,73 +444,18 @@ void RunUpdateOnFileThread( |
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.DirName().Append( |
- icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old")); |
- |
- enum FolderOperationResult { |
- SUCCESS = 0, |
- DELETE_DEST_FAILED = 1 << 0, |
- 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 << 5 |
- }; |
- |
- // This variable records the status of three folder operations. |
- uint32_t folder_operation_status = FolderOperationResult::SUCCESS; |
+ // This variable records the delete status of folder JumpListIcons. |
+ int folder_delete_status = SUCCEED; |
grt (UTC plus 2)
2017/03/21 08:40:26
int -> FolderDeleteResult
chengx
2017/03/22 06:46:28
Done.
|
base::ScopedClosureRunner log_operation_status_when_done(base::Bind( |
- [](uint32_t* folder_operation_status_ptr) { |
- UMA_HISTOGRAM_ENUMERATION( |
- "WinJumplist.DetailedFolderResultsDeleteUpdated", |
- *folder_operation_status_ptr, FolderOperationResult::END); |
+ [](int* folder_delete_status_ptr) { |
+ UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIcons", |
+ *folder_delete_status_ptr, END); |
}, |
- base::Unretained(&folder_operation_status))); |
- |
- // 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 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)) { |
- if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) { |
- folder_operation_status |= |
- FolderOperationResult::DELETE_SRC_CONTENT_FAILED; |
- return; |
- } |
- folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED; |
- } |
- } 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)) { |
- if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) { |
- folder_operation_status |= |
- FolderOperationResult::DELETE_SRC_CONTENT_FAILED; |
- return; |
- } |
- folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED; |
- } |
- } |
+ base::Unretained(&folder_delete_status))); |
- // If CreateDirectory() fails, exit early. |
- if (!base::CreateDirectory(icon_dir)) { |
- folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED; |
+ // If failing to delete the content in |icon_dir|, exit early. |
+ if (!DeleteDirectoryContent(icon_dir, true, &folder_delete_status)) { |
return; |
} |
@@ -349,6 +471,25 @@ void RunUpdateOnFileThread( |
// with it. |
UpdateJumpList(app_id.c_str(), local_most_visited_pages, |
local_recently_closed_pages, incognito_availability); |
+ |
+ // Post a background task to delete JumpListIconsOld folder if it exists and |
+ // log the delete results to UMA. |
+ base::FilePath icon_dir_old = icon_dir.DirName().Append( |
+ icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old")); |
+ |
+ if (::PathFileExists(icon_dir_old.value().c_str())) { |
+ int jumplisticons_old_delete_status = SUCCEED; |
grt (UTC plus 2)
2017/03/21 08:40:26
move the definition of this |delete_status| variab
chengx
2017/03/22 06:46:28
Done.
|
+ std::string histogram_name = "WinJumplist.DeleteStatusJumpListIconsOld"; |
+ base::PostTaskWithTraits( |
grt (UTC plus 2)
2017/03/21 08:40:26
since a new task is posted for each jumplist icon
chengx
2017/03/22 06:46:28
Thanks for the suggestion. I have added a member n
|
+ FROM_HERE, |
+ base::TaskTraits() |
+ .WithPriority(base::TaskPriority::BACKGROUND) |
+ .WithShutdownBehavior( |
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) |
+ .MayBlock(), |
+ base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old, true, |
+ &jumplisticons_old_delete_status, histogram_name)); |
+ } |
} |
} // namespace |