NOT FOR COMMIT: Add DeviceInfo type and ChangeProcessor

Sync: Add DeviceInfo type and ChangeProcessor

This change adds a new type to sync. It is the first new type to make
use of the "control type" infrastructure added in r154522.

Each client owns exactly one node of type DEVICE_INFO. The node is
identified in part by a UNIQUE_CLIENT_TAG that includes the client's
cache_guid. Each client will create and update its node with its
current version, platform, and other metadata that could help a user
identify the device.

This data type replaces the device information fields that used to be
contained in the nigori node.

Also included in this commit:
- A first attempt at a ChangeProcessor for this type.  The
  SyncedDeviceTracker should eventually be extended to allow other
  components to receive notifications when new device info is available.
- Tests for the SyncedDeviceTracker.
- These tests introduce and make use of MockTransactionObserver.  It
  implements the TransactionObserver interface and can maintain a count
  of emitted transaction notifications.
- A change to the signature of
  ProfileSyncServiceTestHelper::CreateRoot() and all of its callers to
  remove the need for a TestIdFactory instance


BUG=122825

[rlarocque, 2012-09-04 21:01:39]

Here's the first attempt at adding the DEVICE_INFO type.

I'm probably breaking a few sync style rules, especially around
device_info_util.h.  I've uploaded both syncable and internal_api versions of
that file.

The syncable version re-implements some of the logic to create and update unique
client nodes.  There are pros and cons to this approach.  

Also notable is that the UNIQUE_CLIENT_TAG for the DEVICE_INFO node is not
hashed in the syncable version.  I think that's OK.  We have some control the
length of the UNIQUE_CLIENT_TAG value, so we know it won't grow to an
unmanageable size.

Tim: Please review device_info_util.cc/h.
Nicolas: Please review everything else.
Raz: We're going to need server support for the new data type before this patch
is committed.  We also have plans to add two more data types after this one. 
Let me know if it would be helpful to see those types in advance so you could
add all three in one CL.

[Nicolas Zea, 2012-09-05 01:07:02]

I might be forgetting the discussion, but why is this implemented at the
syncable level and not at a higher level that's accessible within chrome? It
seems odd to use the syncable API instead of the normal sync_api (e.g.
MutableEntrys vs WriteNodes), as well as having it live within syncable instead
of glue (with the other datatypes)

[rlarocque, 2012-09-08 01:20:44]

Here's the first draft of a ChangeProcessor implementation for DEVICE_INFO.  It
turned out much better than I expected, though I did run into a few issues
because there's never been a ChangeProcessor like this one before.

I've commented on a few of the uglier parts.  Let me know if you see any way
these could be cleaned up.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/DE...
chrome/browser/sync/glue/DEPS:8: "+sync/test",
To get access to the MockTransactionObserver.  I'm open to alternative
suggestions.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/ch...
File chrome/browser/sync/glue/change_processor.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/ch...
chrome/browser/sync/glue/change_processor.cc:21: DCHECK(!share_handle_);
This is unfortunate.  The only implementation of DataTypeErrorHandler is a
DataTypeController.  DeviceInfo doesn't have a DataTypeController.

This makes me think that the abstract ChangeProcessor class is overreaching a
bit.  There is room for another ChangeProcessorInterface class that would not
require a DataTypeErrorHandler.

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

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1186: if
(sync_manager_->GetUserShare()) { // NULL in some tests.
FakeSyncManager::GetUserShare() returns NULL.

It's used in sync_backend_host_unittest.cc.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:89: syncer::GetSessionName(
I'd prefer it if we could just block the sync thread here.  We can't do anything
while we're waiting.  It would also help avoid some races (ie. what happens if
we are told to shut down while this call is in progress?)

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:62: base::RunLoop
run_loop;
In case you were wondering, this is the reason why the MockTransactionObserver
is exported all the way to this file.  We need to be able to run the message
loop before we check its value.

[Nicolas Zea, 2012-09-13 00:45:55]

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:44: std::string
DeviceInfoLookupString(const std::string& cache_guid) {
nit: I find it a bit clearer when local helper function like these are at the
top of the file. Up to you though.

Also, a comment about what this method is for

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:68: bool
SyncedDeviceTracker::GetLocalDeviceInfo(
Does this need to be public? If not, GetLocalDeviceInfoImpl might be more
appropriate, and have it be private.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:71: const std::string
lookup_string =
make const ref?

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:75: if (lookup_result !=
syncer::BaseNode::INIT_OK)
nit: may as well just do the InitByClient.. call inside of the if statement

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:78: const
sync_pb::DeviceInfoSpecifics& specifics = node.GetDeviceInfoSpecifics();
I'm a bit hesitant about how this will discard unknown fields, but I guess since
this is the local device info it doesn't really matter...

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:89: syncer::GetSessionName(
On 2012/09/08 01:20:44, rlarocque wrote:
> I'd prefer it if we could just block the sync thread here.  We can't do
anything
> while we're waiting.  It would also help avoid some races (ie. what happens if
> we are told to shut down while this call is in progress?)

Since you're using weak pointers, this class can be destroyed during shutdown
without problem right?

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:128: if (lookup_result ==
syncer::BaseNode::INIT_OK) {
nit: no need to have lookup_result in it's own var right?

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:134:
DCHECK(type_root_lookup_result == syncer::BaseNode::INIT_OK);
dcheck_eq here and below.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:7: 
include string

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:13: // TODO: Move this into its
own class.
nit: TODO(rlarocque)

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:21: class SyncedDeviceTracker :
public base::SupportsWeakPtr<SyncedDeviceTracker>,
Does this need to inherit from SupportsWeakPtr? Unless someone is actually
externally calling AsWeakPtr(), I think it's preferable to use a weak ptr
factory

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:23: friend class
SyncedDeviceTrackerTest;
nit: move this to the private section below

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:39: bool
GetLocalDeviceInfo(syncer::BaseTransaction &trans,
const syncer::BaseTransaction& trans

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:93: EXPECT_EQ(1,
GetObservedChangesCounter());
this seems kindda "white box", does the number of transactions really matter? Or
are you trying to ensure we don't trigger unnecessary sync cycles (which isn't
necessarily tied to number of transactions, since they might be read
transactions or write transactions that don't change the specifics)

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

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:321:
TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
why was this removed?

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:111: 
remove newline

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl_unittest.cc:1352: #if 0 // FIXME -> Move
these
did you mean to leave this?

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:21: optional string cache_guid = 1;
comment about this field

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:24: // On a Chromebook this is
"Chromebook", on Linux the distribution name, on
I don't think it's necessarily worth enumerating all types of names, as it's
likely this will get out of date (you left out Android and ChromeBox for
instance). Mentioning that it's platform-specific is enough.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:29: optional string platform = 3;
Note that we have an enum for the supported platforms in
session_specifics.proto. It might be worth doing the same here, so you don't
have to do any parsing of the platform string. WDYT?

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:75: // This has been moved to the
PerDeviceSpecific message.
PerDeviceSpecifics -> DeviceInfoSpecifics?

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.cc:396: SET_FIELD(device_info,
DeviceInfoSpecificsToValue);
abc

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.h:32: class DeviceInfoSpecifics;
abc order

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/sync_proto.gyp
File sync/protocol/sync_proto.gyp (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/sync_proto.gy...
sync/protocol/sync_proto.gyp:27: 'device_info_specifics.proto',
abc

http://codereview.chromium.org/10911073/diff/6002/sync/test/engine/test_direc...
File sync/test/engine/test_directory_setter_upper.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/test/engine/test_direc...
sync/test/engine/test_directory_setter_upper.h:72:
syncable::MockTransactionObserver* mock_transaction_observer_;
scoped_ptr? Or if you pass ownership, comment here who you pass it to.

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
File sync/test/mock_transaction_observer.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
sync/test/mock_transaction_observer.h:16: class MockTransactionObserver :
comment about purpose of the class

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
sync/test/mock_transaction_observer.h:33:
base::WeakPtrFactory<MockTransactionObserver> weak_ptr_factory_;
weak ptr factory and SupportsWeakPtr?

[Raz Mathias, 2012-09-13 16:07:01]

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:26: optional string name = 2;
Is this field for UI display or programatic use?  If for programmatic use,
please use the UserAgent.  We are getting lost/buried in client enums and
strings server-side :-)  Will it be stamped on every upgrade?  If so, then we
should talk about maybe letting the server stamp this field for you.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:30: 
I presume nzea means the SessionHeader.DeviceType.  Yes, please do use that.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:32: optional string chrome_version =
4;
We already have functions to extract this info server-side from the UserAgent
today.  Will the server be keeping this up-to-date for you?  (we should chat
about the different version nums we have and which we use for what purposes...)

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:76: // repeated DeviceInformation
device_information = 28;
Please mark this field // @deprecated

[rlarocque, 2012-09-14 01:03:07]

I've updated the patch to address some review comments.

I was hoping to avoid properly setting up the DeviceInfo class in this commit,
but I'm afraid that the GetLocalDeviceInfo() function leaves me no choice.  That
function needs a home, and SyncedDeviceInfoTracker is not the right place for
it.  I'll fix that and upload another patch soon.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:44: std::string
DeviceInfoLookupString(const std::string& cache_guid) {
On 2012/09/13 00:45:55, nzea wrote:
> nit: I find it a bit clearer when local helper function like these are at the
> top of the file. Up to you though.
> 
> Also, a comment about what this method is for

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:68: bool
SyncedDeviceTracker::GetLocalDeviceInfo(
On 2012/09/13 00:45:55, nzea wrote:
> Does this need to be public? If not, GetLocalDeviceInfoImpl might be more
> appropriate, and have it be private.

It's not currently used anywhere, but I expect this will be part of the
functionality we want to expose to clients of this class.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:71: const std::string
lookup_string =
On 2012/09/13 00:45:55, nzea wrote:
> make const ref?

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:75: if (lookup_result !=
syncer::BaseNode::INIT_OK)
On 2012/09/13 00:45:55, nzea wrote:
> nit: may as well just do the InitByClient.. call inside of the if statement

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:78: const
sync_pb::DeviceInfoSpecifics& specifics = node.GetDeviceInfoSpecifics();
On 2012/09/13 00:45:55, nzea wrote:
> I'm a bit hesitant about how this will discard unknown fields, but I guess
since
> this is the local device info it doesn't really matter...

The client has no business processing fields that it didn't know about at
compile time.  I think dropping those fields is the right thing to do.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:89: syncer::GetSessionName(
On 2012/09/13 00:45:55, nzea wrote:
> On 2012/09/08 01:20:44, rlarocque wrote:
> > I'd prefer it if we could just block the sync thread here.  We can't do
> anything
> > while we're waiting.  It would also help avoid some races (ie. what happens
if
> > we are told to shut down while this call is in progress?)
> 
> Since you're using weak pointers, this class can be destroyed during shutdown
> without problem right?

In theory, yes.  I think this code is thread safe.  I'd prefer to not have to
think about it at all, though.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:128: if (lookup_result ==
syncer::BaseNode::INIT_OK) {
On 2012/09/13 00:45:55, nzea wrote:
> nit: no need to have lookup_result in it's own var right?

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.cc:134:
DCHECK(type_root_lookup_result == syncer::BaseNode::INIT_OK);
On 2012/09/13 00:45:55, nzea wrote:
> dcheck_eq here and below.

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:7: 
On 2012/09/13 00:45:55, nzea wrote:
> include string

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:13: // TODO: Move this into its
own class.
On 2012/09/13 00:45:55, nzea wrote:
> nit: TODO(rlarocque)

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:21: class SyncedDeviceTracker :
public base::SupportsWeakPtr<SyncedDeviceTracker>,
On 2012/09/13 00:45:55, nzea wrote:
> Does this need to inherit from SupportsWeakPtr? Unless someone is actually
> externally calling AsWeakPtr(), I think it's preferable to use a weak ptr
> factory

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:23: friend class
SyncedDeviceTrackerTest;
On 2012/09/13 00:45:55, nzea wrote:
> nit: move this to the private section below

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:23: friend class
SyncedDeviceTrackerTest;
On 2012/09/13 00:45:55, nzea wrote:
> nit: move this to the private section below

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker.h:39: bool
GetLocalDeviceInfo(syncer::BaseTransaction &trans,
On 2012/09/13 00:45:55, nzea wrote:
> const syncer::BaseTransaction& trans

Done.

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:93: EXPECT_EQ(1,
GetObservedChangesCounter());
On 2012/09/13 00:45:55, nzea wrote:
> this seems kindda "white box", does the number of transactions really matter?
Or
> are you trying to ensure we don't trigger unnecessary sync cycles (which isn't
> necessarily tied to number of transactions, since they might be read
> transactions or write transactions that don't change the specifics)

Actually, I believe the observer will only see write transactions that make
meaningful, sync-nudging changes.  

See syncable::WriteTransaction::UnlockAndNotify(), which conditionally invokes
NotifyTransactionComplete().

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

http://codereview.chromium.org/10911073/diff/6002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:321:
TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
On 2012/09/13 00:45:55, nzea wrote:
> why was this removed?

It looks like I was a bit too hasty with this test.  I didn't put much effort
into saving it, since I think a lot of the stuff its assertions are better
tested elsewhere.  

The problem was the call to StartSyncServiceAndSetInitialSyncEnded().  It passes
in a set_initial_sync_ended value of false.  That leads us to a scenario that is
no longer possible in our code.  FailToOpenDatabase also does this, but I think
it can get away with it because it fails to open the database (STORAGE_INVALID).

I tried toggling the parameter, and that seems to have fixed it.  I'm not sure
why this test wanted the syncer to fail to set initial_sync_ended here.  I'm
going to assume that it was not an essential part of the test.

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:111: 
On 2012/09/13 00:45:55, nzea wrote:
> remove newline

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/6002/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl_unittest.cc:1352: #if 0 // FIXME -> Move
these
On 2012/09/13 00:45:55, nzea wrote:
> did you mean to leave this?

Nope, that was a mistake.  The tests have already been moved to
synced_device_tracker_unittest.cc.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:20: message DeviceInfoSpecifics {
Feel free to question the contents of this proto.  I just copied the old version
from the Nigori node without putting much thought into it.  We may want to add
or remove members.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:21: optional string cache_guid = 1;
On 2012/09/13 00:45:55, nzea wrote:
> comment about this field

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:24: // On a Chromebook this is
"Chromebook", on Linux the distribution name, on
On 2012/09/13 00:45:55, nzea wrote:
> I don't think it's necessarily worth enumerating all types of names, as it's
> likely this will get out of date (you left out Android and ChromeBox for
> instance). Mentioning that it's platform-specific is enough.

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:26: optional string name = 2;
On 2012/09/13 16:07:01, Raz Mathias wrote:
> Is this field for UI display or programatic use?  If for programmatic use,
> please use the UserAgent.  We are getting lost/buried in client enums and
> strings server-side :-)  Will it be stamped on every upgrade?  If so, then we
> should talk about maybe letting the server stamp this field for you.

The value of this field comes from GetSessionNameSynchronously in
get_session_name.cc.  I think it's intended to be displayed in the "Other
Devices" list on the new tab page.  Some day, we might even be able to modify
it: crbug.com/124964

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:29: optional string platform = 3;
On 2012/09/13 00:45:55, nzea wrote:
> Note that we have an enum for the supported platforms in
> session_specifics.proto. It might be worth doing the same here, so you don't
> have to do any parsing of the platform string. WDYT?

That sounds reasonable.  Will do.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/device_info_s...
sync/protocol/device_info_specifics.proto:32: optional string chrome_version =
4;
On 2012/09/13 16:07:01, Raz Mathias wrote:
> We already have functions to extract this info server-side from the UserAgent
> today.  Will the server be keeping this up-to-date for you?  (we should chat
> about the different version nums we have and which we use for what
purposes...)

The client will set this.  It uses the same function that generates the version
on the "About Chromium" page.  Maybe we should just record the user agent
instead?

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:75: // This has been moved to the
PerDeviceSpecific message.
On 2012/09/13 00:45:55, nzea wrote:
> PerDeviceSpecifics -> DeviceInfoSpecifics?

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:76: // repeated DeviceInformation
device_information = 28;
On 2012/09/13 16:07:01, Raz Mathias wrote:
> Please mark this field // @deprecated

Is that a java annotation?  What will that do?

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.cc:396: SET_FIELD(device_info,
DeviceInfoSpecificsToValue);
On 2012/09/13 00:45:55, nzea wrote:
> abc

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/proto_value_c...
sync/protocol/proto_value_conversions.h:32: class DeviceInfoSpecifics;
On 2012/09/13 00:45:55, nzea wrote:
> abc order

It was alphabetical order before I did a global
s/PerDeviceSpecifics/DeviceInfoSpecifics/.

Fixed.

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/sync_proto.gyp
File sync/protocol/sync_proto.gyp (right):

http://codereview.chromium.org/10911073/diff/6002/sync/protocol/sync_proto.gy...
sync/protocol/sync_proto.gyp:27: 'device_info_specifics.proto',
On 2012/09/13 00:45:55, nzea wrote:
> abc

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/test/engine/test_direc...
File sync/test/engine/test_directory_setter_upper.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/test/engine/test_direc...
sync/test/engine/test_directory_setter_upper.h:72:
syncable::MockTransactionObserver* mock_transaction_observer_;
On 2012/09/13 00:45:55, nzea wrote:
> scoped_ptr? Or if you pass ownership, comment here who you pass it to.

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
File sync/test/mock_transaction_observer.h (right):

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
sync/test/mock_transaction_observer.h:16: class MockTransactionObserver :
On 2012/09/13 00:45:55, nzea wrote:
> comment about purpose of the class

Done.

http://codereview.chromium.org/10911073/diff/6002/sync/test/mock_transaction_...
sync/test/mock_transaction_observer.h:33:
base::WeakPtrFactory<MockTransactionObserver> weak_ptr_factory_;
On 2012/09/13 00:45:55, nzea wrote:
> weak ptr factory and SupportsWeakPtr?

Whoops.  Fixed.

[Nicolas Zea, 2012-09-14 19:15:08]

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:8: "+sync/test",
Could the mock transaction observer (at least the header) be moved to
sync/internal_api/public/test?

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:21: # TODO(zea): SessionModelAssociator shouldn't
need this.
I guess we can remove this TODO now? or perhaps change it to reflect that we
need to expose the cache guid at the glue layer

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:257: SyncedDeviceTracker
synced_device_tracker_;
There should probably be tests/changes in sync_backend_host_unittest.cc that
ensure this is initialized properly.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1193:
synced_device_tracker_.InitLocalDeviceInfo(
indent

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:373: DOWNLOADING_METADATA,   // The
SyncManager is initialized, but
how about calling these downloading and applying "CONTROL_TYPES", to make it
more explicit what is being done?

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:43: // TODO(ncarter): How to
determine this?
might be worth special casing type == TYPED_URLS here and type == PASSWORDS
below now

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:54: bool
SyncedDeviceTracker::GetLocalDeviceInfo(DeviceInfo* out) {
I wonder if you need to check running() here and return false (and in other
public methods that open transactions)? The caller may/may not be aware of a
shutdown that is in progress, for example, so it seems safer.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:88: DCHECK(running());
does this need to be a dcheck? It seems valid for Stop() to be called before the
callback is invoked, in which case maybe we should invoke the callback early and
return?

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.h:8: #include <string>
newline after

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.h:33: virtual void
ApplyChangesFromSyncModel(
move up a line? (to make clear these are also change processor methods)

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:89:
EXPECT_EQ(write_device_info.chrome_version, read_device_info.chrome_version);
Maybe add a DeviceInfoEquals method, at least for this test? (eventually once
you pull it into a class you can have the class itself provide a comparison
method)

http://codereview.chromium.org/10911073/diff/13002/sync/internal_api/public/t...
File sync/internal_api/public/test/test_user_share.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/internal_api/public/t...
sync/internal_api/public/test/test_user_share.h:68:
syncable::MockTransactionObserver* transaction_observer();
comment about method

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.h:24: class DeviceInfoSpecifics;
down one line

http://codereview.chromium.org/10911073/diff/13002/sync/test/engine/test_dire...
File sync/test/engine/test_directory_setter_upper.cc (left):

http://codereview.chromium.org/10911073/diff/13002/sync/test/engine/test_dire...
sync/test/engine/test_directory_setter_upper.cc:32:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
was this not being used before?

http://codereview.chromium.org/10911073/diff/13002/sync/test/mock_transaction...
File sync/test/mock_transaction_observer.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/test/mock_transaction...
sync/test/mock_transaction_observer.h:17: // It gathers information that is
useful for writing test assertions.
mention that it only counts transactions with syncable changes? I wonder if the
name of the class should reflect that too, otherwise you might think it counts
all transactions.

[rlarocque, 2012-09-15 01:36:37]

The latest upload addresses a few more comments.  

I'm still struggling to find a good solution for some testing and threading
issues.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:8: "+sync/test",
On 2012/09/14 19:15:08, nzea wrote:
> Could the mock transaction observer (at least the header) be moved to
> sync/internal_api/public/test? 

I agree that it should be moved somewhere.  I tentatively put it in sync/test
because the other test transaction observer, NullTransactionObserver, lives
there.  

I don't think that sync/internal_api/public_test is the right place to put it,
though.  The TransactionObserver interface is in syncable, not internal_api. 
I'm also worried about the implications the move would have on linking/symbols.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:21: # TODO(zea): SessionModelAssociator shouldn't
need this.
On 2012/09/14 19:15:08, nzea wrote:
> I guess we can remove this TODO now? or perhaps change it to reflect that we
> need to expose the cache guid at the glue layer

Unfortunately, it's still necessary for several reasons, not just the cache
guid:

ERROR in
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/session_model_associator.cc
  Illegal include: "sync/syncable/directory.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
  Illegal include: "sync/syncable/write_transaction.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.

ERROR in
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/typed_url_change_processor.cc
  Illegal include: "sync/syncable/entry.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.

ERROR in
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/synced_device_tracker.cc
  Illegal include: "sync/syncable/directory.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.

ERROR in
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/generic_change_processor.cc
  Illegal include: "sync/syncable/entry.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.

ERROR in
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/bookmark_change_processor.cc
  Illegal include: "sync/syncable/entry.h"
    Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.

I could update the comment, though.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:257: SyncedDeviceTracker
synced_device_tracker_;
On 2012/09/14 19:15:08, nzea wrote:
> There should probably be tests/changes in sync_backend_host_unittest.cc that
> ensure this is initialized properly.

Unfortunately, we can't test it there.  Those unit tests use a FakeSyncManager,
and the FakeSyncManager doesn't have a directory.  In fact,
sync_backend_host_unittest.cc is the reason why there's an
if(sync_manager_->GetUserShare()) in DoInitialProcessMetadata().

You're right that the SBH is a blind spot in the tests I've written so far. 
I'll have to think about how best to fix it.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1193:
synced_device_tracker_.InitLocalDeviceInfo(
On 2012/09/14 19:15:08, nzea wrote:
> indent

Done.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:373: DOWNLOADING_METADATA,   // The
SyncManager is initialized, but
On 2012/09/14 19:15:08, nzea wrote:
> how about calling these downloading and applying "CONTROL_TYPES", to make it
> more explicit what is being done?

Good idea.  Done.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:43: // TODO(ncarter): How to
determine this?
On 2012/09/14 19:15:08, nzea wrote:
> might be worth special casing type == TYPED_URLS here and type == PASSWORDS
> below now

I'm not sure I follow.  Do you mean:

case syncer::GROUP_HISTORY
  return type == TYPED_URLS;
case ...

Although it makes sense to check that the type matches the group, that still
doesn't tell us which thread we're on.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:54: bool
SyncedDeviceTracker::GetLocalDeviceInfo(DeviceInfo* out) {
On 2012/09/14 19:15:08, nzea wrote:
> I wonder if you need to check running() here and return false (and in other
> public methods that open transactions)? The caller may/may not be aware of a
> shutdown that is in progress, for example, so it seems safer.

Good point.  I'll check running() as a precaution.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:88: DCHECK(running());
On 2012/09/14 19:15:08, nzea wrote:
> does this need to be a dcheck? It seems valid for Stop() to be called before
the
> callback is invoked, in which case maybe we should invoke the callback early
and
> return?

I'm struggling to understand the scenario where this does happen, but I agree
that such a scenario could exist.

When it does happen, the best thing to do might be to drop the callback.  If
we're shutting down the syncer, then the last thing we want is a task getting
posted to HandleInitializationCompletedOnFrontendLoop().

It's almost as if the weak ptr should be invalidated when Stop() is called...

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.h:8: #include <string>
On 2012/09/14 19:15:08, nzea wrote:
> newline after

Done.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.h:33: virtual void
ApplyChangesFromSyncModel(
On 2012/09/14 19:15:08, nzea wrote:
> move up a line? (to make clear these are also change processor methods)

Done.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:89:
EXPECT_EQ(write_device_info.chrome_version, read_device_info.chrome_version);
On 2012/09/14 19:15:08, nzea wrote:
> Maybe add a DeviceInfoEquals method, at least for this test? (eventually once
> you pull it into a class you can have the class itself provide a comparison
> method)

Done.

http://codereview.chromium.org/10911073/diff/13002/sync/internal_api/public/t...
File sync/internal_api/public/test/test_user_share.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/internal_api/public/t...
sync/internal_api/public/test/test_user_share.h:68:
syncable::MockTransactionObserver* transaction_observer();
On 2012/09/14 19:15:08, nzea wrote:
> comment about method

Done.

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.h:24: class DeviceInfoSpecifics;
On 2012/09/14 19:15:08, nzea wrote:
> down one line

Not according to ASCII sort.

http://codereview.chromium.org/10911073/diff/13002/sync/test/engine/test_dire...
File sync/test/engine/test_directory_setter_upper.cc (left):

http://codereview.chromium.org/10911073/diff/13002/sync/test/engine/test_dire...
sync/test/engine/test_directory_setter_upper.cc:32:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2012/09/14 19:15:08, nzea wrote:
> was this not being used before?

Nope.  I must have forgotten to remove the temp_dir_ member when I switched this
class to use InMemoryDirectoryBackingStore.

I should not have included an unrelated change like this.  I'll move it to a
separate CL.

http://codereview.chromium.org/10911073/diff/13002/sync/test/mock_transaction...
File sync/test/mock_transaction_observer.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/test/mock_transaction...
sync/test/mock_transaction_observer.h:17: // It gathers information that is
useful for writing test assertions.
On 2012/09/14 19:15:08, nzea wrote:
> mention that it only counts transactions with syncable changes? I wonder if
the
> name of the class should reflect that too, otherwise you might think it counts
> all transactions.

I added a comment to the transactions_observed() method.

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:85: if (!running()) {
I think this is the wrong way to handle it, but I'm not sure.

http://codereview.chromium.org/10911073/diff/13010/sync/protocol/device_info_...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10911073/diff/13010/sync/protocol/device_info_...
sync/protocol/device_info_specifics.proto:33: optional string chrome_version =
4;
Raz: You wanted to replace this with the UserAgent, right?  I presume you meant
the sync user agent, and not the one used for regular web requests?

[Nicolas Zea, 2012-09-17 17:54:00]

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:8: "+sync/test",
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > Could the mock transaction observer (at least the header) be moved to
> > sync/internal_api/public/test? 
> 
> I agree that it should be moved somewhere.  I tentatively put it in sync/test
> because the other test transaction observer, NullTransactionObserver, lives
> there.  
> 
> I don't think that sync/internal_api/public_test is the right place to put it,
> though.  The TransactionObserver interface is in syncable, not internal_api. 
> I'm also worried about the implications the move would have on
linking/symbols.

One solution is to just have the header file not expose the observer
implementation, and instead have the only transactions_observed() method. You'd
then have a a transaction observer object (which you forward declare in the
header), whose implementation is in the cc file. Similar to SBH::Core.

I think in that scenario it would also be appropriate to rename the main class
to SyncNudgeObserver or something, since the fact that you're using a
transaction observer is an implementation detail.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:21: # TODO(zea): SessionModelAssociator shouldn't
need this.
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > I guess we can remove this TODO now? or perhaps change it to reflect that we
> > need to expose the cache guid at the glue layer
> 
> Unfortunately, it's still necessary for several reasons, not just the cache
> guid:
> 
> ERROR in
>
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/session_model_associator.cc
>   Illegal include: "sync/syncable/directory.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
>   Illegal include: "sync/syncable/write_transaction.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
> 
> ERROR in
>
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/typed_url_change_processor.cc
>   Illegal include: "sync/syncable/entry.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
> 
> ERROR in
>
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/synced_device_tracker.cc
>   Illegal include: "sync/syncable/directory.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
> 
> ERROR in
>
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/generic_change_processor.cc
>   Illegal include: "sync/syncable/entry.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
> 
> ERROR in
>
/usr/local/google/storage/chromium2/src/chrome/browser/sync/glue/bookmark_change_processor.cc
>   Illegal include: "sync/syncable/entry.h"
>     Because of "-sync/syncable" from chrome/browser/sync/glue's include_rules.
> 
> I could update the comment, though.

Right, I meant the comment itself is no longer quite appropriate, not that we
can remove the dependency itself.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:257: SyncedDeviceTracker
synced_device_tracker_;
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > There should probably be tests/changes in sync_backend_host_unittest.cc that
> > ensure this is initialized properly.
> 
> Unfortunately, we can't test it there.  Those unit tests use a
FakeSyncManager,
> and the FakeSyncManager doesn't have a directory.  In fact,
> sync_backend_host_unittest.cc is the reason why there's an
> if(sync_manager_->GetUserShare()) in DoInitialProcessMetadata().
> 
> You're right that the SBH is a blind spot in the tests I've written so far. 
> I'll have to think about how best to fix it.

How about passing in the sync guid when you initialize the synced device tracker
instead of querying from within, which would let you remove the dependency on
directory?

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_registrar.cc:43: // TODO(ncarter): How to
determine this?
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > might be worth special casing type == TYPED_URLS here and type == PASSWORDS
> > below now
> 
> I'm not sure I follow.  Do you mean:
> 
> case syncer::GROUP_HISTORY
>   return type == TYPED_URLS;
> case ...
> 
> Although it makes sense to check that the type matches the group, that still
> doesn't tell us which thread we're on.

Right, that's what I mean. And yes, it's not a full solution, but now that we
have type information here seems like a decent thing a to check.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:88: DCHECK(running());
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > does this need to be a dcheck? It seems valid for Stop() to be called before
> the
> > callback is invoked, in which case maybe we should invoke the callback early
> and
> > return?
> 
> I'm struggling to understand the scenario where this does happen, but I agree
> that such a scenario could exist.
> 
> When it does happen, the best thing to do might be to drop the callback.  If
> we're shutting down the syncer, then the last thing we want is a task getting
> posted to HandleInitializationCompletedOnFrontendLoop().
> 
> It's almost as if the weak ptr should be invalidated when Stop() is called...

That's an option. Weak ptr factory has an InvalidateWeakPtrs method.

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.h:24: class DeviceInfoSpecifics;
On 2012/09/15 01:36:37, rlarocque wrote:
> On 2012/09/14 19:15:08, nzea wrote:
> > down one line
> 
> Not according to ASCII sort.

*Specifics vs *rmation? I would have thought r sorts before S, is it treating
capitals differently?

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:30: // Compares this objects fields with
another.
"object's", "another's"

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:36: 
remove newline

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:37: 
should we expose guid as well? (can be a TODO)

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:60: const
syncer::BaseTransaction& trans, DeviceInfo* out) {
Since this is a public method, I wonder if we should check running here too.
Although the caller must already have opened a transaction, so presumably
they're a datatype and can therefore block shutdown...

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:88: callback.Run();
return after.

[rlarocque, 2012-09-19 21:37:58]

The latest patch adds:
- UserShare::cache_guid(), which allowed me to remove most of the dependencies
on syncable from glue.
- Better testing of the SBH's interaction with the SyncedDeviceTracker.
- Adds more functionality to the FakeSyncManager and moves the CreateRoot()
helper function in order to enable this testing.
- Replaces the version_info field with sync_user_agent.

I think this addresses all of the review comments so far.  Let me know if you'd
like to see other changes.

Unfortunately, this patch has grown out of control.

$ git diff master --shortstat
 62 files changed, 1003 insertions(+), 361 deletions(-)

I don't want to introduce 700 lines of code in a single commit.  I'll have to
split this up into smaller chunks.

You're welcome to add more comments to this review, but I won't be committing it
in this state.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:257: SyncedDeviceTracker
synced_device_tracker_;
On 2012/09/17 17:54:00, nzea wrote:
> On 2012/09/15 01:36:37, rlarocque wrote:
> > On 2012/09/14 19:15:08, nzea wrote:
> > > There should probably be tests/changes in sync_backend_host_unittest.cc
that
> > > ensure this is initialized properly.
> > 
> > Unfortunately, we can't test it there.  Those unit tests use a
> FakeSyncManager,
> > and the FakeSyncManager doesn't have a directory.  In fact,
> > sync_backend_host_unittest.cc is the reason why there's an
> > if(sync_manager_->GetUserShare()) in DoInitialProcessMetadata().
> > 
> > You're right that the SBH is a blind spot in the tests I've written so far. 
> > I'll have to think about how best to fix it.
> 
> How about passing in the sync guid when you initialize the synced device
tracker
> instead of querying from within, which would let you remove the dependency on
> directory?

Fixed by adding an accessor to the cache_guid to UserShare.

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

http://codereview.chromium.org/10911073/diff/13002/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/synced_device_tracker.cc:88: DCHECK(running());
On 2012/09/17 17:54:00, nzea wrote:
> On 2012/09/15 01:36:37, rlarocque wrote:
> > On 2012/09/14 19:15:08, nzea wrote:
> > > does this need to be a dcheck? It seems valid for Stop() to be called
before
> > the
> > > callback is invoked, in which case maybe we should invoke the callback
early
> > and
> > > return?
> > 
> > I'm struggling to understand the scenario where this does happen, but I
agree
> > that such a scenario could exist.
> > 
> > When it does happen, the best thing to do might be to drop the callback.  If
> > we're shutting down the syncer, then the last thing we want is a task
getting
> > posted to HandleInitializationCompletedOnFrontendLoop().
> > 
> > It's almost as if the weak ptr should be invalidated when Stop() is
called...
> 
> That's an option. Weak ptr factory has an InvalidateWeakPtrs method.

Yep.  I'm not sure whether or not it's considered to be good style, but it does
give us the desired semantics.  A stopped ChangeProcessor is just as useless as
if it had been deleted.

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
File sync/protocol/proto_value_conversions.h (right):

http://codereview.chromium.org/10911073/diff/13002/sync/protocol/proto_value_...
sync/protocol/proto_value_conversions.h:24: class DeviceInfoSpecifics;
On 2012/09/17 17:54:00, nzea wrote:
> On 2012/09/15 01:36:37, rlarocque wrote:
> > On 2012/09/14 19:15:08, nzea wrote:
> > > down one line
> > 
> > Not according to ASCII sort.
> 
> *Specifics vs *rmation? I would have thought r sorts before S, is it treating
> capitals differently? 

'S' is 0x53, r is '0x72'.  'S' < 'r'.

The sort behaviour of most programs is dependent on LC_COLLATE.  I try to sort
with LC_COLLATE=C, which is why I get this unintuitive ordering.

[rlarocque, 2012-09-19 23:03:08]

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:30: // Compares this objects fields with
another.
On 2012/09/17 17:54:00, nzea wrote:
> "object's", "another's"

Done. (will be fixed in next upload)

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:36: 
On 2012/09/17 17:54:00, nzea wrote:
> remove newline

Done.

http://codereview.chromium.org/10911073/diff/13010/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:37: 
On 2012/09/17 17:54:00, nzea wrote:
> should we expose guid as well? (can be a TODO)

I left it out intentionally.  It's an opaque value, at best useful for comparing
it against the guid of other DeviceInfos.  We can make that operation available
without exposing the string.  

Even the comparison wouldn't mean much, given the possibility of multiple user
data dirs, etc.  We should think through those issues before we make this
information available.

[rlarocque, 2012-11-15 01:06:36]

I'm closing this review.  

The first half of this has been committed, and the second half is now up for
review at http://codereview.chromium.org/11360259/.