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"
@@ skipping 269 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) {
+  std::set<base::FilePath> iconpath_set;

[grt (UTC plus 2), 2017/05/11 20:29:13]

iconpath_set -> cached_files here, too

[chengx, 2017/05/12 01:04:24]

Done.

+
+  if (category == JumpListCategory::MOST_VISITED) {

[grt (UTC plus 2), 2017/05/11 20:29:13]

this is a great place for a case statement. if you do this:
  switch (category) {
    case JumpListCategory::kMostVisited:
      ...
      break;
    case JumpListCategory::kRecentlyClosed:
      ...
      break;
  }
then the code is protected against a change 3 years from now when someone adds a
third category -- they'll get a compile failure here rather than silently end up
with an empty set for the delete call.

you can also get rid of the code duplication below with:

  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;
  }
  ...pull from source_map here...

[chengx, 2017/05/12 01:04:24]

Done. Thanks so so so much for this suggestion!

+    for (auto url_path_pair : most_visited_map_)

[grt (UTC plus 2), 2017/05/11 20:29:13]

const auto& url_path_pair

[chengx, 2017/05/12 01:04:24]

Done.

+      iconpath_set.insert(url_path_pair.second);

[grt (UTC plus 2), 2017/05/11 20:29:12]

flat_set seems like a good choice here -- you can reserve() exactly the space
needed and then cached_files.insert(url_path_pair.second, cached_files.end()) to
build the set in O(N).

[chengx, 2017/05/12 01:04:24]

Thanks. I've changed to flat_set. 
fyi, it should be the position to insert as the first argument, the the value to
insert, i.e.,
cached_files.insert(cached_files.end(), url_path_pair.second)

+  } else if (category == JumpListCategory::RECENTLY_CLOSED) {
+    for (auto url_path_pair : recently_closed_map_)

[grt (UTC plus 2), 2017/05/11 20:29:12]

const auto&

[chengx, 2017/05/12 01:04:24]

Done.

+      iconpath_set.insert(url_path_pair.second);
+  }
+  DeleteNonCachedFiles(icon_dir, iconpath_set);
+}
+
 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");
 
-  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 the current URL is cached which means there's already an icon for this
+  // URL, then we simply re-use the icon. Otherwise, we need to create a new
+  // icon for this URL.
+
+  if (category == JumpListCategory::MOST_VISITED) {

[grt (UTC plus 2), 2017/05/11 20:29:12]

please use the technique above to collapse the duplicated code blocks here

[chengx, 2017/05/12 01:04:24]

Done. Again, thanks so much for this suggestion!

+    base::flat_map<std::string, base::FilePath> most_visited_map_updated;
+    for (ShellLinkItemList::const_iterator item = item_list.begin();
+         item != item_list.end() && max_items > 0; ++item, --max_items) {
+      if (most_visited_map_.find((*item)->url()) != most_visited_map_.end()) {
+        base::FilePath path = most_visited_map_[(*item)->url()];
+        (*item)->set_icon(most_visited_map_[(*item)->url()].value(), 0);
+        most_visited_map_updated[(*item)->url()] = path;
+      } else {
+        base::FilePath icon_path;
+        if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path)) {
+          (*item)->set_icon(icon_path.value(), 0);
+          most_visited_map_updated[(*item)->url()] = icon_path;
+        }
+      }
+    }
+    most_visited_map_.swap(most_visited_map_updated);
+  } else if (category == JumpListCategory::RECENTLY_CLOSED) {
+    base::flat_map<std::string, base::FilePath> recently_closed_map_updated;
+    for (ShellLinkItemList::const_iterator item = item_list.begin();
+         item != item_list.end() && max_items > 0; ++item, --max_items) {
+      if (recently_closed_map_.find((*item)->url()) !=
+          recently_closed_map_.end()) {
+        base::FilePath path = recently_closed_map_[(*item)->url()];
+        (*item)->set_icon(path.value(), 0);
+        recently_closed_map_updated[(*item)->url()] = path;
+      } else {
+        base::FilePath icon_path;
+        if (CreateIconFile((*item)->icon_image(), icon_dir, &icon_path)) {
+          (*item)->set_icon(icon_path.value(), 0);
+          recently_closed_map_updated[(*item)->url()] = icon_path;
+        }
+      }
+    }
+    recently_closed_map_.swap(recently_closed_map_updated);
   }
 }
 
 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::MOST_VISITED) ? 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::MOST_VISITED &&
+       most_visited_map_.empty()) ||
+      (category == JumpListCategory::RECENTLY_CLOSED &&
+       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);
+    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::MOST_VISITED);
 
     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::RECENTLY_CLOSED);
 
     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;
   }
 }