[Reland] Read later: Implement more ReadingListModelObserver methods.

Implements more ReadingListModelObserver methods. When sync happens, we
need to sync the in-memory bookmark tree.

OCL:https://chromium-review.googlesource.com/c/chromium/src/+/2518774
Revert:https://chromium-review.googlesource.com/c/chromium/src/+/2532461

Difference:
1. Add comment for Add() about invalidated pointer when adding existing
URL.
2. Fix test, don't use invalidated pointer.

TBR=dtrainor@chromium.org

Bug: 1133504,1147311
Change-Id: I9e2b85ceb0c6ba49e625dc2e9fd3789b56d55dae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532911Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826660}

chrome/browser/reading_list/android/reading_list_manager.h

@@ -42,8 +42,8 @@ class ReadingListManager : public KeyedService {
 
   // Adds a reading list article to the unread section, and return the bookmark
   // node representation. The bookmark node is owned by this class. If there is
-  // a duplicate URL, swaps the current reading list item. Returns nullptr on
-  // failure.
+  // a duplicate URL, a new bookmark node will be created, and the old bookmark
+  // node pointer will be invalidated.
   virtual const bookmarks::BookmarkNode* Add(const GURL& url,
                                              const std::string& title) = 0;
 

chrome/browser/reading_list/android/reading_list_manager_impl.cc

@@ -65,7 +65,7 @@ void ReadingListManagerImpl::ReadingListModelLoaded(
   // Constructs the bookmark tree.
   root_->DeleteAll();
   for (const auto& url : model->Keys())
-    AddBookmark(model->GetEntryByURL(url));
+    AddOrUpdateBookmark(model->GetEntryByURL(url));
 
   loaded_ = true;
 
@@ -77,7 +77,22 @@ void ReadingListManagerImpl::ReadingListDidAddEntry(
     const ReadingListModel* model,
     const GURL& url,
     reading_list::EntrySource source) {
-  AddBookmark(model->GetEntryByURL(url));
+  AddOrUpdateBookmark(model->GetEntryByURL(url));
+}
+
+void ReadingListManagerImpl::ReadingListWillRemoveEntry(
+    const ReadingListModel* model,
+    const GURL& url) {
+  RemoveBookmark(url);
+}
+
+void ReadingListManagerImpl::ReadingListDidMoveEntry(
+    const ReadingListModel* model,
+    const GURL& url) {
+  DCHECK(reading_list_model_->loaded());
+  const auto* moved_entry = reading_list_model_->GetEntryByURL(url);
+  DCHECK(moved_entry);
+  AddOrUpdateBookmark(moved_entry);
 }
 
 void ReadingListManagerImpl::AddObserver(Observer* observer) {
@@ -96,7 +111,8 @@ const BookmarkNode* ReadingListManagerImpl::Add(const GURL& url,
   const auto& new_entry = reading_list_model_->AddEntry(
       url, title, reading_list::ADDED_VIA_CURRENT_APP);
   const auto* node = FindBookmarkByURL(new_entry.URL());
-  DCHECK(node) << "Bookmark node should have been created.";
+  DCHECK(node)
+      << "Bookmark node should have been create in ReadingListDidAddEntry().";
   return node;
 }
 
@@ -128,8 +144,6 @@ bool ReadingListManagerImpl::IsReadingListBookmark(
 
 void ReadingListManagerImpl::Delete(const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-
-  RemoveBookmark(url);
   reading_list_model_->RemoveEntryByURL(url);
 }
 
@@ -201,8 +215,7 @@ void ReadingListManagerImpl::RemoveBookmark(const GURL& url) {
     root_->Remove(root_->GetIndexOf(node));
 }
 
-// Adds a reading list entry to the bookmark tree.
-const BookmarkNode* ReadingListManagerImpl::AddBookmark(
+const BookmarkNode* ReadingListManagerImpl::AddOrUpdateBookmark(
     const ReadingListEntry* entry) {
   if (!entry)
     return nullptr;

chrome/browser/reading_list/android/reading_list_manager_impl.h

@@ -30,6 +30,10 @@ class ReadingListManagerImpl : public ReadingListManager,
   void ReadingListDidAddEntry(const ReadingListModel* model,
                               const GURL& url,
                               reading_list::EntrySource source) override;
+  void ReadingListWillRemoveEntry(const ReadingListModel* model,
+                                  const GURL& url) override;
+  void ReadingListDidMoveEntry(const ReadingListModel* model,
+                               const GURL& url) override;
 
   // ReadingListManager implementation.
   void AddObserver(Observer* observer) override;
@@ -54,7 +58,8 @@ class ReadingListManagerImpl : public ReadingListManager,
   bookmarks::BookmarkNode* FindBookmarkByURL(const GURL& url) const;
 
   void RemoveBookmark(const GURL& url);
-  const bookmarks::BookmarkNode* AddBookmark(const ReadingListEntry* entry);
+  const bookmarks::BookmarkNode* AddOrUpdateBookmark(
+      const ReadingListEntry* entry);
 
   // Contains reading list data, outlives this class.
   ReadingListModel* reading_list_model_;

chrome/browser/reading_list/android/reading_list_manager_impl_unittest.cc

@@ -149,15 +149,15 @@ TEST_F(ReadingListManagerImplTest, GetNodeByIDIsReadingListBookmark) {
   EXPECT_FALSE(manager()->IsReadingListBookmark(node_same_url.get()));
 }
 
-// Verifies Add() the same URL twice will not invalidate returned pointers, and
-// the content is updated.
+// If Add() the same URL twice, the first bookmark node pointer will be
+// invalidated.
 TEST_F(ReadingListManagerImplTest, AddTwice) {
-  // Adds a node.
+  // Adds a node twice.
   GURL url(kURL);
-  const auto* node = manager()->Add(url, kTitle);
+  manager()->Add(url, kTitle);
   const auto* new_node = manager()->Add(url, kTitle1);
-  EXPECT_EQ(node, new_node) << "Add same URL shouldn't invalidate pointers.";
-  EXPECT_EQ(kTitle1, base::UTF16ToUTF8(node->GetTitle()));
+  EXPECT_EQ(kTitle1, base::UTF16ToUTF8(new_node->GetTitle()));
+  EXPECT_EQ(url, new_node->url());
 }
 
 // Verifes SetReadStatus()/GetReadStatus() API.
@@ -197,8 +197,8 @@ TEST_F(ReadingListManagerImplTest, ReadStatus) {
   EXPECT_FALSE(manager()->GetReadStatus(manager()->GetRoot()));
 }
 
-// Verifies ReadingListDidAddEntry() API that is being called after
-// ReadingListModel::AddEntry() is called.
+// Verifies the bookmark node is added when sync or other source adds the
+// reading list entry from |reading_list_model_|.
 TEST_F(ReadingListManagerImplTest, ReadingListDidAddEntry) {
   GURL url(kURL);
   reading_list_model()->AddEntry(url, kTitle, reading_list::ADDED_VIA_SYNC);
@@ -209,4 +209,38 @@ TEST_F(ReadingListManagerImplTest, ReadingListDidAddEntry) {
   EXPECT_EQ(1u, manager()->size());
 }
 
+// Verifies the bookmark node is deleted when sync or other source deletes the
+// reading list entry from |reading_list_model_|.
+TEST_F(ReadingListManagerImplTest, ReadingListWillRemoveEntry) {
+  GURL url(kURL);
+
+  // Adds a node.
+  manager()->Add(url, kTitle);
+  const auto* node = manager()->Get(url);
+  EXPECT_TRUE(node);
+  EXPECT_EQ(url, node->url());
+  EXPECT_EQ(1u, manager()->size());
+
+  // Removes it from |reading_list_model_|.
+  reading_list_model()->RemoveEntryByURL(url);
+  node = manager()->Get(url);
+  EXPECT_FALSE(node);
+  EXPECT_EQ(0u, manager()->size());
+}
+
+// Verifies the bookmark node is updated when sync or other source updates the
+// reading list entry from |reading_list_model_|.
+TEST_F(ReadingListManagerImplTest, ReadingListWillMoveEntry) {
+  GURL url(kURL);
+
+  // Adds a node.
+  manager()->Add(url, kTitle);
+  const auto* node = manager()->Get(url);
+  EXPECT_TRUE(node);
+  EXPECT_FALSE(manager()->GetReadStatus(node));
+
+  reading_list_model()->SetReadStatus(url, true);
+  EXPECT_TRUE(manager()->GetReadStatus(node));
+}
+
 }  // namespace