Implement a bag of chips for sync.

Implement a bag of chips for sync.

 The bag of chips is a per-client state used by the server. This state is
stored on the client and send back to the server on all communication.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156123

[Nicolas Zea, 2012-09-07 18:38:42]

Mostly looks good, some comments.

http://codereview.chromium.org/10916174/diff/1/sync/engine/model_changing_syn...
File sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/model_changing_syn...
sync/engine/model_changing_syncer_command.cc:51:
SyncerProtoUtil::PersistBagOfChips(session->context()->directory(),
we actually want to be able to persist the bag of chips even if the request
failed (it's still useful to let the server track client state even if it's
throttling for example)

Additionally, this is getting run on every command, and there are reallly only
two commands that interact with the sync server. I'd rather have this call
explicitly in those two commands.

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
sync/engine/process_commit_response_command_unittest.cc:384:
sync_pb::ClientToServerResponse& response =
nit: make pointer (references should always be const)

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
sync/engine/process_commit_response_command_unittest.cc:386:
session()->status_controller().updates_response());
use mutable_updates_response() instead of const_cast

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util.cc
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.cc:357: DCHECK(msg.has_store_birthday() ||
IsVeryFirstGetUpdates(msg))
I think the has_bag_of_chips dcheck should be separate and here instead

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util.h
File sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.h:77: // Persist an eventual bag of chips sent by
the server.
"Persist a bag of chips"? Not sure what you mean by eventual.

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.h:78: static void
PersistBagOfChips(syncable::Directory* dir,
nit: move first param onto new line so they line up

http://codereview.chromium.org/10916174/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

http://codereview.chromium.org/10916174/diff/1/sync/syncable/directory.h#newc...
sync/syncable/directory.h:174: // opaque to the client.
Mention the ChipBag message defined in sync.proto

[qsr, 2012-09-10 08:12:37]

http://codereview.chromium.org/10916174/diff/1/sync/engine/model_changing_syn...
File sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/model_changing_syn...
sync/engine/model_changing_syncer_command.cc:51:
SyncerProtoUtil::PersistBagOfChips(session->context()->directory(),
 And I was missing one of the 2. Thanks.

 On the other hand, to have this work, I have to override the ExecuteImpl method
-> The other method is called one for each worker... Which mean that it doesn't
work if there is no worker (won't happen I guess, but still), but will be called
more than once if there is more than one worker.

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
sync/engine/process_commit_response_command_unittest.cc:384:
sync_pb::ClientToServerResponse& response =
On 2012/09/07 18:38:43, nzea wrote:
> nit: make pointer (references should always be const)

Done.

http://codereview.chromium.org/10916174/diff/1/sync/engine/process_commit_res...
sync/engine/process_commit_response_command_unittest.cc:386:
session()->status_controller().updates_response());
 Problem is that status_controller is a const method. Changed the const cast for
status_controller, and then used mutable_updates_response.

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util.cc
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.cc:357: DCHECK(msg.has_store_birthday() ||
IsVeryFirstGetUpdates(msg))
 Sorry, typo. Done

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util.h
File sync/engine/syncer_proto_util.h (right):

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.h:77: // Persist an eventual bag of chips sent by
the server.
 I mean that if there is not bag of chips, nothing happen. Will clarify.

http://codereview.chromium.org/10916174/diff/1/sync/engine/syncer_proto_util....
sync/engine/syncer_proto_util.h:78: static void
PersistBagOfChips(syncable::Directory* dir,
On 2012/09/07 18:38:43, nzea wrote:
> nit: move first param onto new line so they line up
 Done, but I followed the style of the next method.

http://codereview.chromium.org/10916174/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

http://codereview.chromium.org/10916174/diff/1/sync/syncable/directory.h#newc...
sync/syncable/directory.h:174: // opaque to the client.
On 2012/09/07 18:38:43, nzea wrote:
> Mention the ChipBag message defined in sync.proto

Done.

[tim (not reviewing), 2012-09-10 17:52:55]

http://codereview.chromium.org/10916174/diff/7002/sync/engine/process_commit_...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10916174/diff/7002/sync/engine/process_commit_...
sync/engine/process_commit_response_command.cc:65: SyncerError
ProcessCommitResponseCommand::ExecuteImpl(SyncSession* session) {
mm, I understand how you got here, but lets not override this :)

Maybe lets leave ProcessCommitResponseCommand to do the "model changing work",
and move this meta-work to commit.cc, right before invoking
ProcessCommitResponseCommand?

[qsr, 2012-09-11 15:35:55]

On 2012/09/10 17:52:55, timsteele wrote:
>
http://codereview.chromium.org/10916174/diff/7002/sync/engine/process_commit_...
> File sync/engine/process_commit_response_command.cc (right):
> 
>
http://codereview.chromium.org/10916174/diff/7002/sync/engine/process_commit_...
> sync/engine/process_commit_response_command.cc:65: SyncerError
> ProcessCommitResponseCommand::ExecuteImpl(SyncSession* session) {
> mm, I understand how you got here, but lets not override this :)
> 
> Maybe lets leave ProcessCommitResponseCommand to do the "model changing work",
> and move this meta-work to commit.cc, right before invoking
> ProcessCommitResponseCommand?

 Problem is that it looks of out place. Instead I put it where the message is
received in syncer_proto_util.cc in SyncerProtoUtil::PostClientToServerMessage,
will that be fine with you? And will it handle the timestamp command?

[tim (not reviewing), 2012-09-11 17:57:20]

Actually, you picked an even better place.  LGTM!

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/protocol/sync...
File sync/protocol/sync.proto (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/protocol/sync...
sync/protocol/sync.proto:691: // sent with any subsequent ClientToServerMessages
nit - period.

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
File sync/syncable/directory.h (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory.h:175: // ChipBag defined in sync.proto.
This is free of NULL characters?

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory_backing_store_unittest.cc:2075: // Check that the
initial value is the serialization of an empty ChigBag.
nit ChipBag

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory_backing_store_unittest.cc:2078:
ASSERT_TRUE(chip_bag.SerializeToString(&serialized_chip_bag));
maybe also ASSERT_TRUE(!serialized_chip_bag.empty()) for clarity.

[commit-bot: I haz the power, 2012-09-11 18:43:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/10916174/3004

[qsr (NOT THE RIGHT qsr), 2012-09-11 18:48:49]

lgtm

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/protocol/sync...
File sync/protocol/sync.proto (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/protocol/sync...
sync/protocol/sync.proto:691: // sent with any subsequent ClientToServerMessages
On 2012/09/11 17:57:20, timsteele wrote:
> nit - period.

Done.

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
File sync/syncable/directory.h (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory.h:175: // ChipBag defined in sync.proto.
On 2012/09/11 17:57:20, timsteele wrote:
> This is free of NULL characters?

 Added comment.

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory_backing_store_unittest.cc:2075: // Check that the
initial value is the serialization of an empty ChigBag.
On 2012/09/11 17:57:20, timsteele wrote:
> nit ChipBag

Done.

https://chromiumcodereview.appspot.com/10916174/diff/10001/sync/syncable/dire...
sync/syncable/directory_backing_store_unittest.cc:2078:
ASSERT_TRUE(chip_bag.SerializeToString(&serialized_chip_bag));
On 2012/09/11 17:57:20, timsteele wrote:
> maybe also ASSERT_TRUE(!serialized_chip_bag.empty()) for clarity.
 That would be wrong, as the default value is an empty string.

 I'll add a test for persistence instead.

[commit-bot: I haz the power, 2012-09-11 21:13:37]

Change committed as 156123