ReadingList: Introduce ReadingListModel::GetAccountWhereEntryIsSavedTo()

This CL introduces the ReadingListModel::GetAccountWhereEntryIsSavedTo()
method and implements it in ReadingListModelImpl and
DualReadingListModel. The method receives a URL as a parameter and
identifies which account is syncing the entry that has the given URL. If
no such account exists, an empty account will be returned.

Bug: 1402196
Change-Id: I20b0faa34f8e56fd06d5f4d286016d98e9a79341
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4324257
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Mahmoud Rashad <mmrashad@google.com>
Cr-Commit-Position: refs/heads/main@{#1115841}

components/reading_list/DEPS

@@ -2,5 +2,6 @@ include_rules = [
   "+components/keyed_service",
   "+components/prefs",
   "+components/sync",
+  "+google_apis",
   "+net",
 ]

components/reading_list/core/BUILD.gn

@@ -35,6 +35,7 @@ source_set("core") {
   public_deps = [
     "//base",
     "//components/reading_list/core/proto",
+    "//google_apis",
   ]
 }
 

components/reading_list/core/dual_reading_list_model.cc

@@ -12,6 +12,7 @@
 #include "components/reading_list/core/reading_list_entry.h"
 #include "components/reading_list/core/reading_list_model_impl.h"
 #include "components/reading_list/features/reading_list_switches.h"
+#include "google_apis/gaia/core_account_id.h"
 #include "url/gurl.h"
 
 namespace reading_list {
@@ -180,6 +181,20 @@ bool DualReadingListModel::IsUrlSupported(const GURL& url) {
   return local_or_syncable_model_->IsUrlSupported(url);
 }
 
+CoreAccountId DualReadingListModel::GetAccountWhereEntryIsSavedTo(
+    const GURL& url) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(loaded());
+
+  CoreAccountId account_id = account_model_->GetAccountWhereEntryIsSavedTo(url);
+  if (!account_id.empty()) {
+    return account_id;
+  }
+  // `local_or_syncable_model_` may return an account for the case where it's
+  // sync-ing.
+  return local_or_syncable_model_->GetAccountWhereEntryIsSavedTo(url);
+}
+
 bool DualReadingListModel::NeedsExplicitUploadToSyncServer(
     const GURL& url) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -208,11 +223,17 @@ const ReadingListEntry& DualReadingListModel::AddOrReplaceEntry(
   }
 
   if (account_model_->IsTrackingSyncMetadata()) {
-    return account_model_->AddOrReplaceEntry(url, title, source,
-                                             estimated_read_time);
+    const ReadingListEntry& entry = account_model_->AddOrReplaceEntry(
+        url, title, source, estimated_read_time);
+    DCHECK(!GetAccountWhereEntryIsSavedTo(url).empty());
+    return entry;
   }
-  return local_or_syncable_model_->AddOrReplaceEntry(url, title, source,
-                                                     estimated_read_time);
+
+  const ReadingListEntry& entry = local_or_syncable_model_->AddOrReplaceEntry(
+      url, title, source, estimated_read_time);
+  DCHECK(!local_or_syncable_model_->IsTrackingSyncMetadata() ||
+         !GetAccountWhereEntryIsSavedTo(url).empty());
+  return entry;
 }
 
 void DualReadingListModel::RemoveEntryByURL(const GURL& url) {

components/reading_list/core/dual_reading_list_model.h

@@ -16,6 +16,7 @@
 #include "components/reading_list/core/reading_list_model.h"
 #include "components/reading_list/core/reading_list_model_impl.h"
 #include "components/reading_list/core/reading_list_model_observer.h"
+#include "google_apis/gaia/core_account_id.h"
 #include "url/gurl.h"
 
 namespace reading_list {
@@ -63,6 +64,7 @@ class DualReadingListModel : public ReadingListModel,
   scoped_refptr<const ReadingListEntry> GetEntryByURL(
       const GURL& gurl) const override;
   bool IsUrlSupported(const GURL& url) override;
+  CoreAccountId GetAccountWhereEntryIsSavedTo(const GURL& url) override;
   bool NeedsExplicitUploadToSyncServer(const GURL& url) const override;
   const ReadingListEntry& AddOrReplaceEntry(
       const GURL& url,

components/reading_list/core/dual_reading_list_model_unittest.cc

@@ -188,6 +188,7 @@ class DualReadingListModelTest : public testing::Test {
     auto metadata_batch = std::make_unique<syncer::MetadataBatch>();
     sync_pb::ModelTypeState state;
     state.set_initial_sync_done(true);
+    state.set_authenticated_account_id(kTestAccountId);
     metadata_batch->SetModelTypeState(state);
 
     std::vector<scoped_refptr<ReadingListEntry>> initial_local_entries;
@@ -213,6 +214,7 @@ class DualReadingListModelTest : public testing::Test {
     auto metadata_batch = std::make_unique<syncer::MetadataBatch>();
     sync_pb::ModelTypeState state;
     state.set_initial_sync_done(true);
+    state.set_authenticated_account_id(kTestAccountId);
     metadata_batch->SetModelTypeState(state);
 
     std::vector<scoped_refptr<ReadingListEntry>> initial_syncable_entries;
@@ -228,6 +230,7 @@ class DualReadingListModelTest : public testing::Test {
 
  protected:
   const GURL kUrl = GURL("http://url.com/");
+  const std::string kTestAccountId = "TestAccountId";
   base::SimpleTestClock clock_;
   testing::NiceMock<MockReadingListModelObserver> observer_;
   base::WeakPtr<FakeReadingListModelStorage>
@@ -468,6 +471,64 @@ TEST_F(DualReadingListModelTest, DeleteAllEntries) {
   EXPECT_THAT(dual_model_->GetEntryByURL(common_url), IsNull());
 }
 
+TEST_F(DualReadingListModelTest, GetAccountWhereEntryIsSavedToWhenSignedOut) {
+  ASSERT_TRUE(ResetStorageAndMimicSignedOut(/*initial_local_entries_builders=*/{
+      TestEntryBuilder(kUrl, clock_.Now())}));
+  ASSERT_EQ(dual_model_->GetStorageStateForURLForTesting(kUrl),
+            StorageStateForTesting::kExistsInLocalOrSyncableModelOnly);
+
+  EXPECT_TRUE(dual_model_->GetAccountWhereEntryIsSavedTo(kUrl).empty());
+  EXPECT_TRUE(
+      dual_model_
+          ->GetAccountWhereEntryIsSavedTo(GURL("http://non_existing_url.com/"))
+          .empty());
+}
+
+TEST_F(DualReadingListModelTest,
+       GetAccountWhereEntryIsSavedToWhenSignedInSyncDisabled) {
+  const GURL kLocalUrl("http://local_url.com/");
+  const GURL kAccountUrl("http://account_url.com/");
+  const GURL kCommonUrl("http://common_url.com/");
+  ASSERT_TRUE(ResetStorageAndMimicSignedInSyncDisabled(
+      /*initial_local_entries_builders=*/
+      {TestEntryBuilder(kLocalUrl, clock_.Now()),
+       TestEntryBuilder(kCommonUrl, clock_.Now())},
+      /*initial_account_entries_builders=*/{
+          TestEntryBuilder(kAccountUrl, clock_.Now()),
+          TestEntryBuilder(kCommonUrl, clock_.Now())}));
+  ASSERT_EQ(dual_model_->GetStorageStateForURLForTesting(kLocalUrl),
+            StorageStateForTesting::kExistsInLocalOrSyncableModelOnly);
+  ASSERT_EQ(dual_model_->GetStorageStateForURLForTesting(kAccountUrl),
+            StorageStateForTesting::kExistsInAccountModelOnly);
+  ASSERT_EQ(dual_model_->GetStorageStateForURLForTesting(kCommonUrl),
+            StorageStateForTesting::kExistsInBothModels);
+
+  EXPECT_TRUE(dual_model_->GetAccountWhereEntryIsSavedTo(kLocalUrl).empty());
+  EXPECT_EQ(dual_model_->GetAccountWhereEntryIsSavedTo(kAccountUrl).ToString(),
+            kTestAccountId);
+  EXPECT_EQ(dual_model_->GetAccountWhereEntryIsSavedTo(kCommonUrl).ToString(),
+            kTestAccountId);
+  EXPECT_TRUE(
+      dual_model_
+          ->GetAccountWhereEntryIsSavedTo(GURL("http://non_existing_url.com/"))
+          .empty());
+}
+
+TEST_F(DualReadingListModelTest, GetAccountWhereEntryIsSavedToWhenSyncEnabled) {
+  ASSERT_TRUE(
+      ResetStorageAndMimicSyncEnabled(/*initial_syncable_entries_builders=*/{
+          TestEntryBuilder(kUrl, clock_.Now())}));
+  ASSERT_EQ(dual_model_->GetStorageStateForURLForTesting(kUrl),
+            StorageStateForTesting::kExistsInLocalOrSyncableModelOnly);
+
+  EXPECT_EQ(dual_model_->GetAccountWhereEntryIsSavedTo(kUrl).ToString(),
+            kTestAccountId);
+  EXPECT_TRUE(
+      dual_model_
+          ->GetAccountWhereEntryIsSavedTo(GURL("http://non_existing_url.com/"))
+          .empty());
+}
+
 TEST_F(DualReadingListModelTest, NeedsExplicitUploadToSyncServerWhenSignedOut) {
   ASSERT_TRUE(ResetStorageAndMimicSignedOut(/*initial_local_entries_builders=*/{
       TestEntryBuilder(kUrl, clock_.Now())}));

components/reading_list/core/reading_list_model.h

@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/reading_list/core/reading_list_entry.h"
+#include "google_apis/gaia/core_account_id.h"
 
 class GURL;
 class ReadingListModelObserver;
@@ -83,12 +84,22 @@ class ReadingListModel : public KeyedService {
   virtual bool DeleteAllEntries() = 0;
 
   // Returns a specific entry. Returns null if the entry does not exist.
+  // Please note that the value saved to the account may not be identical to the
+  // value returned by GetEntryByURL(). This may happen if DualReadingListModel
+  // is dealing with two entries with the same URL, one local and one from the
+  // account. In this case, GetEntryByURL() will return the merged view of the
+  // two entries.
   virtual scoped_refptr<const ReadingListEntry> GetEntryByURL(
       const GURL& gurl) const = 0;
 
   // Returns true if |url| can be added to the reading list.
   virtual bool IsUrlSupported(const GURL& url) = 0;
 
+  // If an account exists that syncs the entry which has the given `url`, that
+  // account will be returned. Otherwise, the entry may be saved locally on the
+  // device or may not exist, in that case an empty account will be returned.
+  virtual CoreAccountId GetAccountWhereEntryIsSavedTo(const GURL& url) = 0;
+
   // Returns true if the entry with `url` requires explicit user action to
   // upload to sync servers.
   virtual bool NeedsExplicitUploadToSyncServer(const GURL& url) const = 0;
@@ -97,7 +108,9 @@ class ReadingListModel : public KeyedService {
   // same |url| from everywhere else if they exist. The entry title will be a
   // trimmed copy of |title|. |time_to_read_minutes| is the estimated time to
   // read the page. The addition may be asynchronous, and the data will be
-  // available only once the observers are notified.
+  // available only once the observers are notified. Callers may use
+  // GetAccountWhereEntryIsSavedTo() to determine whether the result of this
+  // operation lead to data being saved to a particular account.
   virtual const ReadingListEntry& AddOrReplaceEntry(
       const GURL& url,
       const std::string& title,

components/reading_list/core/reading_list_model_impl.cc

@@ -18,6 +18,7 @@
 #include "components/reading_list/core/reading_list_model_storage.h"
 #include "components/reading_list/core/reading_list_sync_bridge.h"
 #include "components/sync/model/client_tag_based_model_type_processor.h"
+#include "google_apis/gaia/core_account_id.h"
 #include "url/gurl.h"
 
 ReadingListModelImpl::ScopedReadingListBatchUpdateImpl::
@@ -327,6 +328,18 @@ bool ReadingListModelImpl::IsUrlSupported(const GURL& url) {
   return url.SchemeIsHTTPOrHTTPS();
 }
 
+CoreAccountId ReadingListModelImpl::GetAccountWhereEntryIsSavedTo(
+    const GURL& url) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(loaded());
+
+  if (entries_.find(url) == entries_.end()) {
+    return CoreAccountId();
+  }
+  return CoreAccountId::FromString(
+      sync_bridge_.change_processor()->TrackedAccountId());
+}
+
 bool ReadingListModelImpl::NeedsExplicitUploadToSyncServer(
     const GURL& url) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

components/reading_list/core/reading_list_model_impl.h

@@ -18,6 +18,7 @@
 #include "components/reading_list/core/reading_list_model_storage.h"
 #include "components/reading_list/core/reading_list_sync_bridge.h"
 #include "components/sync/base/storage_type.h"
+#include "google_apis/gaia/core_account_id.h"
 
 namespace base {
 class Clock;
@@ -60,6 +61,7 @@ class ReadingListModelImpl : public ReadingListModel {
   scoped_refptr<const ReadingListEntry> GetEntryByURL(
       const GURL& gurl) const override;
   bool IsUrlSupported(const GURL& url) override;
+  CoreAccountId GetAccountWhereEntryIsSavedTo(const GURL& url) override;
   bool NeedsExplicitUploadToSyncServer(const GURL& url) const override;
   const ReadingListEntry& AddOrReplaceEntry(
       const GURL& url,

components/reading_list/core/reading_list_model_unittest.cc

@@ -86,6 +86,26 @@ class ReadingListModelTest : public FakeReadingListModelStorage::Observer,
     return storage_ptr;
   }
 
+  bool ResetStorageAndMimicSignedOut(
+      std::vector<scoped_refptr<ReadingListEntry>> initial_entries = {}) {
+    return ResetStorage()->TriggerLoadCompletion(std::move(initial_entries));
+  }
+
+  bool ResetStorageAndMimicSyncEnabled(
+      std::vector<scoped_refptr<ReadingListEntry>> initial_syncable_entries =
+          {}) {
+    base::WeakPtr<FakeReadingListModelStorage> storage = ResetStorage();
+
+    auto metadata_batch = std::make_unique<syncer::MetadataBatch>();
+    sync_pb::ModelTypeState state;
+    state.set_initial_sync_done(true);
+    state.set_authenticated_account_id(kTestAccountId);
+    metadata_batch->SetModelTypeState(state);
+
+    return storage->TriggerLoadCompletion(std::move(initial_syncable_entries),
+                                          std::move(metadata_batch));
+  }
+
   void ClearCounts() {
     testing::Mock::VerifyAndClearExpectations(&observer_);
     storage_saved_ = 0;
@@ -120,6 +140,8 @@ class ReadingListModelTest : public FakeReadingListModelStorage::Observer,
   }
 
  protected:
+  const std::string kTestAccountId = "TestAccountId";
+
   int storage_saved_ = 0;
   int storage_removed_ = 0;
 
@@ -227,6 +249,33 @@ TEST_F(ReadingListModelTest, DeleteAllEntries) {
   EXPECT_THAT(model_->GetEntryByURL(example2), IsNull());
 }
 
+TEST_F(ReadingListModelTest, GetAccountWhereEntryIsSavedToWhenSignedOut) {
+  const GURL example("http://example.com/");
+  ASSERT_TRUE(ResetStorageAndMimicSignedOut(
+      /*initial_entries=*/{base::MakeRefCounted<ReadingListEntry>(
+          example, "example_title", clock_.Now())}));
+
+  EXPECT_TRUE(model_->GetAccountWhereEntryIsSavedTo(example).empty());
+  EXPECT_TRUE(
+      model_
+          ->GetAccountWhereEntryIsSavedTo(GURL("http://non_existing_url.com/"))
+          .empty());
+}
+
+TEST_F(ReadingListModelTest, GetAccountWhereEntryIsSavedToWhenSyncEnabled) {
+  const GURL example("http://example.com/");
+  ASSERT_TRUE(ResetStorageAndMimicSyncEnabled(
+      /*initial_syncable_entries=*/{base::MakeRefCounted<ReadingListEntry>(
+          example, "example_title", clock_.Now())}));
+
+  EXPECT_EQ(model_->GetAccountWhereEntryIsSavedTo(example).ToString(),
+            kTestAccountId);
+  EXPECT_TRUE(
+      model_
+          ->GetAccountWhereEntryIsSavedTo(GURL("http://non_existing_url.com/"))
+          .empty());
+}
+
 // Tests adding entry.
 TEST_F(ReadingListModelTest, AddEntry) {
   const GURL url("http://example.com");