Read later: Implement ReadingListManager.

Reading list manager talks to reading list backend and converts data
to a BookmarkNode tree.

Bug: 1128074
Change-Id: I0a59530b667021e2aa0d58842a4c0495bfde7af5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427635Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811774}

chrome/browser/android/reading_list/reading_list_manager_factory.cc

@@ -39,5 +39,5 @@ ReadingListManagerFactory::BuildServiceInstanceFor(
       ProfileKey::FromSimpleFactoryKey(key));
   auto* reading_list_model =
       ReadingListModelFactory::GetForBrowserContext(profile);
-  return std::make_unique<ReadingListManager>(reading_list_model);
+  return ReadingListManager::Create(reading_list_model);
 }

chrome/browser/reading_list/OWNERS

+dtrainor@chromium.org
+xingliu@chromium.org
+
+# Backup reviewers:
+twellington@chromium.org
+wylieb@chromium.org
+
+# COMPONENT: UI>Browser>ReadLater
+# OS: Android

chrome/browser/reading_list/android/BUILD.gn

@@ -19,7 +19,24 @@ source_set("reading_list") {
   # This target should not depend on anything in //chrome/* except the proto library.
   deps = [
     "//base",
+    "//components/bookmarks/browser",
     "//components/keyed_service/core",
     "//components/reading_list/core",
+    "//url",
+  ]
+}
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [ "reading_list_manager_unittest.cc" ]
+  deps = [
+    ":android",
+    "//base/test:test_support",
+    "//components/bookmarks/browser",
+    "//components/reading_list/core:core",
+    "//components/sync",
+    "//components/sync:test_support_model",
+    "//testing/gmock",
+    "//testing/gtest",
   ]
 }

chrome/browser/reading_list/android/reading_list_manager.cc

@@ -4,11 +4,173 @@
 
 #include "chrome/browser/reading_list/android/reading_list_manager.h"
 
+#include <utility>
+
+#include "base/guid.h"
+#include "base/logging.h"
+#include "base/strings/utf_string_conversions.h"
+#include "components/bookmarks/browser/bookmark_node.h"
 #include "components/reading_list/core/reading_list_model.h"
+#include "components/reading_list/core/reading_list_model_observer.h"
+#include "url/gurl.h"
 
-ReadingListManager::ReadingListManager(ReadingListModel* reading_list_model)
-    : reading_list_model_(reading_list_model) {
-  DCHECK(reading_list_model_);
-}
+using BookmarkNode = bookmarks::BookmarkNode;
+
+constexpr char kReadStatusKey[] = "read_status";
+constexpr char kReadStatusRead[] = "true";
+constexpr char kReadStatusUnread[] = "false";
+
+// Implementation of ReadingListManager.
+// 1. Holds a in memory bookmark node tree. Contains a folder root and reading
+// list nodes as children. Only has one level of children.
+// 2. Talk to reading list model, and sync with the in memory bookmark tree.
+// 3. TODO(xingliu): Add an observer and broadcast events to caller.
+class ReadingListManagerImpl : public ReadingListManager,
+                               public ReadingListModelObserver {
+ public:
+  explicit ReadingListManagerImpl(ReadingListModel* reading_list_model)
+      : reading_list_model_(reading_list_model), maximum_id_(0L) {
+    DCHECK(reading_list_model_);
+    root_ = std::make_unique<BookmarkNode>(maximum_id_++, base::GenerateGUID(),
+                                           GURL());
+    DCHECK(root_->is_folder());
+    reading_list_model_->AddObserver(this);
+  }
+
+  ~ReadingListManagerImpl() override {
+    reading_list_model_->RemoveObserver(this);
+  }
+
+  // ReadingListModelObserver overrides.
+  void ReadingListModelLoaded(const ReadingListModel* model) override {
+    // Constructs the bookmark tree.
+    root_->DeleteAll();
+    for (const auto& url : model->Keys())
+      AddBookmark(model->GetEntryByURL(url));
+  }
+
+  // ReadingListManager implementation.
+  const BookmarkNode* Add(const GURL& url, const std::string& title) override {
+    DCHECK(reading_list_model_->loaded());
+
+    // Add or swap the reading list entry.
+    const auto& new_entry = reading_list_model_->AddEntry(
+        url, title, reading_list::ADDED_VIA_CURRENT_APP);
+    return AddBookmark(&new_entry);
+  }
+
+  const BookmarkNode* Get(const GURL& url) override {
+    DCHECK(reading_list_model_->loaded());
+    return FindBookmarkByURL(url);
+  }
+
+  void Delete(const GURL& url) override {
+    DCHECK(reading_list_model_->loaded());
+
+    RemoveBookmark(url);
+    reading_list_model_->RemoveEntryByURL(url);
+  }
+
+  const BookmarkNode* GetRoot() const override {
+    DCHECK(reading_list_model_->loaded());
+    return root_.get();
+  }
+
+  size_t size() const override {
+    DCHECK(reading_list_model_->loaded());
+    return reading_list_model_->size();
+  }
+
+  size_t unread_size() const override {
+    DCHECK(reading_list_model_->loaded());
+    return reading_list_model_->unread_size();
+  }
 
-ReadingListManager::~ReadingListManager() = default;
+  void SetReadStatus(const GURL& url, bool read) override {
+    DCHECK(reading_list_model_->loaded());
+    const auto* entry = reading_list_model_->GetEntryByURL(url);
+    if (!entry)
+      return;
+
+    reading_list_model_->SetReadStatus(url, read);
+    auto* node = FindBookmarkByURL(url);
+    if (node) {
+      node->SetMetaInfo(kReadStatusKey,
+                        read ? kReadStatusRead : kReadStatusUnread);
+    }
+  }
+
+ private:
+  // Finds the child in the bookmark tree by URL. Returns nullptr if not found.
+  // Not recursive since the reading list bookmark tree only has a folder root
+  // node and one level of children.
+  BookmarkNode* FindBookmarkByURL(const GURL& url) {
+    for (const auto& child : root_->children()) {
+      if (url == child->url())
+        return child.get();
+    }
+    return nullptr;
+  }
+
+  // Removes a reading list bookmark node by |url|.
+  void RemoveBookmark(const GURL& url) {
+    const BookmarkNode* node = FindBookmarkByURL(url);
+    if (node)
+      root_->Remove(root_->GetIndexOf(node));
+  }
+
+  // Adds a reading list entry to the bookmark tree.
+  const BookmarkNode* AddBookmark(const ReadingListEntry* entry) {
+    if (!entry)
+      return nullptr;
+
+    // Update the existing bookmark node if possible.
+    BookmarkNode* node = FindBookmarkByURL(entry->URL());
+    if (node) {
+      bool success = SyncToBookmark(*entry, node);
+      return success ? node : nullptr;
+    }
+
+    // Add a new node.
+    auto new_node = std::make_unique<BookmarkNode>(
+        maximum_id_++, base::GenerateGUID(), entry->URL());
+    bool success = SyncToBookmark(*entry, new_node.get());
+    return success ? root_->Add(std::move(new_node)) : nullptr;
+  }
+
+  // Sync the bookmark node with |entry|. Returns whether the conversion is
+  // succeeded.
+  static bool SyncToBookmark(const ReadingListEntry& entry,
+                             BookmarkNode* bookmark) {
+    DCHECK(bookmark);
+    base::string16 title;
+    if (!base::UTF8ToUTF16(entry.Title().c_str(), entry.Title().size(),
+                           &title)) {
+      LOG(ERROR) << "Failed to convert the title to string16.";
+      return false;
+    }
+
+    bookmark->set_url(entry.URL());
+    bookmark->set_date_added(base::Time::FromDeltaSinceWindowsEpoch(
+        base::TimeDelta::FromMicroseconds(entry.CreationTime())));
+    bookmark->SetTitle(title);
+    bookmark->SetMetaInfo(kReadStatusKey,
+                          entry.IsRead() ? kReadStatusRead : kReadStatusUnread);
+    return true;
+  }
+
+  // Contains reading list data, outlives this class.
+  ReadingListModel* reading_list_model_;
+
+  // The bookmark root for reading list articles.
+  std::unique_ptr<BookmarkNode> root_;
+
+  // Auto increment bookmark id. Will not be persisted and only used in memory.
+  int64_t maximum_id_;
+};
+
+// static
+std::unique_ptr<ReadingListManager> ReadingListManager::Create(
+    ReadingListModel* reading_list_model) {
+  return std::make_unique<ReadingListManagerImpl>(reading_list_model);
+}

chrome/browser/reading_list/android/reading_list_manager.h

@@ -7,20 +7,59 @@
 
 #include "components/keyed_service/core/keyed_service.h"
 
+#include <memory>
+#include <string>
+
+namespace bookmarks {
+class BookmarkNode;
+}  // namespace bookmarks
+
+class GURL;
 class ReadingListModel;
 
 // Owns a reading list model and converts reading list data to bookmark nodes.
+// The bookmark nodes won't be persisted across sessions.
 class ReadingListManager : public KeyedService {
  public:
-  explicit ReadingListManager(ReadingListModel* reading_list_model);
-  ~ReadingListManager() override;
+  static std::unique_ptr<ReadingListManager> Create(
+      ReadingListModel* reading_list_model);
+
+  ReadingListManager() = default;
+  ~ReadingListManager() override = default;
 
   ReadingListManager(const ReadingListManager&) = delete;
   ReadingListManager& operator=(const ReadingListManager&) = delete;
 
- private:
-  // Contains reading list data, outlives this class.
-  ReadingListModel* reading_list_model_;
+  // 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.
+  virtual const bookmarks::BookmarkNode* Add(const GURL& url,
+                                             const std::string& title) = 0;
+
+  // Gets the bookmark node representation of a reading list article. The
+  // bookmark node is owned by this class. Returns nullptr if no such reading
+  // list.
+  virtual const bookmarks::BookmarkNode* Get(const GURL& url) = 0;
+
+  // Deletes a reading list article.
+  virtual void Delete(const GURL& url) = 0;
+
+  // Returns the root bookmark node for the reading list article. The bookmark
+  // node tree is owned by this class. All reading list articles are children of
+  // this root.
+  virtual const bookmarks::BookmarkNode* GetRoot() const = 0;
+
+  // Returns the total number of reading list articles. This doesn't include the
+  // bookmark root.
+  virtual size_t size() const = 0;
+
+  // Returns the total number of unread articles.
+  virtual size_t unread_size() const = 0;
+
+  // Sets the read status for a reading list article. No op if such reading list
+  // article doesn't exist.
+  virtual void SetReadStatus(const GURL& url, bool read) = 0;
 };
 
 #endif  // CHROME_BROWSER_READING_LIST_ANDROID_READING_LIST_MANAGER_H_

chrome/browser/reading_list/android/reading_list_manager_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/reading_list/android/reading_list_manager.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/simple_test_clock.h"
+#include "components/bookmarks/browser/bookmark_node.h"
+#include "components/reading_list/core/reading_list_model_impl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using BookmarkNode = bookmarks::BookmarkNode;
+using ReadingListEntries = ReadingListModelImpl::ReadingListEntries;
+
+namespace {
+
+constexpr char kURL[] = "https://www.example.com";
+constexpr char kTitle[] =
+    "In earlier tellings, the dog had a better reputation than the cat, "
+    "however the president vetoed it.";
+constexpr char kTitle1[] = "boring title.";
+constexpr char kReadStatusKey[] = "read_status";
+constexpr char kReadStatusRead[] = "true";
+constexpr char kReadStatusUnread[] = "false";
+
+class ReadingListManagerTest : public testing::Test {
+ public:
+  ReadingListManagerTest() = default;
+  ~ReadingListManagerTest() override = default;
+
+  void SetUp() override {
+    reading_list_model_ = std::make_unique<ReadingListModelImpl>(
+        /*storage_layer=*/nullptr, /*pref_service=*/nullptr, &clock_);
+    manager_ = ReadingListManager::Create(reading_list_model_.get());
+  }
+
+ protected:
+  ReadingListManager* manager() { return manager_.get(); }
+  ReadingListModelImpl* reading_list_model() {
+    return reading_list_model_.get();
+  }
+  base::SimpleTestClock* clock() { return &clock_; }
+
+ private:
+  base::SimpleTestClock clock_;
+  std::unique_ptr<ReadingListModelImpl> reading_list_model_;
+  std::unique_ptr<ReadingListManager> manager_;
+};
+
+// Verifies the states without any reading list data.
+TEST_F(ReadingListManagerTest, RootWithEmptyReadingList) {
+  const auto* root = manager()->GetRoot();
+  ASSERT_TRUE(root);
+  EXPECT_TRUE(root->is_folder());
+  EXPECT_EQ(0u, manager()->size());
+}
+
+// Verifies load data into reading list model will update |manager_| as well.
+TEST_F(ReadingListManagerTest, Load) {
+  // Load data into reading list model.
+  auto entries = std::make_unique<ReadingListEntries>();
+  GURL url(kURL);
+  entries->emplace(url, ReadingListEntry(url, kTitle, clock()->Now()));
+  reading_list_model()->StoreLoaded(std::move(entries));
+
+  const auto* node = manager()->Get(url);
+  EXPECT_TRUE(node);
+  EXPECT_EQ(url, node->url());
+  EXPECT_EQ(1u, manager()->size());
+  EXPECT_EQ(1u, manager()->unread_size());
+}
+
+// Verifes Add(), Get(), Delete() API in reading list manager.
+TEST_F(ReadingListManagerTest, AddGetDelete) {
+  // Adds a node.
+  GURL url(kURL);
+  manager()->Add(url, kTitle);
+  EXPECT_EQ(1u, manager()->size());
+  EXPECT_EQ(1u, manager()->unread_size());
+  EXPECT_EQ(1u, manager()->GetRoot()->children().size())
+      << "The reading list node should be the child of the root.";
+
+  // Gets the node, and verifies its content.
+  const BookmarkNode* node = manager()->Get(url);
+  ASSERT_TRUE(node);
+  EXPECT_EQ(url, node->url());
+  EXPECT_EQ(kTitle, base::UTF16ToUTF8(node->GetTitle()));
+  std::string read_status;
+  node->GetMetaInfo(kReadStatusKey, &read_status);
+  EXPECT_EQ(kReadStatusUnread, read_status)
+      << "By default the reading list node is marked as unread.";
+
+  // Deletes the node.
+  manager()->Delete(url);
+  EXPECT_EQ(0u, manager()->size());
+  EXPECT_EQ(0u, manager()->unread_size());
+  EXPECT_TRUE(manager()->GetRoot()->children().empty());
+}
+
+// Verifies Add() the same URL twice will not invalidate returned pointers, and
+// the content is updated.
+TEST_F(ReadingListManagerTest, AddTwice) {
+  // Adds a node.
+  GURL url(kURL);
+  const auto* node = 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()));
+}
+
+// Verifes SetReadStatus() API.
+TEST_F(ReadingListManagerTest, SetReadStatus) {
+  GURL url(kURL);
+  manager()->SetReadStatus(url, true);
+  EXPECT_EQ(0u, manager()->size());
+
+  // Add a node.
+  manager()->Add(url, kTitle);
+  manager()->SetReadStatus(url, true);
+
+  // Mark as read.
+  const BookmarkNode* node = manager()->Get(url);
+  ASSERT_TRUE(node);
+  EXPECT_EQ(url, node->url());
+  std::string read_status;
+  node->GetMetaInfo(kReadStatusKey, &read_status);
+  EXPECT_EQ(kReadStatusRead, read_status);
+  EXPECT_EQ(0u, manager()->unread_size());
+
+  // Mark as unread.
+  manager()->SetReadStatus(url, false);
+  node = manager()->Get(url);
+  node->GetMetaInfo(kReadStatusKey, &read_status);
+  EXPECT_EQ(kReadStatusUnread, read_status);
+  EXPECT_EQ(1u, manager()->unread_size());
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -4193,6 +4193,7 @@ test("unit_tests") {
       "//chrome/browser/optimization_guide/android:native_j_unittests_jni_headers",
       "//chrome/browser/optimization_guide/android:native_java_unittests",
       "//chrome/browser/password_check/android:unit_tests",
+      "//chrome/browser/reading_list/android:unit_tests",
       "//chrome/browser/thumbnail:unit_tests",
       "//chrome/browser/updates:unit_tests",
       "//chrome/services/media_gallery_util:unit_tests",