Add attachments support to DomDistillerStore

components/dom_distiller/core/dom_distiller_store.cc

Index: components/dom_distiller/core/dom_distiller_store.cc
diff --git a/components/dom_distiller/core/dom_distiller_store.cc b/components/dom_distiller/core/dom_distiller_store.cc
index 8a3aad588139ad4f1082b11d26a44b5b27e287fd..f8be2132bef418ce479f5c55397c5fcbff958b2f 100644
--- a/components/dom_distiller/core/dom_distiller_store.cc
+++ b/components/dom_distiller/core/dom_distiller_store.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/message_loop/message_loop.h"
 #include "components/dom_distiller/core/article_entry.h"
 #include "sync/api/sync_change.h"
 #include "sync/protocol/article_specifics.pb.h"
@@ -29,6 +30,7 @@ DomDistillerStore::DomDistillerStore(
     const base::FilePath& database_dir)
     : database_(database.Pass()),
       database_loaded_(false),
+      attachment_store_(syncer::AttachmentStore::CreateInMemoryStore()),

[maniscalco, 2014/11/12 18:59:44]

Is the async construction requirement of the on disk attachment store the
blocker on using it here?

[cjhopman, 2014/11/14 02:45:15]

yes.

       weak_ptr_factory_(this) {
   database_->Init(database_dir, base::Bind(&DomDistillerStore::OnDatabaseInit,
                                            weak_ptr_factory_.GetWeakPtr()));
@@ -40,6 +42,7 @@ DomDistillerStore::DomDistillerStore(
     const base::FilePath& database_dir)
     : database_(database.Pass()),
       database_loaded_(false),
+      attachment_store_(syncer::AttachmentStore::CreateInMemoryStore()),
       model_(initial_data),
       weak_ptr_factory_(this) {
   database_->Init(database_dir, base::Bind(&DomDistillerStore::OnDatabaseInit,
@@ -62,6 +65,102 @@ bool DomDistillerStore::GetEntryByUrl(const GURL& url, ArticleEntry* entry) {
   return model_.GetEntryByUrl(url, entry);
 }
 
+void DomDistillerStore::UpdateAttachments(
+    const std::string& entry_id,
+    scoped_ptr<ArticleAttachmentsData> attachments_data,
+    UpdateAttachmentsCallback callback) {

[maniscalco, 2014/11/12 18:59:44]

Can be passed by const&.

[cjhopman, 2014/11/14 02:45:15]

Done.

+  if (!GetEntryById(entry_id, nullptr)) {
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           base::Bind(callback, false));
+  }
+
+  scoped_ptr<sync_pb::ArticleAttachments> article_attachments(
+      new sync_pb::ArticleAttachments());
+  syncer::AttachmentList attachment_list;
+  attachments_data->CreateSyncAttachments(&attachment_list,
+                                          article_attachments.get());
+
+  attachment_store_->Write(
+      attachment_list,
+      base::Bind(&DomDistillerStore::OnAttachmentsWrite,
+                 weak_ptr_factory_.GetWeakPtr(), entry_id,
+                 base::Passed(&article_attachments), callback));
+}
+
+void DomDistillerStore::OnAttachmentsWrite(
+    const std::string& entry_id,
+    scoped_ptr<sync_pb::ArticleAttachments> article_attachments,
+    UpdateAttachmentsCallback callback,

[maniscalco, 2014/11/12 18:59:43]

Can be passed by const&.

[cjhopman, 2014/11/14 02:45:15]

Done.

+    const syncer::AttachmentStore::Result& result) {
+  bool success = result == syncer::AttachmentStore::SUCCESS;

[maniscalco, 2014/11/12 18:59:43]

WDYT about using a switch statement without a default case?  The motivation
would be to create compile error when someone adds a new result type and forgets
to update the places where it's used.

[cjhopman, 2014/11/14 02:45:15]

Done.

+  if (success) {

[maniscalco, 2014/11/12 18:59:43]

I'm thinking about cases where the write might fail.  Disk full seems like an
interesting case.  Should this code behave differently if the write fails
because disk is full vs. hard drive has failed ?  I wonder if the
AttachmentStore result code needs to differentiate between disk full and other
types of failures.

[cjhopman, 2014/11/14 02:45:15]

Yeah, I could see us handling disk full differently (or maybe even a
quota_exceeded if the attachment store allowed us to set a maximum size).

For RL, we are going to want something that handles dropping old attachments and
keeping the size of the locally stored ones down below some limit. How that is
going to work is unclear at this point.

+    ArticleEntry entry;
+    bool has_entry = GetEntryById(entry_id, &entry);
+    if (!has_entry) {
+      success = false;
+      attachment_store_->Drop(GetAttachmentIds(*article_attachments),
+                              syncer::AttachmentStore::DropCallback());
+    } else {
+      if (entry.has_attachments()) {
+        attachment_store_->Drop(GetAttachmentIds(entry.attachments()),
+                                syncer::AttachmentStore::DropCallback());
+      }
+      entry.set_allocated_attachments(article_attachments.release());
+
+      SyncChangeList changes_to_apply;
+      changes_to_apply.push_back(SyncChange(
+          FROM_HERE, SyncChange::ACTION_UPDATE, 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());
+
+      ApplyChangesToSync(FROM_HERE, changes_applied);
+      ApplyChangesToDatabase(changes_applied);
+    }
+  }
+  callback.Run(success);

[maniscalco, 2014/11/12 18:59:44]

Is it important that callback executes before control returns from
OnAttachmentsWrite?  If not, using PostTask here might eliminate a potential
source of re-entrancy bugs (comment applies elsewhere in this file).

[cjhopman, 2014/11/14 02:45:15]

Done.

+}
+
+void DomDistillerStore::GetAttachments(
+    const std::string& entry_id,
+    GetAttachmentsCallback callback) {

[maniscalco, 2014/11/12 18:59:44]

Can be passed by const&.

[cjhopman, 2014/11/14 02:45:15]

Done.

+  ArticleEntry entry;
+  if (!model_.GetEntryById(entry_id, &entry)
+      || !entry.has_attachments()) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, false, nullptr));
+    return;
+  }
+
+  // TODO(cjhopman): This should use GetOrDownloadAttachments() once there is a
+  // feasible way to use that.
+  attachment_store_->Read(GetAttachmentIds(entry.attachments()),
+                          base::Bind(&DomDistillerStore::OnAttachmentsRead,
+                                     weak_ptr_factory_.GetWeakPtr(),
+                                     entry.attachments(), callback));
+}
+
+void DomDistillerStore::OnAttachmentsRead(
+    const sync_pb::ArticleAttachments& attachments_proto,
+    GetAttachmentsCallback callback,

[maniscalco, 2014/11/12 18:59:43]

Can be passed by const&.

[cjhopman, 2014/11/14 02:45:15]

Done.

+    const syncer::AttachmentStore::Result& result,
+    scoped_ptr<syncer::AttachmentMap> attachments,
+    scoped_ptr<syncer::AttachmentIdList> missing) {
+  bool success =
+      (result == syncer::AttachmentStore::SUCCESS) && missing->empty();
+  scoped_ptr<ArticleAttachmentsData> attachments_data;
+  if (success) {
+    attachments_data = ArticleAttachmentsData::GetFromAttachmentMap(
+        attachments_proto, *attachments);
+  }
+  callback.Run(success, attachments_data.Pass());
+}
+
 bool DomDistillerStore::AddEntry(const ArticleEntry& entry) {
   return ChangeEntry(entry, SyncChange::ACTION_ADD);
 }
@@ -86,6 +185,15 @@ bool DomDistillerStore::ChangeEntry(const ArticleEntry& entry,
       DVLOG(1) << "Already have entry with id " << entry.entry_id() << ".";
       return false;
     }
+#ifndef NDEBUG

[maniscalco, 2014/11/12 18:59:44]

WDYT about moving this code into function and calling it from a DCHECK?  E.g.

namespace {

bool VerifyEntry(const ArticleEntry& entry,
                 const DomDistillerModel& model) {
   ArticleEntry currentEntry;
   model_.GetEntryById(entry.entry_id(), &currentEntry);
   DCHECK_EQ(currentEntry.has_attachments(), entry.has_attachments());
   if (currentEntry.has_attachments()) {
     DCHECK_EQ(currentEntry.attachments().SerializeAsString(),
               entry.attachments().SerializeAsString());
   }
   return true;
}

}

...

DCHECK(VerifyEntry(entry, model_));

[cjhopman, 2014/11/14 02:45:15]

I think it's a great idea.

+    ArticleEntry currentEntry;
+    model_.GetEntryById(entry.entry_id(), &currentEntry);
+    DCHECK_EQ(currentEntry.has_attachments(), entry.has_attachments());
+    if (currentEntry.has_attachments()) {
+      DCHECK_EQ(currentEntry.attachments().SerializeAsString(),
+                entry.attachments().SerializeAsString());
+    }
+#endif
   } else if (changeType != SyncChange::ACTION_ADD) {
     DVLOG(1) << "No entry with id " << entry.entry_id() << " found.";
     return false;