Cache JumpList icons to avoid unnecessary creation and deletion

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 24ac405c820d75e2c19b92784cc03fe21829bc8d..d79dae180d7feb40036c2dbca98197baa16dc06f 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -8,6 +8,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/containers/flat_set.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
@@ -287,6 +288,7 @@ void JumpList::OnMostVisitedURLsAvailable(
       link->GetCommandLine()->AppendSwitchASCII(
           switches::kWinJumplistAction, jumplist::kMostVisitedCategory);
       link->set_title(!url.title.empty() ? url.title : url_string_wide);
+      link->set_url(url_string);
       data->most_visited_pages_.push_back(link);
       data->icon_urls_.push_back(std::make_pair(url_string, link));
     }
@@ -339,6 +341,7 @@ bool JumpList::AddTab(const sessions::TabRestoreService::Tab& tab,
   link->GetCommandLine()->AppendSwitchASCII(switches::kWinJumplistAction,
                                             jumplist::kRecentlyClosedCategory);
   link->set_title(current_navigation.title());
+  link->set_url(url);
   data->recently_closed_pages_.push_back(link);
   data->icon_urls_.push_back(std::make_pair(std::move(url), std::move(link)));
 
@@ -566,28 +569,102 @@ void JumpList::DeferredTabRestoreServiceChanged() {
   StartLoadingFavicon();
 }
 
+void JumpList::DeleteIconFiles(const base::FilePath& icon_dir,
+                               JumpListCategory category) {
+  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
+  switch (category) {
+    case JumpListCategory::kMostVisited:
+      source_map = &most_visited_map_;
+      break;
+    case JumpListCategory::kRecentlyClosed:
+      source_map = &recently_closed_map_;
+      break;
+  }
+
+  if (!source_map)

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

remove this check -- it can't be hit under normal circumstances, and a crash
down below is preferable to silently proceeding under crazy circumstances.

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

Done.

+    return;
+
+  // Put all cached icon file paths into a set.
+  base::flat_set<base::FilePath> cached_files;
+  cached_files.reserve(source_map->size());
+
+  for (const auto& url_path_pair : *source_map)
+    cached_files.insert(cached_files.end(), url_path_pair.second);

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

oops, i was wrong about this being O(N) -- that would only be the case if
source_map was sorted on path, which it isn't. you can go two ways here: 1) add
a little more complexity by populating a vector then constructing the set from
the vector (this is optimal according to base/containers/README.md), or 2)
figure that the container is small enough and paths are movable so the simple
insert is good enough. i would go with the latter, so just remove the hint and
let insert to its thing.

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

I would go with the latter one too, as the size of these sets should be either 3
or 5, which is small enough.

+
+  DeleteNonCachedFiles(icon_dir, cached_files);
+}
+
 void JumpList::CreateIconFiles(const base::FilePath& icon_dir,
                                const ShellLinkItemList& item_list,
-                               size_t max_items) {
+                               size_t max_items,
+                               JumpListCategory category) {
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration");
 
+  // If the current URL is cached which means there's already an icon for this

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

maybe this can be simplified by removing the idea of a cache:

"Reuse icons for urls that were already present in the jumplist for this
category."

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

Done.

+  // URL, then we simply re-use the icon. Otherwise, we need to create a new
+  // icon for this URL.
+
+  base::flat_map<std::string, base::FilePath>* source_map = nullptr;
+  switch (category) {
+    case JumpListCategory::kMostVisited:
+      source_map = &most_visited_map_;
+      break;
+    case JumpListCategory::kRecentlyClosed:
+      source_map = &recently_closed_map_;
+      break;
+  }
+
+  if (!source_map)

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

remove as above

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

Done.

+    return;
+
+  base::flat_map<std::string, base::FilePath> updated_map;
+
   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);
+    if (source_map->find((*item)->url()) != source_map->end()) {

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

nit: (*item)-> everywhere is a bit ugly. how about changing |item| in the loop
to |iter| and introducing:
    ShellLinkItem* item = iter->get();

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

Done.

+      base::FilePath path = (*source_map)[(*item)->url()];

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

rather than indexing into the map a second time, capture the result of find()
above and use it here:
    auto cache_iter = source_map->find(item->url());
    if (cache_iter != source_map->end()) {
      item->set_icon(cache_iter->second.value(), 0);
      updated_map[item->url()] = cache_iter->second.value();
    } else {
      ...
    }

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

Done.

+      (*item)->set_icon((*source_map)[(*item)->url()].value(), 0);
+      updated_map[(*item)->url()] = path;
+    } else {
+      base::FilePath icon_path;
+      if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path)) {

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

rather than creating new icons as you go here and then deleting old ones
sometime later, how about:
- figure out which items in item_list need new icons and which entries in
source_map are to be retained (add them to updated_map)
- delete files on disk that are not represented in updated_map
- create new icons and add them to updated_map

will this help prevent another file explosion in the future?

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

I understand what you suggest here, but IMHO it doesn't make a difference from
the current implementation where deletion comes after creation. The
UpdateIconFiles calls FilesExceedLimitInDir() in L657 before CreateIconFiles()
and DeleteIconFiles() to make sure the jumplisticon folders are healthy before
any operations. So file explosion won't happen in the current implementation.

+        (*item)->set_icon(icon_path.value(), 0);
+        updated_map[(*item)->url()] = icon_path;
+      }
+    }
   }
+  source_map->swap(updated_map);
 }
 
 void JumpList::UpdateIconFiles(const base::FilePath& icon_dir,
                                const ShellLinkItemList& page_list,
-                               size_t slot_limit) {
-  DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
-
-  // Create new icons only when the directory exists and is empty.
-  if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
-    CreateIconFiles(icon_dir, page_list, slot_limit);
+                               size_t slot_limit,
+                               JumpListCategory category) {
+  // Maximum number of icon files that each JumpList icon folder allows to hold.
+  size_t icon_limit = (category == JumpListCategory::kMostVisited) ? 12 : 6;
+
+  // Clear the JumpList icon folder at |icon_dir| and the cache when
+  // 1) "Most visited" category updates for the 1st time after Chrome is
+  //     launched. This actually happens right after Chrome is launched.
+  // 2) "Recently closed" category updates for the 1st time after Chrome is
+  //     launched.
+  // 3) The number of icons in |icon_dir| has exceeded the limit.
+  if ((category == JumpListCategory::kMostVisited &&
+       most_visited_map_.empty()) ||
+      (category == JumpListCategory::kRecentlyClosed &&
+       recently_closed_map_.empty()) ||
+      FilesExceedLimitInDir(icon_dir, icon_limit)) {
+    DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit);
+    most_visited_map_.clear();
+    recently_closed_map_.clear();
+    // Create new icons only when the directory exists and is empty.
+    if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir))
+      CreateIconFiles(icon_dir, page_list, slot_limit, category);
+  } else if (base::CreateDirectory(icon_dir)) {
+    CreateIconFiles(icon_dir, page_list, slot_limit, category);

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

should this check that the max # of files isn't being exceeded? if no, could
this potentially lead to the dir-with-too-many-files problem again?

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

No, this won't cause the dir-with-too-many-files problem.
There's a FilesExceedLimitInDir(icon_dir, icon_limit) check above in L657. If
the file limit is reached, this "else if" section of code won't be hit. 

+    DeleteIconFiles(icon_dir, category);
+  }
 }
 
 bool JumpList::UpdateJumpList(
@@ -654,7 +731,7 @@ bool JumpList::UpdateJumpList(
         profile_dir, FILE_PATH_LITERAL("MostVisited"));
 
     UpdateIconFiles(icon_dir_most_visited, most_visited_pages,
-                    most_visited_items);
+                    most_visited_items, JumpListCategory::kMostVisited);
 
     icons_to_create += std::min(most_visited_pages.size(), most_visited_items);
   }
@@ -665,7 +742,7 @@ bool JumpList::UpdateJumpList(
         profile_dir, FILE_PATH_LITERAL("RecentClosed"));
 
     UpdateIconFiles(icon_dir_recent_closed, recently_closed_pages,
-                    recently_closed_items);
+                    recently_closed_items, JumpListCategory::kRecentlyClosed);
 
     icons_to_create +=
         std::min(recently_closed_pages.size(), recently_closed_items);