Add attachments support to DomDistillerStore

Add attachments support to DomDistillerStore

The DomDistillerStore now provides simple, asynchronous Get and Update
for attachments. When an update is requested, we add the attachments to
the attachment store, and when that succeeds update the model/sync/db
entry. So, while non-attachment updates are applied asynchronously, that
is not the case for attachment updates.

Most of the mapping between the ids stored in the specifics and the actual
attachment objects is handled by ArticleAttachmentsData.

BUG=430552
Committed: https://crrev.com/ce6d65fc6d4030cad4db59d6aa6d6285b4de660c
Cr-Commit-Position: refs/heads/master@{#304917}

[cjhopman, 2014-11-12 02:51:01]

cjhopman@chromium.org changed reviewers:
+ maniscalco@chromium.org

[cjhopman, 2014-11-12 02:51:02]

maniscalco: ptal.

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

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/article_attachments_data.cc (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.cc:50: }
nit:  // namespace dom_distiller

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/article_attachments_data.h (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.h:17: class
ArticleAttachmentsData {
How should I think about this class?  Is it essentially a distilled article and
its attachments?  I'm still trying to wrap my head around the dom distiller
code.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.h:45: #endif
nit:  // COMPONENTS_DOM_DISTILLER_CORE_ARTICLE_ATTACHMENTS_DATA_H_

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:33:
attachment_store_(syncer::AttachmentStore::CreateInMemoryStore()),
Is the async construction requirement of the on disk attachment store the
blocker on using it here?

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:71:
UpdateAttachmentsCallback callback) {
Can be passed by const&.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:93:
UpdateAttachmentsCallback callback,
Can be passed by const&.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:95: bool success = result
== syncer::AttachmentStore::SUCCESS;
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.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:96: if (success) {
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.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:126: callback.Run(success);
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).

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:131: GetAttachmentsCallback
callback) {
Can be passed by const&.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:150: GetAttachmentsCallback
callback,
Can be passed by const&.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:188: #ifndef NDEBUG
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_));

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.h (left):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.h:41: 
Accidental line removal?

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.h (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.h:51: typedef
base::Callback<void(bool)>
What does the bool indicate?

[cjhopman, 2014-11-14 02:45:03]

Patchset #4 (id:60001) has been deleted

[cjhopman, 2014-11-14 02:45:08]

Patchset #4 (id:80001) has been deleted

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

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/article_attachments_data.cc (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.cc:50: }
On 2014/11/12 18:59:43, maniscalco wrote:
> nit:  // namespace dom_distiller

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/article_attachments_data.h (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.h:17: class
ArticleAttachmentsData {
On 2014/11/12 18:59:43, maniscalco wrote:
> How should I think about this class?  Is it essentially a distilled article
and
> its attachments?  I'm still trying to wrap my head around the dom distiller
> code.

I've documented it a bit. Let me know if that doesn't help.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/article_attachments_data.h:45: #endif
On 2014/11/12 18:59:43, maniscalco wrote:
> nit:  // COMPONENTS_DOM_DISTILLER_CORE_ARTICLE_ATTACHMENTS_DATA_H_

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:33:
attachment_store_(syncer::AttachmentStore::CreateInMemoryStore()),
On 2014/11/12 18:59:44, maniscalco wrote:
> Is the async construction requirement of the on disk attachment store the
> blocker on using it here?

yes.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:71:
UpdateAttachmentsCallback callback) {
On 2014/11/12 18:59:44, maniscalco wrote:
> Can be passed by const&.

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:93:
UpdateAttachmentsCallback callback,
On 2014/11/12 18:59:43, maniscalco wrote:
> Can be passed by const&.

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:95: bool success = result
== syncer::AttachmentStore::SUCCESS;
On 2014/11/12 18:59:43, maniscalco wrote:
> 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.

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:96: if (success) {
On 2014/11/12 18:59:43, maniscalco wrote:
> 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.

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.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:126: callback.Run(success);
On 2014/11/12 18:59:44, maniscalco wrote:
> 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).

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:131: GetAttachmentsCallback
callback) {
On 2014/11/12 18:59:44, maniscalco wrote:
> Can be passed by const&.

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:150: GetAttachmentsCallback
callback,
On 2014/11/12 18:59:43, maniscalco wrote:
> Can be passed by const&.

Done.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.cc:188: #ifndef NDEBUG
On 2014/11/12 18:59:44, maniscalco wrote:
> 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_));

I think it's a great idea.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.h (left):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.h:41: 
On 2014/11/12 18:59:44, maniscalco wrote:
> Accidental line removal?

Done. pulled the line removal into different change.

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_store.h (right):

https://codereview.chromium.org/717793007/diff/40001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_store.h:51: typedef
base::Callback<void(bool)>
On 2014/11/12 18:59:44, maniscalco wrote:
> What does the bool indicate?

Done. added names here and documentation to the functions below

[pavely, 2014-11-17 19:09:00]

pavely@chromium.org changed reviewers:
+ pavely@chromium.org

[pavely, 2014-11-17 19:09:01]

I only had couple of tiny comments.

I'll figure out async initialization of OnDiskAttachmentStore story this week.

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.cc:168: success =
missing->empty();
Minor comment: if result is SUCCESS then |missing|  should be empty. You can
DCHECK(missing->empty()); and success = true; here.

https://codereview.chromium.org/717793007/diff/100001/sync/protocol/article_s...
File sync/protocol/article_specifics.proto (right):

https://codereview.chromium.org/717793007/diff/100001/sync/protocol/article_s...
sync/protocol/article_specifics.proto:18: // Next ID to use: 4
Could you update "Next ID"? Do we even need it?

[maniscalco, 2014-11-17 19:12:48]

LGTM modulo pavely's feedback.

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
File components/dom_distiller/core/article_attachments_data.h (right):

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
components/dom_distiller/core/article_attachments_data.h:17: // When stored,
article attachments are split into two pieces, the actual data
Helpful comment!

[cjhopman, 2014-11-18 20:57:45]

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/100001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.cc:168: success =
missing->empty();
On 2014/11/17 19:09:00, pavely wrote:
> Minor comment: if result is SUCCESS then |missing|  should be empty. You can
> DCHECK(missing->empty()); and success = true; here.

Done.

https://codereview.chromium.org/717793007/diff/100001/sync/protocol/article_s...
File sync/protocol/article_specifics.proto (right):

https://codereview.chromium.org/717793007/diff/100001/sync/protocol/article_s...
sync/protocol/article_specifics.proto:18: // Next ID to use: 4
On 2014/11/17 19:09:00, pavely wrote:
> Could you update "Next ID"? Do we even need it?

Done.

[cjhopman, 2014-11-18 20:58:19]

cjhopman@chromium.org changed reviewers:
+ nyquist@chromium.org

[pavely, 2014-11-18 21:04:46]

lgtm

[nyquist, 2014-11-19 02:26:38]

lgtm

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.cc:222:
DCHECK(VerifyAttachmentsUnchanged(entry, model_));
This always returns true, does it really need to be a bool function and be
DCHECK'ed? Are you planning on expanding this method to at some point returning
something else than true?

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.h (right):

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.h:56: // success==true once
the new attachments have been stored locally and the
Optional nit: |success| here and below? Given how you've removed the whitespace
around == I don't think it adds a lot of clarity to do that though. I'm fine
either way.

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.h:67: // the a pointer to the
attachments. Otherwise it will be called with
Nit: remove "the"?

[cjhopman, 2014-11-19 19:31:54]

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.cc (right):

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.cc:222:
DCHECK(VerifyAttachmentsUnchanged(entry, model_));
On 2014/11/19 02:26:37, nyquist wrote:
> This always returns true, does it really need to be a bool function and be
> DCHECK'ed? Are you planning on expanding this method to at some point
returning
> something else than true?

The DCHECK ensures that the function isn't called in non-debug builds.

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_store.h (right):

https://codereview.chromium.org/717793007/diff/120001/components/dom_distille...
components/dom_distiller/core/dom_distiller_store.h:67: // the a pointer to the
attachments. Otherwise it will be called with
On 2014/11/19 02:26:37, nyquist wrote:
> Nit: remove "the"?

Done.

[cjhopman, 2014-11-19 22:19:20]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-11-19 22:20:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717793007/140001

[commit-bot: I haz the power, 2014-11-19 23:13:19]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2014-11-19 23:14:36]

Patchset 6 (id:??) landed as
https://crrev.com/ce6d65fc6d4030cad4db59d6aa6d6285b4de660c
Cr-Commit-Position: refs/heads/master@{#304917}