Remove multiplier on launcher search provider groups.

This is the first in a series of changes to how search providers are
created and configured, with the aim of reducing the number of ranking
and filtering steps between the providers and the UI.

Eventually I'd like to repurpose the concept of a 'mixer group'.
This change makes a small start by removing a feature of
groups that we don't use anymore: multipliers on result scores.

This CL also deletes some obsolete tests that don't take into account
modern ranking. This coverage is mostly duplicated by search result
ranker tests.

Bug: 1028447
Change-Id: Ib6d27975ede3b9ee0a9d4b508d262ebff154670b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2342533
Commit-Queue: Tony Yeoman <tby@chromium.org>
Reviewed-by: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798526}

chrome/browser/ui/app_list/search/mixer.cc

@@ -45,8 +45,8 @@ bool Mixer::SortData::operator<(const SortData& other) const {
 // Used to group relevant providers together for mixing their results.
 class Mixer::Group {
  public:
-  Group(size_t max_results, double multiplier, double boost)
-      : max_results_(max_results), multiplier_(multiplier), boost_(boost) {}
+  Group(size_t max_results, double boost)
+      : max_results_(max_results), boost_(boost) {}
   ~Group() {}
 
   void AddProvider(SearchProvider* provider) {
@@ -64,8 +64,7 @@ class Mixer::Group {
         // [0.0, 1.0]. Clamp to that range.
         const double relevance =
             base::ClampToRange(result->relevance(), 0.0, 1.0);
-        double boost = boost_;
-        results_.emplace_back(result.get(), relevance * multiplier_ + boost);
+        results_.emplace_back(result.get(), relevance + boost_);
       }
     }
 
@@ -81,7 +80,6 @@ class Mixer::Group {
  private:
   typedef std::vector<SearchProvider*> Providers;
   const size_t max_results_;
-  const double multiplier_;
   const double boost_;
 
   Providers providers_;  // Not owned.
@@ -104,8 +102,8 @@ void Mixer::InitializeRankers(Profile* profile,
   }
 }
 
-size_t Mixer::AddGroup(size_t max_results, double multiplier, double boost) {
-  groups_.push_back(std::make_unique<Group>(max_results, multiplier, boost));
+size_t Mixer::AddGroup(size_t max_results, double boost) {
+  groups_.push_back(std::make_unique<Group>(max_results, boost));
   return groups_.size() - 1;
 }
 

chrome/browser/ui/app_list/search/mixer.h

@@ -34,8 +34,7 @@ enum class RankingItemType;
 
 // Mixer collects results from providers, sorts them and publishes them to the
 // SearchResults UI model. The targeted results have 6 slots to hold the
-// result. The search controller can specify any number of groups, each with a
-// different number of results and priority boost.
+// result.
 class Mixer {
  public:
   explicit Mixer(AppListModelUpdater* model_updater);
@@ -45,9 +44,8 @@ class Mixer {
   // chosen from this group (if 0, will allow unlimited results from this
   // group). If there aren't enough results from all groups, more than
   // |max_results| may be chosen from this group. Each result in the group will
-  // have its score multiplied by |multiplier| and added by |boost|. Returns the
-  // group's group_id.
-  size_t AddGroup(size_t max_results, double multiplier, double boost);
+  // have its score increased by |boost|. Returns the group's group_id.
+  size_t AddGroup(size_t max_results, double boost);
 
   // Associates a provider with a mixer group.
   void AddProviderToGroup(size_t group_id, SearchProvider* provider);

chrome/browser/ui/app_list/search/search_controller.cc

@@ -133,10 +133,8 @@ void SearchController::InvokeResultAction(ChromeSearchResult* result,
   result->InvokeAction(action_index, event_flags);
 }
 
-size_t SearchController::AddGroup(size_t max_results,
-                                  double multiplier,
-                                  double boost) {
-  return mixer_->AddGroup(max_results, multiplier, boost);
+size_t SearchController::AddGroup(size_t max_results, double boost) {
+  return mixer_->AddGroup(max_results, boost);
 }
 
 void SearchController::AddProvider(size_t group_id,

chrome/browser/ui/app_list/search/search_controller.h

@@ -59,7 +59,7 @@ class SearchController {
                           int event_flags);
 
   // Adds a new mixer group. See Mixer::AddGroup.
-  size_t AddGroup(size_t max_results, double multiplier, double boost);
+  size_t AddGroup(size_t max_results, double boost = 0.0f);
 
   // Takes ownership of |provider| and associates it with given mixer group.
   void AddProvider(size_t group_id, std::unique_ptr<SearchProvider> provider);

chrome/browser/ui/app_list/search/search_controller_factory.cc

@@ -80,9 +80,7 @@ constexpr size_t kMaxAssistantResults = 1;
 // TODO(wutao): Need UX spec.
 constexpr size_t kMaxSettingsShortcutResults = 6;
 
-constexpr float kBoostOfSettingsShortcut = 10.0f;
-// Keep in sync with value in search_result_ranker.cc.
-constexpr float kBoostOfApps = 8.0f;
+constexpr double kAppBoost = 8.0;
 
 }  // namespace
 
@@ -103,33 +101,34 @@ std::unique_ptr<SearchController> CreateSearchController(
   // a query turns up very few results, the mixer may take more than this
   // maximum from a particular group.
 
-  // For fullscreen app list, Settings shortcuts will show on the very top and
-  // apps and answer card in the middle and other search results in the bottom.
-  // So set boost 10.0, 8.0, 5.0, 0.0 respectively.
-  size_t answer_card_group_id = controller->AddGroup(1, 1.0, 5.0);
-  size_t apps_group_id =
-      controller->AddGroup(kMaxAppsGroupResults, 1.0, kBoostOfApps);
+  // For the fullscreen app list, apps appear above the answer card, which
+  // appears above other results. Set boosts to |kAppBoost| for apps of all
+  // kinds, 5.0 for answer card, and otherwise 0.0.
+  size_t apps_group_id = controller->AddGroup(kMaxAppsGroupResults, kAppBoost);
+  size_t answer_card_group_id = controller->AddGroup(1, 5.0);
+
   size_t omnibox_group_id = controller->AddGroup(
-      ash::AppListConfig::instance().max_search_result_list_items(), 1.0, 0.0);
+      ash::AppListConfig::instance().max_search_result_list_items());
 
   // Add search providers.
   controller->AddProvider(
       apps_group_id, std::make_unique<AppSearchProvider>(
                          profile, list_controller,
                          base::DefaultClock::GetInstance(), model_updater));
-  controller->AddProvider(omnibox_group_id, std::make_unique<OmniboxProvider>(
-                                                profile, list_controller));
   if (app_list_features::IsAnswerCardEnabled()) {
     controller->AddProvider(answer_card_group_id,
                             std::make_unique<AnswerCardSearchProvider>(
                                 profile, model_updater, list_controller));
   }
 
+  controller->AddProvider(omnibox_group_id, std::make_unique<OmniboxProvider>(
+                                                profile, list_controller));
+
   // The Assistant search provider currently only contributes search results
   // when launcher chip integration is enabled.
   if (chromeos::assistant::features::IsLauncherChipIntegrationEnabled()) {
-    size_t assistant_group_id = controller->AddGroup(
-        kMaxAssistantResults, /*multiplier=*/1.0, kBoostOfApps);
+    size_t assistant_group_id =
+        controller->AddGroup(kMaxAssistantResults, kAppBoost);
     controller->AddProvider(assistant_group_id,
                             std::make_unique<AssistantSearchProvider>());
   }
@@ -138,7 +137,7 @@ std::unique_ptr<SearchController> CreateSearchController(
   // session and running on Chrome OS.
   if (!profile->IsGuestSession()) {
     size_t search_api_group_id =
-        controller->AddGroup(kMaxLauncherSearchResults, 1.0, 0.0);
+        controller->AddGroup(kMaxLauncherSearchResults);
     controller->AddProvider(search_api_group_id,
                             std::make_unique<LauncherSearchProvider>(profile));
   }
@@ -147,7 +146,7 @@ std::unique_ptr<SearchController> CreateSearchController(
   if (app_list_features::IsAppReinstallZeroStateEnabled() &&
       arc::IsArcAllowedForProfile(profile)) {
     size_t recommended_app_group_id =
-        controller->AddGroup(kMaxAppReinstallSearchResults, 1.0, kBoostOfApps);
+        controller->AddGroup(kMaxAppReinstallSearchResults, kAppBoost);
     controller->AddProvider(recommended_app_group_id,
                             std::make_unique<ArcAppReinstallSearchProvider>(
                                 profile, kMaxAppReinstallSearchResults));
@@ -157,7 +156,7 @@ std::unique_ptr<SearchController> CreateSearchController(
     // Set same boost as apps group since Play store results are placed
     // with apps.
     size_t playstore_api_group_id =
-        controller->AddGroup(kMaxPlayStoreResults, 1.0, kBoostOfApps);
+        controller->AddGroup(kMaxPlayStoreResults, kAppBoost);
     controller->AddProvider(
         playstore_api_group_id,
         std::make_unique<ArcPlayStoreSearchProvider>(kMaxPlayStoreResults,
@@ -166,15 +165,17 @@ std::unique_ptr<SearchController> CreateSearchController(
 
   if (app_list_features::IsAppDataSearchEnabled()) {
     size_t app_data_api_group_id =
-        controller->AddGroup(kMaxAppDataResults, 1.0, kBoostOfApps);
+        controller->AddGroup(kMaxAppDataResults, kAppBoost);
     controller->AddProvider(app_data_api_group_id,
                             std::make_unique<ArcAppDataSearchProvider>(
                                 kMaxAppDataResults, list_controller));
   }
 
+  // TODO(crbug.com/1028447): Remove the settings shortcut provider, superseded
+  // by OsSettingsProvider.
   if (app_list_features::IsSettingsShortcutSearchEnabled()) {
-    size_t settings_shortcut_group_id = controller->AddGroup(
-        kMaxSettingsShortcutResults, 1.0, kBoostOfSettingsShortcut);
+    size_t settings_shortcut_group_id =
+        controller->AddGroup(kMaxSettingsShortcutResults);
     controller->AddProvider(
         settings_shortcut_group_id,
         std::make_unique<SettingsShortcutProvider>(profile));
@@ -182,7 +183,7 @@ std::unique_ptr<SearchController> CreateSearchController(
 
   if (arc::IsArcAllowedForProfile(profile)) {
     size_t app_shortcut_group_id =
-        controller->AddGroup(kMaxAppShortcutResults, 1.0, kBoostOfApps);
+        controller->AddGroup(kMaxAppShortcutResults, kAppBoost);
     controller->AddProvider(
         app_shortcut_group_id,
         std::make_unique<ArcAppShortcutsSearchProvider>(
@@ -194,11 +195,11 @@ std::unique_ptr<SearchController> CreateSearchController(
   // scores changed to fit with these providers.
   if (app_list_features::IsZeroStateMixedTypesRankerEnabled()) {
     size_t zero_state_files_group_id =
-        controller->AddGroup(kMaxZeroStateFileResults, 1.0, 0.0);
+        controller->AddGroup(kMaxZeroStateFileResults);
     controller->AddProvider(zero_state_files_group_id,
                             std::make_unique<ZeroStateFileProvider>(profile));
     size_t drive_quick_access_group_id =
-        controller->AddGroup(kMaxDriveQuickAccessResults, 1.0, 0.0);
+        controller->AddGroup(kMaxDriveQuickAccessResults);
     controller->AddProvider(
         drive_quick_access_group_id,
         std::make_unique<DriveQuickAccessProvider>(profile, controller.get()));
@@ -206,7 +207,7 @@ std::unique_ptr<SearchController> CreateSearchController(
 
   if (app_list_features::IsLauncherSettingsSearchEnabled()) {
     size_t os_settings_search_group_id =
-        controller->AddGroup(kGenericMaxResults, 1.0, 0.0);
+        controller->AddGroup(kGenericMaxResults);
     controller->AddProvider(os_settings_search_group_id,
                             std::make_unique<OsSettingsProvider>(profile));
   }

chrome/browser/ui/app_list/search/tests/mixer_unittest.cc

@@ -157,10 +157,9 @@ class MixerTest : public testing::Test {
 
     // TODO(warx): when fullscreen app list is default enabled, modify this test
     // to test answer card/apps group having relevance boost.
-    size_t apps_group_id = mixer_->AddGroup(kMaxAppsGroupResults, 1.0, 0.0);
-    size_t omnibox_group_id = mixer_->AddGroup(kMaxOmniboxResults, 1.0, 0.0);
-    size_t playstore_group_id =
-        mixer_->AddGroup(kMaxPlaystoreResults, 0.5, 0.0);
+    size_t apps_group_id = mixer_->AddGroup(kMaxAppsGroupResults, 0.0);
+    size_t omnibox_group_id = mixer_->AddGroup(kMaxOmniboxResults, 0.0);
+    size_t playstore_group_id = mixer_->AddGroup(kMaxPlaystoreResults, 0.0);
 
     mixer_->AddProviderToGroup(apps_group_id, providers_[0].get());
     mixer_->AddProviderToGroup(omnibox_group_id, providers_[1].get());
@@ -210,58 +209,6 @@ class MixerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(MixerTest);
 };
 
-TEST_F(MixerTest, Basic) {
-  CreateMixer();
-
-  // Note: Some cases in |expected| have vastly more results than others, due to
-  // the "at least 6" mechanism. If it gets at least 6 results from all
-  // providers, it stops at 6. If not, it fetches potentially many more results
-  // from all providers. Not ideal, but currently by design.
-  struct TestCase {
-    const size_t app_results;
-    const size_t omnibox_results;
-    const size_t playstore_results;
-    const char* expected;
-  } kTestCases[] = {
-      {0, 0, 0, ""},
-      {10, 0, 0, "app0,app1,app2,app3,app4,app5,app6,app7,app8,app9"},
-      {0, 0, 10,
-       "playstore0,playstore1,playstore2,playstore3,playstore4,playstore5,"
-       "playstore6,playstore7,playstore8,playstore9"},
-      {4, 6, 0, "app0,omnibox0,app1,omnibox1,app2,omnibox2,app3,omnibox3"},
-      {4, 6, 2,
-       "app0,omnibox0,app1,omnibox1,app2,omnibox2,app3,omnibox3,playstore0,"
-       "playstore1"},
-      {10, 10, 10,
-       "app0,omnibox0,app1,omnibox1,app2,omnibox2,app3,omnibox3,playstore0,"
-       "playstore1"},
-      {0, 10, 0,
-       "omnibox0,omnibox1,omnibox2,omnibox3,omnibox4,omnibox5,omnibox6,"
-       "omnibox7,omnibox8,omnibox9"},
-      {0, 10, 1,
-       "omnibox0,omnibox1,omnibox2,omnibox3,playstore0,omnibox4,omnibox5,"
-       "omnibox6,omnibox7,omnibox8,omnibox9"},
-      {0, 10, 2, "omnibox0,omnibox1,omnibox2,omnibox3,playstore0,playstore1"},
-      {1, 10, 0,
-       "app0,omnibox0,omnibox1,omnibox2,omnibox3,omnibox4,omnibox5,omnibox6,"
-       "omnibox7,omnibox8,omnibox9"},
-      {2, 10, 0, "app0,omnibox0,app1,omnibox1,omnibox2,omnibox3"},
-      {2, 10, 1, "app0,omnibox0,app1,omnibox1,omnibox2,omnibox3,playstore0"},
-      {2, 10, 2,
-       "app0,omnibox0,app1,omnibox1,omnibox2,omnibox3,playstore0,playstore1"},
-      {2, 0, 2, "app0,app1,playstore0,playstore1"},
-      {0, 0, 0, ""}};
-
-  for (size_t i = 0; i < base::size(kTestCases); ++i) {
-    app_provider()->set_count(kTestCases[i].app_results);
-    omnibox_provider()->set_count(kTestCases[i].omnibox_results);
-    playstore_provider()->set_count(kTestCases[i].playstore_results);
-    RunQuery();
-
-    EXPECT_EQ(kTestCases[i].expected, GetResults()) << "Case " << i;
-  }
-}
-
 // Tests that results with display index defined, will be shown in the final
 // results.
 TEST_F(MixerTest, ResultsWithDisplayIndex) {
@@ -275,8 +222,8 @@ TEST_F(MixerTest, ResultsWithDisplayIndex) {
   RunQuery();
 
   EXPECT_EQ(
-      "app0,omnibox0,app1,omnibox1,app2,omnibox2,app3,omnibox3,playstore0,"
-      "playstore1",
+      "app0,omnibox0,playstore0,app1,omnibox1,playstore1,app2,omnibox2,app3,"
+      "omnibox3",
       GetResults());
 
   // If the last result has display index defined, it will be in the final
@@ -285,8 +232,8 @@ TEST_F(MixerTest, ResultsWithDisplayIndex) {
   RunQuery();
 
   EXPECT_EQ(
-      "app5,app0,omnibox0,app1,omnibox1,app2,omnibox2,omnibox3,playstore0,"
-      "playstore1",
+      "app5,app0,omnibox0,playstore0,app1,omnibox1,playstore1,app2,omnibox2,"
+      "omnibox3",
       GetResults());
 }