[memories] Replace cluster_visits with context_annotations table.

- This is CL 1 of 5 merging `ClusterVisitDatabase` with
`VisitAnnotationsDatabase`.

- This naming is consistent with the `content_annotations` table.

- Also drops the `cluster_visit_id` column & field; we'll instead use
`visit_id`s as the primary key as `context_annotations` have a 1-or-0 :
1 relationship with `visits`. This too is consistent with
`content_annotations`.

- Likewise, drops the `url_id` column & field; it's redundant since
`context_annotations` are join with `visits` which already have
`url_id`.

- Renames the `cluster_visit_context_signal_bitmask` column to
`context_annotation_flags` to be consistent with `content_annotations`'s
`annotation_flags` column. Associated variables and methods will be
renamed in a followup CL, but the column rename is done here to avoid an
additional migration.

- Removes `NOT NULL` requirement on some columns.

- Follow up CLs will rename `ClusterVisit`, related structs, method
names, and file names with `VisitContextAnnotations`. This is consistent
with `VisitContentAnnotations`.

Bug: 1179068, 1184879, 1184875, 1171352
Change-Id: I8ae50c70908c955a3547ab6da342c064a45e743f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2869588
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#879611}

components/history/core/browser/BUILD.gn

@@ -180,6 +180,7 @@ bundle_data("unit_tests_bundle_data") {
     "//components/test/data/history/history.41.sql",
     "//components/test/data/history/history.42.sql",
     "//components/test/data/history/history.43.sql",
+    "//components/test/data/history/history.44.sql",
     "//components/test/data/history/thumbnail_wild/Favicons.corrupt_meta.disable",
     "//components/test/data/history/thumbnail_wild/Favicons.v2.init.sql",
     "//components/test/data/history/thumbnail_wild/Favicons.v3.init.sql",

components/history/core/browser/cluster_visit_database.cc

@@ -86,12 +86,11 @@ ClusterVisitContextSignals ConstructContextSignalsWithFlags(
 // Convenience to construct a `ClusterVisitRow`. Assumes the visit values are
 // bound starting at index 0.
 ClusterVisitRow StatementToVisitRow(const sql::Statement& statement) {
-  return {statement.ColumnInt64(0), statement.ColumnInt64(1),
-          statement.ColumnInt64(2),
+  return {statement.ColumnInt64(0),
           ConstructContextSignalsWithFlags(
-              statement.ColumnInt64(3),
-              base::TimeDelta::FromMicroseconds(statement.ColumnInt64(4)),
-              statement.ColumnInt(5))};
+              statement.ColumnInt64(1),
+              base::TimeDelta::FromMicroseconds(statement.ColumnInt64(2)),
+              statement.ColumnInt(3))};
 }
 
 // Like `StatementToVisitRow()` but for multiple rows.
@@ -107,31 +106,24 @@ std::vector<ClusterVisitRow> StatementToVisitRowVector(
 
 }  // namespace
 
-// Columns, in order, excluding `cluster_visit_id`.
-#define HISTORY_CLUSTER_VISIT_ROW_FIELDS_WITHOUT_ID           \
-  " url_id, visit_id, cluster_visit_context_signal_bitmask, " \
-  "duration_since_last_visit, page_end_reason "
-
-// Columns, in order, including `cluster_visit_id`.
-#define HISTORY_CLUSTER_VISIT_ROW_FIELDS \
-  " cluster_visit_id," HISTORY_CLUSTER_VISIT_ROW_FIELDS_WITHOUT_ID
+// Columns, in order.
+#define HISTORY_CLUSTER_VISIT_ROW_FIELDS                             \
+  " visit_id, context_annotation_flags, duration_since_last_visit, " \
+  "page_end_reason "
 
 ClusterVisitDatabase::ClusterVisitDatabase() = default;
 
 ClusterVisitDatabase::~ClusterVisitDatabase() = default;
 
 bool ClusterVisitDatabase::InitClusterVisitTable() {
-  if (!GetDB().DoesTableExist("cluster_visits")) {
+  if (!GetDB().DoesTableExist("context_annotations")) {
     // See `ClusterVisitRow` and `ClusterVisitContextSignals` for details about
     // these fields.
-    if (!GetDB().Execute(
-            "CREATE TABLE cluster_visits("
-            "cluster_visit_id INTEGER PRIMARY KEY,"
-            "url_id INTEGER NOT NULL,"
-            "visit_id INTEGER NOT NULL,"
-            "cluster_visit_context_signal_bitmask INTEGER NOT NULL,"
-            "duration_since_last_visit INTEGER NOT NULL,"
-            "page_end_reason INTEGER NOT NULL)")) {
+    if (!GetDB().Execute("CREATE TABLE context_annotations("
+                         "visit_id INTEGER PRIMARY KEY,"
+                         "context_annotation_flags INTEGER DEFAULT 0 NOT NULL,"
+                         "duration_since_last_visit INTEGER,"
+                         "page_end_reason INTEGER)")) {
       return false;
     }
   }
@@ -140,48 +132,54 @@ bool ClusterVisitDatabase::InitClusterVisitTable() {
 }
 
 bool ClusterVisitDatabase::DropClusterVisitTable() {
-  return GetDB().Execute("DROP TABLE cluster_visits");
+  return GetDB().Execute("DROP TABLE context_annotations");
 }
 
 void ClusterVisitDatabase::AddClusterVisit(const ClusterVisitRow& row) {
   sql::Statement statement(GetDB().GetCachedStatement(
       SQL_FROM_HERE,
-      "INSERT INTO cluster_visits (" HISTORY_CLUSTER_VISIT_ROW_FIELDS_WITHOUT_ID
-      ") VALUES (?, ?, ?, ?, ?)"));
-  statement.BindInt64(0, row.url_id);
-  statement.BindInt64(1, row.visit_id);
-  statement.BindInt64(2, ContextSignalsToFlags(row.context_signals));
+      "INSERT INTO context_annotations (" HISTORY_CLUSTER_VISIT_ROW_FIELDS
+      ") VALUES (?, ?, ?, ?)"));
+  statement.BindInt64(0, row.visit_id);
+  statement.BindInt64(1, ContextSignalsToFlags(row.context_signals));
   statement.BindInt64(
-      3, row.context_signals.duration_since_last_visit.InMicroseconds());
-  statement.BindInt(4, row.context_signals.page_end_reason);
+      2, row.context_signals.duration_since_last_visit.InMicroseconds());
+  statement.BindInt(3, row.context_signals.page_end_reason);
 
   if (!statement.Run()) {
     DVLOG(0) << "Failed to execute cluster visit insert statement:  "
-             << "url_id = " << row.url_id << ", visit_id = " << row.visit_id;
+             << "visit_id = " << row.visit_id;
   }
 }
 
-void ClusterVisitDatabase::DeleteClusterVisit(int64_t cluster_visit_id) {
+void ClusterVisitDatabase::DeleteClusterVisit(VisitID visit_id) {
   sql::Statement statement(GetDB().GetCachedStatement(
-      SQL_FROM_HERE, "DELETE FROM cluster_visits WHERE cluster_visit_id = ?"));
-  statement.BindInt64(0, cluster_visit_id);
+      SQL_FROM_HERE, "DELETE FROM context_annotations WHERE visit_id = ?"));
+  statement.BindInt64(0, visit_id);
 
   if (!statement.Run()) {
     DVLOG(0) << "Failed to execute cluster visit delete statement:  "
-             << "cluster_visit_id = " << cluster_visit_id;
+             << "visit_id = " << visit_id;
   }
 }
 
 std::vector<ClusterVisitRow> ClusterVisitDatabase::GetClusterVisits(
     int max_results) {
   sql::Statement statement(GetDB().GetCachedStatement(
       SQL_FROM_HERE, "SELECT" HISTORY_CLUSTER_VISIT_ROW_FIELDS
-                     "FROM cluster_visits "
+                     "FROM context_annotations "
                      "JOIN visits ON visit_id = visits.id "
                      "ORDER BY visits.visit_time DESC "
                      "LIMIT ?"));
   statement.BindInt64(0, max_results);
   return StatementToVisitRowVector(statement);
 }
 
+bool ClusterVisitDatabase::MigrateReplaceClusterVisitsTable() {
+  // We don't need to actually copy values from the previous table; it's only
+  // rolled out behind a flag.
+  return !GetDB().DoesTableExist("cluster_visits") ||
+         GetDB().Execute("DROP TABLE cluster_visits");
+}
+
 }  // namespace history

components/history/core/browser/cluster_visit_database.h

@@ -20,6 +20,7 @@ namespace history {
 // databases, as this joins with the visit table and could be thought of as
 // inheriting from VisitDatabase. However, this inheritance is not explicit as
 // things would get too complicated and have multiple inheritance.
+// TODO(manukh) Merge with VisitAnnotationsDatabase.
 class ClusterVisitDatabase {
  public:
   // Must call InitClusterVisitTable() before using to make sure the database is
@@ -36,7 +37,7 @@ class ClusterVisitDatabase {
   void AddClusterVisit(const ClusterVisitRow& row);
 
   // Delete a `ClusterVisitRow` from the table.
-  void DeleteClusterVisit(int64_t cluster_visit_id);
+  void DeleteClusterVisit(VisitID visit_id);
 
   // Get the `max_results` most recent `ClusterVisitRow`s.
   std::vector<ClusterVisitRow> GetClusterVisits(int max_results);
@@ -48,6 +49,12 @@ class ClusterVisitDatabase {
   // Called by the derived classes on initialization to make sure the tables
   // and indices are properly set up. Must be called before anything else.
   bool InitClusterVisitTable();
+
+  // Replaces `cluster_visits` with `context_annotations`. Besides the name
+  // change, the new table drops 2 columns: cluster_visit_id (obsolete) and
+  // url_id (redundant); and renames 1 column:
+  // cluster_visit_context_signal_bitmask to context_annotation_flags.
+  bool MigrateReplaceClusterVisitsTable();
 };
 
 }  // namespace history

components/history/core/browser/cluster_visit_database_unittest.cc

@@ -33,8 +33,9 @@ class ClusterVisitDatabaseTest : public testing::Test,
   }
   ~ClusterVisitDatabaseTest() override { db_.Close(); }
 
-  void AddVisitWithVisitTime(base::Time visit_time) {
+  void AddVisitWithDetails(URLID url_id, base::Time visit_time) {
     VisitRow visit_row;
+    visit_row.url_id = url_id;
     visit_row.visit_time = visit_time;
     AddVisit(&visit_row, VisitSource::SOURCE_BROWSED);
   }
@@ -47,63 +48,35 @@ class ClusterVisitDatabaseTest : public testing::Test,
 };
 
 TEST_F(ClusterVisitDatabaseTest, AddDeleteAndGet) {
-  AddVisitWithVisitTime(IntToTime(20));
-  AddVisitWithVisitTime(IntToTime(30));
-  AddVisitWithVisitTime(IntToTime(10));
+  AddVisitWithDetails(1, IntToTime(20));
+  AddVisitWithDetails(1, IntToTime(30));
+  AddVisitWithDetails(2, IntToTime(10));
 
-  AddClusterVisit({0, 1, 1, {true}});   // Ordered 2rd
-  AddClusterVisit({0, 2, 2, {false}});  // Ordered 1st
-  AddClusterVisit({0, 1, 1, {true}});   // Ordered 3nd
-  AddClusterVisit({0, 2, 3, {false}});  // Ordered 4th
+  AddClusterVisit({1, {true}});   // Ordered 2nd
+  AddClusterVisit({2, {false}});  // Ordered 1st
+  AddClusterVisit({3, {false}});  // Ordered 3rd
 
   std::vector<ClusterVisitRow> rows = GetClusterVisits(10);
-  ASSERT_EQ(rows.size(), 4u);
-  EXPECT_EQ(rows[0].cluster_visit_id, 2);
-  EXPECT_EQ(rows[0].url_id, 2);
+  ASSERT_EQ(rows.size(), 3u);
   EXPECT_EQ(rows[0].visit_id, 2);
   EXPECT_FALSE(rows[0].context_signals.omnibox_url_copied);
-  EXPECT_EQ(rows[1].cluster_visit_id, 1);
-  EXPECT_EQ(rows[1].url_id, 1);
   EXPECT_EQ(rows[1].visit_id, 1);
   EXPECT_TRUE(rows[1].context_signals.omnibox_url_copied);
-  EXPECT_EQ(rows[2].cluster_visit_id, 3);
-  EXPECT_EQ(rows[2].url_id, 1);
-  EXPECT_EQ(rows[2].visit_id, 1);
-  EXPECT_TRUE(rows[2].context_signals.omnibox_url_copied);
-  EXPECT_EQ(rows[3].cluster_visit_id, 4);
-  EXPECT_EQ(rows[3].url_id, 2);
-  EXPECT_EQ(rows[3].visit_id, 3);
-  EXPECT_FALSE(rows[3].context_signals.omnibox_url_copied);
+  EXPECT_EQ(rows[2].visit_id, 3);
+  EXPECT_FALSE(rows[2].context_signals.omnibox_url_copied);
 
   rows = GetClusterVisits(2);
   ASSERT_EQ(rows.size(), 2u);
-  EXPECT_EQ(rows[0].cluster_visit_id, 2);
-  EXPECT_EQ(rows[0].url_id, 2);
   EXPECT_EQ(rows[0].visit_id, 2);
   EXPECT_FALSE(rows[0].context_signals.omnibox_url_copied);
-  EXPECT_EQ(rows[1].cluster_visit_id, 1);
-  EXPECT_EQ(rows[1].url_id, 1);
   EXPECT_EQ(rows[1].visit_id, 1);
   EXPECT_TRUE(rows[1].context_signals.omnibox_url_copied);
 
   DeleteClusterVisit(1);
   DeleteClusterVisit(3);
 
   rows = GetClusterVisits(10);
-  ASSERT_EQ(rows.size(), 2u);
-  EXPECT_EQ(rows[0].cluster_visit_id, 2);
-  EXPECT_EQ(rows[0].url_id, 2);
-  EXPECT_EQ(rows[0].visit_id, 2);
-  EXPECT_FALSE(rows[0].context_signals.omnibox_url_copied);
-  EXPECT_EQ(rows[1].cluster_visit_id, 4);
-  EXPECT_EQ(rows[1].url_id, 2);
-  EXPECT_EQ(rows[1].visit_id, 3);
-  EXPECT_FALSE(rows[1].context_signals.omnibox_url_copied);
-
-  rows = GetClusterVisits(1);
   ASSERT_EQ(rows.size(), 1u);
-  EXPECT_EQ(rows[0].cluster_visit_id, 2);
-  EXPECT_EQ(rows[0].url_id, 2);
   EXPECT_EQ(rows[0].visit_id, 2);
   EXPECT_FALSE(rows[0].context_signals.omnibox_url_copied);
 }

components/history/core/browser/history_backend.cc

@@ -1421,13 +1421,10 @@ void HistoryBackend::DeleteMatchingURLsForKeyword(KeywordID keyword_id,
 
 void HistoryBackend::AddClusterVisit(const ClusterVisitRow& row) {
   TRACE_EVENT0("browser", "HistoryBackend::AddClusterVisit");
-  DCHECK(row.url_id && row.visit_id);
-  URLRow url_row;
+  DCHECK(row.visit_id);
   VisitRow visit_row;
-  if (!db_ || !db_->GetURLRow(row.url_id, &url_row) ||
-      !db_->GetRowForVisit(row.visit_id, &visit_row)) {
+  if (!db_ || !db_->GetRowForVisit(row.visit_id, &visit_row))
     return;
-  }
   db_->AddClusterVisit(row);
   ScheduleCommit();
 }
@@ -1441,11 +1438,11 @@ std::vector<ClusterVisit> HistoryBackend::GetClusterVisits(int max_results) {
   for (const auto& row : db_->GetClusterVisits(max_results)) {
     URLRow url_row;
     VisitRow visit_row;
-    if (db_->GetURLRow(row.url_id, &url_row) &&
-        db_->GetRowForVisit(row.visit_id, &visit_row)) {
+    if (db_->GetRowForVisit(row.visit_id, &visit_row) &&
+        db_->GetURLRow(visit_row.url_id, &url_row)) {
       cluster_visits.push_back({url_row, visit_row, row.context_signals});
     } else {
-      db_->DeleteClusterVisit(row.cluster_visit_id);
+      db_->DeleteClusterVisit(row.visit_id);
       deleted_any_visits = true;
     }
   }

components/history/core/browser/history_backend_db_unittest.cc

@@ -1894,6 +1894,66 @@ TEST_F(HistoryBackendDBTest, MigrateFlocAllowedToAnnotationsTable) {
   }
 }
 
+TEST_F(HistoryBackendDBTest, MigrateReplaceClusterVisitsTable) {
+  ASSERT_NO_FATAL_FAILURE(CreateDBVersion(44));
+
+  sql::Database db;
+  ASSERT_TRUE(db.Open(history_dir_.Append(kHistoryFilename)));
+
+  const char kInsertVisitStatement[] =
+      "INSERT INTO visits "
+      "(id, url, visit_time) VALUES (?, ?, ?)";
+
+  const char kInsertAnnotationsStatement[] =
+      "INSERT INTO cluster_visits "
+      "(cluster_visit_id, url_id, visit_id, "
+      "cluster_visit_context_signal_bitmask, duration_since_last_visit, "
+      "page_end_reason) "
+      "VALUES (?, ?, ?, ?, ?, ?)";
+
+  // Add a row to `visits` table.
+  {
+    sql::Statement s(db.GetUniqueStatement(kInsertVisitStatement));
+    s.BindInt64(0, 1);
+    s.BindInt64(1, 1);
+    s.BindTime(2, base::Time::Now());
+    ASSERT_TRUE(s.Run());
+  }
+
+  // Add a row to the `cluster_visits` table.
+  {
+    sql::Statement s(db.GetUniqueStatement(kInsertAnnotationsStatement));
+    s.BindInt64(0, 1);
+    s.BindInt64(1, 1);
+    s.BindInt64(2, 1);
+    s.BindInt64(3, 0);
+    s.BindInt64(4, 0);
+    s.BindInt(5, 0);
+    ASSERT_TRUE(s.Run());
+  }
+
+  // Re-open the db, triggering migration.
+  CreateBackendAndDatabase();
+
+  // The version should have been updated.
+  ASSERT_GE(HistoryDatabase::GetCurrentVersion(), 45);
+
+  // Confirm the old `cluster_visits` table no longer exists.
+  ASSERT_FALSE(db.DoesTableExist("cluster_visits"));
+
+  // Confirm the new `context_annotations` exists.
+  ASSERT_TRUE(db.DoesTableExist("context_annotations"));
+
+  // Check `context_annotations` is empty.
+  {
+    sql::Statement s(
+        db.GetUniqueStatement("SELECT COUNT(*) FROM content_annotations"));
+    EXPECT_TRUE(s.Step());
+    EXPECT_EQ(s.ColumnInt64(0), 0u);
+    EXPECT_FALSE(s.Step());
+  }
+}
+
 // Tests that the migration code correctly replaces the lower_term column in the
 // keyword search terms table which normalized_term which contains the
 // normalized search term during migration to version 42.

components/history/core/browser/history_backend_unittest.cc

@@ -3188,32 +3188,26 @@ TEST_F(HistoryBackendTest, ClusterVisits) {
             (std::pair<URLID, VisitID>{2, 2}));
   EXPECT_EQ(add_url_and_visit("http://1.com/"),
             (std::pair<URLID, VisitID>{1, 3}));
-  backend_->AddClusterVisit({0, 1, 1, {true}});
-  backend_->AddClusterVisit({0, 1, 3, {false}});
-  backend_->AddClusterVisit({0, 2, 2, {true}});
+  backend_->AddClusterVisit({1, {true}});
+  backend_->AddClusterVisit({3, {false}});
+  backend_->AddClusterVisit({2, {true}});
   EXPECT_EQ(backend_->db_->GetClusterVisits(10).size(), 3u);
 
-  // Cluster visits should have URL & visit IDs
-  EXPECT_DCHECK_DEATH(backend_->AddClusterVisit({0, 0, 4, {true}}));
-  EXPECT_DCHECK_DEATH(backend_->AddClusterVisit({0, 3, 0, {true}}));
+  // Cluster visits should have a visit IDs.
+  EXPECT_DCHECK_DEATH(backend_->AddClusterVisit({0, {true}}));
   EXPECT_EQ(backend_->db_->GetClusterVisits(10).size(), 3u);
 
-  // Cluster visits without an associated URL or visit should not be added.
-  backend_->AddClusterVisit({0, 3, 1, {true}});
-  backend_->AddClusterVisit({0, 1, 4, {true}});
+  // Cluster visits without an associated visit should not be added.
+  backend_->AddClusterVisit({4, {true}});
   EXPECT_EQ(add_url_and_visit("http://3.com/"),
             (std::pair<URLID, VisitID>{3, 4}));
   EXPECT_EQ(backend_->db_->GetClusterVisits(10).size(), 3u);
 
-  // Cluster visits associated with a removed URL or visit should not be added.
+  // Cluster visits associated with a removed visit should not be added.
   EXPECT_EQ(add_url_and_visit("http://4.com/"),
             (std::pair<URLID, VisitID>{4, 5}));
-  EXPECT_EQ(add_url_and_visit("http://5.com/"),
-            (std::pair<URLID, VisitID>{5, 6}));
-  delete_url(4);
-  delete_visit(6);
-  backend_->AddClusterVisit({0, 4, 1, {true}});
-  backend_->AddClusterVisit({0, 1, 6, {true}});
+  delete_visit(5);
+  backend_->AddClusterVisit({5, {true}});
   EXPECT_EQ(backend_->db_->GetClusterVisits(10).size(), 3u);
 
   // Verify only the correct cluster visits are retrieved ordered recent visits