[Media History] Fix for multiple origins

We were using GURL/url::Origin in different
places for storing origins which meant some
had a trailing / and some did not. This
resulted in duplicates.

BUG=1051204

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

chrome/browser/media/history/media_history_origin_table.cc

@@ -4,13 +4,23 @@
 
 #include "chrome/browser/media/history/media_history_origin_table.h"
 
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "sql/statement.h"
+#include "url/origin.h"
 
 namespace media_history {
 
 const char MediaHistoryOriginTable::kTableName[] = "origin";
 
+// static
+std::string MediaHistoryOriginTable::GetOriginForStorage(
+    const url::Origin& origin) {
+  auto str = origin.Serialize();
+  base::TrimString(str, "/", base::TrimPositions::TRIM_TRAILING);
+  return str;
+}
+
 MediaHistoryOriginTable::MediaHistoryOriginTable(
     scoped_refptr<base::UpdateableSequencedTaskRunner> db_task_runner)
     : MediaHistoryTableBase(std::move(db_task_runner)) {}
@@ -44,7 +54,7 @@ sql::InitStatus MediaHistoryOriginTable::CreateTableIfNonExistent() {
   return sql::INIT_OK;
 }
 
-bool MediaHistoryOriginTable::CreateOriginId(const std::string& origin) {
+bool MediaHistoryOriginTable::CreateOriginId(const url::Origin& origin) {
   DCHECK_LT(0, DB()->transaction_nesting());
   if (!CanAccessDatabase())
     return false;
@@ -56,7 +66,7 @@ bool MediaHistoryOriginTable::CreateOriginId(const std::string& origin) {
                          "(origin, last_updated_time_s) VALUES (?, ?)",
                          kTableName)
           .c_str()));
-  statement.BindString(0, origin);
+  statement.BindString(0, GetOriginForStorage(origin));
   statement.BindInt64(1,
                       base::Time::Now().ToDeltaSinceWindowsEpoch().InSeconds());
   if (!statement.Run()) {
@@ -68,7 +78,7 @@ bool MediaHistoryOriginTable::CreateOriginId(const std::string& origin) {
 }
 
 bool MediaHistoryOriginTable::IncrementAggregateAudioVideoWatchTime(
-    const std::string& origin,
+    const url::Origin& origin,
     const base::TimeDelta& time) {
   DCHECK_LT(0, DB()->transaction_nesting());
   if (!CanAccessDatabase())
@@ -87,7 +97,7 @@ bool MediaHistoryOriginTable::IncrementAggregateAudioVideoWatchTime(
   statement.BindInt64(0, time.InSeconds());
   statement.BindInt64(1,
                       base::Time::Now().ToDeltaSinceWindowsEpoch().InSeconds());
-  statement.BindString(2, origin);
+  statement.BindString(2, GetOriginForStorage(origin));
 
   if (!statement.Run()) {
     LOG(ERROR) << "Failed to update the watchtime.";

chrome/browser/media/history/media_history_origin_table.h

@@ -9,12 +9,19 @@
 #include "chrome/browser/media/history/media_history_table_base.h"
 #include "sql/init_status.h"
 
+namespace url {
+class Origin;
+}  // namespace url
+
 namespace media_history {
 
 class MediaHistoryOriginTable : public MediaHistoryTableBase {
  public:
   static const char kTableName[];
 
+  // Returns the origin as a string for storage.
+  static std::string GetOriginForStorage(const url::Origin& origin);
+
  private:
   friend class MediaHistoryStoreInternal;
 
@@ -26,10 +33,10 @@ class MediaHistoryOriginTable : public MediaHistoryTableBase {
   sql::InitStatus CreateTableIfNonExistent() override;
 
   // Returns a flag indicating whether the origin id was created successfully.
-  bool CreateOriginId(const std::string& origin);
+  bool CreateOriginId(const url::Origin& origin);
 
   // Returns a flag indicating whether watchtime was increased successfully.
-  bool IncrementAggregateAudioVideoWatchTime(const std::string& origin,
+  bool IncrementAggregateAudioVideoWatchTime(const url::Origin& origin,
                                              const base::TimeDelta& time);
 
   DISALLOW_COPY_AND_ASSIGN(MediaHistoryOriginTable);

chrome/browser/media/history/media_history_playback_table.cc

@@ -6,6 +6,7 @@
 
 #include "base/strings/stringprintf.h"
 #include "base/updateable_sequenced_task_runner.h"
+#include "chrome/browser/media/history/media_history_origin_table.h"
 #include "content/public/browser/media_player_watch_time.h"
 #include "sql/statement.h"
 
@@ -72,7 +73,8 @@ bool MediaHistoryPlaybackTable::SavePlayback(
                          "?, ?, ?, ?, ?)",
                          kTableName)
           .c_str()));
-  statement.BindString(0, watch_time.origin.spec());
+  statement.BindString(0, MediaHistoryOriginTable::GetOriginForStorage(
+                              url::Origin::Create(watch_time.origin)));
   statement.BindString(1, watch_time.url.spec());
   statement.BindInt64(2, watch_time.cumulative_watch_time.InSeconds());
   statement.BindInt(3, watch_time.has_video);

chrome/browser/media/history/media_history_session_table.cc

@@ -7,6 +7,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/updateable_sequenced_task_runner.h"
+#include "chrome/browser/media/history/media_history_origin_table.h"
 #include "chrome/browser/media/history/media_history_store.h"
 #include "services/media_session/public/cpp/media_image.h"
 #include "services/media_session/public/cpp/media_metadata.h"
@@ -84,7 +85,7 @@ base::Optional<int64_t> MediaHistorySessionTable::SavePlaybackSession(
           "((SELECT id FROM origin WHERE origin = ?), ?, ?, ?, ?, ?, ?, ?, ?)",
           kTableName)
           .c_str()));
-  statement.BindString(0, origin.Serialize());
+  statement.BindString(0, MediaHistoryOriginTable::GetOriginForStorage(origin));
   statement.BindString(1, url.spec());
 
   if (position.has_value()) {

chrome/browser/media/history/media_history_store.cc

@@ -59,7 +59,7 @@ class MediaHistoryStoreInternal
   sql::Database* DB();
 
   // Returns a flag indicating whether the origin id was created successfully.
-  bool CreateOriginId(const std::string& origin);
+  bool CreateOriginId(const url::Origin& origin);
 
   void SavePlayback(const content::MediaPlayerWatchTime& watch_time);
 
@@ -135,7 +135,7 @@ void MediaHistoryStoreInternal::SavePlayback(
     return;
   }
 
-  auto origin = watch_time.origin.spec();
+  auto origin = url::Origin::Create(watch_time.origin);
 
   if (!(CreateOriginId(origin) && playback_table_->SavePlayback(watch_time))) {
     DB()->RollbackTransaction();
@@ -211,7 +211,7 @@ sql::InitStatus MediaHistoryStoreInternal::InitializeTables() {
   return status;
 }
 
-bool MediaHistoryStoreInternal::CreateOriginId(const std::string& origin) {
+bool MediaHistoryStoreInternal::CreateOriginId(const url::Origin& origin) {
   DCHECK(db_task_runner_->RunsTasksInCurrentSequence());
   if (!initialization_successful_)
     return false;
@@ -320,7 +320,7 @@ void MediaHistoryStoreInternal::SavePlaybackSession(
   }
 
   auto origin = url::Origin::Create(url);
-  if (!CreateOriginId(origin.Serialize())) {
+  if (!CreateOriginId(origin)) {
     DB()->RollbackTransaction();
     return;
   }