Add attachments support to DomDistillerStore

components/dom_distiller/core/dom_distiller_store.cc

 // Copyright 2013 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 "components/dom_distiller/core/dom_distiller_store.h"
 
 #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"
 #include "sync/protocol/sync.pb.h"
 
 using leveldb_proto::ProtoDatabase;
 using sync_pb::ArticleSpecifics;
 using sync_pb::EntitySpecifics;
 using syncer::ModelType;
 using syncer::SyncChange;
 using syncer::SyncChangeList;
 using syncer::SyncData;
 using syncer::SyncDataList;
 using syncer::SyncError;
 using syncer::SyncMergeResult;
 
 namespace dom_distiller {
 
 DomDistillerStore::DomDistillerStore(
     scoped_ptr<ProtoDatabase<ArticleEntry> > database,
     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()));
 }
 
 DomDistillerStore::DomDistillerStore(
     scoped_ptr<ProtoDatabase<ArticleEntry> > database,
     const std::vector<ArticleEntry>& initial_data,
     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,
                                            weak_ptr_factory_.GetWeakPtr()));
 }
 
 DomDistillerStore::~DomDistillerStore() {}
 
 // DomDistillerStoreInterface implementation.
 syncer::SyncableService* DomDistillerStore::GetSyncableService() {
   return this;
 }
 
 bool DomDistillerStore::GetEntryById(const std::string& entry_id,
                                      ArticleEntry* entry) {
   return model_.GetEntryById(entry_id, entry);
 }
 
 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);
 }
 
 bool DomDistillerStore::UpdateEntry(const ArticleEntry& entry) {
   return ChangeEntry(entry, SyncChange::ACTION_UPDATE);
 }
 
 bool DomDistillerStore::RemoveEntry(const ArticleEntry& entry) {
   return ChangeEntry(entry, SyncChange::ACTION_DELETE);
 }
 
 bool DomDistillerStore::ChangeEntry(const ArticleEntry& entry,
                                     SyncChange::SyncChangeType changeType) {
   if (!database_loaded_) {
     return false;
   }
 
   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;
     }
+#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;
   }
 
   SyncChangeList changes_to_apply;
   changes_to_apply.push_back(
       SyncChange(FROM_HERE, changeType, CreateLocalData(entry)));
 
   SyncChangeList changes_applied;
@@ skipping 234 matching lines @@
   result.set_num_items_deleted(0);
 
   result.set_pre_association_version(0);
   result.set_num_items_after_association(model_.GetNumEntries());
   result.set_error(error);
 
   return result;
 }
 
 }  // namespace dom_distiller