Adding performance tests for HQP that represent importance of optimising HistoryItemsForTerms method

components/omnibox/browser/history_quick_provider_unittest.cc

Index: components/omnibox/browser/history_quick_provider_unittest.cc
diff --git a/components/omnibox/browser/history_quick_provider_unittest.cc b/components/omnibox/browser/history_quick_provider_unittest.cc
index 0a556fd5c6cb5d644e061cc8132c4b76c4cdfe84..d364e64151bd4290bc0bc91fab5705ca338d8f01 100644
--- a/components/omnibox/browser/history_quick_provider_unittest.cc
+++ b/components/omnibox/browser/history_quick_provider_unittest.cc
@@ -13,14 +13,11 @@
 #include <string>
 #include <vector>
 
-#include "base/files/scoped_temp_dir.h"
 #include "base/format_macros.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/sequenced_worker_pool_owner.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/bookmark_test_helpers.h"
 #include "components/bookmarks/test/test_bookmark_client.h"
@@ -30,23 +27,17 @@
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/history/core/browser/url_database.h"
 #include "components/history/core/test/history_service_test_util.h"
-#include "components/metrics/proto/omnibox_event.pb.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/autocomplete_result.h"
+#include "components/omnibox/browser/fake_autocomplete_provider_client.h"
+#include "components/omnibox/browser/history_test_util.h"
 #include "components/omnibox/browser/history_url_provider.h"
-#include "components/omnibox/browser/in_memory_url_index.h"
 #include "components/omnibox/browser/in_memory_url_index_test_util.h"
-#include "components/omnibox/browser/mock_autocomplete_provider_client.h"
-#include "components/omnibox/browser/test_scheme_classifier.h"
-#include "components/omnibox/browser/url_index_private_data.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/search_terms_data.h"
-#include "sql/transaction.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
-using base::Time;
-using base::TimeDelta;
 
 namespace {
 
@@ -178,57 +169,6 @@ class GetURLTask : public history::HistoryDBTask {
   DISALLOW_COPY_AND_ASSIGN(GetURLTask);
 };
 
-class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
- public:
-  FakeAutocompleteProviderClient() : pool_owner_(3, "Background Pool") {
-    bookmark_model_ = bookmarks::TestBookmarkClient::CreateModel();
-    if (history_dir_.CreateUniqueTempDir()) {
-      history_service_ =
-          history::CreateHistoryService(history_dir_.GetPath(), true);
-    }
-
-    in_memory_url_index_.reset(new InMemoryURLIndex(
-        bookmark_model_.get(), history_service_.get(), nullptr,
-        pool_owner_.pool().get(), history_dir_.GetPath(), SchemeSet()));
-    in_memory_url_index_->Init();
-  }
-
-  const AutocompleteSchemeClassifier& GetSchemeClassifier() const override {
-    return scheme_classifier_;
-  }
-
-  const SearchTermsData& GetSearchTermsData() const override {
-    return search_terms_data_;
-  }
-
-  history::HistoryService* GetHistoryService() override {
-    return history_service_.get();
-  }
-
-  bookmarks::BookmarkModel* GetBookmarkModel() override {
-    return bookmark_model_.get();
-  }
-
-  InMemoryURLIndex* GetInMemoryURLIndex() override {
-    return in_memory_url_index_.get();
-  }
-
-  void set_in_memory_url_index(std::unique_ptr<InMemoryURLIndex> index) {
-    in_memory_url_index_ = std::move(index);
-  }
-
- private:
-  base::SequencedWorkerPoolOwner pool_owner_;
-  base::ScopedTempDir history_dir_;
-  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
-  TestSchemeClassifier scheme_classifier_;
-  SearchTermsData search_terms_data_;
-  std::unique_ptr<InMemoryURLIndex> in_memory_url_index_;
-  std::unique_ptr<history::HistoryService> history_service_;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeAutocompleteProviderClient);
-};
-
 }  // namespace
 
 class HistoryQuickProviderTest : public testing::Test {
@@ -337,46 +277,22 @@ void HistoryQuickProviderTest::GetTestData(size_t* data_count,
 }
 
 void HistoryQuickProviderTest::FillData() {
-  sql::Connection& db(history_backend()->db()->GetDB());
-  ASSERT_TRUE(db.is_open());
-
   size_t data_count = 0;
-  TestURLInfo* test_data = NULL;
+  TestURLInfo* test_data = nullptr;
   GetTestData(&data_count, &test_data);
-  size_t visit_id = 1;
-  for (size_t i = 0; i < data_count; ++i) {
-    const TestURLInfo& cur(test_data[i]);
-    Time visit_time = Time::Now() - TimeDelta::FromDays(cur.days_from_now);
-    sql::Transaction transaction(&db);
-
-    // Add URL.
-    transaction.Begin();
-    std::string sql_cmd_line = base::StringPrintf(
-        "INSERT INTO \"urls\" VALUES(%" PRIuS ", \'%s\', \'%s\', %d, %d, %"
-        PRId64 ", 0, 0)",
-        i + 1, cur.url.c_str(), cur.title.c_str(), cur.visit_count,
-        cur.typed_count, visit_time.ToInternalValue());
-    sql::Statement sql_stmt(db.GetUniqueStatement(sql_cmd_line.c_str()));
-    EXPECT_TRUE(sql_stmt.Run());
-    transaction.Commit();
-
-    // Add visits.
-    for (int j = 0; j < cur.visit_count; ++j) {
-      // Assume earlier visits are at one-day intervals.
-      visit_time -= TimeDelta::FromDays(1);
-      transaction.Begin();
-      // Mark the most recent |cur.typed_count| visits as typed.
-      std::string sql_cmd_line = base::StringPrintf(
-          "INSERT INTO \"visits\" VALUES(%" PRIuS ", %" PRIuS ", %" PRId64
-          ", 0, %d, 0, 1)",
-          visit_id++, i + 1, visit_time.ToInternalValue(),
-          (j < cur.typed_count) ? ui::PAGE_TRANSITION_TYPED :
-                                  ui::PAGE_TRANSITION_LINK);
-
-      sql::Statement sql_stmt(db.GetUniqueStatement(sql_cmd_line.c_str()));
-      EXPECT_TRUE(sql_stmt.Run());
-      transaction.Commit();
-    }
+
+  for (auto* it = test_data; it != test_data + data_count; ++it) {

[Peter Kasting, 2016/10/27 00:59:39]

Nit: May be out of scope for this patch, but it might be a nice cleanup to
change the GetTestData() methods in this file to return a vector (instead of
using size/pointer outparams), which could then be iterated over here with
range-based for.

Returning a vector could be done fairly easily by moving the file-scope data
constants into the functions akin to:

std::vector<TestURLInfo> HistoryQuickProviderTest::GetTestData() {
  return {
      {"http://www.google.com/", "Google", 3, 3, 0},
      {"http://slashdot.org/favorite_page.html", "Favorite page", 200, 100, 0},
      ...
  };
}

(Not sure offhand whether an explicit "std::vector<TestURLInfo>(" is needed here
to make this compile.)

[dyaroshev, 2016/10/27 06:09:21]

Done.

+    const TestURLInfo& info = *it;
+
+    history::URLRow row{GURL(info.url)};
+    CHECK(row.url().is_valid());
+    row.set_title(base::UTF8ToUTF16(info.title));
+    row.set_visit_count(info.visit_count);
+    row.set_typed_count(info.typed_count);
+    row.set_last_visit(base::Time::Now() -
+                       base::TimeDelta::FromDays(info.days_from_now));
+
+    AddFakeURLToHistoryDB(history_backend()->db(), row);
   }
 }
 
@@ -840,38 +756,53 @@ TEST_F(HistoryQuickProviderTest, DoesNotProvideMatchesOnFocus) {
 // HQPOrderingTest -------------------------------------------------------------
 
 TestURLInfo ordering_test_db[] = {
-  {"http://www.teamliquid.net/tlpd/korean/games/21648_bisu_vs_iris", "", 6, 3,
-      256},
-  {"http://www.amazon.com/", "amazon.com: online shopping for electronics, "
-      "apparel, computers, books, dvds & more", 20, 20, 10},
-  {"http://www.teamliquid.net/forum/viewmessage.php?topic_id=52045&"
-      "currentpage=83", "google images", 6, 6, 0},
-  {"http://www.tempurpedic.com/", "tempur-pedic", 7, 7, 0},
-  {"http://www.teamfortress.com/", "", 5, 5, 6},
-  {"http://www.rottentomatoes.com/", "", 3, 3, 7},
-  {"http://music.google.com/music/listen?u=0#start_pl", "", 3, 3, 9},
-  {"https://www.emigrantdirect.com/", "high interest savings account, high "
-      "yield savings - emigrantdirect", 5, 5, 3},
-  {"http://store.steampowered.com/", "", 6, 6, 1},
-  {"http://techmeme.com/", "techmeme", 111, 110, 4},
-  {"http://www.teamliquid.net/tlpd", "team liquid progaming database", 15, 15,
-      2},
-  {"http://store.steampowered.com/", "the steam summer camp sale", 6, 6, 1},
-  {"http://www.teamliquid.net/tlpd/korean/players", "tlpd - bw korean - player "
-      "index", 25, 7, 219},
-  {"http://slashdot.org/", "slashdot: news for nerds, stuff that matters", 3, 3,
-      6},
-  {"http://translate.google.com/", "google translate", 3, 3, 0},
-  {"http://arstechnica.com/", "ars technica", 3, 3, 3},
-  {"http://www.rottentomatoes.com/", "movies | movie trailers | reviews - "
-      "rotten tomatoes", 3, 3, 7},
-  {"http://www.teamliquid.net/", "team liquid - starcraft 2 and brood war pro "
-      "gaming news", 26, 25, 3},
-  {"http://metaleater.com/", "metaleater", 4, 3, 8},
-  {"http://half.com/", "half.com: textbooks , books , music , movies , games , "
-      "video games", 4, 4, 6},
-  {"http://teamliquid.net/", "team liquid - starcraft 2 and brood war pro "
-      "gaming news", 8, 5, 9},
+    {"http://www.teamliquid.net/tlpd/korean/games/21648_bisu_vs_iris", "", 6, 3,
+     256},
+    {"http://www.amazon.com/",
+     "amazon.com: online shopping for electronics, "
+     "apparel, computers, books, dvds & more",
+     20, 20, 10},
+    {"http://www.teamliquid.net/forum/viewmessage.php?topic_id=52045&"
+     "currentpage=83",
+     "google images", 6, 6, 0},
+    {"http://www.tempurpedic.com/", "tempur-pedic", 7, 7, 0},
+    {"http://www.teamfortress.com/", "", 5, 5, 6},
+    {"http://www.rottentomatoes.com/", "", 3, 3, 7},
+    {"http://music.google.com/music/listen?u=0#start_pl", "", 3, 3, 9},
+    {"https://www.emigrantdirect.com/",
+     "high interest savings account, high "
+     "yield savings - emigrantdirect",
+     5, 5, 3},
+    {"http://store.steampowered.com/", "", 6, 6, 1},
+    {"http://techmeme.com/", "techmeme", 111, 110, 4},
+    {"http://www.teamliquid.net/tlpd", "team liquid progaming database", 15, 15,
+     2},
+    {"http://store.steampowered.com/", "the steam summer camp sale", 6, 6, 1},
+    {"http://www.teamliquid.net/tlpd/korean/players",
+     "tlpd - bw korean - player "
+     "index",
+     25, 7, 219},
+    {"http://slashdot.org/", "slashdot: news for nerds, stuff that matters", 3,
+     3, 6},
+    {"http://translate.google.com/", "google translate", 3, 3, 0},
+    {"http://arstechnica.com/", "ars technica", 3, 3, 3},
+    {"http://www.rottentomatoes.com/",
+     "movies | movie trailers | reviews - "
+     "rotten tomatoes",
+     3, 3, 7},
+    {"http://www.teamliquid.net/",
+     "team liquid - starcraft 2 and brood war pro "
+     "gaming news",
+     26, 25, 3},
+    {"http://metaleater.com/", "metaleater", 4, 3, 8},
+    {"http://half.com/",
+     "half.com: textbooks , books , music , movies , games , "
+     "video games",
+     4, 4, 6},
+    {"http://teamliquid.net/",
+     "team liquid - starcraft 2 and brood war pro "
+     "gaming news",
+     8, 5, 9},
 };
 
 class HQPOrderingTest : public HistoryQuickProviderTest {