Cache JumpList icons to avoid unnecessary creation and deletion

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 "base/base_paths.h"
 #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"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
@@ skipping 259 matching lines @@
 
     for (size_t i = 0; i < urls.size() && i < kMostVistedCount; i++) {
       const history::MostVisitedURL& url = urls[i];
       scoped_refptr<ShellLinkItem> link = CreateShellLink();
       std::string url_string = url.url.spec();
       base::string16 url_string_wide = base::UTF8ToUTF16(url_string);
       link->GetCommandLine()->AppendArgNative(url_string_wide);
       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));
     }
     data->most_visited_pages_have_updates_ = true;
   }
 
   // Send a query that retrieves the first favicon.
   StartLoadingFavicon();
 }
 
@@ skipping 32 matching lines @@
     return false;
 
   scoped_refptr<ShellLinkItem> link = CreateShellLink();
   const sessions::SerializedNavigationEntry& current_navigation =
       tab.navigations.at(tab.current_navigation_index);
   std::string url = current_navigation.virtual_url().spec();
   link->GetCommandLine()->AppendArgNative(base::UTF8ToUTF16(url));
   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)));
 
   return true;
 }
 
 void JumpList::AddWindow(const sessions::TabRestoreService::Window& window,
                          size_t max_items,
                          JumpListData* data) {
   DCHECK(CalledOnValidThread());
@@ skipping 207 matching lines @@
       }
     }
 
     data->recently_closed_pages_have_updates_ = true;
   }
 
   // Send a query that retrieves the first favicon.
   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);
+                               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;
 
-  // 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);
+  // 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(
     const base::string16& app_id,
     const base::FilePath& profile_dir,
     const ShellLinkItemList& most_visited_pages,
     const ShellLinkItemList& recently_closed_pages,
     bool most_visited_pages_have_updates,
     bool recently_closed_pages_have_updates,
     IncognitoModePrefs::Availability incognito_availability) {
@@ skipping 46 matching lines @@
 
   // Record the desired number of icons to create in this JumpList update.
   int icons_to_create = 0;
 
   // Update the icons for "Most Visisted" category of the JumpList if needed.
   if (most_visited_pages_have_updates) {
     base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName(
         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);
   }
 
   // Update the icons for "Recently Closed" category of the JumpList if needed.
   if (recently_closed_pages_have_updates) {
     base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName(
         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);
   }
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   UMA_HISTOGRAM_COUNTS_100("WinJumplist.CreateIconFilesCount", icons_to_create);
 
   // TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407.
   SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.UpdateJumpListDuration");
@@ skipping 63 matching lines @@
           app_id, profile_dir, local_most_visited_pages,
           local_recently_closed_pages, most_visited_pages_have_updates,
           recently_closed_pages_have_updates, incognito_availability)) {
     base::AutoLock auto_lock(data->list_lock_);
     if (most_visited_pages_have_updates)
       data->most_visited_pages_have_updates_ = true;
     if (recently_closed_pages_have_updates)
       data->recently_closed_pages_have_updates_ = true;
   }
 }