Cache ARC++/crostini icons for app-list searches.
Currently individual ArcAppResult / CrostiniAppResult instance
keeps icon loader and the results will be recreated every time
the search is conducted (including suggestion chip). This means
the ARC++ / Crostini searchers always attempt to load the same
icon every time the app-list is shown and some query is searched.
This is apparently inefficient; this CL allows to cache the icon
images for such search results so that no further image loading
and decoding won't be conducted.
This CL doesn't change the performance numbers on the
LauncherAnimations test cases since they use dummy data. By
measuring on a real session with my test account, I see
improvements AnimationSmoothness.Peeking by ~10%. Mean 51.0 to
60.2 for 10 attempts, mode value improved from 41 to 58.
Bug: 978461
Test: manually
Change-Id: Ie34ad979d6fdf94addc2959ea74a4e4d9652add0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1677260
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#673417}
diff --git a/chrome/browser/ui/app_list/search/app_result.h b/chrome/browser/ui/app_list/search/app_result.h
index 1237bfc..c0d1a6f 100644
--- a/chrome/browser/ui/app_list/search/app_result.h
+++ b/chrome/browser/ui/app_list/search/app_result.h
@@ -10,6 +10,7 @@
#include "ash/public/cpp/app_list/app_list_metrics.h"
#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
#include "chrome/browser/ui/app_list/app_context_menu_delegate.h"
#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
@@ -35,6 +36,10 @@
SearchResultType GetSearchResultType() const override;
+ base::WeakPtr<AppResult> GetWeakPtr() {
+ return weak_ptr_factory_.GetWeakPtr();
+ }
+
protected:
AppResult(Profile* profile,
const std::string& app_id,
@@ -51,6 +56,8 @@
const std::string app_id_;
AppListControllerDelegate* controller_;
+ base::WeakPtrFactory<AppResult> weak_ptr_factory_{this};
+
DISALLOW_COPY_AND_ASSIGN(AppResult);
};
diff --git a/chrome/browser/ui/app_list/search/app_search_provider.cc b/chrome/browser/ui/app_list/search/app_search_provider.cc
index 65f5ed7a..713b345 100644
--- a/chrome/browser/ui/app_list/search/app_search_provider.cc
+++ b/chrome/browser/ui/app_list/search/app_search_provider.cc
@@ -13,12 +13,14 @@
#include <unordered_set>
#include <utility>
+#include "ash/public/cpp/app_list/app_list_config.h"
#include "ash/public/cpp/app_list/app_list_features.h"
#include "ash/public/cpp/app_list/internal_app_id_constants.h"
#include "ash/public/cpp/app_list/tokenized_string.h"
#include "ash/public/cpp/app_list/tokenized_string_match.h"
#include "base/bind.h"
#include "base/callback_list.h"
+#include "base/containers/flat_map.h"
#include "base/location.h"
#include "base/macros.h"
#include "base/metrics/field_trial_params.h"
@@ -41,9 +43,11 @@
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sync/session_sync_service_factory.h"
#include "chrome/browser/ui/app_list/app_list_model_updater.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_icon_loader.h"
#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
#include "chrome/browser/ui/app_list/chrome_app_list_item.h"
+#include "chrome/browser/ui/app_list/crostini/crostini_app_icon_loader.h"
#include "chrome/browser/ui/app_list/extension_app_utils.h"
#include "chrome/browser/ui/app_list/internal_app/internal_app_metadata.h"
#include "chrome/browser/ui/app_list/search/app_service_app_result.h"
@@ -461,12 +465,132 @@
DISALLOW_COPY_AND_ASSIGN(ExtensionDataSource);
};
-class ArcDataSource : public AppSearchProvider::DataSource,
+class AppIconDataSource : public AppIconLoaderDelegate {
+ public:
+ class Delegate {
+ public:
+ virtual std::unique_ptr<AppIconLoader> CreateIconLoader(
+ int icon_size,
+ AppIconLoaderDelegate* delegate) = 0;
+ virtual void IconUpdated(const std::string& app_id,
+ const gfx::ImageSkia& image,
+ bool for_chip) = 0;
+ };
+
+ AppIconDataSource(Delegate* delegate, bool for_chip)
+ : delegate_(delegate),
+ icon_loader_(delegate->CreateIconLoader(IconSize(for_chip), this)),
+ for_chip_(for_chip) {}
+ ~AppIconDataSource() override = default;
+
+ void MaybeFetchIcon(const std::string& app_id) {
+ auto iter = icon_map_.find(app_id);
+ if (iter != icon_map_.end()) {
+ delegate_->IconUpdated(app_id, iter->second, for_chip_);
+ } else {
+ icon_loader_->FetchImage(app_id);
+ }
+ }
+
+ void RemoveIcon(const std::string& app_id) { icon_map_.erase(app_id); }
+
+ private:
+ int IconSize(bool for_chip) const {
+ const auto& config = AppListConfig::instance();
+ return for_chip ? config.suggestion_chip_icon_dimension()
+ : config.GetPreferredIconDimension(
+ ash::SearchResultDisplayType::kTile);
+ }
+
+ void OnAppImageUpdated(const std::string& app_id,
+ const gfx::ImageSkia& image) override {
+ icon_map_[app_id] = image;
+ delegate_->IconUpdated(app_id, image, for_chip_);
+ }
+
+ Delegate* delegate_;
+ std::unique_ptr<AppIconLoader> icon_loader_;
+ std::map<std::string, gfx::ImageSkia> icon_map_;
+ bool for_chip_;
+
+ DISALLOW_COPY_AND_ASSIGN(AppIconDataSource);
+};
+
+class IconCachedDataSource : public AppSearchProvider::DataSource,
+ public AppIconDataSource::Delegate {
+ public:
+ IconCachedDataSource(Profile* profile, AppSearchProvider* owner)
+ : AppSearchProvider::DataSource(profile, owner) {}
+ ~IconCachedDataSource() override = default;
+
+ protected:
+ void InitIconSources() {
+ icon_source_ = std::make_unique<AppIconDataSource>(this, false);
+ chip_icon_source_ = std::make_unique<AppIconDataSource>(this, true);
+ }
+
+ std::unique_ptr<AppResult> WrapResult(std::unique_ptr<AppResult> result) {
+ const std::string& app_id = result->app_id();
+ results_[app_id].push_back(result->GetWeakPtr());
+ icon_source_->MaybeFetchIcon(app_id);
+ if (result->display_type() == ash::SearchResultDisplayType::kRecommendation)
+ chip_icon_source_->MaybeFetchIcon(app_id);
+ return result;
+ }
+
+ void RemoveIcon(const std::string& app_id) {
+ icon_source_->RemoveIcon(app_id);
+ chip_icon_source_->RemoveIcon(app_id);
+ }
+
+ void RevokeInvalidated() {
+ for (auto& p : results_) {
+ for (auto iter = p.second.begin(); iter != p.second.end();) {
+ if (iter->get()) {
+ ++iter;
+ } else {
+ iter = p.second.erase(iter);
+ }
+ }
+ }
+ }
+
+ private:
+ // AppIconDataSource::Delegate:
+ void IconUpdated(const std::string& app_id,
+ const gfx::ImageSkia& image,
+ bool for_chip) override {
+ for (auto result : results_[app_id]) {
+ auto* ptr = result.get();
+ if (!ptr)
+ continue;
+ // There's a chance that both kRecommendation results and kTile results
+ // are in |results_| but we don't need chip icons for kTile results.
+ if (for_chip && ptr->display_type() !=
+ ash::SearchResultDisplayType::kRecommendation) {
+ continue;
+ }
+ if (for_chip)
+ ptr->SetChipIcon(image);
+ else
+ ptr->SetIcon(image);
+ }
+ }
+
+ std::unique_ptr<AppIconDataSource> icon_source_;
+ std::unique_ptr<AppIconDataSource> chip_icon_source_;
+ base::flat_map<std::string, std::vector<base::WeakPtr<AppResult>>> results_;
+
+ DISALLOW_COPY_AND_ASSIGN(IconCachedDataSource);
+};
+
+class ArcDataSource : public IconCachedDataSource,
public ArcAppListPrefs::Observer {
public:
ArcDataSource(Profile* profile, AppSearchProvider* owner)
- : AppSearchProvider::DataSource(profile, owner) {
+ : IconCachedDataSource(profile, owner) {
ArcAppListPrefs::Get(profile)->AddObserver(this);
+ InitIconSources();
}
~ArcDataSource() override {
@@ -475,6 +599,7 @@
// AppSearchProvider::DataSource overrides:
void AddApps(AppSearchProvider::Apps* apps) override {
+ RevokeInvalidated();
ArcAppListPrefs* arc_prefs = ArcAppListPrefs::Get(profile());
CHECK(arc_prefs);
@@ -502,8 +627,8 @@
const std::string& app_id,
AppListControllerDelegate* list_controller,
bool is_recommended) override {
- return std::make_unique<ArcAppResult>(profile(), app_id, list_controller,
- is_recommended);
+ return WrapResult(std::make_unique<ArcAppResult>(
+ profile(), app_id, list_controller, is_recommended));
}
// ArcAppListPrefs::Observer overrides:
@@ -514,19 +639,28 @@
void OnAppStatesChanged(const std::string& app_id,
const ArcAppListPrefs::AppInfo& app_info) override {
+ RemoveIcon(app_id);
owner()->RefreshAppsAndUpdateResultsDeferred();
}
- void OnAppRemoved(const std::string& id) override {
+ void OnAppRemoved(const std::string& app_id) override {
+ RemoveIcon(app_id);
owner()->RefreshAppsAndUpdateResults();
}
- void OnAppNameUpdated(const std::string& id,
+ void OnAppNameUpdated(const std::string& app_id,
const std::string& name) override {
owner()->RefreshAppsAndUpdateResultsDeferred();
}
private:
+ // AppIconDataSource::Delegate:
+ std::unique_ptr<AppIconLoader> CreateIconLoader(
+ int icon_size,
+ AppIconLoaderDelegate* delegate) override {
+ return std::make_unique<ArcAppIconLoader>(profile(), icon_size, delegate);
+ }
+
DISALLOW_COPY_AND_ASSIGN(ArcDataSource);
};
@@ -603,13 +737,14 @@
DISALLOW_COPY_AND_ASSIGN(InternalDataSource);
};
-class CrostiniDataSource : public AppSearchProvider::DataSource,
+class CrostiniDataSource : public IconCachedDataSource,
public crostini::CrostiniRegistryService::Observer {
public:
CrostiniDataSource(Profile* profile, AppSearchProvider* owner)
- : AppSearchProvider::DataSource(profile, owner) {
+ : IconCachedDataSource(profile, owner) {
crostini::CrostiniRegistryServiceFactory::GetForProfile(profile)
->AddObserver(this);
+ InitIconSources();
}
~CrostiniDataSource() override {
@@ -619,6 +754,7 @@
// AppSearchProvider::DataSource overrides:
void AddApps(AppSearchProvider::Apps* apps) override {
+ RevokeInvalidated();
crostini::CrostiniRegistryService* registry_service =
crostini::CrostiniRegistryServiceFactory::GetForProfile(profile());
for (const std::string& app_id : registry_service->GetRegisteredAppIds()) {
@@ -653,8 +789,8 @@
const std::string& app_id,
AppListControllerDelegate* list_controller,
bool is_recommended) override {
- return std::make_unique<CrostiniAppResult>(profile(), app_id,
- list_controller, is_recommended);
+ return WrapResult(std::make_unique<CrostiniAppResult>(
+ profile(), app_id, list_controller, is_recommended));
}
// crostini::CrostiniRegistryService::Observer overrides:
@@ -663,6 +799,10 @@
const std::vector<std::string>& updated_apps,
const std::vector<std::string>& removed_apps,
const std::vector<std::string>& inserted_apps) override {
+ for (const auto& id : updated_apps)
+ RemoveIcon(id);
+ for (const auto& id : removed_apps)
+ RemoveIcon(id);
if (removed_apps.empty())
owner()->RefreshAppsAndUpdateResultsDeferred();
else
@@ -670,6 +810,14 @@
}
private:
+ // AppIconDataSource::Delegate:
+ std::unique_ptr<AppIconLoader> CreateIconLoader(
+ int icon_size,
+ AppIconLoaderDelegate* delegate) override {
+ return std::make_unique<CrostiniAppIconLoader>(profile(), icon_size,
+ delegate);
+ }
+
DISALLOW_COPY_AND_ASSIGN(CrostiniDataSource);
};
diff --git a/chrome/browser/ui/app_list/search/arc_app_result.cc b/chrome/browser/ui/app_list/search/arc_app_result.cc
index bc1f68a..cbc6c0b 100644
--- a/chrome/browser/ui/app_list/search/arc_app_result.cc
+++ b/chrome/browser/ui/app_list/search/arc_app_result.cc
@@ -26,43 +26,10 @@
std::string id = kArcAppPrefix;
id += app_id;
set_id(id);
- icon_loader_ = std::make_unique<ArcAppIconLoader>(
- profile,
- AppListConfig::instance().GetPreferredIconDimension(display_type()),
- this);
- icon_loader_->FetchImage(app_id);
- // Load an additional chip icon when it is a recommendation result
- // so that it renders clearly in both a chip and a tile.
- if (display_type() == ash::SearchResultDisplayType::kRecommendation) {
- chip_icon_loader_ = std::make_unique<ArcAppIconLoader>(
- profile, AppListConfig::instance().suggestion_chip_icon_dimension(),
- this);
- chip_icon_loader_->FetchImage(app_id);
- }
}
ArcAppResult::~ArcAppResult() {}
-void ArcAppResult::OnAppImageUpdated(const std::string& app_id,
- const gfx::ImageSkia& image) {
- const gfx::Size icon_size(
- AppListConfig::instance().GetPreferredIconDimension(display_type()),
- AppListConfig::instance().GetPreferredIconDimension(display_type()));
- const gfx::Size chip_icon_size(
- AppListConfig::instance().suggestion_chip_icon_dimension(),
- AppListConfig::instance().suggestion_chip_icon_dimension());
- DCHECK(icon_size != chip_icon_size);
-
- if (image.size() == icon_size) {
- SetIcon(image);
- } else if (image.size() == chip_icon_size) {
- DCHECK_EQ(ash::SearchResultDisplayType::kRecommendation, display_type());
- SetChipIcon(image);
- } else {
- NOTREACHED();
- }
-}
-
void ArcAppResult::ExecuteLaunchCommand(int event_flags) {
Launch(event_flags, GetContextMenuAppLaunchInteraction());
}
diff --git a/chrome/browser/ui/app_list/search/arc_app_result.h b/chrome/browser/ui/app_list/search/arc_app_result.h
index 00eabf19..f51ebd4b 100644
--- a/chrome/browser/ui/app_list/search/arc_app_result.h
+++ b/chrome/browser/ui/app_list/search/arc_app_result.h
@@ -17,13 +17,11 @@
class AppListControllerDelegate;
class ArcAppContextMenu;
-class ArcAppIconLoader;
class Profile;
namespace app_list {
-class ArcAppResult : public AppResult,
- public AppIconLoaderDelegate {
+class ArcAppResult : public AppResult {
public:
ArcAppResult(Profile* profile,
const std::string& app_id,
@@ -39,10 +37,6 @@
// AppContextMenuDelegate overrides:
void ExecuteLaunchCommand(int event_flags) override;
- // AppIconLoaderDelegate overrides:
- void OnAppImageUpdated(const std::string& app_id,
- const gfx::ImageSkia& image) override;
-
private:
// ChromeSearchResult overrides:
AppContextMenu* GetAppContextMenu() override;
@@ -51,8 +45,6 @@
arc::UserInteractionType GetAppLaunchInteraction();
arc::UserInteractionType GetContextMenuAppLaunchInteraction();
- std::unique_ptr<ArcAppIconLoader> icon_loader_;
- std::unique_ptr<ArcAppIconLoader> chip_icon_loader_;
std::unique_ptr<ArcAppContextMenu> context_menu_;
DISALLOW_COPY_AND_ASSIGN(ArcAppResult);
diff --git a/chrome/browser/ui/app_list/search/crostini_app_result.cc b/chrome/browser/ui/app_list/search/crostini_app_result.cc
index 765e330..b83f4e6 100644
--- a/chrome/browser/ui/app_list/search/crostini_app_result.cc
+++ b/chrome/browser/ui/app_list/search/crostini_app_result.cc
@@ -22,21 +22,6 @@
bool is_recommendation)
: AppResult(profile, app_id, controller, is_recommendation) {
set_id(app_id);
-
- icon_loader_ = std::make_unique<CrostiniAppIconLoader>(
- profile,
- AppListConfig::instance().GetPreferredIconDimension(display_type()),
- this);
- icon_loader_->FetchImage(app_id);
-
- // Load an additional chip icon when it is a recommendation result
- // so that it renders clearly in both a chip and a tile.
- if (display_type() == ash::SearchResultDisplayType::kRecommendation) {
- chip_icon_loader_ = std::make_unique<CrostiniAppIconLoader>(
- profile, AppListConfig::instance().suggestion_chip_icon_dimension(),
- this);
- chip_icon_loader_->FetchImage(app_id);
- }
}
CrostiniAppResult::~CrostiniAppResult() = default;
@@ -61,26 +46,6 @@
return CROSTINI_APP;
}
-void CrostiniAppResult::OnAppImageUpdated(const std::string& app_id,
- const gfx::ImageSkia& image) {
- const gfx::Size icon_size(
- AppListConfig::instance().GetPreferredIconDimension(display_type()),
- AppListConfig::instance().GetPreferredIconDimension(display_type()));
- const gfx::Size chip_icon_size(
- AppListConfig::instance().suggestion_chip_icon_dimension(),
- AppListConfig::instance().suggestion_chip_icon_dimension());
- DCHECK(icon_size != chip_icon_size);
-
- if (image.size() == icon_size) {
- SetIcon(image);
- } else if (image.size() == chip_icon_size) {
- DCHECK(display_type() == ash::SearchResultDisplayType::kRecommendation);
- SetChipIcon(image);
- } else {
- NOTREACHED();
- }
-}
-
AppContextMenu* CrostiniAppResult::GetAppContextMenu() {
return context_menu_.get();
}
diff --git a/chrome/browser/ui/app_list/search/crostini_app_result.h b/chrome/browser/ui/app_list/search/crostini_app_result.h
index cd5bd38..7e7ad68 100644
--- a/chrome/browser/ui/app_list/search/crostini_app_result.h
+++ b/chrome/browser/ui/app_list/search/crostini_app_result.h
@@ -10,16 +10,14 @@
#include "ash/public/cpp/app_list/app_list_metrics.h"
#include "base/macros.h"
-#include "chrome/browser/ui/app_icon_loader_delegate.h"
#include "chrome/browser/ui/app_list/search/app_result.h"
class CrostiniAppContextMenu;
-class CrostiniAppIconLoader;
namespace app_list {
// Result of CrostiniSearchProvider.
-class CrostiniAppResult : public AppResult, public AppIconLoaderDelegate {
+class CrostiniAppResult : public AppResult {
public:
CrostiniAppResult(Profile* profile,
const std::string& app_id,
@@ -34,16 +32,10 @@
void ExecuteLaunchCommand(int event_flags) override;
SearchResultType GetSearchResultType() const override;
- // AppIconLoaderDelegate overrides:
- void OnAppImageUpdated(const std::string& app_id,
- const gfx::ImageSkia& image) override;
-
private:
// ChromeSearchResult overrides:
AppContextMenu* GetAppContextMenu() override;
- std::unique_ptr<CrostiniAppIconLoader> icon_loader_;
- std::unique_ptr<CrostiniAppIconLoader> chip_icon_loader_;
std::unique_ptr<CrostiniAppContextMenu> context_menu_;
DISALLOW_COPY_AND_ASSIGN(CrostiniAppResult);
