Cleaning up test only variables.

components/omnibox/browser/in_memory_url_index_unittest.cc

Index: components/omnibox/browser/in_memory_url_index_unittest.cc
diff --git a/components/omnibox/browser/in_memory_url_index_unittest.cc b/components/omnibox/browser/in_memory_url_index_unittest.cc
index 54119eedb1e94a428f0c11df6d0ff0e3a618f009..41a2c9aa48554828ec99189dfbf2af5ffdb45046 100644
--- a/components/omnibox/browser/in_memory_url_index_unittest.cc
+++ b/components/omnibox/browser/in_memory_url_index_unittest.cc
@@ -88,6 +88,29 @@ void StringToTerms(const char* search_string,
                                    base::SPLIT_WANT_NONEMPTY);
 }
 
+constexpr size_t kItemsToScoreLimit = 500;

[dyaroshev, 2017/02/22 22:21:51]

Make constants local.

[dyaroshev, 2017/02/24 01:27:03]

Done.

+constexpr int kLowVisitCount = 20;
+constexpr int kHighVisitCount = 200;
+constexpr int kLowTypedCount = 2;
+constexpr int kHighTypedCount = 100;
+
+constexpr int kMinRowId = 5000;
+
+base::Time OldVisitTime() {

[Peter Kasting, 2017/02/23 01:04:45]

Nit: I would make these const temps in the caller side instead of helper
functions, for a couple small reasons.

One is that it's more readable to have the definition right there in code than
to look up here for it.  In the case of RecentVisitTime(), there's only one
caller, so we wouldn't even duplicate code.

Another is that it's not clear whether these values are arbitrary, and if they
need to be the same across all callers, or are just generic "any old visit". 
Placing the definitions in the callers makes it more clear the tests are
independent of each other.

The third reason is that this will let you call Now() once and store the result
in a temp instead of recomputing it repeatedly, which is an efficiency win.

[dyaroshev, 2017/02/23 01:41:26]

I think think that calling now is good because we get different timings for
different results.

[dyaroshev, 2017/02/24 01:27:03]

I've done smth. Is it ok?

+  return base::Time::Now() - base::TimeDelta::FromDays(15);
+};
+base::Time RecentVisitTime() {
+  return base::Time::Now() - base::TimeDelta::FromDays(2);
+};
+
+HistoryIDSet FillHistoryIDSet(HistoryID min, HistoryID max) {
+  HistoryIDSet res;
+  // All ids are inserted in the end so the implicit hint would work.
+  for (HistoryID id = min; id < max; ++id)
+    res.insert(id);
+  return res;
+}
+
 }  // namespace
 
 // -----------------------------------------------------------------------------
@@ -160,6 +183,10 @@ class InMemoryURLIndexTest : public testing::Test {
 
   // Pass-through functions to simplify our friendship with URLIndexPrivateData.
   bool UpdateURL(const history::URLRow& row);
+  void AddHistoryEntry(history::URLID row_id,
+                       int typed_count,
+                       int visit_count,
+                       base::Time last_visit);
   bool DeleteURL(const GURL& url);
 
   // Data verification helper functions.
@@ -226,6 +253,18 @@ bool InMemoryURLIndexTest::UpdateURL(const history::URLRow& row) {
       GetPrivateDataTracker());
 }
 
+void InMemoryURLIndexTest::AddHistoryEntry(history::URLID row_id,
+                                           int typed_count,
+                                           int visit_count,
+                                           base::Time last_visit) {
+  history::URLRow new_row(
+      GURL("http://www.fake_url" + std::to_string(row_id) + ".com"), row_id);
+  new_row.set_visit_count(visit_count);
+  new_row.set_typed_count(typed_count);
+  new_row.set_last_visit(last_visit);
+  UpdateURL(std::move(new_row));
+};
+
 bool InMemoryURLIndexTest::DeleteURL(const GURL& url) {
   return GetPrivateData()->DeleteURL(url);
 }
@@ -710,6 +749,66 @@ TEST_F(InMemoryURLIndexTest, ProperStringMatching) {
   EXPECT_EQ(1U, matches.size());
 }
 
+TEST_F(InMemoryURLIndexTest, DontTrimMoreThanLimit) {

[Peter Kasting, 2017/02/23 01:04:45]

Nit: Especially for the second test you add, but really for both of these,
please give a descriptive comment sentence describing what the test is testing.

[dyaroshev, 2017/02/24 01:27:03]

Done?

+  history::URLID row_id = kMinRowId;
+  AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+  for (++row_id; size_t(row_id) < kItemsToScoreLimit - 1; ++row_id) {

[Peter Kasting, 2017/02/23 01:04:45]

I don't think this works right?  kMinRowId = 5000 and kItemsToScoreLimit = 500. 
I think you meant to compare against kMinRowId + kItemsToScoreLimit or
something, or the loop won't execute.

Nit: Don't cast to size_t with this syntax; use a static_cast (2 places).  No
{}.

[dyaroshev, 2017/02/24 01:27:03]

Not applicable.

+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+  }

[dyaroshev, 2017/02/22 22:21:51]

One extra addition is unnecessary.

[Peter Kasting, 2017/02/23 01:04:45]

Not sure what this is saying.  Is it explaining the "- 1" in the loop condition
above?  Because I don't really get that.

[dyaroshev, 2017/02/23 01:41:26]

It's more for me - to avoid extra patches, if I see smth I want fixed, I leave a
comment and then fix with your issues. Here I'm calling AddHistory... 2 times:
one before the loop, one after - this is unnecessary.  

[Peter Kasting, 2017/02/23 01:46:06]

OK.  I still don't really get the "- 1" in the loop conditional then.

[dyaroshev, 2017/02/24 01:27:03]

Not applicable.

+
+  auto history_id_set = FillHistoryIDSet(kMinRowId, row_id);

[dyaroshev, 2017/02/22 22:21:52]

Check that all ids are in

+
+  EXPECT_FALSE(GetPrivateData()->TrimHistoryIdsPool(&history_id_set));
+  EXPECT_EQ(size_t(row_id - kMinRowId), history_id_set.size());
+}
+
+TEST_F(InMemoryURLIndexTest, TrimByProrities) {
+  int expected_worst = 1;

[Peter Kasting, 2017/02/23 01:04:45]

Nit: I can't instantly tell what this code is doing beyond the general "expect
that the right number of things got trimmed".  Add comments above blocks of code
giving the high level summary of what a large block does.

[dyaroshev, 2017/02/24 01:27:03]

Rewrote the test.

+  int expected_recent = 2;
+  int expected_visit_count = 3;
+  int expected_typed_count = kItemsToScoreLimit - expected_visit_count -
+                             expected_recent - expected_worst;
+
+  const history::URLID last_high_typed_count = kMinRowId + expected_typed_count;
+  const history::URLID last_high_visit_count =
+      last_high_typed_count + expected_visit_count;
+  const history::URLID last_recent = last_high_visit_count + expected_recent;
+  const history::URLID last_worst = last_recent + kItemsToScoreLimit;
+
+  history::URLID row_id = kMinRowId;
+  for (; row_id < last_high_typed_count; ++row_id)
+    AddHistoryEntry(row_id, kHighTypedCount, kLowVisitCount, OldVisitTime());
+
+  for (; row_id < last_high_visit_count; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kHighVisitCount, OldVisitTime());
+
+  for (; row_id < last_recent; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, RecentVisitTime());
+
+  for (; row_id < last_worst; ++row_id)
+    AddHistoryEntry(row_id, kLowTypedCount, kLowVisitCount, OldVisitTime());
+
+  auto history_id_set = FillHistoryIDSet(kMinRowId, row_id);
+  EXPECT_TRUE(GetPrivateData()->TrimHistoryIdsPool(&history_id_set));
+  EXPECT_EQ(kItemsToScoreLimit, history_id_set.size());
+
+  for (HistoryID id : history_id_set) {

[dyaroshev, 2017/02/22 22:21:52]

We could do the checks more efficiently for the set but we'll drop the sorting
in the refactoring.

+    if (id < last_high_typed_count)
+      --expected_typed_count;
+    else if (id < last_high_visit_count)
+      --expected_visit_count;
+    else if (id < last_recent)
+      --expected_recent;
+    else if (id < last_worst)
+      --expected_worst;
+  }
+
+  EXPECT_EQ(0, expected_typed_count);
+  EXPECT_EQ(0, expected_visit_count);
+  EXPECT_EQ(0, expected_recent);
+  EXPECT_EQ(0, expected_worst);
+}
+
 TEST_F(InMemoryURLIndexTest, HugeResultSet) {
   // Create a huge set of qualifying history items.
   for (history::URLID row_id = 5000; row_id < 6000; ++row_id) {
@@ -721,12 +820,7 @@ TEST_F(InMemoryURLIndexTest, HugeResultSet) {
 
   ScoredHistoryMatches matches = url_index_->HistoryItemsForTerms(
       ASCIIToUTF16("b"), base::string16::npos, kMaxMatches);
-  URLIndexPrivateData& private_data(*GetPrivateData());
   EXPECT_EQ(kMaxMatches, matches.size());
-  // There are 7 matches already in the database.
-  EXPECT_EQ(1008U, private_data.pre_filter_item_count_);
-  EXPECT_EQ(500U, private_data.post_filter_item_count_);
-  EXPECT_EQ(kMaxMatches, private_data.post_scoring_item_count_);
 }
 
 TEST_F(InMemoryURLIndexTest, TitleSearch) {