[Sync] Initial USS service implementation for DeviceInfo.

[Sync] Initial USS service implementation for DeviceInfo.

BUG=543401
Committed: https://crrev.com/e8ce32d8517a8a5255d5ab89747b2c11daabc4b0
Cr-Commit-Position: refs/heads/master@{#356406}

[skym, 2015-10-21 00:40:12]

Description was changed from

==========
[Sync] Initial USS service implementation for DeviceInfo.

BUG=543401
==========

to

==========
[Sync] Initial USS service implementation for DeviceInfo.

BUG=543401
==========

[skym, 2015-10-21 00:40:12]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org, pavely@chromium.org, stanisc@chromium.org

[skym, 2015-10-21 00:43:00]

I just wanted to send this out in case anyone has any comments. It is pretty
unusable as is, since its service interface is empty, the structs aren't
defined, and there is no durable storage. I switched it to use scoped_ptr
specifics, which ultimately I don't think is going to work, wrt our discussions
today.

At first I thought I could get away from having the local device info provider
reference, and kind of praying that it has been initialized when we need it.
Instead I thought we could subscribe to its events. But now I'm realizing that
we actually have multiple async loading dependencies (level db storage), so I'm
not really sure how initialization is going to work.

[stanisc, 2015-10-22 20:16:53]

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
File components/sync_driver/device_info_service.h (right):

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:29: namespace sync_driver {
Should this be in sync_driver_v2 namespace to avoid confusion with the
SyncableService implementation?

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:51: // TODO(skym): Are all these
processor assessors really needed?
This should be one of the next things to design and implement. Please file a
bug, mark this TODO with the bug number and assign to me.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:52:
syncer_v2::ModelTypeChangeProcessor* get_change_processor();
Perhaps this should be returning weak pointer to the processor.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:107: const LocalDeviceInfoProvider*
const local_device_info_provider_;
Why do you want to remove the local provider?

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:116:
scoped_ptr<sync_pb::DeviceInfoSpecifics>>
It would make sense to store ProtoValuePtr<sync_pb::EntitySpecifics> so that we
could transfer the data in and out without copying. It isn't the big deal for
DeviceInfo specifically but it would good to try the pattern so that we could
use it elsewhere.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
File components/sync_driver/local_device_info_provider_mock.cc (right):

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/local_device_info_provider_mock.cc:49: void
LocalDeviceInfoProviderMock::Initialize(
I am not sure why did you need to add this. You could still construct it in
advance with the needed data and call SetInitialized. That is what tests for
DeviceInfoSyncService do.

[skym, 2015-10-23 20:29:49]

Updated for Stan's comments. I have not changed to use client tag hash as the
key yet, I think it makes the most sense to do that in a subsequent CL.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
File components/sync_driver/device_info_service.h (right):

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:29: namespace sync_driver {
On 2015/10/22 20:16:53, stanisc wrote:
> Should this be in sync_driver_v2 namespace to avoid confusion with the
> SyncableService implementation?

Done.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:51: // TODO(skym): Are all these
processor assessors really needed?
On 2015/10/22 20:16:53, stanisc wrote:
> This should be one of the next things to design and implement. Please file a
> bug, mark this TODO with the bug number and assign to me.

Done.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:52:
syncer_v2::ModelTypeChangeProcessor* get_change_processor();
On 2015/10/22 20:16:53, stanisc wrote:
> Perhaps this should be returning weak pointer to the processor.

My understanding was this get...() was only going to be called by the unittests.
Let's deal with this detail in the new bug.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:107: const LocalDeviceInfoProvider*
const local_device_info_provider_;
On 2015/10/22 20:16:53, stanisc wrote:
> Why do you want to remove the local provider? 

I'll remove this comment, it's a little old, I think we will end up needing to
keep the provider. But let me explain the thought behind this.

We use to provider for two things
1) Checking what our local cache guid id is.
2) Diffing against the stored local device info entity during
MergeDataAndStartSyncing and potentially used to create the local device info.

I saw #2 as really initialization, and once it's done, we don't need the
provider at all. I thought I'd create a curried function as I registered for the
on init callback from the provider, and then I'd only use the provider reference
in my callback function.

But, because we have to wait for 3 different things to initialize before we use
the provider (see other comment), keeping it around as a member variable is
probably the cleanest approach.

Done.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/device_info_service.h:116:
scoped_ptr<sync_pb::DeviceInfoSpecifics>>
On 2015/10/22 20:16:53, stanisc wrote:
> It would make sense to store ProtoValuePtr<sync_pb::EntitySpecifics> so that
we
> could transfer the data in and out without copying. It isn't the big deal for
> DeviceInfo specifically but it would good to try the pattern so that we could
> use it elsewhere.

Team post-scrummed this issue, we decided that the typical service is going to
convert into Model objects and hold a cache of those. It's actually slightly
abnormal this service is holding onto specifics. If we consider this to be model
detail, then we suddenly become okay with any type that is the most convenient
for the service. So, I'm going to keep this is scoped_ptrs to
DeviceInfoSpecifics, at least for now.

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
File components/sync_driver/local_device_info_provider_mock.cc (right):

https://codereview.chromium.org/1408123003/diff/80001/components/sync_driver/...
components/sync_driver/local_device_info_provider_mock.cc:49: void
LocalDeviceInfoProviderMock::Initialize(
On 2015/10/22 20:16:53, stanisc wrote:
> I am not sure why did you need to add this. You could still construct it in
> advance with the needed data and call SetInitialized. That is what tests for
> DeviceInfoSyncService do.

Because then I cannot test all the cases. The way I see it, DeviceInfo's service
has 3 dependencies:

1) Provider
2) DataStore (right now this is directory, in the future it'll be a levelDB)
3) Processor

None of these are ready upon the service's construction, but once all three are
available, the service should compare the stored local device info to the
provider's device info, see if there is any discrepancy, and commit if needed.

The way we test to see if the provider has been initialized or not is by calling
GetLocalDeviceInfo(), and if it returns NULL, then the provider is not ready.
Without my change here, I'm forced to always start the provider as initialized,
regardless of if the initialized callback is going to get invoked or not. I'll
never be able to test the case where DataStore and Processor initialize before
the Provider.

[stanisc, 2015-10-27 20:30:00]

lgtm

[skym, 2015-10-27 20:30:18]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2015-10-27 20:30:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408123003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408123003/100001

[commit-bot: I haz the power, 2015-10-27 21:51:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-27 21:52:49]

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