Cache JumpList icons to avoid unnecessary creation and deletion

chrome/browser/win/jumplist.h

Index: chrome/browser/win/jumplist.h
diff --git a/chrome/browser/win/jumplist.h b/chrome/browser/win/jumplist.h
index c4b6c857584430198509a89a08b224963d7fb3e0..be66aee40b5dd6442f30d628d9e3dd1266c5025e 100644
--- a/chrome/browser/win/jumplist.h
+++ b/chrome/browser/win/jumplist.h
@@ -12,6 +12,7 @@
 #include <string>
 #include <utility>
 
+#include "base/containers/flat_map.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -128,6 +129,8 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   explicit JumpList(Profile* profile);  // Use JumpListFactory instead
   ~JumpList() override;
 
+  enum class JumpListCategory { kMostVisited = 0, kRecentlyClosed };

[grt (UTC plus 2), 2017/05/15 11:28:19]

nit: omit " = 0"

[chengx, 2017/05/17 21:59:36]

Done.

+
   // Adds a new ShellLinkItem for |tab| to |data| provided that doing so will
   // not exceed |max_items|.
   bool AddTab(const sessions::TabRestoreService::Tab& tab,
@@ -179,17 +182,23 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // after requests storms have subsided.
   void DeferredTabRestoreServiceChanged();
 
+  // Deletes icon files in |icon_dir| which are not in the cache anymore.

[grt (UTC plus 2), 2017/05/15 11:28:19]

nit: mention |category| somehow here and in comments below.

[chengx, 2017/05/17 21:59:36]

Done.

+  void DeleteIconFiles(const base::FilePath& icon_dir,
+                       JumpListCategory category);
+
   // Creates at most |max_items| icon files in |icon_dir| for the
   // asynchrounously loaded icons stored in |item_list|.
   void CreateIconFiles(const base::FilePath& icon_dir,
                        const ShellLinkItemList& item_list,
-                       size_t max_items);
+                       size_t max_items,
+                       JumpListCategory category);
 
   // Updates 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);
+                       size_t slot_limit,
+                       JumpListCategory category);
 
   // Updates the jumplist, once all the data has been fetched. This method calls
   // UpdateJumpList() to do most of the work.
@@ -216,7 +225,7 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // Tracks FaviconService tasks.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
-  // The Profile object is used to listen for events
+  // The Profile object is used to listen for events.
   Profile* profile_;
 
   // Lives on the UI thread.
@@ -236,6 +245,11 @@ class JumpList : public sessions::TabRestoreServiceObserver,
   // Holds data that can be accessed from multiple threads.
   scoped_refptr<base::RefCountedData<JumpListData>> jumplist_data_;
 
+  // Cache of the mappings from JumpList items' URLs to icon file paths for

[grt (UTC plus 2), 2017/05/15 11:28:19]

please document which task runner(s) may access these members. it looks like all
accesses must be on the update_jumplist_task_runner_. is this correct?

[chengx, 2017/05/17 21:59:36]

Documentation added. Yes, only update_jumplist_task_runner_ should have access
to them. The update_jumplist_task_runner_ is dealing with
JumpListIcons{MostVistied,RecentClosed} folders which are newly introduced. As
to delete_jumplisticons_task_runner_, it is used to delete the retired
JumpListIcons{,Old} folders. Once these two folders are gone for all Chrome
users, the task runner should be deleted.

+  // "Most visited" and "Recently closed" categories, respectively.
+  base::flat_map<std::string, base::FilePath> most_visited_map_;

[grt (UTC plus 2), 2017/05/15 11:28:19]

based on how this is used, i think an accurate description is:

// The icon file paths of the most visited links in the current jumplist,
indexed by tab url.

naming nit: "map" in the name isn't really needed. how about most_visited_icons_
or something like that?

[chengx, 2017/05/17 21:59:36]

Done. Thanks for the suggestion!

+  base::flat_map<std::string, base::FilePath> recently_closed_map_;
+
   // Id of last favicon task. It's used to cancel current task if a new one
   // comes in before it finishes.
   base::CancelableTaskTracker::TaskId task_id_;