CrOS: Adding new SearchResult flags to filter out Dinner For One.

Adding this filter in search_result_tile_item_list_view will allow for
future-proof placement of dinner for one and other future policy tile
apps.

This change re implements cl/1687296, which was reverted during bisecting
a flickering UI bug.

Bug: 981023

Change-Id: I97b70c297a686ad084f4b93b6a96fd722948bbdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729914
Commit-Queue: Cameron Womack <camw@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683325}

ash/app_list/model/search/search_model.cc

@@ -53,9 +53,8 @@ std::vector<SearchResult*> SearchModel::FilterSearchResultsByFunction(
     if (matches.size() == max_results)
       break;
     SearchResult* item = results->GetItemAt(i);
-    if (result_filter.Run(*item)) {
+    if (result_filter.Run(*item))
       matches.push_back(item);
-    }
   }
   return matches;
 }

ash/app_list/views/search_result_tile_item_list_view.cc

@@ -254,19 +254,20 @@ std::vector<SearchResult*> SearchResultTileItemListView::GetDisplayResults() {
   base::string16 query;
   base::TrimWhitespace(raw_query, base::TRIM_ALL, &query);
 
-  // We ask for |max_search_result_tiles_| total results, and we prefer
-  // reinstall candidates if appropriate. we fetch |reinstall_results| first,
-  // and front-fill the rest from the regular result types.
-  auto reinstall_filter =
+  // We ask for |max_search_result_tiles_| policy tile results first,
+  // then add them to their preferred position in the tile list if found.
+  auto policy_tiles_filter =
       base::BindRepeating([](const SearchResult& r) -> bool {
-        return r.display_type() ==
-                   ash::SearchResultDisplayType::kRecommendation &&
-               r.result_type() == ash::SearchResultType::kPlayStoreReinstallApp;
+        return r.display_location() ==
+                   ash::SearchResultDisplayLocation::kTileListContainer &&
+               r.display_index() != ash::SearchResultDisplayIndex::kUndefined &&
+               r.display_type() ==
+                   ash::SearchResultDisplayType::kRecommendation;
       });
-  std::vector<SearchResult*> reinstall_results =
+  std::vector<SearchResult*> policy_tiles_results =
       is_app_reinstall_recommendation_enabled_ && query.empty()
           ? SearchModel::FilterSearchResultsByFunction(
-                results(), reinstall_filter, max_search_result_tiles_)
+                results(), policy_tiles_filter, max_search_result_tiles_)
           : std::vector<SearchResult*>();
 
   SearchResult::DisplayType display_type =
@@ -274,10 +275,11 @@ std::vector<SearchResult*> SearchResultTileItemListView::GetDisplayResults() {
           ? (query.empty() ? ash::SearchResultDisplayType::kRecommendation
                            : ash::SearchResultDisplayType::kTile)
           : ash::SearchResultDisplayType::kTile;
-  size_t display_num = max_search_result_tiles_ - reinstall_results.size();
+  size_t display_num = max_search_result_tiles_ - policy_tiles_results.size();
 
-  // Do not display the continue reading app in the search result list.
-  auto non_reinstall_filter = base::BindRepeating(
+  // Do not display the repeat reinstall results or continue reading app in the
+  // search result list.
+  auto non_policy_tiles_filter = base::BindRepeating(
       [](const SearchResult::DisplayType& display_type,
          const SearchResult& r) -> bool {
         return r.display_type() == display_type &&
@@ -288,11 +290,23 @@ std::vector<SearchResult*> SearchResultTileItemListView::GetDisplayResults() {
       display_type);
   std::vector<SearchResult*> display_results =
       SearchModel::FilterSearchResultsByFunction(
-          results(), non_reinstall_filter, display_num);
-
-  // Append the reinstalls to the display results.
-  display_results.insert(display_results.end(), reinstall_results.begin(),
-                         reinstall_results.end());
+          results(), non_policy_tiles_filter, display_num);
+
+  // Policy tile results will be appended to the final tiles list
+  // based on their specified index. If the requested index is out of
+  // range of the current list, the result will be appended to the back.
+  std::sort(policy_tiles_results.begin(), policy_tiles_results.end(),
+            [](const SearchResult* r1, const SearchResult* r2) -> bool {
+              return r1->display_index() < r2->display_index();
+            });
+  for (auto* result : policy_tiles_results) {
+    if (result->display_index() > display_results.size() - 1) {
+      display_results.emplace_back(result);
+    } else {
+      display_results.emplace(display_results.begin() + result->display_index(),
+                              result);
+    }
+  }
 
   return display_results;
 }

ash/app_list/views/search_result_tile_item_list_view_unittest.cc

@@ -29,6 +29,10 @@ namespace {
 constexpr size_t kInstalledApps = 4;
 constexpr size_t kPlayStoreApps = 2;
 constexpr size_t kRecommendedApps = 1;
+
+// used to test when multiple chips with specified display
+// indexes have been added
+constexpr size_t kRecommendedAppsWithDisplayIndex = 3;
 }  // namespace
 
 class SearchResultTileItemListViewTest
@@ -125,6 +129,9 @@ class SearchResultTileItemListViewTest
         result->set_result_id("RecommendedApp " + base::NumberToString(i));
         result->set_display_type(ash::SearchResultDisplayType::kRecommendation);
         result->set_result_type(ash::SearchResultType::kPlayStoreReinstallApp);
+        result->set_display_location(
+            ash::SearchResultDisplayLocation::kTileListContainer);
+        result->set_display_index(ash::SearchResultDisplayIndex::kSixthIndex);
         result->set_title(base::ASCIIToUTF16("RecommendedApp ") +
                           base::NumberToString16(i));
         result->SetRating(1 + i);
@@ -137,6 +144,67 @@ class SearchResultTileItemListViewTest
     RunPendingMessages();
   }
 
+  void SetUpSearchResultsWithMultipleDisplayIndexesRequested() {
+    SearchModel::SearchResults* results = GetResults();
+
+    // Populate results for installed applications.
+    for (size_t i = 0; i < kInstalledApps; ++i) {
+      std::unique_ptr<TestSearchResult> result =
+          std::make_unique<TestSearchResult>();
+      result->set_result_id("InstalledApp " + base::NumberToString(i));
+      result->set_display_type(ash::SearchResultDisplayType::kTile);
+      result->set_result_type(ash::SearchResultType::kInstalledApp);
+      result->set_title(base::ASCIIToUTF16("InstalledApp ") +
+                        base::NumberToString16(i));
+      results->Add(std::move(result));
+    }
+
+    // Populate results for Play Store search applications.
+    if (IsPlayStoreAppSearchEnabled()) {
+      for (size_t i = 0; i < kPlayStoreApps; ++i) {
+        std::unique_ptr<TestSearchResult> result =
+            std::make_unique<TestSearchResult>();
+        result->set_result_id("PlayStoreApp " + base::NumberToString(i));
+        result->set_display_type(ash::SearchResultDisplayType::kTile);
+        result->set_result_type(ash::SearchResultType::kPlayStoreApp);
+        result->set_title(base::ASCIIToUTF16("PlayStoreApp ") +
+                          base::NumberToString16(i));
+        result->SetRating(1 + i);
+        result->SetFormattedPrice(base::ASCIIToUTF16("Price ") +
+                                  base::NumberToString16(i));
+        results->Add(std::move(result));
+      }
+    }
+
+    const ash::SearchResultDisplayIndex
+        display_indexes[kRecommendedAppsWithDisplayIndex] = {
+            ash::SearchResultDisplayIndex::kFourthIndex,
+            ash::SearchResultDisplayIndex::kFifthIndex,
+            ash::SearchResultDisplayIndex::kSixthIndex,
+        };
+
+    if (IsReinstallAppRecommendationEnabled()) {
+      for (size_t i = 0; i < kRecommendedAppsWithDisplayIndex; ++i) {
+        std::unique_ptr<TestSearchResult> result =
+            std::make_unique<TestSearchResult>();
+        result->set_result_id("RecommendedApp " + base::NumberToString(i));
+        result->set_display_type(ash::SearchResultDisplayType::kRecommendation);
+        result->set_result_type(ash::SearchResultType::kPlayStoreReinstallApp);
+        result->set_display_location(
+            ash::SearchResultDisplayLocation::kTileListContainer);
+        result->set_display_index(display_indexes[i]);
+        result->set_title(base::ASCIIToUTF16("RecommendedApp ") +
+                          base::NumberToString16(i));
+        result->SetRating(1 + i);
+        results->AddAt(result->display_index(), std::move(result));
+      }
+    }
+
+    // Adding results calls SearchResultContainerView::ScheduleUpdate().
+    // It will post a delayed task to update the results and relayout.
+    RunPendingMessages();
+  }
+
   size_t GetOpenResultCount(int ranking) {
     return view_delegate_.open_search_result_counts()[ranking];
   }
@@ -244,6 +312,31 @@ TEST_P(SearchResultTileItemListViewTest, Basic) {
   }
 }
 
+// Tests that when multiple apps with specified indexes are added to the app
+// results list, they are found at the indexes they requested.
+TEST_P(SearchResultTileItemListViewTest, TestRecommendations) {
+  if (!IsReinstallAppRecommendationEnabled())
+    return;
+
+  CreateSearchResultTileItemListView();
+  SetUpSearchResultsWithMultipleDisplayIndexesRequested();
+
+  const size_t child_step = 2;
+
+  size_t first_index = kInstalledApps + kRecommendedAppsWithDisplayIndex;
+
+  size_t stepper = IsPlayStoreAppSearchEnabled() ? 3 : 2;
+  for (size_t i = 0; i < stepper; ++i) {
+    ui::AXNodeData node_data;
+    view()->children()[first_index + i * child_step]->GetAccessibleNodeData(
+        &node_data);
+    EXPECT_EQ(ax::mojom::Role::kButton, node_data.role);
+    EXPECT_EQ("RecommendedApp " + base::NumberToString(i) + ", Star rating " +
+                  base::NumberToString(i + 1) + ".0, App recommendation",
+              node_data.GetStringAttribute(ax::mojom::StringAttribute::kName));
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(,
                          SearchResultTileItemListViewTest,
                          testing::ValuesIn({std::make_pair(false, false),

ash/app_list/views/suggestion_chip_container_view.cc

@@ -28,16 +28,10 @@ namespace {
 // The spacing between chips.
 constexpr int kChipSpacing = 8;
 
-// Sort suggestion chip results by |display_index| value in ascending order.
-bool IndexOrdering(const SearchResult* result1, const SearchResult* result2) {
-  return result1->display_index() < result2->display_index();
-}
-
-bool IsPolicySuggestionChip(const SearchResult* result) {
-  return result->display_location() ==
+bool IsPolicySuggestionChip(const SearchResult& result) {
+  return result.display_location() ==
              ash::SearchResultDisplayLocation::kSuggestionChipContainer &&
-         result->display_index() !=
-             ash::SearchResultDisplayIndex::kPlacementUndefined;
+         result.display_index() != ash::SearchResultDisplayIndex::kUndefined;
 }
 
 }  // namespace
@@ -87,7 +81,7 @@ int SuggestionChipContainerView::DoUpdate() {
   // Filter out priority suggestion chips with a non-default value
   // for |display_index|.
   auto filter_indexed_policy_chips = [](const SearchResult& r) -> bool {
-    return IsPolicySuggestionChip(&r);
+    return IsPolicySuggestionChip(r);
   };
   std::vector<SearchResult*> indexed_policy_results =
       SearchModel::FilterSearchResultsByFunction(
@@ -95,7 +89,9 @@ int SuggestionChipContainerView::DoUpdate() {
           AppListConfig::instance().num_start_page_tiles());
 
   std::sort(indexed_policy_results.begin(), indexed_policy_results.end(),
-            &IndexOrdering);
+            [](const SearchResult* r1, const SearchResult* r2) -> bool {
+              return r1->display_index() < r2->display_index();
+            });
 
   // Need to filter out kArcAppShortcut since it will be confusing to users
   // if shortcuts are displayed as suggestion chips. Also filter out any
@@ -104,7 +100,7 @@ int SuggestionChipContainerView::DoUpdate() {
     return r.display_type() == ash::SearchResultDisplayType::kRecommendation &&
            r.result_type() != ash::SearchResultType::kPlayStoreReinstallApp &&
            r.result_type() != ash::SearchResultType::kArcAppShortcut &&
-           !IsPolicySuggestionChip(&r);
+           !IsPolicySuggestionChip(r);
   };
   std::vector<SearchResult*> display_results =
       SearchModel::FilterSearchResultsByFunction(
@@ -115,9 +111,8 @@ int SuggestionChipContainerView::DoUpdate() {
   // Update display results list by placing policy result chips at their
   // specified |display_index|.
   for (auto* result : indexed_policy_results) {
-    std::vector<SearchResult*>::iterator desired_index =
-        display_results.begin() + result->display_index();
-    display_results.emplace(desired_index, result);
+    display_results.emplace(display_results.begin() + result->display_index(),
+                            result);
   }
 
   // Update search results here, but wait until layout to add them as child

ash/public/cpp/app_list/app_list_types.h

@@ -143,7 +143,7 @@ enum SearchResultDisplayType {
 enum SearchResultDisplayLocation {
   kSuggestionChipContainer,
   kTileListContainer,
-  kUnknown,
+  kPlacementUndefined,
 };
 
 // Which index in the UI container should the result be placed in.
@@ -154,7 +154,7 @@ enum SearchResultDisplayIndex {
   kFourthIndex,
   kFifthIndex,
   kSixthIndex,
-  kPlacementUndefined,
+  kUndefined,
 };
 
 // Actions for OmniBox zero state suggestion.
@@ -261,11 +261,10 @@ struct ASH_PUBLIC_EXPORT SearchResultMetadata {
 
   // Which UI container should the result be displayed in.
   SearchResultDisplayLocation display_location =
-      SearchResultDisplayLocation::kUnknown;
+      SearchResultDisplayLocation::kPlacementUndefined;
 
   // Which index in the UI container should the result be placed in.
-  SearchResultDisplayIndex display_index =
-      SearchResultDisplayIndex::kPlacementUndefined;
+  SearchResultDisplayIndex display_index = SearchResultDisplayIndex::kUndefined;
 
   // A score to determine the result display order.
   double display_score = 0;

chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.cc

@@ -33,6 +33,8 @@ ArcAppReinstallAppResult::ArcAppReinstallAppResult(
   SetResultType(ash::SearchResultType::kPlayStoreReinstallApp);
   SetTitle(base::UTF8ToUTF16(mojom_data->title));
   SetDisplayType(ash::SearchResultDisplayType::kRecommendation);
+  SetDisplayLocation(ash::SearchResultDisplayLocation::kTileListContainer);
+  SetDisplayIndex(ash::SearchResultDisplayIndex::kSixthIndex);
   set_relevance(kAppReinstallRelevance);
   SetNotifyVisibilityChange(true);
   SetIcon(skia_icon);