FYI: Control Data + Per-Device Metadata

This is a squashed stack of patches to split the nigori node.

My current plan is to split it up into three patches:
1. Remove WITH_NIGORI/WITHOUT_NIGORI flags.
2. Implement support for "Control Data" types and make NIGORI one of
these types.
3. Add the Per-Device Info type and re-enable DeviceInfo tracking.

A fourth patch to support a "flags" type has yet to be written.

I also need to write (or have someone on the server team write) a
patch to add support for these new data types.

I've posted this patch here so we can keep an eye on the big picture
as we review the individual patches.

BUG=122825, 111297

[rlarocque, 2012-08-01 20:07:20]

Here's a rebased version of the control data patch.  I won't try to have it
committed until M22 is branched, so this review is FYI only for the moment.  

I'll send out a ping when I'm ready to have it committed.

http://codereview.chromium.org/10825137/diff/1/sync/internal_api/public/engin...
File sync/internal_api/public/engine/control_model_worker.h (right):

http://codereview.chromium.org/10825137/diff/1/sync/internal_api/public/engin...
sync/internal_api/public/engine/control_model_worker.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
This is mostly copy/pasted from passive_model_worker.h.  I could create a common
base class that allows the Passive and Control workers to share code, but
there's not a lot of code to share.

[rlarocque, 2012-08-11 01:31:52]

Here's a big update.

It introduces the syncer::EncryptableTypes() concept.  Only types returned by
this function are eligible for encryption.  Some types (PER_DEVICE_METADATA and
PER_USER_METADATA) can never be encrypted.

It also introduces the PER_USER_METADATA type, though we have no flags to put in
it at the moment.

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1319:
syncer::ModelTypeSet(syncer::ControlTypes()),
ControlTypes was intended to be a group of types that do not undergo regular
update application and conflict resolution.  All currently defined control types
are expected to be downloaded early, but there's no guarantee that will always
be the case.  Maybe I should define a separate group, the EarlyDownloadTypes(),
which are independent from the control types.

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_registrar.cc:74:
workers_[syncer::GROUP_CONTROL] = new syncer::ControlModelWorker(sync_loop_);
There's a DCHECK in the PassiveModelWorker that prevents us from reusing it
here.

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_unittest.cc (left):

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:320:
TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
This test is not very useful anymore.  I think the tests I added when fixing the
dir corruption issue should cover this scenario.

It uses a set of flags in StartSyncServiceAndSetInitialSyncEnded that don't make
sense now that it's impossible to return from backend init without having
downloaded the nigori node and metadata.  I don't know how we could fix it.

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/test_pr...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10825137/diff/5001/chrome/browser/sync/test_pr...
chrome/browser/sync/test_profile_sync_service.cc:127: if
(!directory->initial_sync_ended_for_type(PER_DEVICE_METADATA)) {
These if statements are probably not required...

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:30: }
TODO: Should manually apply PER_DEVICE_METADATA and PER_USER_METADATA here. 
There is no need for conflict resolution or UpdateApplicators, we just need to
overwrite the LOCAL_ fields with SERVER_ fields.

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:328: 
TODO: Test application of metadata updates.

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_updates_c...
File sync/engine/apply_updates_command.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_updates_c...
sync/engine/apply_updates_command.cc:40: // The INTERNAL types are special. 
They have their own update routines,
s/INTERNAL/CONTROL

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_updates_c...
File sync/engine/apply_updates_command_unittest.cc (left):

http://codereview.chromium.org/10825137/diff/5001/sync/engine/apply_updates_c...
sync/engine/apply_updates_command_unittest.cc:516:
TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) {
These have been moved to ApplyControlDataUpdatesTest

http://codereview.chromium.org/10825137/diff/5001/sync/engine/conflict_resolv...
File sync/engine/conflict_resolver.h (left):

http://codereview.chromium.org/10825137/diff/5001/sync/engine/conflict_resolv...
sync/engine/conflict_resolver.h:68: void
IgnoreLocalChanges(syncable::MutableEntry* entry);
Moved to syncer_util.h

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
File sync/internal_api/public/base/model_type.h (right):

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type.h:87: PER_DEVICE_METADATA,
Maybe this should be named PER_CLIENT_METADATA?  Would that be more clear?

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
sync/internal_api/public/base/model_type.h:89: PER_USER_METADATA,
Rename to PER_ACCOUNT_METADATA?

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
File sync/internal_api/public/base_node.h (right):

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
sync/internal_api/public/base_node.h:175: // Can only be called if
GetModelType() == PER_USERMETADATA.
FIXME: typo.

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/ba...
sync/internal_api/public/base_node.h:176: const sync_pb::PerUserSpecifics&
GetPerUserSpecifics() const;
I'm not sure if this function is necessary.  According to the original design
doc, there's no need for a PerUserSpecifics, since it would always contain
exactly one sub-message and the caller would have to know which one it is.  It
might be easier to just expect the caller to call GetEntitySpecifics and place
all possible sub-messages directly under sync_pb::EntitySpecifics.

That might have undesirable effects on other parts of the code that expect
types' protobufs to be formatted a certain way, ie. proto_value_conversions.cc. 
Maybe it would be better to use slightly uglier protobuf definitions in order to
not disturb the existing code too much.

Server devs: any thoughts?

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/en...
File sync/internal_api/public/engine/control_model_worker.h (right):

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/en...
sync/internal_api/public/engine/control_model_worker.h:21: class SYNC_EXPORT
ControlModelWorker : public ModelSafeWorker {
This code is copy/pasted from PassiveModelWorker.  I feel sort of bad about
that.  

On the other hand, it would require much more code to define a 'BaseModelWorker'
and have both the PassiveModelWorker and ControlModelWorker inherit from it. 
This would enable us to share roughly two lines of actual code between both
classes.  It just doesn't seem worth the effort.

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/en...
File sync/internal_api/public/engine/model_safe_worker.h (right):

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:42: GROUP_CONTROL,       //
Models that require special handling on the sync
Are we still OK with this name?  Does anyone have a better idea?

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:515: void
SyncManagerImpl::InitialProcessMetadataContinuation(
This would be a great place to put the code that checks and sets account-global
flags.

http://codereview.chromium.org/10825137/diff/5001/sync/protocol/per_device_sp...
File sync/protocol/per_device_specifics.proto (right):

http://codereview.chromium.org/10825137/diff/5001/sync/protocol/per_device_sp...
sync/protocol/per_device_specifics.proto:35: message PerDeviceSpecifics {
Feel free to hold a bike-shed discussion about the contents of this file.

(Seriously.  This will be your last chance to recommend different names or
alternate protobuf structures.)

http://codereview.chromium.org/10825137/diff/5001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/syncable/model_type.cc...
sync/syncable/model_type.cc:260: ModelTypeSet
unencryptable_types(PER_DEVICE_METADATA, PER_USER_METADATA);
Why do these types skip encryption?

I believe these types are likely to contain information which we can't do
without.  They might contain flags which affect how we encrypt things, so it
would be bad if a 'passphrase required' state prevented us from reading them.

The justification for downloading them early is similar.

http://codereview.chromium.org/10825137/diff/5001/sync/syncable/nigori_util.cc
File sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/syncable/nigori_util.c...
sync/syncable/nigori_util.cc:88: if (!syncer::EncryptableTypes().Has(type))
This is a new concept that I'm introducing.  We already have PASSWORDS and
NIGORI which have special encryption treatment.  I'm adding EncryptableTypes()
to make it easier to define types which we expect will never support encryption.

http://codereview.chromium.org/10825137/diff/5001/sync/test/engine/syncer_com...
File sync/test/engine/syncer_command_test.cc (right):

http://codereview.chromium.org/10825137/diff/5001/sync/test/engine/syncer_com...
sync/test/engine/syncer_command_test.cc:24: workers()->push_back( // FIXME
comment
FIXME: remove this FIXME.