[Sync] Add context proto and directory support

[Sync] Add context proto and directory support

Still not hooked up to anything, but the proto and directory persistence are
now there.

BUG=358352
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261666

[Nicolas Zea, 2014-04-02 20:05:00]

Here's the follow up patch I mentioned.

[rlarocque, 2014-04-03 01:08:06]

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/proto_valu...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/proto_valu...
sync/protocol/proto_value_conversions.cc:873: SET_INT32(type);
off-topic: We should add a SET_TYPE macro.  I'm tired of looking up specifics
numbers.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store.cc:1277: if (!db_->Execute("ALTER TABLE
models ADD COLUMN context blob"))
Should I expect to see UPDATE and SELECT statements, too, or will those be added
in a separate CL?

[maniscalco, 2014-04-03 16:59:36]

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:633: // Set of optional per-client datatype contexts.
Just for my own benefit... I assume that you're using the term set here
intentionally.  I.e. the client promises not to send a list of contexts with
duplicate types.  Or does it make sense to ever send (or receive) two contexts
with the same type?

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:634: repeated DataTypeContext client_contexts = 11;
I don't think we've been consistent, but going forward what's our naming
convention for repeated fields, singular or plural?

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:682: optional int32 type = 1;
Elsewhere in this file I see "data_type_id".  Is this the same thing?  If so,
consider renaming to data_type_id so it's clear that it's the same thing.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory....
sync/syncable/directory.cc:797: kernel_->transaction_mutex.AssertAcquired();
I noticed that in some methods we acquire a lock (ScopedKernelLock(this);), but
in others (like here), we simply assert we've acquired the transaction_mutex. 
Can you help me understand when we do one vs. the other?

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store_unittest.cc:2538: void
MigrationTest::SetUpVersion87Database(sql::Connection* connection) {
I'm afraid this patch is off an older HEAD.  I recently introduced version 87. 
Can you please merge/rebase (sorry, I know updating this test sucks).

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store_unittest.cc:3316:
ASSERT_TRUE(connection.DoesColumnExist("models", "transaction_version"));
Can you an assert that verifies the DB has the new column?

[rlarocque, 2014-04-03 17:36:55]

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory....
sync/syncable/directory.cc:804: kernel_->transaction_mutex.AssertAcquired();
What's up with this pattern?  I thought the traditional approach was to require
a pointer to the current transaction.  That way you would be able to verify if
it was a read or write transaction.

(Not that it really matters, since we're not using a real read-write mutex, but
still it seems like a good idea to maintain the abstraction.)

[Nicolas Zea, 2014-04-03 21:01:10]

PTAL

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/proto_valu...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/proto_valu...
sync/protocol/proto_value_conversions.cc:873: SET_INT32(type);
On 2014/04/03 01:08:06, rlarocque wrote:
> off-topic: We should add a SET_TYPE macro.  I'm tired of looking up specifics
> numbers.
SGTM

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:633: // Set of optional per-client datatype contexts.
On 2014/04/03 16:59:37, maniscalco wrote:
> Just for my own benefit... I assume that you're using the term set here
> intentionally.  I.e. the client promises not to send a list of contexts with
> duplicate types.  Or does it make sense to ever send (or receive) two contexts
> with the same type?

No, there should only be one per type.

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:634: repeated DataTypeContext client_contexts = 11;
On 2014/04/03 16:59:37, maniscalco wrote:
> I don't think we've been consistent, but going forward what's our naming
> convention for repeated fields, singular or plural?

I don't think we have one. I try to use plural though.

https://codereview.chromium.org/218623014/diff/40001/sync/protocol/sync.proto...
sync/protocol/sync.proto:682: optional int32 type = 1;
On 2014/04/03 16:59:37, maniscalco wrote:
> Elsewhere in this file I see "data_type_id".  Is this the same thing?  If so,
> consider renaming to data_type_id so it's clear that it's the same thing.

Done.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory....
sync/syncable/directory.cc:797: kernel_->transaction_mutex.AssertAcquired();
On 2014/04/03 16:59:37, maniscalco wrote:
> I noticed that in some methods we acquire a lock (ScopedKernelLock(this);),
but
> in others (like here), we simply assert we've acquired the transaction_mutex. 
> Can you help me understand when we do one vs. the other?

It has to do with whether it's expected that a transaction is being held at the
time the change is made. Previously, when I was doing things through a
ProcessSyncChanges call, that would hold true. Now though I realize this is
incorrect. I've updated it to acquire the lock (here and below)

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory....
sync/syncable/directory.cc:804: kernel_->transaction_mutex.AssertAcquired();
On 2014/04/03 17:36:55, rlarocque wrote:
> What's up with this pattern?  I thought the traditional approach was to
require
> a pointer to the current transaction.  That way you would be able to verify if
> it was a read or write transaction.
> 
> (Not that it really matters, since we're not using a real read-write mutex,
but
> still it seems like a good idea to maintain the abstraction.)

This is actually wrong, see above comment. I've updated to acquire the lock
there.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store.cc:1277: if (!db_->Execute("ALTER TABLE
models ADD COLUMN context blob"))
On 2014/04/03 01:08:06, rlarocque wrote:
> Should I expect to see UPDATE and SELECT statements, too, or will those be
added
> in a separate CL?

I've gone ahead and added those to this CL.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store_unittest.cc:2538: void
MigrationTest::SetUpVersion87Database(sql::Connection* connection) {
On 2014/04/03 16:59:37, maniscalco wrote:
> I'm afraid this patch is off an older HEAD.  I recently introduced version 87.

> Can you please merge/rebase (sorry, I know updating this test sucks).

Done.

https://codereview.chromium.org/218623014/diff/40001/sync/syncable/directory_...
sync/syncable/directory_backing_store_unittest.cc:3316:
ASSERT_TRUE(connection.DoesColumnExist("models", "transaction_version"));
On 2014/04/03 16:59:37, maniscalco wrote:
> Can you an assert that verifies the DB has the new column?

Done.

[rlarocque, 2014-04-03 21:17:55]

https://codereview.chromium.org/218623014/diff/60001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/218623014/diff/60001/sync/syncable/directory....
sync/syncable/directory.cc:796: sync_pb::DataTypeContext* context) const {
This is better, but I think you'll still want to have BaseTransaction* and
WriteTransaction* parameters on these functions.  See the atomicity-related
comments on the next CL in the series for details.

[Nicolas Zea, 2014-04-03 22:11:49]

PTAL

https://codereview.chromium.org/218623014/diff/60001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/218623014/diff/60001/sync/syncable/directory....
sync/syncable/directory.cc:796: sync_pb::DataTypeContext* context) const {
On 2014/04/03 21:17:56, rlarocque wrote:
> This is better, but I think you'll still want to have BaseTransaction* and
> WriteTransaction* parameters on these functions.  See the atomicity-related
> comments on the next CL in the series for details.

Done.

[rlarocque, 2014-04-03 22:25:48]

LGTM.

[Nicolas Zea, 2014-04-03 22:56:55]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-03 22:58:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/218623014/80001

[maniscalco, 2014-04-03 23:12:35]

On 2014/04/03 22:58:24, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/zea@chromium.org/218623014/80001

LGTM.

[commit-bot: I haz the power, 2014-04-04 00:08:12]

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

[commit-bot: I haz the power, 2014-04-04 00:08:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Nicolas Zea, 2014-04-04 03:03:36]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-04 03:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/218623014/80001

[commit-bot: I haz the power, 2014-04-04 04:23:16]

Change committed as 261666