[ReadingList] Make ReadingListEntry refcounted

This CL makes ReadingListEntry refcounted. It allows
DualReadingListModel to, in some cases, compute a temporary
ReadingListEntry inside GetEntryByURL(), and return it to the caller
without subtle ownership contracts in the ReadingListModel API. For more
context, see crrev.com/c/4218993.

Bug: 1402196
Change-Id: I1026c9c23f3cb7eb03dd6f1c00c5d9f99701728a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4227345
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Mahmoud Rashad <mmrashad@google.com>
Cr-Commit-Position: refs/heads/main@{#1103916}

chrome/browser/reading_list/android/reading_list_manager_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/guid.h"
 #include "base/logging.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/grit/generated_resources.h"
@@ -70,7 +71,7 @@ void ReadingListManagerImpl::ReadingListModelLoaded(
   // Constructs the bookmark tree.
   root_->DeleteAll();
   for (const auto& url : model->GetKeys()) {
-    AddOrUpdateBookmark(model->GetEntryByURL(url));
+    AddOrUpdateBookmark(model->GetEntryByURL(url).get());
   }
 
   loaded_ = true;
@@ -83,7 +84,7 @@ void ReadingListManagerImpl::ReadingListDidAddEntry(
     const ReadingListModel* model,
     const GURL& url,
     reading_list::EntrySource source) {
-  AddOrUpdateBookmark(model->GetEntryByURL(url));
+  AddOrUpdateBookmark(model->GetEntryByURL(url).get());
 }
 
 void ReadingListManagerImpl::ReadingListWillRemoveEntry(
@@ -96,18 +97,20 @@ void ReadingListManagerImpl::ReadingListDidMoveEntry(
     const ReadingListModel* model,
     const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-  const auto* moved_entry = reading_list_model_->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> moved_entry =
+      reading_list_model_->GetEntryByURL(url);
   DCHECK(moved_entry);
-  AddOrUpdateBookmark(moved_entry);
+  AddOrUpdateBookmark(moved_entry.get());
 }
 
 void ReadingListManagerImpl::ReadingListDidUpdateEntry(
     const ReadingListModel* model,
     const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-  const auto* updated_entry = reading_list_model_->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> updated_entry =
+      reading_list_model_->GetEntryByURL(url);
   DCHECK(updated_entry);
-  AddOrUpdateBookmark(updated_entry);
+  AddOrUpdateBookmark(updated_entry.get());
 }
 
 void ReadingListManagerImpl::ReadingListDidApplyChanges(
@@ -224,7 +227,8 @@ size_t ReadingListManagerImpl::unread_size() const {
 void ReadingListManagerImpl::SetTitle(const GURL& url,
                                       const std::u16string& title) {
   DCHECK(reading_list_model_->loaded());
-  const auto* entry = reading_list_model_->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> entry =
+      reading_list_model_->GetEntryByURL(url);
   if (!entry)
     return;
 
@@ -238,7 +242,8 @@ void ReadingListManagerImpl::SetTitle(const GURL& url,
 
 void ReadingListManagerImpl::SetReadStatus(const GURL& url, bool read) {
   DCHECK(reading_list_model_->loaded());
-  const auto* entry = reading_list_model_->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> entry =
+      reading_list_model_->GetEntryByURL(url);
   if (!entry)
     return;
 

chrome/browser/reading_list/android/reading_list_manager_impl_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/guid.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/simple_test_clock.h"
 #include "chrome/browser/reading_list/android/reading_list_manager.h"
@@ -119,9 +120,10 @@ TEST_F(ReadingListManagerImplTest, Load) {
   ASSERT_FALSE(manager()->IsLoaded());
 
   // Mimic the completion of storage loading with one initial entry.
-  std::vector<ReadingListEntry> entries;
+  std::vector<scoped_refptr<ReadingListEntry>> entries;
   GURL url(kURL);
-  entries.emplace_back(url, kTitle, clock()->Now());
+  entries.push_back(
+      base::MakeRefCounted<ReadingListEntry>(url, kTitle, clock()->Now()));
   ASSERT_TRUE(fake_storage->TriggerLoadCompletion(std::move(entries)));
   EXPECT_TRUE(manager()->IsLoaded());
 

chrome/browser/ui/browser_commands.cc

@@ -10,6 +10,7 @@
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
@@ -1234,7 +1235,7 @@ bool MarkCurrentTabAsReadInReadLater(Browser* browser) {
       browser->tab_strip_model()->GetActiveWebContents();
   if (!model || !GetTabURLAndTitleToSave(web_contents, &url, &title))
     return false;
-  const ReadingListEntry* entry = model->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> entry = model->GetEntryByURL(url);
   // Mark current tab as read.
   if (entry && !entry->IsRead())
     model->SetReadStatusIfExists(url, true);
@@ -1249,7 +1250,7 @@ bool IsCurrentTabUnreadInReadLater(Browser* browser) {
       browser->tab_strip_model()->GetActiveWebContents();
   if (!model || !GetTabURLAndTitleToSave(web_contents, &url, &title))
     return false;
-  const ReadingListEntry* entry = model->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> entry = model->GetEntryByURL(url);
   return entry && !entry->IsRead();
 }
 

chrome/browser/ui/webui/side_panel/reading_list/reading_list_page_handler.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
 #include "base/strings/utf_string_conversions.h"
@@ -182,7 +183,8 @@ void ReadingListPageHandler::OpenURL(
                                 ui::PAGE_TRANSITION_AUTO_BOOKMARK, false);
   browser->OpenURL(params);
 
-  const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+  scoped_refptr<const ReadingListEntry> entry =
+      reading_list_model_->GetEntryByURL(url);
   if (entry) {
     base::RecordAction(base::UserMetricsAction(
         entry->IsRead() ? "DesktopReadingList.Navigation.FromReadList"
@@ -333,12 +335,13 @@ ReadingListPageHandler::CreateReadLaterEntriesByStatusData() {
   auto entries = reading_list::mojom::ReadLaterEntriesByStatus::New();
 
   for (const auto& url : reading_list_model_->GetKeys()) {
-    const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+    scoped_refptr<const ReadingListEntry> entry =
+        reading_list_model_->GetEntryByURL(url);
     DCHECK(entry);
     if (entry->IsRead()) {
-      entries->read_entries.push_back(GetEntryData(entry));
+      entries->read_entries.push_back(GetEntryData(entry.get()));
     } else {
-      entries->unread_entries.push_back(GetEntryData(entry));
+      entries->unread_entries.push_back(GetEntryData(entry.get()));
     }
   }
 

components/reading_list/core/dual_reading_list_model.cc

@@ -4,6 +4,7 @@
 
 #include "components/reading_list/core/dual_reading_list_model.h"
 
+#include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
 #include "base/stl_util.h"
 #include "components/reading_list/features/reading_list_switches.h"
@@ -120,7 +121,7 @@ bool DualReadingListModel::DeleteAllEntries() {
   return false;
 }
 
-const ReadingListEntry* DualReadingListModel::GetEntryByURL(
+scoped_refptr<const ReadingListEntry> DualReadingListModel::GetEntryByURL(
     const GURL& gurl) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(crbug.com/1402196): Implement.

components/reading_list/core/dual_reading_list_model.h

@@ -9,6 +9,7 @@
 #include <memory>
 
 #include "base/memory/raw_ptr.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
 #include "base/sequence_checker.h"
 #include "components/reading_list/core/reading_list_entry.h"
@@ -50,7 +51,8 @@ class DualReadingListModel : public ReadingListModel,
   size_t unseen_size() const override;
   void MarkAllSeen() override;
   bool DeleteAllEntries() override;
-  const ReadingListEntry* GetEntryByURL(const GURL& gurl) const override;
+  scoped_refptr<const ReadingListEntry> GetEntryByURL(
+      const GURL& gurl) const override;
   bool IsUrlSupported(const GURL& url) override;
   const ReadingListEntry& AddOrReplaceEntry(
       const GURL& url,

components/reading_list/core/dual_reading_list_model_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/reading_list/core/dual_reading_list_model.h"
 
+#include "base/memory/scoped_refptr.h"
 #include "base/test/simple_test_clock.h"
 #include "components/reading_list/core/fake_reading_list_model_storage.h"
 #include "components/reading_list/core/mock_reading_list_model_observer.h"
@@ -51,16 +52,16 @@ class DualReadingListModelTest : public testing::Test {
       const std::vector<GURL>& initial_account_urls = {}) {
     ResetStorage();
 
-    std::vector<ReadingListEntry> initial_local_entries;
+    std::vector<scoped_refptr<ReadingListEntry>> initial_local_entries;
     for (const auto& url : initial_local_urls) {
-      initial_local_entries.emplace_back(url, "Title for " + url.spec(),
-                                         clock_.Now());
+      initial_local_entries.push_back(base::MakeRefCounted<ReadingListEntry>(
+          url, "Title for " + url.spec(), clock_.Now()));
     }
 
-    std::vector<ReadingListEntry> initial_account_entries;
+    std::vector<scoped_refptr<ReadingListEntry>> initial_account_entries;
     for (const auto& url : initial_account_urls) {
-      initial_account_entries.emplace_back(url, "Title for " + url.spec(),
-                                           clock_.Now());
+      initial_account_entries.push_back(base::MakeRefCounted<ReadingListEntry>(
+          url, "Title for " + url.spec(), clock_.Now()));
     }
 
     return local_or_syncable_model_storage_ptr_->TriggerLoadCompletion(
@@ -72,7 +73,8 @@ class DualReadingListModelTest : public testing::Test {
   size_t UnreadSize() {
     size_t size = 0;
     for (const auto& url : dual_model_->GetKeys()) {
-      const ReadingListEntry* entry = dual_model_->GetEntryByURL(url);
+      scoped_refptr<const ReadingListEntry> entry =
+          dual_model_->GetEntryByURL(url);
       if (!entry->IsRead()) {
         size++;
       }
@@ -84,7 +86,8 @@ class DualReadingListModelTest : public testing::Test {
   size_t ReadSize() {
     size_t size = 0;
     for (const auto& url : dual_model_->GetKeys()) {
-      const ReadingListEntry* entry = dual_model_->GetEntryByURL(url);
+      scoped_refptr<const ReadingListEntry> entry =
+          dual_model_->GetEntryByURL(url);
       if (entry->IsRead()) {
         size++;
       }

components/reading_list/core/fake_reading_list_model_storage.cc

@@ -4,6 +4,7 @@
 
 #include "components/reading_list/core/fake_reading_list_model_storage.h"
 
+#include "base/memory/scoped_refptr.h"
 #include "components/sync/model/metadata_batch.h"
 
 FakeReadingListModelStorage::FakeScopedBatchUpdate::FakeScopedBatchUpdate(
@@ -51,12 +52,12 @@ bool FakeReadingListModelStorage::TriggerLoadCompletion(
 }
 
 bool FakeReadingListModelStorage::TriggerLoadCompletion(
-    std::vector<ReadingListEntry> entries) {
+    std::vector<scoped_refptr<ReadingListEntry>> entries) {
   LoadResult result;
   result.second = std::make_unique<syncer::MetadataBatch>();
-  for (ReadingListEntry& entry : entries) {
-    GURL url = entry.URL();
-    result.first.emplace(entry.URL(), std::move(entry));
+  for (auto& entry : entries) {
+    GURL url = entry->URL();
+    result.first.emplace(entry->URL(), std::move(entry));
   }
   return TriggerLoadCompletion(std::move(result));
 }

components/reading_list/core/fake_reading_list_model_storage.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "components/reading_list/core/reading_list_entry.h"
 #include "components/reading_list/core/reading_list_model_storage.h"
@@ -57,7 +58,8 @@ class FakeReadingListModelStorage
 
   // Convenience overload that uses sensible defaults (empty store) for success
   // case.
-  bool TriggerLoadCompletion(std::vector<ReadingListEntry> entries = {});
+  bool TriggerLoadCompletion(
+      std::vector<scoped_refptr<ReadingListEntry>> entries = {});
 
   // ReadingListModelStorage implementation.
   void Load(base::Clock* clock, LoadCallback load_cb) override;

components/reading_list/core/reading_list_entry.cc

@@ -8,6 +8,7 @@
 
 #include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
@@ -118,23 +119,6 @@ ReadingListEntry::ReadingListEntry(
   DCHECK(url.is_valid());
 }
 
-ReadingListEntry::ReadingListEntry(ReadingListEntry&& entry)
-    : url_(std::move(entry.url_)),
-      title_(std::move(entry.title_)),
-      estimated_read_time_(std::move(entry.estimated_read_time_)),
-      state_(std::move(entry.state_)),
-      distilled_path_(std::move(entry.distilled_path_)),
-      distilled_url_(std::move(entry.distilled_url_)),
-      distilled_state_(std::move(entry.distilled_state_)),
-      backoff_(std::move(entry.backoff_)),
-      failed_download_counter_(std::move(entry.failed_download_counter_)),
-      creation_time_us_(std::move(entry.creation_time_us_)),
-      first_read_time_us_(std::move(entry.first_read_time_us_)),
-      update_time_us_(std::move(entry.update_time_us_)),
-      update_title_time_us_(std::move(entry.update_title_time_us_)),
-      distillation_time_us_(std::move(entry.distillation_time_us_)),
-      distillation_size_(std::move(entry.distillation_size_)) {}
-
 ReadingListEntry::~ReadingListEntry() {}
 
 const GURL& ReadingListEntry::URL() const {
@@ -177,25 +161,6 @@ int ReadingListEntry::FailedDownloadCounter() const {
   return failed_download_counter_;
 }
 
-ReadingListEntry& ReadingListEntry::operator=(ReadingListEntry&& other) {
-  url_ = std::move(other.url_);
-  title_ = std::move(other.title_);
-  estimated_read_time_ = std::move(other.estimated_read_time_);
-  distilled_path_ = std::move(other.distilled_path_);
-  distilled_url_ = std::move(other.distilled_url_);
-  distilled_state_ = std::move(other.distilled_state_);
-  backoff_ = std::move(other.backoff_);
-  state_ = std::move(other.state_);
-  failed_download_counter_ = std::move(other.failed_download_counter_);
-  creation_time_us_ = std::move(other.creation_time_us_);
-  first_read_time_us_ = std::move(other.first_read_time_us_);
-  update_time_us_ = std::move(other.update_time_us_);
-  update_title_time_us_ = std::move(other.update_title_time_us_);
-  distillation_time_us_ = std::move(other.distillation_time_us_);
-  distillation_size_ = std::move(other.distillation_size_);
-  return *this;
-}
-
 bool ReadingListEntry::operator==(const ReadingListEntry& other) const {
   return url_ == other.url_;
 }
@@ -296,7 +261,7 @@ void ReadingListEntry::MarkEntryUpdated(const base::Time& now) {
 }
 
 // static
-std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
+scoped_refptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
     const reading_list::ReadingListLocal& pb_entry,
     const base::Time& now) {
   if (!pb_entry.has_url()) {
@@ -416,15 +381,16 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
     }
   }
 
-  return base::WrapUnique<ReadingListEntry>(new ReadingListEntry(
+  // MakeRefCounted cannot be used because this constructor is private.
+  return base::WrapRefCounted<ReadingListEntry>(new ReadingListEntry(
       url, title, estimated_read_time, state, creation_time_us,
       first_read_time_us, update_time_us, update_title_time_us,
       distillation_state, distilled_path, distilled_url, distillation_time_us,
       distillation_size, failed_download_counter, std::move(backoff)));
 }
 
 // static
-std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
+scoped_refptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
     const sync_pb::ReadingListSpecifics& pb_entry,
     const base::Time& now) {
   if (!pb_entry.has_url()) {
@@ -483,7 +449,7 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
     }
   }
 
-  return base::WrapUnique<ReadingListEntry>(new ReadingListEntry(
+  return base::WrapRefCounted<ReadingListEntry>(new ReadingListEntry(
       url, title, estimated_read_time, state, creation_time_us,
       first_read_time_us, update_time_us, update_title_time_us, WAITING,
       base::FilePath(), GURL(), 0, 0, 0, nullptr));