app_list: Fix the missing default search result

- Add a equivalent_result_id field to search result;
- OmniboxResult uses stripped search url as its id;
- AnswerCardResult sets equivalent_result_id to a stripped search
  url that is used by search-what-user-typed omnibox result;
- SearchAnswerCardView use the equivalent_result_id to delete
  relevant result when the card is loaded;
- Remove the unused comparable_id from ChromeSearchResult;

Bug: 906890
Test: SearchResultAnswerCardViewTest.RemoveEquivalent
Change-Id: I16529724c8a86790ed1b5366095e2701118f9231
Reviewed-on: https://chromium-review.googlesource.com/c/1351714Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Weidong Guo <weidongg@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611390}

ash/app_list/model/search/search_model.cc

@@ -101,4 +101,14 @@ void SearchModel::DeleteAllResults() {
   PublishResults(std::vector<std::unique_ptr<SearchResult>>());
 }
 
+void SearchModel::DeleteResultById(const std::string& id) {
+  for (size_t i = 0; i < results_->item_count(); ++i) {
+    SearchResult* result = results_->GetItemAt(i);
+    if (result->id() == id) {
+      results_->DeleteAt(i);
+      break;
+    }
+  }
+}
+
 }  // namespace app_list

ash/app_list/model/search/search_model.h

@@ -61,6 +61,9 @@ class APP_LIST_MODEL_EXPORT SearchModel {
   // Deletes all search results. This is used in profile switches.
   void DeleteAllResults();
 
+  // Delete result by the given id.
+  void DeleteResultById(const std::string& id);
+
  private:
   std::unique_ptr<SearchBoxModel> search_box_;
   std::unique_ptr<SearchResults> results_;

ash/app_list/model/search/search_result.h

@@ -83,6 +83,13 @@ class APP_LIST_MODEL_EXPORT SearchResult {
   const base::Optional<GURL>& query_url() const { return metadata_->query_url; }
   void set_query_url(const GURL& url) { metadata_->query_url = url; }
 
+  const base::Optional<std::string>& equivalent_result_id() const {
+    return metadata_->equivalent_result_id;
+  }
+  void set_equivalent_result_id(const std::string& equivalent_result_id) {
+    metadata_->equivalent_result_id = equivalent_result_id;
+  }
+
   const std::string& id() const { return metadata_->id; }
 
   double display_score() const { return metadata_->display_score; }

ash/app_list/views/search_result_answer_card_view.cc

@@ -275,6 +275,11 @@ class SearchResultAnswerCardView::SearchAnswerContainerView
     if (!has_children()) {
       AddChildView(content_view);
       ExcludeCardFromEventHandling(contents_->GetView()->native_view());
+
+      if (search_result_->equivalent_result_id().has_value()) {
+        view_delegate_->GetSearchModel()->DeleteResultById(
+            search_result_->equivalent_result_id().value());
+      }
     }
     SetPreferredSize(content_view->GetPreferredSize());
     container_->Update();

ash/app_list/views/search_result_answer_card_view_unittest.cc

@@ -51,16 +51,18 @@ class SearchResultAnswerCardViewTest : public views::ViewsTestBase {
   }
 
  protected:
-  void SetUpSearchResult() {
+  std::unique_ptr<TestSearchResult> CreateAnswerCardResult() {
     const GURL kFakeQueryUrl = GURL("https://www.google.com/coac?q=fake");
-    SearchModel::SearchResults* results = GetResults();
-    std::unique_ptr<TestSearchResult> result =
-        std::make_unique<TestSearchResult>();
+    auto result = std::make_unique<TestSearchResult>();
     result->set_display_type(ash::SearchResultDisplayType::kCard);
     result->set_title(base::UTF8ToUTF16(kResultTitle));
     result->set_display_score(kDisplayScore);
     result->set_query_url(kFakeQueryUrl);
-    results->Add(std::move(result));
+    return result;
+  }
+
+  void SetUpSearchResult() {
+    GetResults()->Add(CreateAnswerCardResult());
 
     // Adding results will schedule Update().
     view_delegate_.fake_navigable_contents_factory()
@@ -103,6 +105,8 @@ class SearchResultAnswerCardViewTest : public views::ViewsTestBase {
     result_container_view_->child_at(0)->GetAccessibleNodeData(node_data);
   }
 
+  AppListTestViewDelegate& view_delegate() { return view_delegate_; }
+
  private:
   AppListTestViewDelegate view_delegate_;
 
@@ -147,5 +151,35 @@ TEST_F(SearchResultAnswerCardViewTest, DeleteResult) {
   EXPECT_EQ(0, GetContainerScore());
 }
 
+TEST_F(SearchResultAnswerCardViewTest, RemoveEquivalent) {
+  // Ensures no results.
+  DeleteResult();
+
+  // Creates a result that will be removed later when answer card loads.
+  constexpr char kEquivalentResultId[] = "equivalent-id";
+  auto result = std::make_unique<TestSearchResult>();
+  result->set_result_id(kEquivalentResultId);
+  result->set_display_type(ash::SearchResultDisplayType::kList);
+  result->set_display_score(kDisplayScore);
+  GetResults()->Add(std::move(result));
+
+  // Creates an answer card result and associated with an equivalent result id.
+  result = CreateAnswerCardResult();
+  result->set_equivalent_result_id(kEquivalentResultId);
+  GetResults()->Add(std::move(result));
+
+  EXPECT_EQ(2u, GetResults()->item_count());
+  EXPECT_TRUE(
+      view_delegate().GetSearchModel()->FindSearchResult(kEquivalentResultId));
+
+  // Wait for the answer card result to load.
+  RunPendingMessages();
+
+  // Equivalent result should be removed.
+  EXPECT_EQ(1u, GetResults()->item_count());
+  EXPECT_FALSE(
+      view_delegate().GetSearchModel()->FindSearchResult(kEquivalentResultId));
+}
+
 }  // namespace test
 }  // namespace app_list

ash/public/interfaces/app_list.mojom

@@ -67,6 +67,12 @@ struct SearchResultMetadata {
                              // (e.g. displaying inline web contents) is
                              // dependent on the result type.
 
+  string? equivalent_result_id;  // An optional id that identifies an equivalent
+                                 // result to this result. Answer card result
+                                 // has this set to remove the equivalent
+                                 // omnibox search-what-you-typed result when
+                                 // there is an answer card for the query.
+
   gfx.mojom.ImageSkia? icon;  // The icon of this result.
   gfx.mojom.ImageSkia? chip_icon;  // The icon of this result in a smaller
                                    // dimension to be rendered in suggestion

chrome/browser/ui/app_list/search/answer_card/answer_card_result.cc

@@ -14,20 +14,23 @@ AnswerCardResult::AnswerCardResult(Profile* profile,
                                    const GURL& potential_card_url,
                                    const GURL& search_result_url,
                                    const GURL& stripped_search_result_url)
-    : profile_(profile), list_controller_(list_controller) {
+    : profile_(profile),
+      list_controller_(list_controller),
+      search_result_url_(search_result_url) {
   DCHECK(!stripped_search_result_url.is_empty());
   SetDisplayType(ash::SearchResultDisplayType::kCard);
   SetResultType(ash::SearchResultType::kAnswerCard);
   SetQueryUrl(potential_card_url);
-  set_id(search_result_url.spec());
-  set_comparable_id(stripped_search_result_url.spec());
+  SetEquivalentResutlId(stripped_search_result_url.spec());
+  set_id(potential_card_url.spec());
   set_relevance(1);
 }
 
 AnswerCardResult::~AnswerCardResult() = default;
 
 void AnswerCardResult::Open(int event_flags) {
-  list_controller_->OpenURL(profile_, GURL(id()), ui::PAGE_TRANSITION_GENERATED,
+  list_controller_->OpenURL(profile_, search_result_url_,
+                            ui::PAGE_TRANSITION_GENERATED,
                             ui::DispositionFromEventFlags(event_flags));
   RecordHistogram(ANSWER_CARD);
 }

chrome/browser/ui/app_list/search/answer_card/answer_card_result.h

@@ -29,9 +29,12 @@ class AnswerCardResult : public ChromeSearchResult {
 
   void Open(int event_flags) override;
 
+  const GURL& search_result_url() const { return search_result_url_; }
+
  private:
   Profile* const profile_;                            // Unowned
   AppListControllerDelegate* const list_controller_;  // Unowned
+  GURL search_result_url_;
 
   DISALLOW_COPY_AND_ASSIGN(AnswerCardResult);
 };

chrome/browser/ui/app_list/search/answer_card/answer_card_result_unittest.cc

@@ -62,8 +62,8 @@ TEST_F(AnswerCardResultTest, Basic) {
   std::unique_ptr<AnswerCardResult> result = CreateResult(
       GURL(kCardUrl), GURL(kSearchResultUrl), GURL(kStrippedSearchResultUrl));
 
-  EXPECT_EQ(kSearchResultUrl, result->id());
-  EXPECT_EQ(kStrippedSearchResultUrl, result->comparable_id());
+  EXPECT_EQ(kCardUrl, result->id());
+  EXPECT_EQ(kStrippedSearchResultUrl, result->equivalent_result_id().value());
   EXPECT_EQ(kCardUrl, result->query_url()->spec());
   EXPECT_EQ(ash::SearchResultDisplayType::kCard, result->display_type());
   EXPECT_EQ(1, result->relevance());

chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider_unittest.cc

@@ -18,11 +18,13 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/app_list/app_list_test_util.h"
+#include "chrome/browser/ui/app_list/search/answer_card/answer_card_result.h"
 #include "chrome/browser/ui/app_list/search/answer_card/answer_card_search_provider.h"
-#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
 #include "chrome/browser/ui/app_list/test/fake_app_list_model_updater.h"
 #include "chrome/browser/ui/app_list/test/test_app_list_controller_delegate.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/omnibox/browser/autocomplete_input.h"
+#include "components/omnibox/browser/autocomplete_match.h"
 #include "components/search_engines/template_url_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -35,9 +37,9 @@ constexpr char kQueryBase[] = "http://beasts.org/search";
 constexpr char kSomeParam[] = "&some_param=some_value";
 constexpr char kDogQuery[] = "dog";
 constexpr char kSharkQuery[] = "shark";
-constexpr char kDogCardId[] =
+constexpr char kDogSearchUrl[] =
     "https://www.google.com/search?q=dog&sourceid=chrome&ie=UTF-8";
-constexpr char kSharkCardId[] =
+constexpr char kSharkSearchUrl[] =
     "https://www.google.com/search?q=shark&sourceid=chrome&ie=UTF-8";
 
 GURL GetAnswerCardUrl(const std::string& query) {
@@ -93,6 +95,13 @@ class AnswerCardSearchProviderTest : public AppListTestBase {
         profile_.get(), model_updater_.get(), nullptr);
   }
 
+  GURL GetStrippedSearchUrl(const GURL& search_result_url) {
+    return AutocompleteMatch::GURLToStrippedGURL(
+        GURL(search_result_url), AutocompleteInput(),
+        TemplateURLServiceFactory::GetForProfile(profile_.get()),
+        base::string16() /* keyword */);
+  }
+
  private:
   std::unique_ptr<FakeAppListModelUpdater> model_updater_;
   std::unique_ptr<AnswerCardSearchProvider> provider_;
@@ -109,13 +118,19 @@ class AnswerCardSearchProviderTest : public AppListTestBase {
 TEST_F(AnswerCardSearchProviderTest, Start) {
   provider()->Start(base::UTF8ToUTF16(kDogQuery));
   ASSERT_EQ(1u, results().size());
-  EXPECT_EQ(kDogCardId, results()[0]->id());
-  EXPECT_EQ(GetAnswerCardUrl(kDogQuery), results()[0]->query_url()->spec());
+  AnswerCardResult* result = static_cast<AnswerCardResult*>(results()[0].get());
+  EXPECT_EQ(GURL(kDogSearchUrl), result->search_result_url());
+  EXPECT_EQ(GetStrippedSearchUrl(GURL(kDogSearchUrl)),
+            result->equivalent_result_id().value());
+  EXPECT_EQ(GetAnswerCardUrl(kDogQuery), result->query_url()->spec());
 
   provider()->Start(base::UTF8ToUTF16(kSharkQuery));
   ASSERT_EQ(1u, results().size());
-  EXPECT_EQ(kSharkCardId, results()[0]->id());
-  EXPECT_EQ(GetAnswerCardUrl(kSharkQuery), results()[0]->query_url()->spec());
+  result = static_cast<AnswerCardResult*>(results()[0].get());
+  EXPECT_EQ(GURL(kSharkSearchUrl), result->search_result_url());
+  EXPECT_EQ(GetStrippedSearchUrl(GURL(kSharkSearchUrl)),
+            result->equivalent_result_id().value());
+  EXPECT_EQ(GetAnswerCardUrl(kSharkQuery), result->query_url()->spec());
 }
 
 // Queries to non-Google search engines are ignored.

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

@@ -121,6 +121,14 @@ void ChromeSearchResult::SetQueryUrl(const GURL& url) {
     updater->SetSearchResultMetadata(id(), CloneMetadata());
 }
 
+void ChromeSearchResult::SetEquivalentResutlId(
+    const std::string& equivlanet_result_id) {
+  metadata_->equivalent_result_id = equivlanet_result_id;
+  auto* updater = model_updater();
+  if (updater)
+    updater->SetSearchResultMetadata(id(), CloneMetadata());
+}
+
 void ChromeSearchResult::SetIcon(const gfx::ImageSkia& icon) {
   icon.EnsureRepsForSupportedScales();
   metadata_->icon = icon;

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

@@ -50,12 +50,13 @@ class ChromeSearchResult {
   double display_score() const { return metadata_->display_score; }
   bool is_installing() const { return metadata_->is_installing; }
   const base::Optional<GURL>& query_url() const { return metadata_->query_url; }
+  const base::Optional<std::string>& equivalent_result_id() const {
+    return metadata_->equivalent_result_id;
+  }
   const gfx::ImageSkia& icon() const { return metadata_->icon; }
   const gfx::ImageSkia& chip_icon() const { return metadata_->chip_icon; }
   const gfx::ImageSkia& badge_icon() const { return metadata_->badge_icon; }
 
-  const std::string& comparable_id() const { return comparable_id_; }
-
   // The following methods set Chrome side data here, and call model updater
   // interface to update Ash.
   void SetTitle(const base::string16& title);
@@ -72,6 +73,7 @@ class ChromeSearchResult {
   void SetIsOmniboxSearch(bool is_omnibox_search);
   void SetIsInstalling(bool is_installing);
   void SetQueryUrl(const GURL& url);
+  void SetEquivalentResutlId(const std::string& equivlanet_result_id);
   void SetIcon(const gfx::ImageSkia& icon);
   void SetChipIcon(const gfx::ImageSkia& icon);
   void SetBadgeIcon(const gfx::ImageSkia& badge_icon);
@@ -122,19 +124,12 @@ class ChromeSearchResult {
  protected:
   // These id setters should be called in derived class constructors only.
   void set_id(const std::string& id) { metadata_->id = id; }
-  void set_comparable_id(const std::string& comparable_id) {
-    comparable_id_ = comparable_id;
-  }
 
   // Get the context menu of a certain search result. This could be different
   // for different kinds of items.
   virtual app_list::AppContextMenu* GetAppContextMenu();
 
  private:
-  // ID that can be compared across results from different providers to remove
-  // duplicates. May be empty, in which case |id_| will be used for comparison.
-  std::string comparable_id_;
-
   // The relevance of this result to the search, which is determined by the
   // search query. It's used for sorting when we publish the results to the
   // SearchModel in Ash. We'll update metadata_->display_score based on the

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

@@ -18,14 +18,6 @@
 
 namespace app_list {
 
-namespace {
-
-const std::string& GetComparableId(const ChromeSearchResult& result) {
-  return !result.comparable_id().empty() ? result.comparable_id() : result.id();
-}
-
-}  // namespace
-
 Mixer::SortData::SortData() : result(nullptr), score(0.0) {}
 
 Mixer::SortData::SortData(ChromeSearchResult* result, double score)
@@ -149,7 +141,7 @@ void Mixer::RemoveDuplicates(SortedResults* results) {
 
   std::set<std::string> id_set;
   for (const SortData& sort_data : *results) {
-    if (!id_set.insert(GetComparableId(*sort_data.result)).second)
+    if (!id_set.insert(sort_data.result->id()).second)
       continue;
 
     final.emplace_back(sort_data);

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

@@ -133,8 +133,7 @@ OmniboxResult::OmniboxResult(Profile* profile,
     autocomplete_controller_->UpdateMatchDestinationURL(
         *match_.search_terms_args, &match_);
   }
-  set_id(match_.destination_url.spec());
-  set_comparable_id(match_.stripped_destination_url.spec());
+  set_id(match_.stripped_destination_url.spec());
   SetResultType(ash::SearchResultType::kOmnibox);
 
   // Derive relevance from omnibox relevance and normalize it to [0, 1].