Introduce AttachmentStore metadata record

Introduce AttachmentStore metadata record

This change handles different errors during attachment store open:
- create record for empty database
- fail if record is corrupt or of newer version.

BUG=424304
R=maniscalco@chromium.org

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

[pavely, 2014-10-21 18:45:32]

PTAL.

[maniscalco, 2014-10-21 21:41:04]

Very nice.  Just a few questions / nits...

https://codereview.chromium.org/647553009/diff/1/sync/BUILD.gn
File sync/BUILD.gn (right):

https://codereview.chromium.org/647553009/diff/1/sync/BUILD.gn#newcode11
sync/BUILD.gn:11: "//sync/internal_api/attachments/proto",
sort order?

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:203:
leveldb::ReadOptions read_options;
Since we do this in a couple places, consider creating a helper function that
returns a ReadOptions object with the values we use.

(same may apply to WriteOptions)

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:216: leveldb::Status
OnDiskAttachmentStore::UpdateStoreMetadata(
Light suggestion: since this function replaces the existing metadata record,
consider renaming to Write.  Update might be interpreted as updating only the
fields that have been set in the proto.

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:16: // older code
that don't understand it.
don't -> doesn't

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/on_disk_attachment_store.h:57: static
leveldb::Status ReadStoreMetadata(
Light suggestion: unless you need them to be part of the class, consider moving
these functions to an anon namespace in the .cc file.

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode25
sync/sync.gyp:25: 'attachment_store_proto',
sort order?

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode57
sync/sync.gyp:57: 'attachment_store_proto',
sort order?

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode537
sync/sync.gyp:537: 'enable_wexit_time_desctructors': 1,
Can you explain to me what this is for.  Is it necessary?

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode538
sync/sync.gyp:538: 'proto_in_dir': './internal_api/attachments/proto',
Is the leading ./ important?  In other gyp files, it seems like it's usually
omitted for proto_in_dir (enhanced_bookmarks being an exception).

[pavely, 2014-10-24 20:27:36]

Patchset #2 (id:20001) has been deleted

[pavely, 2014-10-24 20:42:55]

PTAL

https://codereview.chromium.org/647553009/diff/1/sync/BUILD.gn
File sync/BUILD.gn (right):

https://codereview.chromium.org/647553009/diff/1/sync/BUILD.gn#newcode11
sync/BUILD.gn:11: "//sync/internal_api/attachments/proto",
On 2014/10/21 21:41:03, maniscalco wrote:
> sort order?

Done.

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:203:
leveldb::ReadOptions read_options;
On 2014/10/21 21:41:03, maniscalco wrote:
> Since we do this in a couple places, consider creating a helper function that
> returns a ReadOptions object with the values we use.
> 
> (same may apply to WriteOptions)

I've created function for WriteOptions.
I want to defer doing the same for ReadingOptions to the change that does
attachment record metadata. Attachment store will use different combination of
read options for different things.

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:216: leveldb::Status
OnDiskAttachmentStore::UpdateStoreMetadata(
On 2014/10/21 21:41:03, maniscalco wrote:
> Light suggestion: since this function replaces the existing metadata record,
> consider renaming to Write.  Update might be interpreted as updating only the
> fields that have been set in the proto.

Done.

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/proto/attachment_store.proto (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/proto/attachment_store.proto:16: // older code
that don't understand it.
On 2014/10/21 21:41:03, maniscalco wrote:
> don't -> doesn't

Done.

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/public/att...
File sync/internal_api/public/attachments/on_disk_attachment_store.h (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/public/att...
sync/internal_api/public/attachments/on_disk_attachment_store.h:57: static
leveldb::Status ReadStoreMetadata(
On 2014/10/21 21:41:03, maniscalco wrote:
> Light suggestion: unless you need them to be part of the class, consider
moving
> these functions to an anon namespace in the .cc file.

Done.

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode25
sync/sync.gyp:25: 'attachment_store_proto',
On 2014/10/21 21:41:03, maniscalco wrote:
> sort order?

Done.

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode57
sync/sync.gyp:57: 'attachment_store_proto',
On 2014/10/21 21:41:03, maniscalco wrote:
> sort order?

Done.

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode537
sync/sync.gyp:537: 'enable_wexit_time_desctructors': 1,
On 2014/10/21 21:41:04, maniscalco wrote:
> Can you explain to me what this is for.  Is it necessary?

It enables "exit time destructors" warning. Allows to catch introduction of exit
time destructors at compile time.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/FM8C-qPm5fk
https://code.google.com/p/chromium/issues/detail?id=101600

Entries in sync.gyp are misspelled. I'm fixing spelling.

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode538
sync/sync.gyp:538: 'proto_in_dir': './internal_api/attachments/proto',
On 2014/10/21 21:41:04, maniscalco wrote:
> Is the leading ./ important?  In other gyp files, it seems like it's usually
> omitted for proto_in_dir (enhanced_bookmarks being an exception).

Removed ./

[maniscalco, 2014-10-24 21:52:39]

LGTM!

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/on_disk_attachment_store.cc (right):

https://codereview.chromium.org/647553009/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/on_disk_attachment_store.cc:203:
leveldb::ReadOptions read_options;
On 2014/10/24 20:42:55, pavely wrote:
> On 2014/10/21 21:41:03, maniscalco wrote:
> > Since we do this in a couple places, consider creating a helper function
that
> > returns a ReadOptions object with the values we use.
> > 
> > (same may apply to WriteOptions)
> 
> I've created function for WriteOptions.
> I want to defer doing the same for ReadingOptions to the change that does
> attachment record metadata. Attachment store will use different combination of
> read options for different things.

SGTM

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/647553009/diff/1/sync/sync.gyp#newcode537
sync/sync.gyp:537: 'enable_wexit_time_desctructors': 1,
On 2014/10/24 20:42:55, pavely wrote:
> On 2014/10/21 21:41:04, maniscalco wrote:
> > Can you explain to me what this is for.  Is it necessary?
> 
> It enables "exit time destructors" warning. Allows to catch introduction of
exit
> time destructors at compile time.
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/FM8C-qPm5fk
> https://code.google.com/p/chromium/issues/detail?id=101600
> 
> Entries in sync.gyp are misspelled. I'm fixing spelling.

Ah!  Cool.  I agree that we want this.  Good catch on the ones in sync.gyp.

[pavely, 2014-10-24 21:53:54]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-10-24 21:55:45]

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

[commit-bot: I haz the power, 2014-10-24 23:51:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 23:51:15]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pavely, 2014-10-25 00:26:05]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-10-25 00:28:11]

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

[commit-bot: I haz the power, 2014-10-25 02:41:05]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-25 02:41:56]

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