Retire some metrics and update file util methods for JumpList

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 <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 105 matching lines @@
   if (!IconUtil::CreateIconFileFromImageFamily(image_family, path,
                                                IconUtil::NORMAL_WRITE))
     return false;
 
   // Add this icon file to the list and return its absolute path.
   // The IShellLink::SetIcon() function needs the absolute path to an icon.
   *icon_path = path;
   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) {
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
 
   for (ShellLinkItemList::const_iterator item = item_list.begin();
        item != item_list.end() && max_items > 0; ++item, --max_items) {
     base::FilePath icon_path;
     if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path))
       (*item)->set_icon(icon_path.value(), 0);
   }
 }
 
+// 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,
     IncognitoModePrefs::Availability incognito_availability) {
   base::FilePath chrome_path;
   if (!PathService::Get(base::FILE_EXE, &chrome_path))
     return false;
 
   int icon_index = install_static::GetIconResourceIndex();
 
@@ skipping 82 matching lines @@
 
   size_t most_visited_items =
       MulDiv(user_max_items_adjusted, kMostVisited, kTotal);
   size_t recently_closed_items = user_max_items_adjusted - most_visited_items;
   if (recently_closed_pages.size() < recently_closed_items) {
     most_visited_items += recently_closed_items - recently_closed_pages.size();
     recently_closed_items = recently_closed_pages.size();
   }
 
   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.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
 
   // Update the "Most Visited" category of the JumpList if it exists.
   // This update request is applied into the JumpList when we commit this
   // transaction.
   if (!jumplist_updater.AddCustomCategory(
           l10n_util::GetStringUTF16(IDS_NEW_TAB_MOST_VISITED),
@@ skipping 97 matching lines @@
   // When we add this object to the observer list, we save the pointer to this
   // TabRestoreService object. This pointer is used when we remove this object
   // from the observer list.
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile_);
   if (!tab_restore_service)
     return;
 
   app_id_ =
       shell_integration::win::GetChromiumModelIdForProfile(profile_->GetPath());
   icon_dir_ = profile_->GetPath().Append(chrome::kJumpListIconDirname);

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

this directory isn't used anymore, right? i think it makes more sense to cache
the profile path in the jumplist instance and then append
chrome::kJumpListIconDirname + kFoo as needed at the various points of use
rather than the current chopping and adding.

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

Agreed. I'll use another small CL for this as it doesn't well fit this CL.

 
   scoped_refptr<history::TopSites> top_sites =
       TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
     // TopSites updates itself after a delay. This is especially noticable when
     // your profile is empty. Ask TopSites to update itself when jumplist is
     // initialized.
     top_sites->SyncWithHistory();
     // Register as TopSitesObserver so that we can update ourselves when the
     // TopSites changes.
@@ skipping 271 matching lines @@
   // Post a task to update the JumpList, which consists of 1) delete old icons,
   // 2) create new icons, 3) notify the OS.
   update_jumplist_task_runner_->PostTask(
       FROM_HERE, base::Bind(&RunUpdateJumpList, incognito_availability, app_id_,
                             icon_dir_, base::RetainedRef(jumplist_data_)));
 
   // Post a task to delete JumpListIcons folder as it's no longer needed.
   // 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) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }