From c7d7e2439dcdfd3f66d1d0b734624aac5f742741 Mon Sep 17 00:00:00 2001 From: calamity Date: Thu, 19 Feb 2015 18:05:34 -0800 Subject: [PATCH] Sort unlaunched apps on app list start page by apps grid order. When recommending, apps now use a small number multiplied by the index of an item in the apps grid as an app's score if the app has never been launched. This causes the apps grid order to be used for apps that have never been launched. The modification is small enough that no apps that have been launched in the past year should rank lower than an unlaunched app. BUG=455338 Review URL: https://codereview.chromium.org/925883003 Cr-Commit-Position: refs/heads/master@{#317218} --- .../browser/ui/app_list/app_list_view_delegate.cc | 3 +- .../ui/app_list/search/app_search_provider.cc | 56 ++++++++++++++++------ .../ui/app_list/search/app_search_provider.h | 7 ++- .../search/app_search_provider_unittest.cc | 42 ++++++++++++++-- .../app_list/search/search_controller_factory.cc | 18 +++---- .../ui/app_list/search/search_controller_factory.h | 3 +- 6 files changed, 97 insertions(+), 32 deletions(-) diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc index fa4a4d26d8c3..c8477565c7e8 100644 --- a/chrome/browser/ui/app_list/app_list_view_delegate.cc +++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc @@ -329,8 +329,7 @@ void AppListViewDelegate::SetUpSearchUI() { model_->search_box(), speech_ui_.get())); - search_controller_ = CreateSearchController( - profile_, model_->search_box(), model_->results(), controller_); + search_controller_ = CreateSearchController(profile_, model_, controller_); } void AppListViewDelegate::SetUpProfileSwitcher() { 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 da8c64f0c3f6..b81afa693e3c 100644 --- a/chrome/browser/ui/app_list/search/app_search_provider.cc +++ b/chrome/browser/ui/app_list/search/app_search_provider.cc @@ -14,17 +14,27 @@ #include "chrome/browser/extensions/extension_ui_util.h" #include "chrome/browser/extensions/extension_util.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/app_list/app_list_syncable_service.h" +#include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h" #include "chrome/browser/ui/app_list/search/app_result.h" #include "extensions/browser/extension_prefs.h" #include "extensions/browser/extension_registry.h" #include "extensions/browser/extension_system.h" #include "extensions/common/extension.h" #include "extensions/common/extension_set.h" +#include "ui/app_list/app_list_item.h" +#include "ui/app_list/app_list_model.h" #include "ui/app_list/search/tokenized_string.h" #include "ui/app_list/search/tokenized_string_match.h" using extensions::ExtensionRegistry; +namespace { + +// The size of each step unlaunched apps should increase their relevance by. +const double kUnlaunchedAppRelevanceStepSize = 0.0001; +} + namespace app_list { class AppSearchProvider::App { @@ -50,10 +60,12 @@ class AppSearchProvider::App { AppSearchProvider::AppSearchProvider(Profile* profile, AppListControllerDelegate* list_controller, - scoped_ptr clock) + scoped_ptr clock, + AppListItemList* top_level_item_list) : profile_(profile), list_controller_(list_controller), extension_registry_observer_(this), + top_level_item_list_(top_level_item_list), clock_(clock.Pass()), update_results_factory_(this) { extension_registry_observer_.Add(ExtensionRegistry::Get(profile_)); @@ -86,23 +98,39 @@ void AppSearchProvider::UpdateResults() { bool show_recommendations = query_.empty(); ClearResults(); - for (Apps::const_iterator app_it = apps_.begin(); - app_it != apps_.end(); - ++app_it) { - scoped_ptr result(new AppResult( - profile_, (*app_it)->app_id(), list_controller_, show_recommendations)); - if (show_recommendations) { - result->set_title((*app_it)->indexed_name().text()); - result->UpdateFromLastLaunched(clock_->Now(), - (*app_it)->last_launch_time()); - } else { + if (show_recommendations) { + // Build a map of app ids to their position in the app list. + std::map id_to_app_list_index; + for (size_t i = 0; i < top_level_item_list_->item_count(); ++i) { + id_to_app_list_index[top_level_item_list_->item_at(i)->id()] = i; + } + + for (const App* app : apps_) { + scoped_ptr result( + new AppResult(profile_, app->app_id(), list_controller_, true)); + result->set_title(app->indexed_name().text()); + + // Use the app list order to tiebreak apps that have never been launched. + if (app->last_launch_time().is_null()) { + result->set_relevance( + kUnlaunchedAppRelevanceStepSize * + (apps_.size() - id_to_app_list_index[app->app_id()])); + } else { + result->UpdateFromLastLaunched(clock_->Now(), app->last_launch_time()); + } + Add(result.Pass()); + } + } else { + for (const App* app : apps_) { + scoped_ptr result( + new AppResult(profile_, app->app_id(), list_controller_, false)); TokenizedStringMatch match; - if (!match.Calculate(query_terms, (*app_it)->indexed_name())) + if (!match.Calculate(query_terms, app->indexed_name())) continue; - result->UpdateFromMatch((*app_it)->indexed_name(), match); + result->UpdateFromMatch(app->indexed_name(), match); + Add(result.Pass()); } - Add(result.Pass()); } update_results_factory_.InvalidateWeakPtrs(); diff --git a/chrome/browser/ui/app_list/search/app_search_provider.h b/chrome/browser/ui/app_list/search/app_search_provider.h index d81a7603ea0f..d2ce5fdaabe2 100644 --- a/chrome/browser/ui/app_list/search/app_search_provider.h +++ b/chrome/browser/ui/app_list/search/app_search_provider.h @@ -26,6 +26,8 @@ class ExtensionSet; namespace app_list { +class AppListItemList; + namespace test { class AppSearchProviderTest; } @@ -35,7 +37,8 @@ class AppSearchProvider : public SearchProvider, public: AppSearchProvider(Profile* profile, AppListControllerDelegate* list_controller, - scoped_ptr clock); + scoped_ptr clock, + AppListItemList* top_level_item_list); ~AppSearchProvider() override; // SearchProvider overrides: @@ -70,6 +73,8 @@ class AppSearchProvider : public SearchProvider, Apps apps_; + AppListItemList* top_level_item_list_; + scoped_ptr clock_; base::WeakPtrFactory update_results_factory_; diff --git a/chrome/browser/ui/app_list/search/app_search_provider_unittest.cc b/chrome/browser/ui/app_list/search/app_search_provider_unittest.cc index b59c139b4c3a..8fb6188a5cf2 100644 --- a/chrome/browser/ui/app_list/search/app_search_provider_unittest.cc +++ b/chrome/browser/ui/app_list/search/app_search_provider_unittest.cc @@ -11,19 +11,24 @@ #include "base/test/simple_test_clock.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/ui/app_list/app_list_test_util.h" +#include "chrome/browser/ui/app_list/extension_app_model_builder.h" #include "chrome/browser/ui/app_list/search/app_search_provider.h" +#include "chrome/browser/ui/app_list/test/test_app_list_controller_delegate.h" #include "chrome/common/chrome_constants.h" #include "chrome/test/base/testing_profile.h" #include "extensions/browser/extension_prefs.h" #include "extensions/browser/uninstall_reason.h" #include "extensions/common/extension_set.h" +#include "sync/api/string_ordinal.h" #include "testing/gtest/include/gtest/gtest.h" +#include "ui/app_list/app_list_item.h" +#include "ui/app_list/app_list_model.h" #include "ui/app_list/search_result.h" namespace app_list { namespace test { -const base::Time kTestCurrentTime = base::Time::FromInternalValue(1000); +const base::Time kTestCurrentTime = base::Time::FromInternalValue(100000); bool MoreRelevant(const SearchResult* result1, const SearchResult* result2) { return result1->relevance() > result2->relevance(); @@ -40,8 +45,12 @@ class AppSearchProviderTest : public AppListTestBase { scoped_ptr clock(new base::SimpleTestClock()); clock->SetNow(kTestCurrentTime); - app_search_.reset( - new AppSearchProvider(profile_.get(), NULL, clock.Pass())); + model_.reset(new app_list::AppListModel); + controller_.reset(new ::test::TestAppListControllerDelegate); + builder_.reset(new ExtensionAppModelBuilder(controller_.get())); + builder_->InitializeWithProfile(profile_.get(), model_.get()); + app_search_.reset(new AppSearchProvider( + profile_.get(), nullptr, clock.Pass(), model_->top_level_item_list())); } std::string RunQuery(const std::string& query) { @@ -64,10 +73,14 @@ class AppSearchProviderTest : public AppListTestBase { return result_str; } + AppListModel* model() { return model_.get(); } const SearchProvider::Results& results() { return app_search_->results(); } private: + scoped_ptr model_; scoped_ptr app_search_; + scoped_ptr builder_; + scoped_ptr<::test::TestAppListControllerDelegate> controller_; DISALLOW_COPY_AND_ASSIGN(AppSearchProviderTest); }; @@ -128,10 +141,10 @@ TEST_F(AppSearchProviderTest, FetchRecommendations) { prefs->SetLastLaunchTime(kHostedAppId, base::Time::FromInternalValue(20)); prefs->SetLastLaunchTime(kPackagedApp1Id, base::Time::FromInternalValue(10)); - prefs->SetLastLaunchTime(kPackagedApp2Id, base::Time::FromInternalValue(0)); + prefs->SetLastLaunchTime(kPackagedApp2Id, base::Time::FromInternalValue(5)); EXPECT_EQ("Hosted App,Packaged App 1,Packaged App 2", RunQuery("")); - prefs->SetLastLaunchTime(kHostedAppId, base::Time::FromInternalValue(0)); + prefs->SetLastLaunchTime(kHostedAppId, base::Time::FromInternalValue(5)); prefs->SetLastLaunchTime(kPackagedApp1Id, base::Time::FromInternalValue(10)); prefs->SetLastLaunchTime(kPackagedApp2Id, base::Time::FromInternalValue(20)); EXPECT_EQ("Packaged App 2,Packaged App 1,Hosted App", RunQuery("")); @@ -140,8 +153,27 @@ TEST_F(AppSearchProviderTest, FetchRecommendations) { prefs->SetLastLaunchTime(kHostedAppId, kTestCurrentTime + base::TimeDelta::FromSeconds(5)); prefs->SetLastLaunchTime(kPackagedApp1Id, base::Time::FromInternalValue(10)); + prefs->SetLastLaunchTime(kPackagedApp2Id, base::Time::FromInternalValue(5)); + EXPECT_EQ("Hosted App,Packaged App 1,Packaged App 2", RunQuery("")); +} + +TEST_F(AppSearchProviderTest, FetchUnlaunchedRecommendations) { + extensions::ExtensionPrefs* prefs = + extensions::ExtensionPrefs::Get(profile_.get()); + + // The order of unlaunched recommendations should be based on the app list + // order and be sorted below launched items. + prefs->SetLastLaunchTime(kHostedAppId, + kTestCurrentTime - base::TimeDelta::FromSeconds(5)); + prefs->SetLastLaunchTime(kPackagedApp1Id, base::Time::FromInternalValue(0)); prefs->SetLastLaunchTime(kPackagedApp2Id, base::Time::FromInternalValue(0)); EXPECT_EQ("Hosted App,Packaged App 1,Packaged App 2", RunQuery("")); + + // Switching the app list order should change the query result. + model()->SetItemPosition( + model()->FindItem(kPackagedApp2Id), + model()->FindItem(kPackagedApp1Id)->position().CreateBefore()); + EXPECT_EQ("Hosted App,Packaged App 2,Packaged App 1", RunQuery("")); } } // namespace test diff --git a/chrome/browser/ui/app_list/search/search_controller_factory.cc b/chrome/browser/ui/app_list/search/search_controller_factory.cc index d1a5a2c4caab..d69db6a9e9f5 100644 --- a/chrome/browser/ui/app_list/search/search_controller_factory.cc +++ b/chrome/browser/ui/app_list/search/search_controller_factory.cc @@ -16,6 +16,7 @@ #include "chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.h" #include "chrome/browser/ui/app_list/search/webstore/webstore_provider.h" #include "chrome/common/chrome_switches.h" +#include "ui/app_list/app_list_model.h" #include "ui/app_list/search/mixer.h" #include "ui/app_list/search_controller.h" @@ -39,16 +40,17 @@ bool IsSuggestionsSearchProviderEnabled() { scoped_ptr CreateSearchController( Profile* profile, - SearchBoxModel* search_box, - AppListModel::SearchResults* results, + AppListModel* model, AppListControllerDelegate* list_controller) { - scoped_ptr controller(new SearchController( - search_box, results, HistoryFactory::GetForBrowserContext(profile))); + scoped_ptr controller( + new SearchController(model->search_box(), model->results(), + HistoryFactory::GetForBrowserContext(profile))); - controller->AddProvider(Mixer::MAIN_GROUP, - scoped_ptr(new AppSearchProvider( - profile, list_controller, - make_scoped_ptr(new base::DefaultClock())))); + controller->AddProvider( + Mixer::MAIN_GROUP, + scoped_ptr(new AppSearchProvider( + profile, list_controller, make_scoped_ptr(new base::DefaultClock()), + model->top_level_item_list()))); controller->AddProvider(Mixer::OMNIBOX_GROUP, scoped_ptr( new OmniboxProvider(profile, list_controller))); diff --git a/chrome/browser/ui/app_list/search/search_controller_factory.h b/chrome/browser/ui/app_list/search/search_controller_factory.h index 2438abb4d0c1..8acac129e05a 100644 --- a/chrome/browser/ui/app_list/search/search_controller_factory.h +++ b/chrome/browser/ui/app_list/search/search_controller_factory.h @@ -19,8 +19,7 @@ class SearchController; // Build a SearchController instance with the profile. scoped_ptr CreateSearchController( Profile* profile, - SearchBoxModel* search_box, - AppListModel::SearchResults* results, + AppListModel* model, AppListControllerDelegate* list_controller); } // namespace app_list -- 2.11.4.GIT