[Feed] Implement LoadAllJournalKeys and DoesJournalExist

this CL revert part of
https://chromium-review.googlesource.com/c/chromium/src/+/1119234,
add LoadAllJournalKeys back and remove LoadAllJournals.

Misunderstood the feed host API definition, right now, the API is
  /** Asynchronously retrieve a list of all current journals */
  void getAllJournals(Consumer<Result<List<String>>> consumer);
I think we should update it to
  /** Asynchronously retrieve a list of all current journals' names. */
I will create another bug for this.

Bug: 828938
Change-Id: I752fd75d4d4c35c924a4189f754cb393c4d88e1f
Reviewed-on: https://chromium-review.googlesource.com/1146067
Commit-Queue: Gang Wu <gangwu@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577690}

components/feed/core/feed_storage_database.cc

@@ -47,20 +47,42 @@ std::string FormatJournalKeyToStorageKey(const std::string& journal_key) {
   return kJournalStoragePrefix + journal_key;
 }
 
+// Check if the |storage_key| is with |prefix|.
+bool IsKeywithPrefix(const std::string& storage_key, const char prefix[]) {
+  return base::StartsWith(storage_key, prefix, base::CompareCase::SENSITIVE);
+}
+
 // Check if the |storage_key| is for content data.
 bool IsValidContentKey(const std::string& storage_key) {
-  return base::StartsWith(storage_key, kContentStoragePrefix,
-                          base::CompareCase::SENSITIVE);
+  return IsKeywithPrefix(storage_key, kContentStoragePrefix);
+}
+
+// Check if the |storage_key| is for journal data.
+bool IsValidJournalKey(const std::string& storage_key) {
+  return IsKeywithPrefix(storage_key, kJournalStoragePrefix);
+}
+
+// Help function |ParseContentKey| and |ParseJournalKey| to parse storage key by
+// remove |prefix| from |storage_key|.
+std::string ParseKeyHelper(const std::string& storage_key,
+                           const char prefix[]) {
+  if (!IsKeywithPrefix(storage_key, prefix)) {
+    return std::string();
+  }
+
+  return storage_key.substr(strlen(prefix));
 }
 
 // Parse content key from storage key. Return an empty string if |storage_key|
 // is not recognized as content key. ex. journal's storage key.
 std::string ParseContentKey(const std::string& storage_key) {
-  if (!IsValidContentKey(storage_key)) {
-    return std::string();
-  }
+  return ParseKeyHelper(storage_key, kContentStoragePrefix);
+}
 
-  return storage_key.substr(strlen(kContentStoragePrefix));
+// Parse journal key from storage key. Return an empty string if |storage_key|
+// is not recognized as journal key. ex. content's storage key.
+std::string ParseJournalKey(const std::string& storage_key) {
+  return ParseKeyHelper(storage_key, kJournalStoragePrefix);
 }
 
 bool DatabaseKeyFilter(const std::unordered_set<std::string>& key_set,
@@ -214,12 +236,21 @@ void FeedStorageDatabase::LoadJournal(const std::string& key,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void FeedStorageDatabase::LoadAllJournals(LoadAllJournalsCallback callback) {
+void FeedStorageDatabase::DoesJournalExist(const std::string& key,
+                                           ConfirmationCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  storage_database_->LoadEntriesWithFilter(
-      base::BindRepeating(&DatabasePrefixFilter, kJournalStoragePrefix),
-      base::BindOnce(&FeedStorageDatabase::OnLoadEntriesForLoadAllJournals,
+  storage_database_->GetEntry(
+      FormatJournalKeyToStorageKey(key),
+      base::BindOnce(&FeedStorageDatabase::OnGetEntryForDoesJournalExist,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
+void FeedStorageDatabase::LoadAllJournalKeys(JournalLoadCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  storage_database_->LoadKeys(
+      base::BindOnce(&FeedStorageDatabase::OnLoadKeysForLoadAllJournalKeys,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
@@ -344,6 +375,14 @@ void FeedStorageDatabase::OnGetEntryForLoadJournal(
   std::move(callback).Run(std::move(results));
 }
 
+void FeedStorageDatabase::OnGetEntryForDoesJournalExist(
+    ConfirmationCallback callback,
+    bool success,
+    std::unique_ptr<FeedStorageProto> journal) {
+  DVLOG_IF(1, !success) << "FeedStorageDatabase load journal failed.";
+  std::move(callback).Run(journal != nullptr);
+}
+
 void FeedStorageDatabase::OnGetEntryAppendToJournal(
     ConfirmationCallback callback,
     const std::string& key,
@@ -398,28 +437,22 @@ void FeedStorageDatabase::OnGetEntryForCopyJournal(
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void FeedStorageDatabase::OnLoadEntriesForLoadAllJournals(
-    LoadAllJournalsCallback callback,
+void FeedStorageDatabase::OnLoadKeysForLoadAllJournalKeys(
+    JournalLoadCallback callback,
     bool success,
-    std::unique_ptr<std::vector<FeedStorageProto>> entries) {
-  std::vector<std::vector<std::string>> results;
+    std::unique_ptr<std::vector<std::string>> keys) {
+  std::vector<std::string> results;
 
-  if (!success || !entries) {
-    DVLOG_IF(1, !success) << "FeedStorageDatabase load journals failed.";
+  if (!success || !keys) {
+    DVLOG_IF(1, !success) << "FeedStorageDatabase load journal keys failed.";
     std::move(callback).Run(std::move(results));
     return;
   }
 
-  for (const auto& entry : *entries) {
-    DCHECK(entry.has_key());
-    DCHECK_NE(entry.journal_data_size(), 0);
-
-    std::vector<std::string> journal;
-    journal.reserve(entry.journal_data_size());
-    for (int i = 0; i < entry.journal_data_size(); ++i) {
-      journal.emplace_back(entry.journal_data(i));
-    }
-    results.push_back(journal);
+  // Filter out content keys, only keep journal keys.
+  for (const std::string& key : *keys) {
+    if (IsValidJournalKey(key))
+      results.emplace_back(ParseJournalKey(key));
   }
 
   std::move(callback).Run(std::move(results));

components/feed/core/feed_storage_database.h

@@ -37,15 +37,14 @@ class FeedStorageDatabase {
   // Returns the content keys as a vector when calling loading all content keys.
   using ContentKeyCallback = base::OnceCallback<void(std::vector<std::string>)>;
 
-  // Returns the journal data as a vector of strings when calling loading data.
+  // Returns the journal data as a vector of strings when calling loading data
+  // or keys.
   using JournalLoadCallback =
       base::OnceCallback<void(std::vector<std::string>)>;
 
-  // Returns a vector of journal data when calling loading all journals.
-  using LoadAllJournalsCallback =
-      base::OnceCallback<void(std::vector<std::vector<std::string>>)>;
-
-  // Returns whether the commit operation succeeded.
+  // Returns whether the commit operation succeeded when calling for database
+  // operations, or return whether the entry exists when calling for checking
+  // the entry's existence.
   using ConfirmationCallback = base::OnceCallback<void(bool)>;
 
   // Initializes the database with |database_folder|.
@@ -100,8 +99,12 @@ class FeedStorageDatabase {
   // Loads the journal data for the |key| and passes it to |callback|.
   void LoadJournal(const std::string& key, JournalLoadCallback callback);
 
-  // Loads all journals in the storage, and passes them to |callback|.
-  void LoadAllJournals(LoadAllJournalsCallback callback);
+  // Checks if the journal for the |key| exists, and return the result to
+  // |callback|.
+  void DoesJournalExist(const std::string& key, ConfirmationCallback callback);
+
+  // Loads all journal keys in the storage, and passes them to |callback|.
+  void LoadAllJournalKeys(JournalLoadCallback callback);
 
   // Appends |entries| to a journal whose key is |key|, if there the journal do
   // not exist, create one. |callback| will be called when the data are saved or
@@ -139,6 +142,9 @@ class FeedStorageDatabase {
   void OnGetEntryForLoadJournal(JournalLoadCallback callback,
                                 bool success,
                                 std::unique_ptr<FeedStorageProto> journal);
+  void OnGetEntryForDoesJournalExist(ConfirmationCallback callback,
+                                     bool success,
+                                     std::unique_ptr<FeedStorageProto> journal);
   void OnGetEntryAppendToJournal(ConfirmationCallback callback,
                                  const std::string& key,
                                  std::vector<std::string> entries,
@@ -148,10 +154,10 @@ class FeedStorageDatabase {
                                 const std::string& to_key,
                                 bool success,
                                 std::unique_ptr<FeedStorageProto> journal);
-  void OnLoadEntriesForLoadAllJournals(
-      LoadAllJournalsCallback callback,
+  void OnLoadKeysForLoadAllJournalKeys(
+      JournalLoadCallback callback,
       bool success,
-      std::unique_ptr<std::vector<FeedStorageProto>> entries);
+      std::unique_ptr<std::vector<std::string>> keys);
   void OnStorageCommitted(ConfirmationCallback callback, bool success);
 
   State database_status_;

components/feed/core/feed_storage_database_unittest.cc

@@ -90,8 +90,7 @@ class FeedStorageDatabaseTest : public testing::Test {
                void(std::vector<std::pair<std::string, std::string>>));
   MOCK_METHOD1(OnContentKeyReceived, void(std::vector<std::string>));
   MOCK_METHOD1(OnJournalEntryReceived, void(std::vector<std::string>));
-  MOCK_METHOD1(OnJournalEntriesReceived,
-               void(std::vector<std::vector<std::string>>));
+  MOCK_METHOD1(JournalExist, void(bool));
   MOCK_METHOD1(OnStorageCommitted, void(bool));
 
  private:
@@ -326,14 +325,14 @@ TEST_F(FeedStorageDatabaseTest, DeleteAllContent) {
   storage_db()->LoadCallback(true);
 
   // Make sure all journals are there.
-  EXPECT_CALL(*this, OnJournalEntriesReceived(_))
-      .WillOnce([](std::vector<std::vector<std::string>> results) {
+  EXPECT_CALL(*this, OnJournalEntryReceived(_))
+      .WillOnce([](std::vector<std::string> results) {
         ASSERT_EQ(results.size(), 3U);
       });
-  db()->LoadAllJournals(
-      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntriesReceived,
+  db()->LoadAllJournalKeys(
+      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntryReceived,
                      base::Unretained(this)));
-  storage_db()->LoadCallback(true);
+  storage_db()->LoadKeysCallback(true);
 }
 
 TEST_F(FeedStorageDatabaseTest, LoadJournalEntry) {
@@ -373,7 +372,31 @@ TEST_F(FeedStorageDatabaseTest, LoadNonExistingJournalEntry) {
   storage_db()->GetCallback(true);
 }
 
-TEST_F(FeedStorageDatabaseTest, LoadAllJournals) {
+TEST_F(FeedStorageDatabaseTest, DoesJournalExist) {
+  CreateDatabase(/*init_database=*/true);
+
+  // Store |kJournalKey1|.
+  InjectJournalStorageProto(kJournalKey1,
+                            {kJournalData1, kJournalData2, kJournalData3});
+
+  // Check if |kJournalKey1| exists.
+  EXPECT_CALL(*this, JournalExist(true));
+  db()->DoesJournalExist(kJournalKey1,
+                         base::BindOnce(&FeedStorageDatabaseTest::JournalExist,
+                                        base::Unretained(this)));
+  storage_db()->GetCallback(true);
+
+  Mock::VerifyAndClearExpectations(this);
+
+  // Check if |kJournalKey2| exists.
+  EXPECT_CALL(*this, JournalExist(false));
+  db()->DoesJournalExist(kJournalKey2,
+                         base::BindOnce(&FeedStorageDatabaseTest::JournalExist,
+                                        base::Unretained(this)));
+  storage_db()->GetCallback(true);
+}
+
+TEST_F(FeedStorageDatabaseTest, LoadAllJournalKeys) {
   CreateDatabase(/*init_database=*/true);
 
   // Store |kContentKey1|, |kContentKey2|, |kJournalKey1|, |kJournalKey2|,
@@ -385,20 +408,17 @@ TEST_F(FeedStorageDatabaseTest, LoadAllJournals) {
   InjectJournalStorageProto(kJournalKey2, {kJournalData4, kJournalData5});
   InjectJournalStorageProto(kJournalKey3, {kJournalData6});
 
-  EXPECT_CALL(*this, OnJournalEntriesReceived(_))
-      .WillOnce([](std::vector<std::vector<std::string>> results) {
+  EXPECT_CALL(*this, OnJournalEntryReceived(_))
+      .WillOnce([](std::vector<std::string> results) {
         ASSERT_EQ(results.size(), 3U);
-        EXPECT_EQ(results[0][0], kJournalData1);
-        EXPECT_EQ(results[0][1], kJournalData2);
-        EXPECT_EQ(results[0][2], kJournalData3);
-        EXPECT_EQ(results[1][0], kJournalData4);
-        EXPECT_EQ(results[1][1], kJournalData5);
-        EXPECT_EQ(results[2][0], kJournalData6);
+        EXPECT_EQ(results[0], kJournalKey1);
+        EXPECT_EQ(results[1], kJournalKey2);
+        EXPECT_EQ(results[2], kJournalKey3);
       });
-  db()->LoadAllJournals(
-      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntriesReceived,
+  db()->LoadAllJournalKeys(
+      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntryReceived,
                      base::Unretained(this)));
-  storage_db()->LoadCallback(true);
+  storage_db()->LoadKeysCallback(true);
 }
 
 TEST_F(FeedStorageDatabaseTest, AppendToJournal_WhenJournalExists) {
@@ -544,18 +564,16 @@ TEST_F(FeedStorageDatabaseTest, DeleteJournal) {
   storage_db()->UpdateCallback(true);
 
   // Make sure |kJournalKey2| got deleted.
-  EXPECT_CALL(*this, OnJournalEntriesReceived(_))
-      .WillOnce([](std::vector<std::vector<std::string>> results) {
+  EXPECT_CALL(*this, OnJournalEntryReceived(_))
+      .WillOnce([](std::vector<std::string> results) {
         ASSERT_EQ(results.size(), 2U);
-        EXPECT_EQ(results[0][0], kJournalData1);
-        EXPECT_EQ(results[0][1], kJournalData2);
-        EXPECT_EQ(results[0][2], kJournalData3);
-        EXPECT_EQ(results[1][0], kJournalData6);
+        EXPECT_EQ(results[0], kJournalKey1);
+        EXPECT_EQ(results[1], kJournalKey3);
       });
-  db()->LoadAllJournals(
-      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntriesReceived,
+  db()->LoadAllJournalKeys(
+      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntryReceived,
                      base::Unretained(this)));
-  storage_db()->LoadCallback(true);
+  storage_db()->LoadKeysCallback(true);
 }
 
 TEST_F(FeedStorageDatabaseTest, DeleteAllJournals) {
@@ -579,14 +597,14 @@ TEST_F(FeedStorageDatabaseTest, DeleteAllJournals) {
   storage_db()->UpdateCallback(true);
 
   // Make sure all journals got deleted.
-  EXPECT_CALL(*this, OnJournalEntriesReceived(_))
-      .WillOnce([](std::vector<std::vector<std::string>> results) {
+  EXPECT_CALL(*this, OnJournalEntryReceived(_))
+      .WillOnce([](std::vector<std::string> results) {
         ASSERT_EQ(results.size(), 0U);
       });
-  db()->LoadAllJournals(
-      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntriesReceived,
+  db()->LoadAllJournalKeys(
+      base::BindOnce(&FeedStorageDatabaseTest::OnJournalEntryReceived,
                      base::Unretained(this)));
-  storage_db()->LoadCallback(true);
+  storage_db()->LoadKeysCallback(true);
 
   // Make sure all content are still there.
   EXPECT_CALL(*this, OnContentEntriesReceived(_))