Looks up keyword search terms table for local history ZPS

Swaps the existing implementation with one that queries the keyword search
terms table in the in-memory URLDatabase instead of the URLs table.

- This table is smaller so querying it is likely faster.
- The step to parse URLs is avoided.
- Is more consistent with how SearchProvider offers local search history
  matches.

Also improves and re-enables LocalHistoryZeroSuggestProviderTest.Delete
which was failing for Linux ChromiumOS MSAN, likely due to using an
uninitialized |history_db_task_id_| which no longer exists.

Bug: 996516,1010944
Change-Id: I183fb7367285dcb453fb42e46651550baf0e3036
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1835182
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704062}

components/history/core/browser/url_database.cc

@@ -669,6 +669,19 @@ bool URLDatabase::DeleteKeywordSearchTerm(const base::string16& term) {
   return statement.Run();
 }
 
+bool URLDatabase::DeleteKeywordSearchTermForNormalizedTerm(
+    KeywordID keyword_id,
+    const base::string16& normalized_term) {
+  sql::Statement statement(
+      GetDB().GetCachedStatement(SQL_FROM_HERE,
+                                 "DELETE FROM keyword_search_terms WHERE "
+                                 "keyword_id = ? AND normalized_term=?"));
+  statement.BindInt64(0, keyword_id);
+  statement.BindString16(1, normalized_term);
+
+  return statement.Run();
+}
+
 bool URLDatabase::DeleteKeywordSearchTermForURL(URLID url_id) {
   sql::Statement statement(GetDB().GetCachedStatement(
       SQL_FROM_HERE, "DELETE FROM keyword_search_terms WHERE url_id=?"));

components/history/core/browser/url_database.h

@@ -233,6 +233,11 @@ class URLDatabase {
   // Deletes all searches matching |term|.
   bool DeleteKeywordSearchTerm(const base::string16& term);
 
+  // Deletes any search corresponding to |normalized_term|.
+  bool DeleteKeywordSearchTermForNormalizedTerm(
+      KeywordID keyword_id,
+      const base::string16& normalized_term);
+
   // Deletes any search corresponding to |url_id|.
   bool DeleteKeywordSearchTermForURL(URLID url_id);
 

components/omnibox/browser/local_history_zero_suggest_provider.cc

@@ -15,8 +15,8 @@
 #include "base/trace_event/trace_event.h"
 #include "components/google/core/common/google_util.h"
 #include "components/history/core/browser/history_database.h"
-#include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/keyword_search_term.h"
 #include "components/history/core/browser/url_database.h"
 #include "components/omnibox/browser/autocomplete_input.h"
 #include "components/omnibox/browser/autocomplete_match.h"
@@ -36,68 +36,6 @@ namespace {
 // Default relevance for the LocalHistoryZeroSuggestProvider query suggestions.
 const int kLocalHistoryZeroSuggestRelevance = 500;
 
-// ProviderHistoryDBTask wraps two non-null callbacks into a HistoryDBTask to
-// passed to HistoryService::ScheduleDBTask. |request_callback| gets invoked on
-// the history backend thread when the on-disk history database becomes
-// available in order to query the database and populate |url_matches_|.
-// |result_callback_| is then invoked with |url_matches_| back on the main
-// thread.
-class ProviderHistoryDBTask : public history::HistoryDBTask {
- public:
-  using RequestCallback =
-      base::OnceCallback<history::URLRows(history::URLDatabase* url_db)>;
-  using ResultCallback = base::OnceCallback<void(const history::URLRows&)>;
-
-  ProviderHistoryDBTask(RequestCallback request_callback,
-                        ResultCallback result_callback)
-      : request_callback_(std::move(request_callback)),
-        result_callback_(std::move(result_callback)) {
-    DCHECK(!request_callback_.is_null());
-    DCHECK(!result_callback_.is_null());
-  }
-
-  ~ProviderHistoryDBTask() override {}
-
- private:
-  // history::HistoryDBTask
-  bool RunOnDBThread(history::HistoryBackend* history_backend,
-                     history::HistoryDatabase* db) override {
-    // Query the database.
-    if (db)
-      url_matches_ = std::move(request_callback_).Run(db);
-    return true;
-  }
-
-  void DoneRunOnMainThread() override {
-    // Return the query results to the originating thread.
-    std::move(result_callback_).Run(url_matches_);
-  }
-
-  RequestCallback request_callback_;
-  ResultCallback result_callback_;
-  history::URLRows url_matches_;
-
-  DISALLOW_COPY_AND_ASSIGN(ProviderHistoryDBTask);
-};
-
-// Queries |url_db| for URLs matching |google_search_url| prefix and returns
-// the results. |url_db| must not be null. May be called on the history backend
-// thread.
-history::URLRows QueryURLDatabaseOnHistoryThread(
-    const std::string& google_search_url,
-    const size_t provider_max_matches,
-    history::URLDatabase* url_db) {
-  DCHECK(url_db);
-
-  // Request 5x more URLs than the number of matches the provider intends to
-  // return hoping to have enough URLs to work with once duplicates are filtered
-  // out.
-  history::URLRows url_matches;
-  url_db->AutocompleteForPrefix(google_search_url, 5 * provider_max_matches,
-                                false, &url_matches);
-  return url_matches;
-}
-
 // Extracts the search terms from |url|. Collapses whitespaces, converts them to
 // lowercase and returns them. |template_url_service| must not be null.
 base::string16 GetSearchTermsFromURL(const GURL& url,
@@ -126,7 +64,6 @@ void LocalHistoryZeroSuggestProvider::Start(const AutocompleteInput& input,
                                             bool minimal_changes) {
   TRACE_EVENT0("omnibox", "LocalHistoryZeroSuggestProvider::Start");
 
-  history_task_tracker_.TryCancel(history_db_task_id_);
   done_ = true;
   matches_.clear();
 
@@ -159,34 +96,7 @@ void LocalHistoryZeroSuggestProvider::Start(const AutocompleteInput& input,
     return;
   }
 
-  history::HistoryService* const history_service = client_->GetHistoryService();
-  if (!history_service)
-    return;
-
-  GURL google_base_url(
-      template_url_service->search_terms_data().GoogleBaseURLValue());
-  std::string google_search_url =
-      google_util::GetGoogleSearchURL(google_base_url).spec();
-
-  // Query the in-memory URL database, if available. Otherwise, schedule a
-  // task to query the on-disk history database on the history backend thread.
-  history::URLDatabase* url_db = history_service->InMemoryDatabase();
-  if (url_db) {
-    const auto url_matches = QueryURLDatabaseOnHistoryThread(
-        google_search_url, max_matches_, url_db);
-    OnQueryURLDatabaseComplete(input, std::move(url_matches));
-  } else {
-    done_ = false;
-    history_db_task_id_ = history_service->ScheduleDBTask(
-        FROM_HERE,
-        std::make_unique<ProviderHistoryDBTask>(
-            base::BindOnce(QueryURLDatabaseOnHistoryThread, google_search_url,
-                           max_matches_),
-            base::BindOnce(
-                &LocalHistoryZeroSuggestProvider::OnQueryURLDatabaseComplete,
-                weak_ptr_factory_.GetWeakPtr(), input)),
-        &history_task_tracker_);
-  }
+  QueryURLDatabase(input);
 }
 
 void LocalHistoryZeroSuggestProvider::DeleteMatch(
@@ -201,40 +111,36 @@ void LocalHistoryZeroSuggestProvider::DeleteMatch(
     return;
   }
 
-  // Generate a Google search URL for the suggestion by replacing the search
-  // terms in the Google search URL template. Note that the suggestion's
-  // original search URL cannot be used here as it is unique due to the assisted
-  // query stats (aka AQS) query param which contains impressions of
-  // autocomplete matches shown at query submission time.
-  GURL google_search_url;
-  TemplateURLRef::SearchTermsArgs search_terms_args(match.contents);
-  const auto* default_search_provider =
-      template_url_service->GetDefaultSearchProvider();
-  const auto& search_terms_data = template_url_service->search_terms_data();
-  default_search_provider->ReplaceSearchTermsInURL(
-      default_search_provider->GenerateSearchURL(search_terms_data),
-      search_terms_args, search_terms_data, &google_search_url);
-
-  // Query the HistoryService for URLs matching the Google search URL. Note that
-  // this step is necessary here because 1) there may be matching fresh URLs
-  // that were not encountered in the suggestion creation phase due to looking
-  // up a maximum number of URLs in that phase. and 2) the performance overhead
-  // of requerying the HistoryService cannot be tolerated in the suggestion
-  // creation phase. Here on the other hand it can due to the small percentage
-  // of suggestions getting deleted relative to the number of suggestions shown.
+  history::URLDatabase* url_db = history_service->InMemoryDatabase();
+  if (!url_db)
+    return;
+
+  // Deletes all the search terms matching the query suggestion.
+  url_db->DeleteKeywordSearchTermForNormalizedTerm(
+      template_url_service->GetDefaultSearchProvider()->id(), match.contents);
+
+  // Generate a Google search URL. Note that the search URL returned by
+  // TemplateURL::GenerateSearchURL() cannot be used here as it contains
+  // Chrome specific query params and therefore only matches search queries
+  // issued from Chrome and not those from the Web.
+  GURL google_base_url(
+      template_url_service->search_terms_data().GoogleBaseURLValue());
+  std::string google_search_url =
+      google_util::GetGoogleSearchURL(google_base_url).spec();
+
+  // Query the HistoryService for fresh Google search URLs. Note that the
+  // performance overhead of querying the HistoryService can be tolerated here
+  // due to the small percentage of suggestions getting deleted relative to the
+  // number of suggestions shown and the async nature of this lookup.
   history::QueryOptions opts;
   opts.duplicate_policy = history::QueryOptions::KEEP_ALL_DUPLICATES;
   opts.begin_time = history::AutocompleteAgeThreshold();
   history_service->QueryHistory(
-      base::ASCIIToUTF16(google_search_url.spec()), opts,
+      base::ASCIIToUTF16(google_search_url), opts,
       base::BindOnce(&LocalHistoryZeroSuggestProvider::OnHistoryQueryResults,
                      weak_ptr_factory_.GetWeakPtr(), match.contents),
       &history_task_tracker_);
 
-  // Prevent the deleted suggestion from being resuggested until the
-  // corresponding URLs are asynchronously deleted.
-  deleted_suggestions_set_.insert(match.contents);
-
   // Immediately update the list of matches to reflect the match was deleted.
   base::EraseIf(matches_, [&](const auto& item) {
     return match.contents == item.contents;
@@ -252,35 +158,43 @@ LocalHistoryZeroSuggestProvider::LocalHistoryZeroSuggestProvider(
 
 LocalHistoryZeroSuggestProvider::~LocalHistoryZeroSuggestProvider() {}
 
-void LocalHistoryZeroSuggestProvider::OnQueryURLDatabaseComplete(
-    const AutocompleteInput& input,
-    const history::URLRows& url_matches) {
+void LocalHistoryZeroSuggestProvider::QueryURLDatabase(
+    const AutocompleteInput& input) {
   done_ = true;
   matches_.clear();
 
-  // Fail if we can't extract the search terms from the URL matches.
+  history::HistoryService* const history_service = client_->GetHistoryService();
+  if (!history_service)
+    return;
+
+  // Fail if the in-memory URL database is not available.
+  history::URLDatabase* url_db = history_service->InMemoryDatabase();
+  if (!url_db)
+    return;
+
+  // Fail if we can't set the clickthrough URL for query suggestions.
   TemplateURLService* template_url_service = client_->GetTemplateURLService();
   if (!template_url_service ||
       !template_url_service->GetDefaultSearchProvider()) {
     return;
   }
 
+  // Request 5x more search terms than the number of matches the provider
+  // intends to return hoping to have enough left once ineligible ones are
+  // filtered out.
+  const auto& results = url_db->GetMostRecentKeywordSearchTerms(
+      template_url_service->GetDefaultSearchProvider()->id(), max_matches_ * 5);
+
   // Used to filter out duplicate query suggestions.
   std::set<base::string16> seen_suggestions_set;
 
   int relevance = kLocalHistoryZeroSuggestRelevance;
-  for (size_t i = 0; i < url_matches.size(); i++) {
-    const history::URLRow& url_row = url_matches[i];
-    const GURL& url = url_row.url();
-
-    // Discard the URL if it is not valid or fresh enough.
-    if (!url.is_valid() ||
-        url_row.last_visit() < history::AutocompleteAgeThreshold()) {
+  for (const auto& result : results) {
+    // Discard the result if it is fresh enough.
+    if (result.time < history::AutocompleteAgeThreshold())
       continue;
-    }
 
-    base::string16 search_terms =
-        GetSearchTermsFromURL(url, template_url_service);
+    base::string16 search_terms = result.normalized_term;
     if (search_terms.empty())
       continue;
 
@@ -289,10 +203,6 @@ void LocalHistoryZeroSuggestProvider::OnQueryURLDatabaseComplete(
       continue;
     seen_suggestions_set.insert(search_terms);
 
-    // Filter out deleted query suggestions.
-    if (deleted_suggestions_set_.count(search_terms))
-      continue;
-
     SearchSuggestionParser::SuggestResult suggestion(
         /*suggestion=*/search_terms, AutocompleteMatchType::SEARCH_HISTORY,
         /*subtype_identifier=*/0, /*from_keyword=*/false, relevance--,

components/omnibox/browser/local_history_zero_suggest_provider.h

@@ -15,7 +15,6 @@ class AutocompleteProviderClient;
 class AutocompleteProviderListener;
 
 namespace history {
-class HistoryDBTask;
 class QueryResults;
 }  // namespace history
 
@@ -42,9 +41,9 @@ class LocalHistoryZeroSuggestProvider : public AutocompleteProvider {
                                   AutocompleteProviderListener* listener);
   ~LocalHistoryZeroSuggestProvider() override;
 
-  // Creates autocomplete matches from |url_matches| and notifies |listener_|.
-  void OnQueryURLDatabaseComplete(const AutocompleteInput& input,
-                                  const history::URLRows& url_matches);
+  // Queries the keyword search terms table of the in-memory URLDatabase for the
+  // recent search terms submitted to the default search provider.
+  void QueryURLDatabase(const AutocompleteInput& input);
 
   // Called when the query results from HistoryService::QueryHistory are ready.
   // Deletes URLs in |results| that would generate |suggestion|.
@@ -54,10 +53,6 @@ class LocalHistoryZeroSuggestProvider : public AutocompleteProvider {
   // The maximum number of matches to return.
   const size_t max_matches_;
 
-  // Used to filter out deleted query suggestions in order to prevent them from
-  // reappearing before the corresponding URLs are asynchronously deleted.
-  std::set<base::string16> deleted_suggestions_set_;
-
   // Client for accessing TemplateUrlService, prefs, etc.
   AutocompleteProviderClient* const client_;
 
@@ -67,9 +62,6 @@ class LocalHistoryZeroSuggestProvider : public AutocompleteProvider {
   // Used for the async tasks querying the HistoryService.
   base::CancelableTaskTracker history_task_tracker_;
 
-  // Task ID for the history::HistoryDBTask running on history backend thread.
-  base::CancelableTaskTracker::TaskId history_db_task_id_;
-
   base::WeakPtrFactory<LocalHistoryZeroSuggestProvider> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LocalHistoryZeroSuggestProvider);

components/omnibox/browser/local_history_zero_suggest_provider_unittest.cc

@@ -5,7 +5,9 @@
 #include "components/omnibox/browser/local_history_zero_suggest_provider.h"
 
 #include <memory>
+#include <vector>
 
+#include "base/i18n/case_conversion.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ -13,6 +15,7 @@
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/keyword_search_term.h"
 #include "components/history/core/browser/url_database.h"
 #include "components/history/core/test/history_service_test_util.h"
 #include "components/omnibox/browser/autocomplete_match.h"
@@ -70,6 +73,14 @@ class LocalHistoryZeroSuggestProviderTest
         metrics::OmniboxEventProto::NTP_REALBOX,
         LocalHistoryZeroSuggestProvider::kZeroSuggestLocalVariant);
 
+    // Add the fallback default search provider to the TemplateURLService so
+    // that it gets a valid unique identifier. Make the newly added provider the
+    // user selected default search provider.
+    TemplateURL* default_provider = client_->GetTemplateURLService()->Add(
+        std::make_unique<TemplateURL>(default_search_provider()->data()));
+    client_->GetTemplateURLService()->SetUserSelectedDefaultSearchProvider(
+        default_provider);
+
     // Verify that Google is the default search provider.
     ASSERT_EQ(SEARCH_ENGINE_GOOGLE,
               default_search_provider()->GetEngineType(
@@ -132,7 +143,6 @@ void LocalHistoryZeroSuggestProviderTest::SetZeroSuggestVariant(
 void LocalHistoryZeroSuggestProviderTest::LoadURLs(
     std::vector<TestURLData> url_data_list) {
   const Time now = Time::Now();
-  history::URLRows rows;
   for (const auto& entry : url_data_list) {
     TemplateURLRef::SearchTermsArgs search_terms_args(
         base::UTF8ToUTF16(entry.search_terms));
@@ -141,17 +151,15 @@ void LocalHistoryZeroSuggestProviderTest::LoadURLs(
     std::string search_url =
         entry.search_provider->url_ref().ReplaceSearchTerms(search_terms_args,
                                                             search_terms_data);
-    history::URLRow row(GURL{search_url});
-    row.set_title(base::UTF8ToUTF16(entry.title));
-    row.set_visit_count(entry.visit_count);
-    row.set_typed_count(entry.typed_count);
-    row.set_last_visit(now - TimeDelta::FromDays(entry.age_in_days));
-    row.set_hidden(entry.hidden);
-    rows.push_back(row);
-  }
-  client_->GetHistoryService()->AddPagesWithDetails(rows,
-                                                    history::SOURCE_BROWSED);
+    client_->GetHistoryService()->AddPageWithDetails(
+        GURL(search_url), base::UTF8ToUTF16(entry.title), entry.visit_count,
+        entry.typed_count, now - TimeDelta::FromDays(entry.age_in_days),
+        entry.hidden, history::SOURCE_BROWSED);
+    client_->GetHistoryService()->SetKeywordSearchTermsForURL(
+        GURL(search_url), entry.search_provider->id(),
+        base::UTF8ToUTF16(entry.search_terms));
     WaitForHistoryService();
+  }
 }
 
 void LocalHistoryZeroSuggestProviderTest::WaitForHistoryService() {
@@ -259,7 +267,6 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, DefaultSearchProvider) {
       std::make_unique<TemplateURL>(*GenerateDummyTemplateURLData("other")));
   LoadURLs({
       {default_search_provider(), "hello world", "&foo=bar", 1},
-      {default_search_provider(), "", "&foo=bar", 1},
       {other_search_provider, "does not matter", "&foo=bar", 1},
   });
 
@@ -277,16 +284,14 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, DefaultSearchProvider) {
 }
 
 // Tests that search terms are extracted with the correct encoding, whitespaces
-// are collapsed, search terms are lowercased and duplicated, and empty searches
-// are ignored.
+// are collapsed, and are lowercased and deduplicated.
 TEST_F(LocalHistoryZeroSuggestProviderTest, SearchTerms) {
   LoadURLs({
       {default_search_provider(), "hello world", "&foo=bar", 1},
       {default_search_provider(), "hello   world", "&foo=bar", 1},
       {default_search_provider(), "hello   world", "&foo=bar", 1},
       {default_search_provider(), "hello world", "&foo=bar", 1},
-      {default_search_provider(), "HELLO WORLD", "&foo=bar", 1},
-      {default_search_provider(), "   ", "&foo=bar", 1},
+      {default_search_provider(), "HELLO   WORLD  ", "&foo=bar", 1},
       {default_search_provider(), "سلام دنیا", "&foo=bar", 2},
   });
 
@@ -318,32 +323,53 @@ TEST_F(LocalHistoryZeroSuggestProviderTest, Suggestions_Freshness) {
   ExpectMatches({{"fresh search", 500}});
 }
 
-// Tests that all the search URLs that would produce a given suggestion get
-// deleted when the autocomplete match is deleted.
-TEST_F(LocalHistoryZeroSuggestProviderTest, DISABLED_Delete) {
+// Tests that the provider supports deletion of matches.
+TEST_F(LocalHistoryZeroSuggestProviderTest, Delete) {
+  auto* template_url_service = client_->GetTemplateURLService();
+  auto* other_search_provider = template_url_service->Add(
+      std::make_unique<TemplateURL>(*GenerateDummyTemplateURLData("other")));
   LoadURLs({
       {default_search_provider(), "hello   world", "&foo=bar&aqs=1", 1},
-      {default_search_provider(), "HELLO WORLD", "&foo=bar&aqs=12", 1},
+      {default_search_provider(), "HELLO   WORLD  ", "&foo=bar&aqs=12", 1},
       {default_search_provider(), "hello world", "&foo=bar&aqs=123", 1},
       {default_search_provider(), "hello world", "&foo=bar&aqs=1234", 1},
+      {default_search_provider(), "not to be deleted", "&foo=bar&aqs=12345", 1},
+      {other_search_provider, "hello world", "&foo=bar&aqs=123456", 1},
   });
 
   StartProviderAndWaitUntilDone();
-  ExpectMatches({{"hello world", 500}});
+  ExpectMatches({{"hello world", 500}, {"not to be deleted", 499}});
 
   provider_->DeleteMatch(provider_->matches()[0]);
 
   // Make sure the deletion takes effect immediately in the provider before the
   // history service asynchronously performs the deletion or even before the
   // provider is started again.
-  ExpectMatches({});
+  ExpectMatches({{"not to be deleted", 499}});
 
   StartProviderAndWaitUntilDone();
-  ExpectMatches({});
+  ExpectMatches({{"not to be deleted", 500}});
 
   // Wait until the history service performs the deletion.
   WaitForHistoryService();
 
   StartProviderAndWaitUntilDone();
-  ExpectMatches({});
+  ExpectMatches({{"not to be deleted", 500}});
+
+  // Make sure all the search terms for the default search provider that would
+  // produce the deleted match are deleted.
+  history::URLDatabase* url_db =
+      client_->GetHistoryService()->InMemoryDatabase();
+  std::vector<history::KeywordSearchTermVisit> visits =
+      url_db->GetMostRecentKeywordSearchTerms(default_search_provider()->id(),
+                                              /*max_count=*/10);
+  EXPECT_EQ(1U, visits.size());
+  EXPECT_EQ(base::ASCIIToUTF16("not to be deleted"), visits[0].term);
+
+  // Make sure search terms from other search providers that would produce the
+  // deleted match are not deleted.
+  visits = url_db->GetMostRecentKeywordSearchTerms(other_search_provider->id(),
+                                                   /*max_count=*/10);
+  EXPECT_EQ(1U, visits.size());
+  EXPECT_EQ(base::ASCIIToUTF16("hello world"), visits[0].term);
 }