[Media History] Make URL unique for playback sessions

For playback sessions (the continue watching table)
we actually just want the most recent playback for
a URL so we can make it unique.

BUG=1041662

Change-Id: Ib7ad1d9f276ffe1b669fff36aa3154daf6b23ffe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033908
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737851}

chrome/browser/media/history/media_history_session_table.cc

@@ -31,7 +31,7 @@ sql::InitStatus MediaHistorySessionTable::CreateTableIfNonExistent() {
       DB()->Execute(base::StringPrintf("CREATE TABLE IF NOT EXISTS %s("
                                        "id INTEGER PRIMARY KEY AUTOINCREMENT,"
                                        "origin_id INTEGER NOT NULL,"
-                                       "url TEXT,"
+                                       "url TEXT NOT NULL UNIQUE,"
                                        "duration_ms INTEGER,"
                                        "position_ms INTEGER,"
                                        "last_updated_time_s BIGINT NOT NULL,"
@@ -76,7 +76,7 @@ bool MediaHistorySessionTable::SavePlaybackSession(
   sql::Statement statement(DB()->GetCachedStatement(
       SQL_FROM_HERE,
       base::StringPrintf(
-          "INSERT INTO %s "
+          "REPLACE INTO %s "
           "(origin_id, url, duration_ms, position_ms, last_updated_time_s, "
           "title, artist, album, source_title) "
           "VALUES "
@@ -104,7 +104,12 @@ bool MediaHistorySessionTable::SavePlaybackSession(
   statement.BindString16(7, metadata.album);
   statement.BindString16(8, metadata.source_title);
 
-  return statement.Run();
+  if (statement.Run()) {
+    DCHECK(DB()->GetLastInsertRowId());
+    return true;
+  }
+
+  return false;
 }
 
 base::Optional<MediaHistoryStore::MediaPlaybackSessionList>
@@ -121,16 +126,9 @@ MediaHistorySessionTable::GetPlaybackSessions(
                          kTableName)
           .c_str()));
 
-  std::set<GURL> previous_urls;
   MediaHistoryStore::MediaPlaybackSessionList sessions;
 
   while (statement.Step()) {
-    // Check if we already have a playback session for this url.
-    GURL url(statement.ColumnString(0));
-    if (base::Contains(previous_urls, url))
-      continue;
-    previous_urls.insert(url);
-
     auto duration = base::TimeDelta::FromMilliseconds(statement.ColumnInt64(1));
     auto position = base::TimeDelta::FromMilliseconds(statement.ColumnInt64(2));
 
@@ -139,7 +137,7 @@ MediaHistorySessionTable::GetPlaybackSessions(
       continue;
 
     MediaHistoryStore::MediaPlaybackSession session;
-    session.url = url;
+    session.url = GURL(statement.ColumnString(0));
     session.duration = duration;
     session.position = position;
     session.metadata.title = statement.ColumnString16(3);

chrome/browser/media/history/media_history_store_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool/pooled_sequenced_task_runner.h"
@@ -16,6 +17,8 @@
 #include "content/public/browser/media_player_watch_time.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
+#include "services/media_session/public/cpp/media_metadata.h"
+#include "services/media_session/public/cpp/media_position.h"
 #include "sql/database.h"
 #include "sql/statement.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -179,4 +182,53 @@ TEST_F(MediaHistoryStoreUnitTest, GetStats) {
   }
 }
 
+TEST_F(MediaHistoryStoreUnitTest, UrlShouldBeUniqueForSessions) {
+  GURL url_a("https://www.google.com");
+  GURL url_b("https://www.example.org");
+
+  {
+    mojom::MediaHistoryStatsPtr stats = GetStatsSync();
+    EXPECT_EQ(0, stats->table_row_counts[MediaHistorySessionTable::kTableName]);
+  }
+
+  // Save a couple of sessions on different URLs.
+  GetMediaHistoryStore()->SavePlaybackSession(
+      url_a, media_session::MediaMetadata(), base::nullopt);
+  GetMediaHistoryStore()->SavePlaybackSession(
+      url_b, media_session::MediaMetadata(), base::nullopt);
+
+  // Wait until the sessions have finished saving.
+  content::RunAllTasksUntilIdle();
+
+  {
+    mojom::MediaHistoryStatsPtr stats = GetStatsSync();
+    EXPECT_EQ(2, stats->table_row_counts[MediaHistorySessionTable::kTableName]);
+
+    sql::Statement s(GetDB().GetUniqueStatement(
+        "SELECT id FROM playbackSession WHERE url = ?"));
+    s.BindString(0, url_a.spec());
+    ASSERT_TRUE(s.Step());
+    EXPECT_EQ(1, s.ColumnInt(0));
+  }
+
+  // Save a session on the first URL.
+  GetMediaHistoryStore()->SavePlaybackSession(
+      url_a, media_session::MediaMetadata(), base::nullopt);
+
+  // Wait until the sessions have finished saving.
+  content::RunAllTasksUntilIdle();
+
+  {
+    mojom::MediaHistoryStatsPtr stats = GetStatsSync();
+    EXPECT_EQ(2, stats->table_row_counts[MediaHistorySessionTable::kTableName]);
+
+    // The row for |url_a| should have been replaced so we should have a new ID.
+    sql::Statement s(GetDB().GetUniqueStatement(
+        "SELECT id FROM playbackSession WHERE url = ?"));
+    s.BindString(0, url_a.spec());
+    ASSERT_TRUE(s.Step());
+    EXPECT_EQ(3, s.ColumnInt(0));
+  }
+}
+
 }  // namespace media_history