[CrOS Settings] Use strict weak ordering when sorting results

Before this change, we arbitrarily returned "true" for search result
items which were deemed equal to each other, but this was incorrect
because the comparison function is testing whether the first item is
less than the second item.

This CL updates this function so that it returns false in this case,
which induces a strict weak ordering of results.

Fixed: 1090184
Change-Id: I763112cef36d68731a88fd8a3696a800292899a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225930
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774366}

chrome/browser/ui/webui/settings/chromeos/search/search_handler.cc

@@ -20,44 +20,6 @@ namespace chromeos {
 namespace settings {
 namespace {
 
-// Returns true if |first| should be ranked before |second|.
-bool CompareSearchResults(const mojom::SearchResultPtr& first,
-                          const mojom::SearchResultPtr& second) {
-  // Compute the difference between the results' default rankings. Note that
-  // kHigh is declared before kMedium which is declared before kLow, so a
-  // negative value indicates that |first| is ranked higher than |second| and a
-  // positive value indicates that |second| is ranked higher than |first|.
-  int32_t default_rank_diff = static_cast<int32_t>(first->default_rank) -
-                              static_cast<int32_t>(second->default_rank);
-  if (default_rank_diff < 0)
-    return true;
-  if (default_rank_diff > 0)
-    return false;
-
-  // At this point, the default ranks are equal, so compare relevance scores. A
-  // higher relevance score indicates a better text match, so the reverse is
-  // true this time.
-  if (first->relevance_score > second->relevance_score)
-    return true;
-  if (first->relevance_score < second->relevance_score)
-    return false;
-
-  // Default rank and relevance scores are equal, so prefer the result which is
-  // higher on the hierarchy. kSection is declared before kSubpage which is
-  // declared before kSetting, so follow the same pattern from default ranks
-  // above.
-  int32_t type_diff =
-      static_cast<int32_t>(first->type) - static_cast<int32_t>(second->type);
-  if (type_diff < 0)
-    return true;
-  if (type_diff > 0)
-    return false;
-
-  // If still equal at this point, arbitrarily choose than the |first| is ranked
-  // before |second|.
-  return true;
-}
-
 bool ContainsSectionResult(const std::vector<mojom::SearchResultPtr>& results,
                            mojom::Section section) {
   return std::find_if(
@@ -300,5 +262,35 @@ std::string SearchHandler::GetModifiedUrl(const SearchConcept& concept,
       concept.type, concept.id, concept.url_path_with_parameters);
 }
 
+// static
+bool SearchHandler::CompareSearchResults(const mojom::SearchResultPtr& first,
+                                         const mojom::SearchResultPtr& second) {
+  // Compute the difference between the results' default rankings. Note that
+  // kHigh is declared before kMedium which is declared before kLow, so a
+  // negative value indicates that |first| is ranked higher than |second| and a
+  // positive value indicates that |second| is ranked higher than |first|.
+  int32_t default_rank_diff = static_cast<int32_t>(first->default_rank) -
+                              static_cast<int32_t>(second->default_rank);
+  if (default_rank_diff < 0)
+    return true;
+  if (default_rank_diff > 0)
+    return false;
+
+  // At this point, the default ranks are equal, so compare relevance scores. A
+  // higher relevance score indicates a better text match, so the reverse is
+  // true this time.
+  if (first->relevance_score > second->relevance_score)
+    return true;
+  if (first->relevance_score < second->relevance_score)
+    return false;
+
+  // Default rank and relevance scores are equal, so prefer the result which is
+  // higher on the hierarchy. kSection is declared before kSubpage which is
+  // declared before kSetting, so follow the same pattern from default ranks
+  // above. Note that if the types are equal, this will return false, which
+  // induces a strict weak ordering.
+  return static_cast<int32_t>(first->type) - static_cast<int32_t>(second->type);
+}
+
 }  // namespace settings
 }  // namespace chromeos

chrome/browser/ui/webui/settings/chromeos/search/search_handler.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/optional.h"
 #include "chrome/browser/chromeos/local_search_service/index.h"
 #include "chrome/browser/ui/webui/settings/chromeos/search/search.mojom.h"
@@ -60,6 +61,8 @@ class SearchHandler : public mojom::SearchHandler {
               SearchCallback callback) override;
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SearchHandlerTest, CompareIdenticalResults);
+
   std::vector<mojom::SearchResultPtr> GenerateSearchResultsArray(
       const std::vector<local_search_service::Result>&
           local_search_service_results,
@@ -87,6 +90,10 @@ class SearchHandler : public mojom::SearchHandler {
   std::string GetModifiedUrl(const SearchConcept& concept,
                              mojom::Section section) const;
 
+  // Returns true if |first| should be ranked before |second|.
+  static bool CompareSearchResults(const mojom::SearchResultPtr& first,
+                                   const mojom::SearchResultPtr& second);
+
   SearchTagRegistry* search_tag_registry_;
   OsSettingsSections* sections_;
   Hierarchy* hierarchy_;

chrome/browser/ui/webui/settings/chromeos/search/search_handler_unittest.cc

@@ -55,6 +55,17 @@ const std::vector<SearchConcept>& GetPrintingSearchConcepts() {
   return *tags;
 }
 
+// Creates a result with some default values.
+mojom::SearchResultPtr CreateDummyResult() {
+  return mojom::SearchResult::New(
+      /*result_text=*/base::string16(), /*url=*/"",
+      mojom::SearchResultIcon::kPrinter, /*relevance_score=*/0.5,
+      /*hierarchy_strings=*/std::vector<base::string16>(),
+      mojom::SearchResultDefaultRank::kMedium,
+      mojom::SearchResultType::kSection,
+      mojom::SearchResultIdentifier::NewSection(mojom::Section::kPrinting));
+}
+
 }  // namespace
 
 class SearchHandlerTest : public testing::Test {
@@ -193,5 +204,17 @@ TEST_F(SearchHandlerTest, DefaultRank) {
             search_results[2]->result_text);
 }
 
+// Regression test for https://crbug.com/1090184.
+TEST_F(SearchHandlerTest, CompareIdenticalResults) {
+  // Create two equal dummy results.
+  mojom::SearchResultPtr a = CreateDummyResult();
+  mojom::SearchResultPtr b = CreateDummyResult();
+
+  // CompareSearchResults() returns whether |a| < |b|; since they are equal, it
+  // should return false regardless of the order of parameters.
+  EXPECT_FALSE(SearchHandler::CompareSearchResults(a, b));
+  EXPECT_FALSE(SearchHandler::CompareSearchResults(b, a));
+}
+
 }  // namespace settings
 }  // namespace chromeos