Commit 54f178ae authored by tby's avatar tby Committed by Commit Bot

Add search result type getter to all SearchProviders.

The plan for integration testing in the launcher is roughly as follows:

 1. Give search providers a method to identify themselves with a
    search result type (<- this CL).
 2. Use the getter to inform the Mixer _which_ search provider is
    triggering a OnResultsChanged call.
 3. Add hooks into the Mixer allowing a RunLoop to wait until a list of
    providers have created results and called OnResultsChanged.
 4. Use the RunLoop hooks to build integration tests that inspect the
    result of a query after all relevant providers have completed.

This CL adds a getter method that lets each SearchProvider identify
itself with a particular search result type. Two providers,
SettingsShortcutProvider and ArcAppDataSearchProvider, don't
appear to have enum entries for their result types. I've given them
kUnknown for now, and we can update them if they ever need to be
integration tested.

Due to the asynchrony of the search backend, we need this kind of "wait
until providers are done" system in order to know how long to wait for
in a browser test, as browser tests don't support
ScopedTasksEnvironments and the like.

In future we could use this system to optimize search ranking, as we
would know which results are new, and could avoid re-ranking already
ranked results.

Bug: 1086713
Change-Id: Icac3efaaf963a4b5e445dd0a0de8fd435fd27f61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214736Reviewed-by: default avatarJia Meng <jiameng@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774434}
parent 4065cf23
...@@ -35,6 +35,10 @@ AnswerCardSearchProvider::AnswerCardSearchProvider( ...@@ -35,6 +35,10 @@ AnswerCardSearchProvider::AnswerCardSearchProvider(
AnswerCardSearchProvider::~AnswerCardSearchProvider() = default; AnswerCardSearchProvider::~AnswerCardSearchProvider() = default;
ash::AppListSearchResultType AnswerCardSearchProvider::ResultType() {
return ash::AppListSearchResultType::kAnswerCard;
}
void AnswerCardSearchProvider::Start(const base::string16& query) { void AnswerCardSearchProvider::Start(const base::string16& query) {
if (!model_updater_->SearchEngineIsGoogle()) { if (!model_updater_->SearchEngineIsGoogle()) {
UpdateQuery(base::string16()); UpdateQuery(base::string16());
......
...@@ -29,6 +29,7 @@ class AnswerCardSearchProvider : public SearchProvider { ...@@ -29,6 +29,7 @@ class AnswerCardSearchProvider : public SearchProvider {
// SearchProvider overrides: // SearchProvider overrides:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
void UpdateQuery(const base::string16& query); void UpdateQuery(const base::string16& query);
......
...@@ -448,6 +448,10 @@ void AppSearchProvider::ViewClosing() { ...@@ -448,6 +448,10 @@ void AppSearchProvider::ViewClosing() {
data_source->ViewClosing(); data_source->ViewClosing();
} }
ash::AppListSearchResultType AppSearchProvider::ResultType() {
return ash::AppListSearchResultType::kInstalledApp;
}
void AppSearchProvider::RefreshAppsAndUpdateResults() { void AppSearchProvider::RefreshAppsAndUpdateResults() {
// Clear any pending requests if any. // Clear any pending requests if any.
refresh_apps_factory_.InvalidateWeakPtrs(); refresh_apps_factory_.InvalidateWeakPtrs();
......
...@@ -48,6 +48,7 @@ class AppSearchProvider : public SearchProvider { ...@@ -48,6 +48,7 @@ class AppSearchProvider : public SearchProvider {
// SearchProvider overrides: // SearchProvider overrides:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
void ViewClosing() override; void ViewClosing() override;
ash::AppListSearchResultType ResultType() override;
// Refreshes apps and updates results inline // Refreshes apps and updates results inline
void RefreshAppsAndUpdateResults(); void RefreshAppsAndUpdateResults();
......
...@@ -34,6 +34,10 @@ ArcAppDataSearchProvider::ArcAppDataSearchProvider( ...@@ -34,6 +34,10 @@ ArcAppDataSearchProvider::ArcAppDataSearchProvider(
ArcAppDataSearchProvider::~ArcAppDataSearchProvider() = default; ArcAppDataSearchProvider::~ArcAppDataSearchProvider() = default;
ash::AppListSearchResultType ArcAppDataSearchProvider::ResultType() {
return ash::AppListSearchResultType::kUnknown;
}
void ArcAppDataSearchProvider::Start(const base::string16& query) { void ArcAppDataSearchProvider::Start(const base::string16& query) {
arc::mojom::AppInstance* app_instance = arc::mojom::AppInstance* app_instance =
arc::ArcServiceManager::Get() arc::ArcServiceManager::Get()
......
...@@ -24,6 +24,7 @@ class ArcAppDataSearchProvider : public SearchProvider { ...@@ -24,6 +24,7 @@ class ArcAppDataSearchProvider : public SearchProvider {
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
void OnResults(arc::mojom::AppDataRequestState state, void OnResults(arc::mojom::AppDataRequestState state,
......
...@@ -237,6 +237,10 @@ void ArcAppReinstallSearchProvider::StopRepeatingFetch() { ...@@ -237,6 +237,10 @@ void ArcAppReinstallSearchProvider::StopRepeatingFetch() {
UpdateResults(); UpdateResults();
} }
ash::AppListSearchResultType ArcAppReinstallSearchProvider::ResultType() {
return ash::AppListSearchResultType::kPlayStoreReinstallApp;
}
void ArcAppReinstallSearchProvider::Start(const base::string16& query) { void ArcAppReinstallSearchProvider::Start(const base::string16& query) {
if (query_is_empty_ == query.empty()) if (query_is_empty_ == query.empty())
return; return;
......
...@@ -75,6 +75,7 @@ class ArcAppReinstallSearchProvider ...@@ -75,6 +75,7 @@ class ArcAppReinstallSearchProvider
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
// Used by unit tests. SearchProvider takes ownership of pointer. // Used by unit tests. SearchProvider takes ownership of pointer.
void SetTimerForTesting(std::unique_ptr<base::RepeatingTimer> timer); void SetTimerForTesting(std::unique_ptr<base::RepeatingTimer> timer);
......
...@@ -28,6 +28,10 @@ ArcAppShortcutsSearchProvider::ArcAppShortcutsSearchProvider( ...@@ -28,6 +28,10 @@ ArcAppShortcutsSearchProvider::ArcAppShortcutsSearchProvider(
ArcAppShortcutsSearchProvider::~ArcAppShortcutsSearchProvider() = default; ArcAppShortcutsSearchProvider::~ArcAppShortcutsSearchProvider() = default;
ash::AppListSearchResultType ArcAppShortcutsSearchProvider::ResultType() {
return ash::AppListSearchResultType::kArcAppShortcut;
}
void ArcAppShortcutsSearchProvider::Start(const base::string16& query) { void ArcAppShortcutsSearchProvider::Start(const base::string16& query) {
arc::mojom::AppInstance* app_instance = arc::mojom::AppInstance* app_instance =
arc::ArcServiceManager::Get() arc::ArcServiceManager::Get()
......
...@@ -27,6 +27,7 @@ class ArcAppShortcutsSearchProvider : public SearchProvider { ...@@ -27,6 +27,7 @@ class ArcAppShortcutsSearchProvider : public SearchProvider {
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
void OnGetAppShortcutGlobalQueryItems( void OnGetAppShortcutGlobalQueryItems(
......
...@@ -94,6 +94,10 @@ ArcPlayStoreSearchProvider::ArcPlayStoreSearchProvider( ...@@ -94,6 +94,10 @@ ArcPlayStoreSearchProvider::ArcPlayStoreSearchProvider(
ArcPlayStoreSearchProvider::~ArcPlayStoreSearchProvider() = default; ArcPlayStoreSearchProvider::~ArcPlayStoreSearchProvider() = default;
ash::AppListSearchResultType ArcPlayStoreSearchProvider::ResultType() {
return ash::AppListSearchResultType::kPlayStoreApp;
}
void ArcPlayStoreSearchProvider::Start(const base::string16& query) { void ArcPlayStoreSearchProvider::Start(const base::string16& query) {
last_query_ = query; last_query_ = query;
......
...@@ -30,6 +30,7 @@ class ArcPlayStoreSearchProvider : public SearchProvider { ...@@ -30,6 +30,7 @@ class ArcPlayStoreSearchProvider : public SearchProvider {
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
void OnResults(const base::string16& query, void OnResults(const base::string16& query,
......
...@@ -131,6 +131,10 @@ AssistantSearchProvider::~AssistantSearchProvider() { ...@@ -131,6 +131,10 @@ AssistantSearchProvider::~AssistantSearchProvider() {
} }
} }
ash::AppListSearchResultType AssistantSearchProvider::ResultType() {
return ash::AppListSearchResultType::kAssistantChip;
}
void AssistantSearchProvider::OnAssistantControllerDestroying() { void AssistantSearchProvider::OnAssistantControllerDestroying() {
ash::AssistantSuggestionsController::Get()->GetModel()->RemoveObserver(this); ash::AssistantSuggestionsController::Get()->GetModel()->RemoveObserver(this);
assistant_state_observer_.Remove(ash::AssistantState::Get()); assistant_state_observer_.Remove(ash::AssistantState::Get());
......
...@@ -33,6 +33,7 @@ class AssistantSearchProvider : public SearchProvider, ...@@ -33,6 +33,7 @@ class AssistantSearchProvider : public SearchProvider,
private: private:
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override {} void Start(const base::string16& query) override {}
ash::AppListSearchResultType ResultType() override;
// ash::AssistantControllerObserver: // ash::AssistantControllerObserver:
void OnAssistantControllerDestroying() override; void OnAssistantControllerDestroying() override;
......
...@@ -135,6 +135,10 @@ void DriveQuickAccessProvider::StartSearchController() { ...@@ -135,6 +135,10 @@ void DriveQuickAccessProvider::StartSearchController() {
search_controller_->Start(base::string16()); search_controller_->Start(base::string16());
} }
ash::AppListSearchResultType DriveQuickAccessProvider::ResultType() {
return ash::AppListSearchResultType::kDriveQuickAccess;
}
void DriveQuickAccessProvider::Start(const base::string16& query) { void DriveQuickAccessProvider::Start(const base::string16& query) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
ClearResultsSilently(); ClearResultsSilently();
......
...@@ -34,6 +34,7 @@ class DriveQuickAccessProvider : public SearchProvider, ...@@ -34,6 +34,7 @@ class DriveQuickAccessProvider : public SearchProvider,
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
void AppListShown() override; void AppListShown() override;
ash::AppListSearchResultType ResultType() override;
// drive::DriveIntegrationServiceObserver: // drive::DriveIntegrationServiceObserver:
void OnFileSystemMounted() override; void OnFileSystemMounted() override;
......
...@@ -74,6 +74,10 @@ void LauncherSearchProvider::SetSearchResults( ...@@ -74,6 +74,10 @@ void LauncherSearchProvider::SetSearchResults(
SwapResults(&new_results); SwapResults(&new_results);
} }
ash::AppListSearchResultType LauncherSearchProvider::ResultType() {
return ash::AppListSearchResultType::kLauncher;
}
void LauncherSearchProvider::DelayQuery(const base::Closure& closure) { void LauncherSearchProvider::DelayQuery(const base::Closure& closure) {
base::TimeDelta delay = base::TimeDelta delay =
base::TimeDelta::FromMilliseconds(kLauncherSearchProviderQueryDelayInMs); base::TimeDelta::FromMilliseconds(kLauncherSearchProviderQueryDelayInMs);
......
...@@ -31,6 +31,7 @@ class LauncherSearchProvider : public SearchProvider { ...@@ -31,6 +31,7 @@ class LauncherSearchProvider : public SearchProvider {
void SetSearchResults( void SetSearchResults(
const extensions::ExtensionId& extension_id, const extensions::ExtensionId& extension_id,
std::vector<std::unique_ptr<LauncherSearchResult>> extension_results); std::vector<std::unique_ptr<LauncherSearchResult>> extension_results);
ash::AppListSearchResultType ResultType() override;
private: private:
// Delays query for |kLauncherSearchProviderQueryDelayInMs|. This dispatches // Delays query for |kLauncherSearchProviderQueryDelayInMs|. This dispatches
......
...@@ -63,6 +63,10 @@ void OmniboxProvider::Start(const base::string16& query) { ...@@ -63,6 +63,10 @@ void OmniboxProvider::Start(const base::string16& query) {
controller_->Start(input); controller_->Start(input);
} }
ash::AppListSearchResultType OmniboxProvider::ResultType() {
return ash::AppListSearchResultType::kOmnibox;
}
void OmniboxProvider::PopulateFromACResult(const AutocompleteResult& result) { void OmniboxProvider::PopulateFromACResult(const AutocompleteResult& result) {
SearchProvider::Results new_results; SearchProvider::Results new_results;
new_results.reserve(result.size()); new_results.reserve(result.size());
......
...@@ -28,6 +28,7 @@ class OmniboxProvider : public SearchProvider, ...@@ -28,6 +28,7 @@ class OmniboxProvider : public SearchProvider,
// SearchProvider overrides: // SearchProvider overrides:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
// Populates result list from AutocompleteResult. // Populates result list from AutocompleteResult.
......
...@@ -165,6 +165,10 @@ OsSettingsProvider::OsSettingsProvider(Profile* profile) ...@@ -165,6 +165,10 @@ OsSettingsProvider::OsSettingsProvider(Profile* profile)
OsSettingsProvider::~OsSettingsProvider() = default; OsSettingsProvider::~OsSettingsProvider() = default;
ash::AppListSearchResultType OsSettingsProvider::ResultType() {
return ash::AppListSearchResultType::kOsSettings;
}
void OsSettingsProvider::Start(const base::string16& query) { void OsSettingsProvider::Start(const base::string16& query) {
if (!search_handler_) if (!search_handler_)
return; return;
......
...@@ -68,6 +68,7 @@ class OsSettingsProvider : public SearchProvider, ...@@ -68,6 +68,7 @@ class OsSettingsProvider : public SearchProvider,
// SearchProvider: // SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
// apps::AppRegistryCache::Observer: // apps::AppRegistryCache::Observer:
void OnAppUpdate(const apps::AppUpdate& update) override; void OnAppUpdate(const apps::AppUpdate& update) override;
......
...@@ -6,8 +6,10 @@ ...@@ -6,8 +6,10 @@
#define CHROME_BROWSER_UI_APP_LIST_SEARCH_SEARCH_PROVIDER_H_ #define CHROME_BROWSER_UI_APP_LIST_SEARCH_SEARCH_PROVIDER_H_
#include <memory> #include <memory>
#include <string>
#include <vector> #include <vector>
#include "ash/public/cpp/app_list/app_list_types.h"
#include "base/callback.h" #include "base/callback.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/strings/string16.h" #include "base/strings/string16.h"
...@@ -38,6 +40,8 @@ class SearchProvider { ...@@ -38,6 +40,8 @@ class SearchProvider {
// Invoked when the app list is shown. This can optionally be used by a // Invoked when the app list is shown. This can optionally be used by a
// provider to eg. warm up a cache of results. // provider to eg. warm up a cache of results.
virtual void AppListShown() {} virtual void AppListShown() {}
// Returns the main result type created by this provider.
virtual ash::AppListSearchResultType ResultType() = 0;
void set_result_changed_callback(const ResultChangedCallback& callback) { void set_result_changed_callback(const ResultChangedCallback& callback) {
result_changed_callback_ = callback; result_changed_callback_ = callback;
......
...@@ -35,4 +35,8 @@ void SettingsShortcutProvider::Start(const base::string16& query) { ...@@ -35,4 +35,8 @@ void SettingsShortcutProvider::Start(const base::string16& query) {
SwapResults(&search_results); SwapResults(&search_results);
} }
ash::AppListSearchResultType SettingsShortcutProvider::ResultType() {
return ash::AppListSearchResultType::kUnknown;
}
} // namespace app_list } // namespace app_list
...@@ -23,6 +23,7 @@ class SettingsShortcutProvider : public SearchProvider { ...@@ -23,6 +23,7 @@ class SettingsShortcutProvider : public SearchProvider {
// SearchProvider overrides: // SearchProvider overrides:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
private: private:
Profile* const profile_; Profile* const profile_;
......
...@@ -30,7 +30,7 @@ class FakeAppListModelUpdater; ...@@ -30,7 +30,7 @@ class FakeAppListModelUpdater;
namespace app_list { namespace app_list {
namespace test { namespace test {
using ResultType = ash::AppListSearchResultType; using SearchResultType = ash::AppListSearchResultType;
// Maximum number of results to show in each mixer group. // Maximum number of results to show in each mixer group.
const size_t kMaxAppsGroupResults = 4; const size_t kMaxAppsGroupResults = 4;
...@@ -72,7 +72,7 @@ int TestSearchResult::instantiation_count = 0; ...@@ -72,7 +72,7 @@ int TestSearchResult::instantiation_count = 0;
class TestSearchProvider : public SearchProvider { class TestSearchProvider : public SearchProvider {
public: public:
TestSearchProvider(const std::string& prefix, ResultType result_type) TestSearchProvider(const std::string& prefix, SearchResultType result_type)
: prefix_(prefix), : prefix_(prefix),
count_(0), count_(0),
bad_relevance_range_(false), bad_relevance_range_(false),
...@@ -108,6 +108,10 @@ class TestSearchProvider : public SearchProvider { ...@@ -108,6 +108,10 @@ class TestSearchProvider : public SearchProvider {
} }
} }
ash::AppListSearchResultType ResultType() override {
return ash::AppListSearchResultType::kUnknown;
}
void set_prefix(const std::string& prefix) { prefix_ = prefix; } void set_prefix(const std::string& prefix) { prefix_ = prefix; }
void SetDisplayType(ChromeSearchResult::DisplayType display_type) { void SetDisplayType(ChromeSearchResult::DisplayType display_type) {
display_type_ = display_type; display_type_ = display_type;
...@@ -126,7 +130,7 @@ class TestSearchProvider : public SearchProvider { ...@@ -126,7 +130,7 @@ class TestSearchProvider : public SearchProvider {
bool small_relevance_range_; bool small_relevance_range_;
bool last_result_has_display_index_; bool last_result_has_display_index_;
ChromeSearchResult::DisplayType display_type_; ChromeSearchResult::DisplayType display_type_;
ResultType result_type_; SearchResultType result_type_;
DISALLOW_COPY_AND_ASSIGN(TestSearchProvider); DISALLOW_COPY_AND_ASSIGN(TestSearchProvider);
}; };
...@@ -140,12 +144,12 @@ class MixerTest : public testing::Test { ...@@ -140,12 +144,12 @@ class MixerTest : public testing::Test {
void SetUp() override { void SetUp() override {
model_updater_ = std::make_unique<FakeAppListModelUpdater>(); model_updater_ = std::make_unique<FakeAppListModelUpdater>();
providers_.push_back(
std::make_unique<TestSearchProvider>("app", ResultType::kInternalApp));
providers_.push_back(
std::make_unique<TestSearchProvider>("omnibox", ResultType::kOmnibox));
providers_.push_back(std::make_unique<TestSearchProvider>( providers_.push_back(std::make_unique<TestSearchProvider>(
"playstore", ResultType::kPlayStoreApp)); "app", SearchResultType::kInternalApp));
providers_.push_back(std::make_unique<TestSearchProvider>(
"omnibox", SearchResultType::kOmnibox));
providers_.push_back(std::make_unique<TestSearchProvider>(
"playstore", SearchResultType::kPlayStoreApp));
} }
void CreateMixer() { void CreateMixer() {
......
...@@ -83,6 +83,10 @@ ZeroStateFileProvider::ZeroStateFileProvider(Profile* profile) ...@@ -83,6 +83,10 @@ ZeroStateFileProvider::ZeroStateFileProvider(Profile* profile)
ZeroStateFileProvider::~ZeroStateFileProvider() = default; ZeroStateFileProvider::~ZeroStateFileProvider() = default;
ash::AppListSearchResultType ZeroStateFileProvider::ResultType() {
return ash::AppListSearchResultType::kZeroStateFile;
}
void ZeroStateFileProvider::Start(const base::string16& query) { void ZeroStateFileProvider::Start(const base::string16& query) {
query_start_time_ = base::TimeTicks::Now(); query_start_time_ = base::TimeTicks::Now();
ClearResultsSilently(); ClearResultsSilently();
......
...@@ -42,7 +42,9 @@ class ZeroStateFileProvider : public SearchProvider, ...@@ -42,7 +42,9 @@ class ZeroStateFileProvider : public SearchProvider,
explicit ZeroStateFileProvider(Profile* profile); explicit ZeroStateFileProvider(Profile* profile);
~ZeroStateFileProvider() override; ~ZeroStateFileProvider() override;
// SearchProvider:
void Start(const base::string16& query) override; void Start(const base::string16& query) override;
ash::AppListSearchResultType ResultType() override;
// file_manager::file_tasks::FileTaskObserver: // file_manager::file_tasks::FileTaskObserver:
void OnFilesOpened(const std::vector<FileOpenEvent>& file_opens) override; void OnFilesOpened(const std::vector<FileOpenEvent>& file_opens) override;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment