Read later: Add a method to notify update for reading list backend.

Adds an observer method to notify update for reading list backend.

Bug: 1139133
Change-Id: Ic5483e075086d48334a9e347125048b5699c2560
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519151Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825086}

chrome/browser/android/bookmarks/bookmark_bridge.cc

@@ -1133,9 +1133,6 @@ void BookmarkBridge::ExtensiveBookmarkChangesEnded(BookmarkModel* model) {
 }
 
 void BookmarkBridge::PartnerShimChanged(PartnerBookmarksShim* shim) {
-  if (!IsLoaded())
-    return;
-
   BookmarkModelChanged();
 }
 
@@ -1151,6 +1148,10 @@ void BookmarkBridge::ReadingListLoaded() {
   NotifyIfDoneLoading();
 }
 
+void BookmarkBridge::ReadingListChanged() {
+  BookmarkModelChanged();
+}
+
 void BookmarkBridge::ReorderChildren(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,

chrome/browser/android/bookmarks/bookmark_bridge.h

@@ -290,6 +290,7 @@ class BookmarkBridge : public bookmarks::BaseBookmarkModelObserver,
 
   // Override ReadingListManager::Observer
   void ReadingListLoaded() override;
+  void ReadingListChanged() override;
 
   void DestroyJavaObject();
 

chrome/browser/reading_list/android/reading_list_manager.h

@@ -32,6 +32,9 @@ class ReadingListManager : public KeyedService {
     // Called when the reading list backend is loaded.
     virtual void ReadingListLoaded() {}
 
+    // Called when the reading list backend is changed.
+    virtual void ReadingListChanged() {}
+
     Observer() = default;
     ~Observer() override = default;
   };

chrome/browser/reading_list/android/reading_list_manager_impl.cc

@@ -47,7 +47,10 @@ bool SyncToBookmark(const ReadingListEntry& entry, BookmarkNode* bookmark) {
 
 ReadingListManagerImpl::ReadingListManagerImpl(
     ReadingListModel* reading_list_model)
-    : reading_list_model_(reading_list_model), maximum_id_(0L), loaded_(false) {
+    : reading_list_model_(reading_list_model),
+      maximum_id_(0L),
+      loaded_(false),
+      performing_batch_update_(false) {
   DCHECK(reading_list_model_);
   root_ = std::make_unique<BookmarkNode>(maximum_id_++, base::GenerateGUID(),
                                          GURL());
@@ -95,6 +98,28 @@ void ReadingListManagerImpl::ReadingListDidMoveEntry(
   AddOrUpdateBookmark(moved_entry);
 }
 
+void ReadingListManagerImpl::ReadingListDidApplyChanges(
+    ReadingListModel* model) {
+  // Ignores ReadingListDidApplyChanges() invocations during batch update.
+  if (performing_batch_update_)
+    return;
+
+  NotifyReadingListChanged();
+}
+
+void ReadingListManagerImpl::ReadingListModelBeganBatchUpdates(
+    const ReadingListModel* model) {
+  performing_batch_update_ = true;
+}
+
+void ReadingListManagerImpl::ReadingListModelCompletedBatchUpdates(
+    const ReadingListModel* model) {
+  performing_batch_update_ = false;
+
+  // Batch update is done, notify the observer only once.
+  NotifyReadingListChanged();
+}
+
 void ReadingListManagerImpl::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -233,3 +258,8 @@ const BookmarkNode* ReadingListManagerImpl::AddOrUpdateBookmark(
   bool success = SyncToBookmark(*entry, new_node.get());
   return success ? root_->Add(std::move(new_node)) : nullptr;
 }
+
+void ReadingListManagerImpl::NotifyReadingListChanged() {
+  for (Observer& observer : observers_)
+    observer.ReadingListChanged();
+}

chrome/browser/reading_list/android/reading_list_manager_impl.h

@@ -34,6 +34,11 @@ class ReadingListManagerImpl : public ReadingListManager,
                                   const GURL& url) override;
   void ReadingListDidMoveEntry(const ReadingListModel* model,
                                const GURL& url) override;
+  void ReadingListDidApplyChanges(ReadingListModel* model) override;
+  void ReadingListModelBeganBatchUpdates(
+      const ReadingListModel* model) override;
+  void ReadingListModelCompletedBatchUpdates(
+      const ReadingListModel* model) override;
 
   // ReadingListManager implementation.
   void AddObserver(Observer* observer) override;
@@ -60,6 +65,7 @@ class ReadingListManagerImpl : public ReadingListManager,
   void RemoveBookmark(const GURL& url);
   const bookmarks::BookmarkNode* AddOrUpdateBookmark(
       const ReadingListEntry* entry);
+  void NotifyReadingListChanged();
 
   // Contains reading list data, outlives this class.
   ReadingListModel* reading_list_model_;
@@ -73,6 +79,9 @@ class ReadingListManagerImpl : public ReadingListManager,
   // Whether the |reading_list_model_| is loaded.
   bool loaded_;
 
+  // Whether |reading_list_model_| is in batch update mode.
+  bool performing_batch_update_;
+
   base::ObserverList<Observer> observers_;
 };
 

chrome/browser/reading_list/android/reading_list_manager_impl_unittest.cc

@@ -38,6 +38,7 @@ class MockObserver : public ReadingListManager::Observer {
 
   // ReadingListManager::Observer implementation.
   MOCK_METHOD(void, ReadingListLoaded, (), (override));
+  MOCK_METHOD(void, ReadingListChanged, (), (override));
 };
 
 class ReadingListManagerImplTest : public testing::Test {
@@ -64,6 +65,21 @@ class ReadingListManagerImplTest : public testing::Test {
   base::SimpleTestClock* clock() { return &clock_; }
   MockObserver* observer() { return &observer_; }
 
+  const BookmarkNode* Add(const GURL& url, const std::string& title) {
+    EXPECT_CALL(*observer(), ReadingListChanged());
+    return manager()->Add(url, title);
+  }
+
+  void Delete(const GURL& url) {
+    EXPECT_CALL(*observer(), ReadingListChanged());
+    manager()->Delete(url);
+  }
+
+  void SetReadStatus(const GURL& url, bool read) {
+    EXPECT_CALL(*observer(), ReadingListChanged());
+    manager()->SetReadStatus(url, read);
+  }
+
  private:
   base::SimpleTestClock clock_;
   std::unique_ptr<ReadingListModelImpl> reading_list_model_;
@@ -98,7 +114,7 @@ TEST_F(ReadingListManagerImplTest, Load) {
 TEST_F(ReadingListManagerImplTest, AddGetDelete) {
   // Adds a node.
   GURL url(kURL);
-  manager()->Add(url, kTitle);
+  Add(url, kTitle);
   EXPECT_EQ(1u, manager()->size());
   EXPECT_EQ(1u, manager()->unread_size());
   EXPECT_EQ(1u, manager()->GetRoot()->children().size())
@@ -118,7 +134,7 @@ TEST_F(ReadingListManagerImplTest, AddGetDelete) {
   EXPECT_EQ(nullptr, manager()->Get(GURL("invalid spec")));
 
   // Deletes the node.
-  manager()->Delete(url);
+  Delete(url);
   EXPECT_EQ(0u, manager()->size());
   EXPECT_EQ(0u, manager()->unread_size());
   EXPECT_TRUE(manager()->GetRoot()->children().empty());
@@ -127,7 +143,7 @@ TEST_F(ReadingListManagerImplTest, AddGetDelete) {
 // Verifies GetNodeByID() and IsReadingListBookmark() works correctly.
 TEST_F(ReadingListManagerImplTest, GetNodeByIDIsReadingListBookmark) {
   GURL url(kURL);
-  const auto* node = manager()->Add(url, kTitle);
+  const auto* node = Add(url, kTitle);
 
   // Find the root.
   EXPECT_EQ(manager()->GetRoot(),
@@ -154,8 +170,8 @@ TEST_F(ReadingListManagerImplTest, GetNodeByIDIsReadingListBookmark) {
 TEST_F(ReadingListManagerImplTest, AddTwice) {
   // Adds a node.
   GURL url(kURL);
-  const auto* node = manager()->Add(url, kTitle);
-  const auto* new_node = manager()->Add(url, kTitle1);
+  const auto* node = Add(url, kTitle);
+  const auto* new_node = Add(url, kTitle1);
   EXPECT_EQ(node, new_node) << "Add same URL shouldn't invalidate pointers.";
   EXPECT_EQ(kTitle1, base::UTF16ToUTF8(node->GetTitle()));
 }
@@ -163,14 +179,15 @@ TEST_F(ReadingListManagerImplTest, AddTwice) {
 // Verifes SetReadStatus()/GetReadStatus() API.
 TEST_F(ReadingListManagerImplTest, ReadStatus) {
   GURL url(kURL);
+
+  // No op when no reading list entries.
   manager()->SetReadStatus(url, true);
   EXPECT_EQ(0u, manager()->size());
 
-  // Add a node.
-  manager()->Add(url, kTitle);
-  manager()->SetReadStatus(url, true);
+  // Add a node and mark as read.
+  Add(url, kTitle);
+  SetReadStatus(url, true);
 
-  // Mark as read.
   const BookmarkNode* node = manager()->Get(url);
   ASSERT_TRUE(node);
   EXPECT_EQ(url, node->url());
@@ -181,7 +198,7 @@ TEST_F(ReadingListManagerImplTest, ReadStatus) {
   EXPECT_TRUE(manager()->GetReadStatus(node));
 
   // Mark as unread.
-  manager()->SetReadStatus(url, false);
+  SetReadStatus(url, false);
   node = manager()->Get(url);
   node->GetMetaInfo(kReadStatusKey, &read_status);
   EXPECT_EQ(kReadStatusUnread, read_status);
@@ -201,6 +218,7 @@ TEST_F(ReadingListManagerImplTest, ReadStatus) {
 // reading list entry from |reading_list_model_|.
 TEST_F(ReadingListManagerImplTest, ReadingListDidAddEntry) {
   GURL url(kURL);
+  EXPECT_CALL(*observer(), ReadingListChanged()).RetiresOnSaturation();
   reading_list_model()->AddEntry(url, kTitle, reading_list::ADDED_VIA_SYNC);
 
   const auto* node = manager()->Get(url);
@@ -215,13 +233,13 @@ TEST_F(ReadingListManagerImplTest, ReadingListWillRemoveEntry) {
   GURL url(kURL);
 
   // Adds a node.
-  manager()->Add(url, kTitle);
-  const auto* node = manager()->Get(url);
+  const auto* node = Add(url, kTitle);
   EXPECT_TRUE(node);
   EXPECT_EQ(url, node->url());
   EXPECT_EQ(1u, manager()->size());
 
   // Removes it from |reading_list_model_|.
+  EXPECT_CALL(*observer(), ReadingListChanged()).RetiresOnSaturation();
   reading_list_model()->RemoveEntryByURL(url);
   node = manager()->Get(url);
   EXPECT_FALSE(node);
@@ -234,12 +252,11 @@ TEST_F(ReadingListManagerImplTest, ReadingListWillMoveEntry) {
   GURL url(kURL);
 
   // Adds a node.
-  manager()->Add(url, kTitle);
-  const auto* node = manager()->Get(url);
+  const auto* node = Add(url, kTitle);
   EXPECT_TRUE(node);
   EXPECT_FALSE(manager()->GetReadStatus(node));
 
-  reading_list_model()->SetReadStatus(url, true);
+  SetReadStatus(url, true);
   EXPECT_TRUE(manager()->GetReadStatus(node));
 }