Index: chrome/browser/win/jumplist.cc |
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc |
index bbca037062d132ddc5c693646170827aee422cee..3eeda2e95f8eac9dd8feb2edefc6b6cc2cae57a9 100644 |
--- a/chrome/browser/win/jumplist.cc |
+++ b/chrome/browser/win/jumplist.cc |
@@ -194,9 +194,9 @@ base::FilePath GenerateJumplistIconDirName( |
} // namespace |
-JumpList::UpdateResults::UpdateResults() {} |
+JumpList::UpdateTransaction::UpdateTransaction() {} |
-JumpList::UpdateResults::~UpdateResults() {} |
+JumpList::UpdateTransaction::~UpdateTransaction() {} |
// static |
bool JumpList::Enabled() { |
@@ -552,35 +552,38 @@ void JumpList::PostRunUpdate() { |
// Make local copies of JumpList member variables and use them for an update. |
ShellLinkItemList local_most_visited_pages = most_visited_pages_; |
ShellLinkItemList local_recently_closed_pages = recently_closed_pages_; |
- |
bool most_visited_should_update = most_visited_should_update_; |
bool recently_closed_should_update = recently_closed_should_update_; |
- auto update_results = base::MakeUnique<UpdateResults>(); |
- update_results->most_visited_icons_in_update = most_visited_icons_; |
- update_results->recently_closed_icons_in_update = recently_closed_icons_; |
+ auto update_results = base::MakeUnique<UpdateTransaction>(); |
+ update_results->most_visited_icons = most_visited_icons_; |
+ update_results->recently_closed_icons = recently_closed_icons_; |
- // Parameter evaluation order is unspecified in C++. Ensure the pointer value |
- // is obtained before base::Passed() is called. |
- auto* update_results_raw = update_results.get(); |
+ // Parameter evaluation order is unspecified in C++. Do the first bind and |
+ // then move it into PostTaskAndReply to ensure the pointer value is obtained |
+ // before base::Passed() is called. |
+ auto run_update = |
+ base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir, |
+ local_most_visited_pages, local_recently_closed_pages, |
+ most_visited_should_update, recently_closed_should_update, |
grt (UTC plus 2)
2017/06/23 09:36:14
is it possible for either this->*_should_update_ t
chengx
2017/06/23 18:52:38
You are right. There is no need to bind these two
|
+ incognito_availability, update_results.get()); |
// Post a task to update the JumpList, which consists of 1) create new icons, |
- // 2) delete old icons, 3) notify the OS. |
+ // 2) notify the OS, 3) delete old icons. |
if (!update_jumplist_task_runner_->PostTaskAndReply( |
- FROM_HERE, |
- base::Bind(&JumpList::RunUpdateJumpList, app_id_, profile_dir, |
- local_most_visited_pages, local_recently_closed_pages, |
- most_visited_should_update, recently_closed_should_update, |
- incognito_availability, update_results_raw), |
- base::Bind(&JumpList::OnRunUpdateCompletion, |
- weak_ptr_factory_.GetWeakPtr(), |
- base::Passed(std::move(update_results))))) { |
- OnRunUpdateCompletion(base::MakeUnique<UpdateResults>()); |
+ FROM_HERE, std::move(run_update), |
+ base::Bind( |
+ &JumpList::OnRunUpdateCompletion, weak_ptr_factory_.GetWeakPtr(), |
+ base::Passed(std::move(update_results)), |
+ most_visited_should_update, recently_closed_should_update))) { |
+ OnRunUpdateCompletion(base::MakeUnique<UpdateTransaction>(), false, false); |
} |
} |
void JumpList::OnRunUpdateCompletion( |
- std::unique_ptr<UpdateResults> update_results) { |
+ std::unique_ptr<UpdateTransaction> update_results, |
+ bool most_visited_should_update, |
+ bool recently_closed_should_update) { |
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); |
// Update JumpList member variables based on the results from the update run |
@@ -589,11 +592,14 @@ void JumpList::OnRunUpdateCompletion( |
updates_to_skip_ = kUpdatesToSkipUnderHeavyLoad; |
if (update_results->update_success) { |
- most_visited_icons_.swap(update_results->most_visited_icons_in_update); |
- recently_closed_icons_.swap( |
- update_results->recently_closed_icons_in_update); |
- most_visited_should_update_ = false; |
- recently_closed_should_update_ = false; |
+ if (most_visited_should_update) { |
+ most_visited_icons_.swap(update_results->most_visited_icons); |
grt (UTC plus 2)
2017/06/23 09:36:14
nit: swap() used to be the efficient way to move s
chengx
2017/06/23 18:52:38
Done. I think std::move better conveys the intent
|
+ most_visited_should_update_ = false; |
+ } |
+ if (recently_closed_should_update) { |
+ recently_closed_icons_.swap(update_results->recently_closed_icons); |
+ recently_closed_should_update_ = false; |
+ } |
} |
update_in_progress_ = false; |
@@ -655,18 +661,16 @@ void JumpList::Terminate() { |
} |
// static |
-void JumpList::RunUpdateJumpList( |
+void JumpList::CreateNewJumpListAndNotifyOS( |
const base::string16& app_id, |
- const base::FilePath& profile_dir, |
+ const base::FilePath& most_visited_icon_dir, |
+ const base::FilePath& recently_closed_icon_dir, |
const ShellLinkItemList& most_visited_pages, |
const ShellLinkItemList& recently_closed_pages, |
bool most_visited_should_update, |
bool recently_closed_should_update, |
IncognitoModePrefs::Availability incognito_availability, |
- UpdateResults* update_results) { |
- if (!JumpListUpdater::IsEnabled()) |
- return; |
- |
+ UpdateTransaction* update_results) { |
DCHECK(update_results); |
JumpListUpdater jumplist_updater(app_id); |
@@ -678,8 +682,7 @@ void JumpList::RunUpdateJumpList( |
// Discard this JumpList update if JumpListUpdater::BeginUpdate takes longer |
// than the maximum allowed time, as it's very likely the following update |
- // steps will also take a long time. As we've not updated the icons on the |
- // disk, discarding this update wont't affect the current JumpList used by OS. |
+ // steps will also take a long time. |
if (begin_update_timer.Elapsed() >= kTimeOutForJumplistBeginUpdate) { |
update_results->update_timeout = true; |
return; |
@@ -688,24 +691,21 @@ void JumpList::RunUpdateJumpList( |
// Record the desired number of icons created in this JumpList update. |
int icons_created = 0; |
+ URLIconCache most_visited_icons_next; |
+ URLIconCache recently_closed_icons_next; |
+ |
// Update the icons for "Most Visisted" category of the JumpList if needed. |
if (most_visited_should_update) { |
- base::FilePath icon_dir_most_visited = GenerateJumplistIconDirName( |
- profile_dir, FILE_PATH_LITERAL("MostVisited")); |
- |
icons_created += UpdateIconFiles( |
- icon_dir_most_visited, most_visited_pages, kMostVisitedItems, |
- &update_results->most_visited_icons_in_update); |
+ most_visited_icon_dir, most_visited_pages, kMostVisitedItems, |
+ &update_results->most_visited_icons, &most_visited_icons_next); |
} |
// Update the icons for "Recently Closed" category of the JumpList if needed. |
if (recently_closed_should_update) { |
- base::FilePath icon_dir_recent_closed = GenerateJumplistIconDirName( |
- profile_dir, FILE_PATH_LITERAL("RecentClosed")); |
- |
icons_created += UpdateIconFiles( |
- icon_dir_recent_closed, recently_closed_pages, kRecentlyClosedItems, |
- &update_results->recently_closed_icons_in_update); |
+ recently_closed_icon_dir, recently_closed_pages, kRecentlyClosedItems, |
+ &update_results->recently_closed_icons, &recently_closed_icons_next); |
} |
// TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407. |
@@ -732,16 +732,13 @@ void JumpList::RunUpdateJumpList( |
return; |
} |
- // If JumpListUpdater::AddCustomCategory or JumpListUpdater::CommitUpdate |
- // takes longer than the maximum allowed time, skip the next |
- // |kUpdatesToSkipUnderHeavyLoad| updates. This update should be finished |
- // because we've already updated the icons on the disk. If discarding this |
- // update from here, some items in the current JumpList may not have icons |
- // as they've been delete from the disk. In this case, the background color of |
- // the JumpList panel is used instead, which doesn't look nice. |
- |
- if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory) |
+ // If AddCustomCategory call takes longer than the maximum allowed time, abort |
+ // the current update and skip the next |kUpdatesToSkipUnderHeavyLoad| |
+ // updates. |
+ if (add_custom_category_timer.Elapsed() >= kTimeOutForAddCustomCategory) { |
update_results->update_timeout = true; |
+ return; |
+ } |
// Update the "Tasks" category of the JumpList. |
if (!UpdateTaskCategory(&jumplist_updater, incognito_availability)) |
@@ -750,35 +747,85 @@ void JumpList::RunUpdateJumpList( |
base::ElapsedTimer commit_update_timer; |
// Commit this transaction and send the updated JumpList to Windows. |
- if (jumplist_updater.CommitUpdate()) |
- update_results->update_success = true; |
+ bool commit_success = jumplist_updater.CommitUpdate(); |
+ // If CommitUpdate call takes longer than the maximum allowed time, abort the |
+ // current update and skip the next |kUpdatesToSkipUnderHeavyLoad| updates. |
grt (UTC plus 2)
2017/06/23 09:36:14
this doesn't abort the current update, as the curr
chengx
2017/06/23 18:52:38
Done.
|
if (commit_update_timer.Elapsed() >= kTimeOutForJumplistCommitUpdate) |
update_results->update_timeout = true; |
+ |
+ if (commit_success) { |
+ update_results->update_success = true; |
+ // The swaps below ensure update_results always has the icons to keep. |
+ if (most_visited_should_update) |
+ update_results->most_visited_icons.swap(most_visited_icons_next); |
+ if (recently_closed_should_update) |
+ update_results->recently_closed_icons.swap(recently_closed_icons_next); |
+ } |
+} |
+ |
+// static |
+void JumpList::RunUpdateJumpList( |
grt (UTC plus 2)
2017/06/23 09:36:14
nit: move this above CreateNewJumpListAndNotifyOS?
chengx
2017/06/23 18:52:38
Done.
|
+ const base::string16& app_id, |
+ const base::FilePath& profile_dir, |
+ const ShellLinkItemList& most_visited_pages, |
+ const ShellLinkItemList& recently_closed_pages, |
+ bool most_visited_should_update, |
+ bool recently_closed_should_update, |
+ IncognitoModePrefs::Availability incognito_availability, |
+ UpdateTransaction* update_results) { |
+ if (!JumpListUpdater::IsEnabled()) |
grt (UTC plus 2)
2017/06/23 09:36:14
is this needed? can IsEnabled() change from true t
chengx
2017/06/23 18:52:38
I think it should be moved to the very beginning o
|
+ return; |
+ |
+ DCHECK(update_results); |
+ |
+ base::FilePath most_visited_icon_dir = GenerateJumplistIconDirName( |
+ profile_dir, FILE_PATH_LITERAL("MostVisited")); |
+ base::FilePath recently_closed_icon_dir = GenerateJumplistIconDirName( |
+ profile_dir, FILE_PATH_LITERAL("RecentClosed")); |
+ |
+ CreateNewJumpListAndNotifyOS( |
+ app_id, most_visited_icon_dir, recently_closed_icon_dir, |
+ most_visited_pages, recently_closed_pages, most_visited_should_update, |
+ recently_closed_should_update, incognito_availability, update_results); |
+ |
+ // Delete any obsolete icon files. |
+ if (most_visited_should_update) |
+ DeleteIconFiles(most_visited_icon_dir, update_results->most_visited_icons); |
+ if (recently_closed_should_update) { |
+ DeleteIconFiles(recently_closed_icon_dir, |
+ update_results->recently_closed_icons); |
+ } |
} |
// static |
int JumpList::UpdateIconFiles(const base::FilePath& icon_dir, |
const ShellLinkItemList& page_list, |
size_t slot_limit, |
- URLIconCache* icon_cache) { |
+ URLIconCache* icon_cur, |
+ URLIconCache* icon_next) { |
+ DCHECK(icon_cur); |
+ DCHECK(icon_next); |
+ |
int icons_created = 0; |
grt (UTC plus 2)
2017/06/23 09:36:14
nit: how about cleaning this up a tiny bit:
//
chengx
2017/06/23 18:52:38
Done. Thanks for the suggestion!
|
- // Clear the JumpList icon folder at |icon_dir| and the cache when |
- // 1) |icon_cache| is empty. This happens when "Most visited" or "Recently |
+ // Clear the JumpList icon folder at |icon_dir| and the caches when |
+ // 1) |icon_cur| is empty. This happens when "Most visited" or "Recently |
// closed" category updates for the 1st time after Chrome is launched. |
// 2) The number of icons in |icon_dir| has exceeded the limit. |
- if (icon_cache->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) { |
+ if (icon_cur->empty() || FilesExceedLimitInDir(icon_dir, slot_limit * 2)) { |
DeleteDirectoryContentAndLogRuntime(icon_dir, kFileDeleteLimit); |
- icon_cache->clear(); |
+ icon_cur->clear(); |
+ icon_next->clear(); |
+ |
// Create new icons only when the directory exists and is empty. |
- if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir)) |
+ if (base::CreateDirectory(icon_dir) && base::IsDirectoryEmpty(icon_dir)) { |
icons_created += |
- CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache); |
+ CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next); |
+ } |
} else if (base::CreateDirectory(icon_dir)) { |
icons_created += |
- CreateIconFiles(icon_dir, page_list, slot_limit, icon_cache); |
- DeleteIconFiles(icon_dir, icon_cache); |
+ CreateIconFiles(icon_dir, page_list, slot_limit, icon_cur, icon_next); |
} |
return icons_created; |
@@ -788,43 +835,45 @@ int JumpList::UpdateIconFiles(const base::FilePath& icon_dir, |
int JumpList::CreateIconFiles(const base::FilePath& icon_dir, |
const ShellLinkItemList& item_list, |
size_t max_items, |
- URLIconCache* icon_cache) { |
+ URLIconCache* icon_cur, |
grt (UTC plus 2)
2017/06/23 09:36:14
nit: pass |icon_cur| by constref
chengx
2017/06/23 18:52:38
Done.
|
+ URLIconCache* icon_next) { |
+ DCHECK(icon_cur); |
+ DCHECK(icon_next); |
+ |
// TODO(chengx): Remove the UMA histogram after fixing http://crbug.com/40407. |
SCOPED_UMA_HISTOGRAM_TIMER("WinJumplist.CreateIconFilesDuration"); |
int icons_created = 0; |
// Reuse icons for urls that already present in the current JumpList. |
- URLIconCache updated_map; |
for (ShellLinkItemList::const_iterator iter = item_list.begin(); |
iter != item_list.end() && max_items > 0; ++iter, --max_items) { |
ShellLinkItem* item = iter->get(); |
- auto cache_iter = icon_cache->find(item->url()); |
- if (cache_iter != icon_cache->end()) { |
+ auto cache_iter = icon_cur->find(item->url()); |
+ if (cache_iter != icon_cur->end()) { |
item->set_icon(cache_iter->second.value(), 0); |
- updated_map[item->url()] = cache_iter->second; |
+ (*icon_next)[item->url()] = cache_iter->second; |
} else { |
base::FilePath icon_path; |
if (CreateIconFile(item->icon_image(), icon_dir, &icon_path)) { |
++icons_created; |
item->set_icon(icon_path.value(), 0); |
- updated_map[item->url()] = icon_path; |
+ (*icon_next)[item->url()] = icon_path; |
} |
} |
} |
- icon_cache->swap(updated_map); |
return icons_created; |
} |
// static |
void JumpList::DeleteIconFiles(const base::FilePath& icon_dir, |
- URLIconCache* icon_cache) { |
+ const URLIconCache& icons_cache) { |
// Put all cached icon file paths into a set. |
base::flat_set<base::FilePath> cached_files; |
- cached_files.reserve(icon_cache->size()); |
+ cached_files.reserve(icons_cache.size()); |
- for (const auto& url_path_pair : *icon_cache) |
+ for (const auto& url_path_pair : icons_cache) |
cached_files.insert(url_path_pair.second); |
DeleteNonCachedFiles(icon_dir, cached_files); |