Implementation of OnDiskAttachmentStore.

Implementation of OnDiskAttachmentStore.

In this change only attachment payload is stored, support for metadata
will be in separate change.
Also in_memory_attachment_store_unittest is moved into test_template.
This test is now used for both stores.

BUG=
R=maniscalco@chromium.org

Committed: https://crrev.com/c00ea0662879684dc1c435ce6168550a27800205
Cr-Commit-Position: refs/heads/master@{#300041}

[maniscalco, 2014-10-14 21:03:19]

Nice.  Here's my feedback.  Let's be sure to talk about a couple things in
person (detecting version mismatches and whether we can merge Load and the
Create functions).

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:54: virtual void Load(const
LoadCallback& callback) = 0;
What's the workflow here?  Call a Create method, then Load?  Is it illegal to
call Read/Write/Drop before Load completes successfully?

I wonder if there's a better name for this method.  When I see Load, I wonder
how it's different from Read.  Would this be better described as Open?

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:108: static
scoped_refptr<AttachmentStore> CreateOnDiskStore(
Can you please document this method?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_store_test_template.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_store_test_template.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Too bad the diff came out this like.  It's not easy to see what's new here. 
Would re-uploading the patch with a different --similarity provide a more usable
diff?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:21: const char
kAttachmentDataStart[] = "attachment_data1-";
WDYT about just "data-" ?  Seems like we'd benefit from a short prefix since the
DB will store it over and over.

Also, what's the 1 for?  I guess I was expecting the entire DB to be versioned,
not each key/value pair.  Let's chat about this one in person.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:24: // const char
kAttachmentDataEnd[] = "attachment_data2-";
Please remove this.  Let's avoid checking in any commented out code.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:47: // TODO(pavely):
Consider adding info_log, block_cache and filter_policy to
Can you please create crbugs for TODOs like this and reference the bug number in
the TODO (like we do for other attachment related TODOs).

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:49: status =
leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);
Any reason not to define status here instead of above?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:75:
AttachmentIdList::const_iterator end = ids.end();
Opportunity for const?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:78:
FormatAttachmentDataKey(*iter, &key);
Why take the string as an output param?  Why not just return std::string by
value and rely-on/hope-for NRVO?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:80: leveldb::Status
status = db_->Get(read_options, key, &data_str);
Opportunity for const?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:124: // metadata
record instead of payload record.
Can you create crbug to add metadata records?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:158:
AttachmentIdList::const_iterator end = ids.end();
Opportunity for const?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:162: status =
db_->Delete(write_options, key);
Declaring status outside the loop on purpose?  Would be good to be consistent
about in loop vs out of loop if all else is equal.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:177:
attachment_id.GetProto().AppendToString(key);
So the DB key is "attachment_data1-<serialized attachment id proto>" ?

I guess I would have expected the key to contain the unique_id string and not
the proto itself.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store_unittest.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:65: // Ensure
that store can be closed and reopen and keeps attachment stored in it.
How about "closed and reopened while retaining the stored attachments" ?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:92: store_ =
nullptr;
Just doing this for emphasis?  I ask because the next statement will cause the
previously pointed to object to be destroyed.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:29: // created.
What happens if the path does not exist?

What happens if a database already exists at the path?

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:48: void
FormatAttachmentDataKey(const AttachmentId& attachment_id,
Would CreateKeyFromAttachmentId be a better name?  When I see a FormatFoo method
on a database-like class, I wonder if it's similar to formatting a disk.

[maniscalco, 2014-10-15 16:16:37]

Just adding comments for the stuff we discussed yesterday in person so nothing
gets lost.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:21: const char
kAttachmentDataStart[] = "attachment_data1-";
As we talked about in person...

- an alternative to using the start/end approach is to use a key prefix.  When
iterating keys, when you find a key whose prefix does not match the one you
started with, you know you've seen all the records of the type you were looking
for.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:31: : path_(path),
As we talked about in person...

- in the future, we may change from leveldb to some other data store.  We can
make doing so easier if we make the leveldb dir (the one we pass to
level::DB::Open) a subdir under the AttachmentStore dir (e.g.
/my-attachment-store/level-db/).  Future version of AS can look in the
AttachmentStore dir to figure out what kind of AS they are using (no need to
create an external description file at this point).

- let's ensure that old code pointed at a newer db, will recognize that it
cannot possibly understand the new db and refuse to open it.  We talked about
doing this using a well known metadata record (proto value) in the store.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:46:
options.create_if_missing = true;
As we talked about in person...

- what does leveldb do if we point it as some non-empty dir that contains
non-leveldb files?  Any chance we could do harm by pointing an AttachmentStore
at a path exists and contains non-AS/non-leveldb files?  Would be great to have
the property that the worst you can do by giving a wrong path is create some new
files on disk that won't get cleaned up.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:39: virtual void
Load(const LoadCallback& callback) override;
As we talked about in person...

- it would be nice if AttachmentStores were "protocol-less".  That is, if you
have an instance of one, you don't have to worry about it being properly
initialized or not because whatever created it took care of that already.

- we can make the static factory functions that create+initialize
AttachmentStores asynchronous (i.e. they will take a callback parameter that
"returns" the newly created and initialized AttachmentStore).

[pavely, 2014-10-16 22:13:30]

PTAL

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:54: virtual void Load(const
LoadCallback& callback) = 0;
On 2014/10/14 21:03:18, maniscalco wrote:
> What's the workflow here?  Call a Create method, then Load?  Is it illegal to
> call Read/Write/Drop before Load completes successfully?
> 
> I wonder if there's a better name for this method.  When I see Load, I wonder
> how it's different from Read.  Would this be better described as Open?

Removed method altogether.

https://codereview.chromium.org/652723003/diff/40001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:108: static
scoped_refptr<AttachmentStore> CreateOnDiskStore(
On 2014/10/14 21:03:18, maniscalco wrote:
> Can you please document this method?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_store_test_template.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_store_test_template.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/10/14 21:03:18, maniscalco wrote:
> Too bad the diff came out this like.  It's not easy to see what's new here. 
> Would re-uploading the patch with a different --similarity provide a more
usable
> diff?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:21: const char
kAttachmentDataStart[] = "attachment_data1-";
On 2014/10/14 21:03:18, maniscalco wrote:
> WDYT about just "data-" ?  Seems like we'd benefit from a short prefix since
the
> DB will store it over and over.
> 
> Also, what's the 1 for?  I guess I was expecting the entire DB to be
versioned,
> not each key/value pair.  Let's chat about this one in person.

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:24: // const char
kAttachmentDataEnd[] = "attachment_data2-";
On 2014/10/14 21:03:19, maniscalco wrote:
> Please remove this.  Let's avoid checking in any commented out code.

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:31: : path_(path),
On 2014/10/15 16:16:36, maniscalco wrote:
> As we talked about in person...
> 
> - in the future, we may change from leveldb to some other data store.  We can
> make doing so easier if we make the leveldb dir (the one we pass to
> level::DB::Open) a subdir under the AttachmentStore dir (e.g.
> /my-attachment-store/level-db/).  Future version of AS can look in the
> AttachmentStore dir to figure out what kind of AS they are using (no need to
> create an external description file at this point).
> 
> - let's ensure that old code pointed at a newer db, will recognize that it
> cannot possibly understand the new db and refuse to open it.  We talked about
> doing this using a well known metadata record (proto value) in the store.

Done with creating leveldb in subdir. Will implement metadata record in separate
change.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:46:
options.create_if_missing = true;
On 2014/10/15 16:16:36, maniscalco wrote:
> As we talked about in person...
> 
> - what does leveldb do if we point it as some non-empty dir that contains
> non-leveldb files?  Any chance we could do harm by pointing an AttachmentStore
> at a path exists and contains non-AS/non-leveldb files?  Would be great to
have
> the property that the worst you can do by giving a wrong path is create some
new
> files on disk that won't get cleaned up.

During normal operations leveldb only affects files with names in format it
recognizes, everything else stays intact.
There is DestroyDB function that deletes leveldb files individually, but then
calls env->DeleteDir. Posix implementation of it will keep directory intact if
it still has files, but there is no requirement explained on env.h for this
function.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:47: // TODO(pavely):
Consider adding info_log, block_cache and filter_policy to
On 2014/10/14 21:03:18, maniscalco wrote:
> Can you please create crbugs for TODOs like this and reference the bug number
in
> the TODO (like we do for other attachment related TODOs).

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:49: status =
leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);
On 2014/10/14 21:03:18, maniscalco wrote:
> Any reason not to define status here instead of above?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:75:
AttachmentIdList::const_iterator end = ids.end();
On 2014/10/14 21:03:18, maniscalco wrote:
> Opportunity for const?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:78:
FormatAttachmentDataKey(*iter, &key);
On 2014/10/14 21:03:18, maniscalco wrote:
> Why take the string as an output param?  Why not just return std::string by
> value and rely-on/hope-for NRVO?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:80: leveldb::Status
status = db_->Get(read_options, key, &data_str);
On 2014/10/14 21:03:18, maniscalco wrote:
> Opportunity for const?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:124: // metadata
record instead of payload record.
On 2014/10/14 21:03:18, maniscalco wrote:
> Can you create crbug to add metadata records?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:158:
AttachmentIdList::const_iterator end = ids.end();
On 2014/10/14 21:03:18, maniscalco wrote:
> Opportunity for const?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:162: status =
db_->Delete(write_options, key);
On 2014/10/14 21:03:18, maniscalco wrote:
> Declaring status outside the loop on purpose?  Would be good to be consistent
> about in loop vs out of loop if all else is equal.

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:177:
attachment_id.GetProto().AppendToString(key);
On 2014/10/14 21:03:19, maniscalco wrote:
> So the DB key is "attachment_data1-<serialized attachment id proto>" ?
> 
> I guess I would have expected the key to contain the unique_id string and not
> the proto itself.

Tradeoffs:
+ If I use unique_id, keys are smaller and more readable.
+ Using unique_id is more future safe. With proto in key any change to proto
will require database upgrade.
- when enumerating attachments I'll likely need to construct AttachmentIdProto,
this representation allows me to do this directly without storing
AttachmentIdProto separately in metadata record and without making significant
assumptions about future AttachmentIdProto content.

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store_unittest.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:65: // Ensure
that store can be closed and reopen and keeps attachment stored in it.
On 2014/10/14 21:03:19, maniscalco wrote:
> How about "closed and reopened while retaining the stored attachments" ?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:92: store_ =
nullptr;
On 2014/10/14 21:03:19, maniscalco wrote:
> Just doing this for emphasis?  I ask because the next statement will cause the
> previously pointed to object to be destroyed.
Yes, this is just to make it easier to read this test.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:29: // created.
On 2014/10/14 21:03:19, maniscalco wrote:
> What happens if the path does not exist?
> 
> What happens if a database already exists at the path?

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:39: virtual void
Load(const LoadCallback& callback) override;
On 2014/10/15 16:16:36, maniscalco wrote:
> As we talked about in person...
> 
> - it would be nice if AttachmentStores were "protocol-less".  That is, if you
> have an instance of one, you don't have to worry about it being properly
> initialized or not because whatever created it took care of that already.
> 
> - we can make the static factory functions that create+initialize
> AttachmentStores asynchronous (i.e. they will take a callback parameter that
> "returns" the newly created and initialized AttachmentStore).

Done.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/public...
sync/internal_api/public/attachments/on_disk_attachment_store.h:48: void
FormatAttachmentDataKey(const AttachmentId& attachment_id,
On 2014/10/14 21:03:19, maniscalco wrote:
> Would CreateKeyFromAttachmentId be a better name?  When I see a FormatFoo
method
> on a database-like class, I wonder if it's similar to formatting a disk.

Done.

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_store.cc:24: scoped_refptr<AttachmentStore>
AttachmentStore::CreateInMemoryStore() {
InMemoryAttachmentStore is still created synchronously because fair number of
tests rely on AttachmentServiceImpl::CreateForTest to return attachment service
right away.

[maniscalco, 2014-10-17 00:00:19]

Nice work.  LGTM with a few final comments.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:31: : path_(path),
On 2014/10/16 22:13:29, pavely wrote:
> On 2014/10/15 16:16:36, maniscalco wrote:
> > As we talked about in person...
> > 
> > - in the future, we may change from leveldb to some other data store.  We
can
> > make doing so easier if we make the leveldb dir (the one we pass to
> > level::DB::Open) a subdir under the AttachmentStore dir (e.g.
> > /my-attachment-store/level-db/).  Future version of AS can look in the
> > AttachmentStore dir to figure out what kind of AS they are using (no need to
> > create an external description file at this point).
> > 
> > - let's ensure that old code pointed at a newer db, will recognize that it
> > cannot possibly understand the new db and refuse to open it.  We talked
about
> > doing this using a well known metadata record (proto value) in the store.
> 
> Done with creating leveldb in subdir. Will implement metadata record in
separate
> change.

SGTM

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:46:
options.create_if_missing = true;
On 2014/10/16 22:13:29, pavely wrote:
> On 2014/10/15 16:16:36, maniscalco wrote:
> > As we talked about in person...
> > 
> > - what does leveldb do if we point it as some non-empty dir that contains
> > non-leveldb files?  Any chance we could do harm by pointing an
AttachmentStore
> > at a path exists and contains non-AS/non-leveldb files?  Would be great to
> have
> > the property that the worst you can do by giving a wrong path is create some
> new
> > files on disk that won't get cleaned up.
> 
> During normal operations leveldb only affects files with names in format it
> recognizes, everything else stays intact.
> There is DestroyDB function that deletes leveldb files individually, but then
> calls env->DeleteDir. Posix implementation of it will keep directory intact if
> it still has files, but there is no requirement explained on env.h for this
> function.

Acknowledged.

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store_unittest.cc (right):

https://codereview.chromium.org/652723003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store_unittest.cc:92: store_ =
nullptr;
On 2014/10/16 22:13:29, pavely wrote:
> On 2014/10/14 21:03:19, maniscalco wrote:
> > Just doing this for emphasis?  I ask because the next statement will cause
the
> > previously pointed to object to be destroyed.
> Yes, this is just to make it easier to read this test.

Acknowledged.

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_store.cc:24: scoped_refptr<AttachmentStore>
AttachmentStore::CreateInMemoryStore() {
On 2014/10/16 22:13:30, pavely wrote:
> InMemoryAttachmentStore is still created synchronously because fair number of
> tests rely on AttachmentServiceImpl::CreateForTest to return attachment
service
> right away.

Ah, right.  That seems OK to me.

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_store.cc:71: callback.Run(result, store);
It's not a big deal in this case, but in general it seems like PostTask is
usually better than calling .Run directly since it eliminates the possibility of
unexpected re-entrancy.

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:113: const
scoped_refptr<base::SequencedTaskRunner> backend_task_runner,
Can be passed as const&.

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:53: std::string key =
CreateDataKeyFromAttachmentId(*iter);
Opportunity for const.

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:92: for (; iter !=
end; ++iter) {
Opportunity for const.  I often wish automatic variables were const by default
:)

[pavely, 2014-10-17 00:13:25]

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
File sync/api/attachments/attachment_store.h (right):

https://codereview.chromium.org/652723003/diff/60001/sync/api/attachments/att...
sync/api/attachments/attachment_store.h:113: const
scoped_refptr<base::SequencedTaskRunner> backend_task_runner,
On 2014/10/17 00:00:19, maniscalco wrote:
> Can be passed as const&.

Done.

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:53: std::string key =
CreateDataKeyFromAttachmentId(*iter);
On 2014/10/17 00:00:19, maniscalco wrote:
> Opportunity for const.

Done.

https://codereview.chromium.org/652723003/diff/60001/sync/internal_api/attach...
sync/internal_api/attachments/on_disk_attachment_store.cc:92: for (; iter !=
end; ++iter) {
On 2014/10/17 00:00:19, maniscalco wrote:
> Opportunity for const.  I often wish automatic variables were const by default
> :)

Done.

[pavely, 2014-10-17 00:13:29]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-10-17 00:16:17]

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

[commit-bot: I haz the power, 2014-10-17 01:59:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-17 01:59:53]

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