Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(668)

Unified Diff: chrome/browser/win/jumplist_file_util.cc

Issue 2752063002: Remove JumpListIconsOld directory and set upper limit for delete attempts (Closed)
Patch Set: Add jumplist_util_file* and unit tests, plus address other comments. Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/win/jumplist_file_util.cc
diff --git a/chrome/browser/win/jumplist_file_util.cc b/chrome/browser/win/jumplist_file_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9b970ba78e13cad7096f2a55c25e99e97fcd1a9d
--- /dev/null
+++ b/chrome/browser/win/jumplist_file_util.cc
@@ -0,0 +1,158 @@
+// Copyright (c) 2017 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_file_util.h"
+
+#include <windows.h>
+
+#include "base/files/file_enumerator.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_restrictions.h"
+
+bool DeleteFiles(const base::FilePath& path,
+ const base::FilePath::StringType& pattern,
+ bool has_upper_limit,
+ FolderDeleteResult* delete_status) {
+ base::FileEnumerator traversal(
+ path, false,
+ base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+ int succees_count = 0;
+ int failure_count = 0;
+ 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) {
+ SetFileAttributes(
+ current.value().c_str(),
+ info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+ }
+
+ if (info.IsDirectory()) {
+ // JumpListIcons{,Old} directories shouldn't have sub-directories.
+ // If it does for unknown reasons, don't try to delete it. Instead,
+ // increment failure count if |has_upper_limit| is true and record this
+ // information in |delete_status|.
+ *delete_status = FAIL_SUBDIRECTORY_EXISTS;
+ if (!has_upper_limit)
+ return false;
+ failure_count++;
+ } else if (!::DeleteFile(current.value().c_str())) {
+ // If there is no upper limit, return false intermediately. Otherwise,
+ // increment |failure_count| until it hits the upper limit.
+ if (!has_upper_limit)
+ return false;
+ failure_count++;
+ } else {
+ succees_count++;
+ }
+ // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files without
grt (UTC plus 2) 2017/03/22 12:07:21 i think this is more clear with both limit checks
chengx 2017/03/22 21:16:36 Agreed and updated accordingly.
+ // any failures, return true. If some files fail to be delete, record this
+ // information in |delete_status| and returns false.
+ if (has_upper_limit && succees_count >= kMaxIconFilesDeletedPerUpdate) {
+ if (failure_count > 0) {
+ *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY;
grt (UTC plus 2) 2017/03/22 12:07:22 this reads to me like "we failed because we perfec
chengx 2017/03/22 21:16:36 Done.
+ }
+ return failure_count == 0;
+ }
+ // Return false when failing to delete |kMaxIconFilesDeletedPerUpdate| files
+ // comes before successfully deleting |kMaxIconFilesDeletedPerUpdate| files.
+ if (has_upper_limit && failure_count >= kMaxIconFilesDeletedPerUpdate) {
+ *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY;
grt (UTC plus 2) 2017/03/22 12:07:21 FAIL_MAX_DELETE_FAILURES -> "we failed because we
chengx 2017/03/22 21:16:36 Sounds good.
+ return false;
+ }
+ }
+ return true;
+}
+
+bool DeleteDirectoryContent(const base::FilePath& path,
grt (UTC plus 2) 2017/03/22 12:07:21 return FolderDeleteResult rather than bool (which
chengx 2017/03/22 21:16:36 Done.
+ bool has_upper_limit,
+ FolderDeleteResult* delete_status) {
+ base::ThreadRestrictions::AssertIOAllowed();
grt (UTC plus 2) 2017/03/22 12:07:21 this function seems to assume that |delete_status|
chengx 2017/03/22 21:16:36 Changed to explicitly set it.
+
+ if (path.empty())
+ return true;
+
+ // For JumpListIcons{,Old} directories, since their names are shorter than
+ // MAX_PATH, hitting the code block below is unexpected.
+ if (path.value().length() >= MAX_PATH) {
+ *delete_status = FAIL_INVALID_FILE_PATH;
+ return false;
+ }
+
+ 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;
+ }
+
+ // If |path| is a file, simply delete it.
+ // Since JumpListIcons{,Old} are directories, hitting the code block below is
+ // unexpected.
+ 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 DeleteFiles(path, L"*", has_upper_limit, delete_status);
+}
+
+void DeleteDirectory(const base::FilePath& path,
grt (UTC plus 2) 2017/03/22 12:07:21 return FolderDeleteResult rather than void w/ out
chengx 2017/03/22 21:16:36 Done.
+ bool has_upper_limit,
+ FolderDeleteResult* delete_status) {
+ base::ThreadRestrictions::AssertIOAllowed();
+
+ if (path.empty())
+ return;
+
+ // For JumpListIcons{,Old} directories, since their names are shorter than
+ // MAX_PATH, hitting the code block below is unexpected.
+ 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.
+ // Since JumpListIcons{,Old} are directories, hitting the code block below is
+ // unexpected.
+ 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 (DeleteFiles(path, L"*", has_upper_limit, delete_status))
+ ::RemoveDirectory(path.value().c_str());
+ return;
+}
+
+void DeleteDirectoryAndLogResults(const base::FilePath& path,
+ bool has_upper_limit) {
+ FolderDeleteResult delete_status = SUCCEED;
+ DeleteDirectory(path, has_upper_limit, &delete_status);
+ UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIconsOld",
+ static_cast<int>(delete_status), END);
grt (UTC plus 2) 2017/03/22 12:07:21 since FolderDeleteResult is an old-school enum and
chengx 2017/03/22 21:16:36 You are right. It is not needed.
+}

Powered by Google App Engine
This is Rietveld 408576698