Small refactoring of DomDistillerStore

This refactors Add/Update/Delete to follow one main code path.

Also fixes the case where sync initialization happens before the
database is loaded (and adds a test for that).

Review URL: https://codereview.chromium.org/735823005

Cr-Commit-Position: refs/heads/master@{#304871}

3 files changed, 51 insertions, 54 deletions

diff --git a/components/dom_distiller/core/dom_distiller_store.cc b/components/dom_distiller/core/dom_distiller_store.cc
index c6def8d..b0c445e 100644
--- a/components/dom_distiller/core/dom_distiller_store.cc
+++ b/components/dom_distiller/core/dom_distiller_store.cc
@@ -63,82 +63,48 @@ bool DomDistillerStore::GetEntryByUrl(const GURL& url, ArticleEntry* entry) {
 }
 
 bool DomDistillerStore::AddEntry(const ArticleEntry& entry) {
-  if (!database_loaded_) {
-    return false;
-  }
-
-  if (model_.GetEntryById(entry.entry_id(), NULL)) {
-    DVLOG(1) << "Already have entry with id " << entry.entry_id() << ".";
-    return false;
-  }
-
-  SyncChangeList changes_to_apply;
-  changes_to_apply.push_back(
-      SyncChange(FROM_HERE, SyncChange::ACTION_ADD, CreateLocalData(entry)));
-
-  SyncChangeList changes_applied;
-  SyncChangeList changes_missing;
-
-  ApplyChangesToModel(changes_to_apply, &changes_applied, &changes_missing);
-
-  DCHECK_EQ(size_t(0), changes_missing.size());
-  DCHECK_EQ(size_t(1), changes_applied.size());
+  return ChangeEntry(entry, SyncChange::ACTION_ADD);
+}
 
-  ApplyChangesToSync(FROM_HERE, changes_applied);
-  ApplyChangesToDatabase(changes_applied);
+bool DomDistillerStore::UpdateEntry(const ArticleEntry& entry) {
+  return ChangeEntry(entry, SyncChange::ACTION_UPDATE);
+}
 
-  return true;
+bool DomDistillerStore::RemoveEntry(const ArticleEntry& entry) {
+  return ChangeEntry(entry, SyncChange::ACTION_DELETE);
 }
 
-bool DomDistillerStore::UpdateEntry(const ArticleEntry& entry) {
+bool DomDistillerStore::ChangeEntry(const ArticleEntry& entry,
+                                    SyncChange::SyncChangeType changeType) {
   if (!database_loaded_) {
     return false;
   }
 
-  if (!model_.GetEntryById(entry.entry_id(), NULL)) {
+  bool hasEntry = model_.GetEntryById(entry.entry_id(), NULL);
+  if (hasEntry) {
+    if (changeType == SyncChange::ACTION_ADD) {
+      DVLOG(1) << "Already have entry with id " << entry.entry_id() << ".";
+      return false;
+    }
+  } else if (changeType != SyncChange::ACTION_ADD) {
     DVLOG(1) << "No entry with id " << entry.entry_id() << " found.";
     return false;
   }
 
   SyncChangeList changes_to_apply;
   changes_to_apply.push_back(
-      SyncChange(FROM_HERE, SyncChange::ACTION_UPDATE, CreateLocalData(entry)));
+      SyncChange(FROM_HERE, changeType, CreateLocalData(entry)));
 
   SyncChangeList changes_applied;
   SyncChangeList changes_missing;
 
   ApplyChangesToModel(changes_to_apply, &changes_applied, &changes_missing);
 
-  if (changes_applied.size() != 1) {
+  if (changeType == SyncChange::ACTION_UPDATE && changes_applied.size() != 1) {
     DVLOG(1) << "Failed to update entry with id " << entry.entry_id() << ".";
     return false;
   }
 
-  ApplyChangesToSync(FROM_HERE, changes_applied);
-  ApplyChangesToDatabase(changes_applied);
-
-  return true;
-}
-
-bool DomDistillerStore::RemoveEntry(const ArticleEntry& entry) {
-  if (!database_loaded_) {
-    return false;
-  }
-
-  if (!model_.GetEntryById(entry.entry_id(), NULL)) {
-    DVLOG(1) << "No entry with id " << entry.entry_id() << " found.";
-    return false;
-  }
-
-  SyncChangeList changes_to_apply;
-  changes_to_apply.push_back(
-      SyncChange(FROM_HERE, SyncChange::ACTION_DELETE, CreateLocalData(entry)));
-
-  SyncChangeList changes_applied;
-  SyncChangeList changes_missing;
-
-  ApplyChangesToModel(changes_to_apply, &changes_applied, &changes_missing);
-
   DCHECK_EQ(size_t(0), changes_missing.size());
   DCHECK_EQ(size_t(1), changes_applied.size());
 
@@ -268,6 +234,7 @@ void DomDistillerStore::OnDatabaseLoad(bool success,
   SyncChangeList database_changes_needed;
   MergeDataWithModel(data, &changes_applied, &database_changes_needed);
   ApplyChangesToDatabase(database_changes_needed);
+  ApplyChangesToSync(FROM_HERE, changes_applied);
 }
 
 void DomDistillerStore::OnDatabaseSave(bool success) {
@@ -330,6 +297,8 @@ bool DomDistillerStore::ApplyChangesToDatabase(
 SyncMergeResult DomDistillerStore::MergeDataWithModel(
     const SyncDataList& data, SyncChangeList* changes_applied,
     SyncChangeList* changes_missing) {
+  // TODO(cjhopman): This naive merge algorithm could cause flip-flopping
+  // between database/sync of multiple clients.
   DCHECK(changes_applied);
   DCHECK(changes_missing);
 
diff --git a/components/dom_distiller/core/dom_distiller_store.h b/components/dom_distiller/core/dom_distiller_store.h
index 186c273..2a741e0 100644
--- a/components/dom_distiller/core/dom_distiller_store.h
+++ b/components/dom_distiller/core/dom_distiller_store.h
@@ -38,10 +38,8 @@ class DomDistillerStoreInterface {
   virtual syncer::SyncableService* GetSyncableService() = 0;
 
   virtual bool AddEntry(const ArticleEntry& entry) = 0;
-
   // Returns false if |entry| is not present or |entry| was not updated.
   virtual bool UpdateEntry(const ArticleEntry& entry) = 0;
-
   virtual bool RemoveEntry(const ArticleEntry& entry) = 0;
 
   // Lookup an ArticleEntry by ID or URL. Returns whether a corresponding entry
@@ -122,6 +120,10 @@ class DomDistillerStore : public syncer::SyncableService,
   void OnDatabaseLoad(bool success, scoped_ptr<EntryVector> entries);
   void OnDatabaseSave(bool success);
 
+  // Returns true if the change is successfully applied.
+  bool ChangeEntry(const ArticleEntry& entry,
+                   syncer::SyncChange::SyncChangeType changeType);
+
   syncer::SyncMergeResult MergeDataWithModel(
       const syncer::SyncDataList& data, syncer::SyncChangeList* changes_applied,
       syncer::SyncChangeList* changes_missing);
diff --git a/components/dom_distiller/core/dom_distiller_store_unittest.cc b/components/dom_distiller/core/dom_distiller_store_unittest.cc
index b2d0efd..86a4e10 100644
--- a/components/dom_distiller/core/dom_distiller_store_unittest.cc
+++ b/components/dom_distiller/core/dom_distiller_store_unittest.cc
@@ -345,6 +345,32 @@ TEST_F(DomDistillerStoreTest, TestSyncMergeAfterDatabaseLoad) {
   EXPECT_TRUE(AreEntryMapsEqual(sync_model_, expected_model));
 }
 
+TEST_F(DomDistillerStoreTest, TestDatabaseLoadAfterSyncMerge) {
+  AddEntry(GetSampleEntry(0), &db_model_);
+  AddEntry(GetSampleEntry(1), &db_model_);
+  AddEntry(GetSampleEntry(2), &db_model_);
+
+  AddEntry(GetSampleEntry(2), &sync_model_);
+  AddEntry(GetSampleEntry(3), &sync_model_);
+  AddEntry(GetSampleEntry(4), &sync_model_);
+
+  EntryMap expected_model(db_model_);
+  AddEntry(GetSampleEntry(3), &expected_model);
+  AddEntry(GetSampleEntry(4), &expected_model);
+
+  CreateStore();
+  StartSyncing();
+
+  EXPECT_TRUE(AreEntriesEqual(store_->GetEntries(), sync_model_));
+
+  fake_db_->InitCallback(true);
+  fake_db_->LoadCallback(true);
+
+  EXPECT_TRUE(AreEntriesEqual(store_->GetEntries(), expected_model));
+  EXPECT_TRUE(AreEntryMapsEqual(db_model_, expected_model));
+  EXPECT_TRUE(AreEntryMapsEqual(sync_model_, expected_model));
+}
+
 TEST_F(DomDistillerStoreTest, TestGetAllSyncData) {
   AddEntry(GetSampleEntry(0), &db_model_);
   AddEntry(GetSampleEntry(1), &db_model_);