Retire some metrics and update file util methods for JumpList

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 6dad5579697389d68a137d050de08c7ae6b97199..26f88b2a3bf2a64b5483c62e43e29774736b51d0 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -123,8 +123,8 @@ bool CreateIconFile(const gfx::ImageSkia& image_skia,
   return true;
 }
 
-// Helper method for RunUpdate to create icon files for the asynchrounously
-// loaded icons.
+// Helper method for RunUpdateJumpList to create icon files for the

[grt (UTC plus 2), 2017/04/28 07:37:07]

please follow the function comment style guidelines here and elsewhere
(https://google.github.io/styleguide/cppguide.html#Function_Comments). for
example "Creates icon files for the ..."

[chengx, 2017/04/28 22:29:29]

Done. I've updated the comments throughout the file.

+// asynchrounously loaded icons.
 void CreateIconFiles(const base::FilePath& icon_dir,
                      const ShellLinkItemList& item_list,
                      size_t max_items) {
@@ -139,6 +139,25 @@ void CreateIconFiles(const base::FilePath& icon_dir,
   }
 }
 
+// Helper method for RunUpdateJumpList to update icon files in |icon_dir|, which
+// includes deleting old icons and creating at most |slot_limit| new icons for
+// |page_list|.
+void UpdateIconFiles(const base::FilePath& icon_dir,
+                     const ShellLinkItemList& page_list,
+                     size_t slot_limit) {
+  DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
+
+  if (!base::DirectoryExists(icon_dir))

[grt (UTC plus 2), 2017/04/28 07:37:07]

base::CreateDirectory returns true if the directory already exists, so i believe
this logic here can be simplified to:

  if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
    CreateIconFiles(icon_dir, page_list, slot_limit);

[chengx, 2017/04/28 22:29:29]

Done.

+    base::CreateDirectory(icon_dir);
+
+  // If the directory still doesn't exist or is non-empty, skip updating the
+  // jumplist icons for this jumplist category. The jumplist links should be
+  // updated later on anyway, as it doesn't involve disk IO. In this case,

[grt (UTC plus 2), 2017/04/28 07:37:07]

regarding this comment, are you saying that calling the shell functions to
update the jumplist doesn't do disk io? have you verified this? based on your
timings indicating that some of the shell functions can take a very long time,
wouldn't you expect that the reason is due to disk io?

[chengx, 2017/04/28 22:29:29]

No, I didn't mean that. Calling the shell functions to update the jumplist
involves disk IO I think although I am not 100% sure. 

I am saying we don't need to write these jumplist links into the JumpListIcons
folder as what we do to the icons. I've removed this comment to avoid confusion.

+  // Chrome's icon will be used for the new links.
+  if (base::DirectoryExists(icon_dir) && base::IsDirectoryEmpty(icon_dir))
+    CreateIconFiles(icon_dir, page_list, slot_limit);
+}
+
 // Updates the "Tasks" category of the JumpList.
 bool UpdateTaskCategory(
     JumpListUpdater* jumplist_updater,
@@ -241,42 +260,23 @@ bool UpdateJumpList(const wchar_t* app_id,
   }
 
   if (most_visited_pages_have_updates) {
-    // Delete the content in JumpListIconsMostVisited folder and log the results
+    // Delete the content in JumpListIconsMostVisited folder and log the runtime
     // to UMA.
     base::FilePath icon_dir_most_visited = icon_dir.DirName().Append(
         icon_dir.BaseName().value() + FILE_PATH_LITERAL("MostVisited"));
 
-    DeleteDirectoryContentAndLogResults(icon_dir_most_visited,
-                                        kFileDeleteLimit);
-
-    // If the directory doesn't exist (we have tried to create it in
-    // DeleteDirectoryContentAndLogResults) or is not empty, skip updating the
-    // jumplist icons. The jumplist links should be updated anyway, as it
-    // doesn't involve disk IO. In this case, Chrome's icon will be used for the
-    // new links.
-    if (base::DirectoryExists(icon_dir_most_visited) &&
-        base::IsDirectoryEmpty(icon_dir_most_visited)) {
-      // Create icon files for shortcuts in the "Most Visited" category.
-      CreateIconFiles(icon_dir_most_visited, most_visited_pages,
-                      most_visited_items);
-    }
+    UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
+                    most_visited_items);
   }
 
   if (recently_closed_pages_have_updates) {
     // Delete the content in JumpListIconsRecentClosed folder and log the
-    // results to UMA.
+    // runtime to UMA.
     base::FilePath icon_dir_recent_closed = icon_dir.DirName().Append(
         icon_dir.BaseName().value() + FILE_PATH_LITERAL("RecentClosed"));
 
-    DeleteDirectoryContentAndLogResults(icon_dir_recent_closed,
-                                        kFileDeleteLimit);
-
-    if (base::DirectoryExists(icon_dir_recent_closed) &&
-        base::IsDirectoryEmpty(icon_dir_recent_closed)) {
-      // Create icon files for shortcuts in the "Recently Closed" category.
-      CreateIconFiles(icon_dir_recent_closed, recently_closed_pages,
-                      recently_closed_items);
-    }
+    UpdateIconFiles(icon_dir_recent_closed, recently_closed_pages,
+                    recently_closed_items);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
@@ -686,16 +686,15 @@ void JumpList::DeferredRunUpdate() {
   // Now we have JumpListIconsMostVisited folder and JumpListIconsRecentClosed
   // folder instead.
   delete_jumplisticons_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&DeleteDirectoryAndLogResults, icon_dir_, kFileDeleteLimit));
+      FROM_HERE, base::Bind(&DeleteDirectory, icon_dir_, kFileDeleteLimit));
 
   // Post a task to delete JumpListIconsOld folder as it's no longer needed.
   base::FilePath icon_dir_old = icon_dir_.DirName().Append(
       icon_dir_.BaseName().value() + FILE_PATH_LITERAL("Old"));
 
   delete_jumplisticons_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&DeleteDirectoryAndLogResults,
-                            std::move(icon_dir_old), kFileDeleteLimit));
+      FROM_HERE,
+      base::Bind(&DeleteDirectory, std::move(icon_dir_old), kFileDeleteLimit));
 }
 
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {