Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - part 3.

Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - part 3.

This change integrates DeviceIfoSyncService and DeviceInfoDataTypeController introduced in an early change
with the rest of the Sync code and removes the old implementation of DEVICE_INFO sync type based on SyncedDeviceTracker.
As a part of this change DEVICE_INFO stops being a specially implemented control type and becomes a priority type
with implementation that follows pattern of the majority of other sync types.
This should help to do some further refactoring including decoupling of DeviceInfo sync code from the browser and moving it to sync_driver component.
This change also adds support for syncing of local device backup timestamp to DeviceInfoSyncService. This functionality was missing when DeviceInfoSyncService was initially introduced.

BUG=395349
Committed: https://crrev.com/3724e60a79c733002e5f3391809f733ae843c8bb
Cr-Commit-Position: refs/heads/master@{#295992}

[stanisc, 2014-09-12 16:21:15]

stanisc@chromium.org changed reviewers:
+ haitaol@chromium.org, xiyuan@chromium.org, yoz@chromium.org

[stanisc, 2014-09-12 16:21:15]

haitaol@chromium.org: Please review changes in the Sync code under
chrome/browsers/sync/, components/sync_driver/, and sync/ 

xiyuan@chromium.org: Please review a small number of changes under
chrome/browser/ui/

yoz@chromium.org: Please review changes under chrome/browser/extensions/

[xiyuan, 2014-09-12 16:36:23]

chrome/browser/ui/ LGTM

[stanisc, 2014-09-12 17:38:57]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org
- haitaol@chromium.org

[stanisc, 2014-09-12 17:41:40]

Changed Sync code reviewer to zea@chromium.org.

[Yoyo Zhou, 2014-09-15 18:56:32]

extensions LGTM

[Nicolas Zea, 2014-09-15 22:40:10]

zea@chromium.org changed reviewers:
+ pavely@chromium.org

[Nicolas Zea, 2014-09-15 22:40:11]

Passing to Pavel for the sync review

[stanisc, 2014-09-16 00:17:40]

A few small issues I found myself.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info.h:82: DeviceInfo* Clone() const;
Would you rather have a copy constructor?

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:47: // Called to update
local device backup time in the directory.
I need to change this comment. It doesn't come from the directory anymore.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:50: base::Time
GetLocalDeviceBackupTime() const;
I need to change this comment. It doesn't come from the directory anymore.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:53: // Create SyncData from
local DeviceInfo (and backup timestamp).
I should replace "backup timestamp" with |local_device_backup_time_|.

[pavely, 2014-09-17 23:04:38]

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc
(right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc:85:
ProfileSyncServiceFactory::GetForProfile(profile)->GetDeviceInfoTracker();
What is the condition when GetDeviceInfoTracker() may return NULL?
Will it be a problem in GetAllSignedInDevices?

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h
(right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h:13:
#include "chrome/browser/sync/glue/device_info_tracker.h"
nit: Please forward declare class instead of including header.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h:20:
class ProfileSyncService;
ProfileSyncService is not needed anymore

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info.h:82: DeviceInfo* Clone() const;
Discussed in person.
You can keep Clone, it depends on typical usage scenario.
Return scoped_ptr<DeviceInfo> instead of raw pointer though.
On 2014/09/16 00:17:40, stanisc wrote:
> Would you rather have a copy constructor?

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service.cc (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:257: bool
DeviceInfoSyncService::UpdateBackupTime(syncer::SyncData& sync_data) {
nit: Pass sync_data as pointer
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg...)

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:86: int64
local_device_backup_time_;
Consider storing local_device_backup_time_ as base::Time, it will be more
descriptive. You can use base::Time() as special value and is_null for check.
This will get rid of family of getters/setters above.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.h:313: // TODO: do we still need
this?
No, we don't.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/history_ui.cc:238:
sync_service->GetDeviceInfoTracker()->GetDeviceInfo(client_id);
Same issue: Under what conditions GetDeviceInfoTracher() returns NULL?

[pavely, 2014-09-17 23:05:32]

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service.cc (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:111: // Add SyncData for
the local deviceif it is new or different than
deviceif => device if

[stanisc, 2014-09-18 22:46:54]

Addressed all CR feedback.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc
(right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.cc:85:
ProfileSyncServiceFactory::GetForProfile(profile)->GetDeviceInfoTracker();
On 2014/09/17 23:04:38, pavely wrote:
> What is the condition when GetDeviceInfoTracker() may return NULL?
> Will it be a problem in GetAllSignedInDevices?

Good catch! It could only be null if either Sync backend or local device info
haven't initialized.
The old code handled this condition. I added a check to return an empty
ScopedVector<DeviceInfo> in that case. Also I've added a unit test to verify
this code handles NULL DeviceInfoTracker as expected.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h
(right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h:13:
#include "chrome/browser/sync/glue/device_info_tracker.h"
On 2014/09/17 23:04:38, pavely wrote:
> nit: Please forward declare class instead of including header.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/signed_in_devices/signed_in_devices_api.h:20:
class ProfileSyncService;
On 2014/09/17 23:04:38, pavely wrote:
> ProfileSyncService is not needed anymore

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info.h:82: DeviceInfo* Clone() const;
On 2014/09/17 23:04:38, pavely wrote:
> Discussed in person.
> You can keep Clone, it depends on typical usage scenario.
> Return scoped_ptr<DeviceInfo> instead of raw pointer though.
> On 2014/09/16 00:17:40, stanisc wrote:
> > Would you rather have a copy constructor?
> 

I decided to get rid of Clone. My next change would make it unnecessary anyway.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:47: // Called to update
local device backup time in the directory.
On 2014/09/16 00:17:40, stanisc wrote:
> I need to change this comment. It doesn't come from the directory anymore.

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:50: base::Time
GetLocalDeviceBackupTime() const;
On 2014/09/16 00:17:40, stanisc wrote:
> I need to change this comment. It doesn't come from the directory anymore.

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:53: // Create SyncData from
local DeviceInfo (and backup timestamp).
On 2014/09/16 00:17:40, stanisc wrote:
> I should replace "backup timestamp" with |local_device_backup_time_|.

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:86: int64
local_device_backup_time_;
On 2014/09/17 23:04:38, pavely wrote:
> Consider storing local_device_backup_time_ as base::Time, it will be more
> descriptive. You can use base::Time() as special value and is_null for check.
> This will get rid of family of getters/setters above.

That is exactly what I had when I first implemented GetLocalDeviceBackupTime and
UpdateBackupTime here. However I had to change it because it has to support  3
states - unset, null, and some valid value.
Initially I used is_null() to check whether it was unset. But it turned out
there is at lest one test that would set null backup time and expect it back.
Converting to an integer value allowed me to distinguish unset and null. Also it
reduced the number of conversions between the proto time and the base time which
made the code look clearer.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.h:313: // TODO: do we still need
this?
On 2014/09/17 23:04:38, pavely wrote:
> No, we don't.

Done.

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/history_ui.cc (right):

https://codereview.chromium.org/566623003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/history_ui.cc:238:
sync_service->GetDeviceInfoTracker()->GetDeviceInfo(client_id);
On 2014/09/17 23:04:38, pavely wrote:
> Same issue: Under what conditions GetDeviceInfoTracher() returns NULL?
This one should be fine. GetDeviceInfoTracker() should be non-NULL when backend
is initialized. We cannot call this method without having a device client_id
which comes from DeviceInfoTracker in the first place.
I'll add a comment.

[pavely, 2014-09-19 17:59:01]

lgtm

[stanisc, 2014-09-22 16:18:15]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-09-22 16:19:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566623003/60001

[commit-bot: I haz the power, 2014-09-22 16:47:02]

Committed patchset #4 (id:60001) as 56e22acbdd9077248336492223565755839cabaa

[commit-bot: I haz the power, 2014-09-22 16:47:39]

Patchset 4 (id:??) landed as
https://crrev.com/3724e60a79c733002e5f3391809f733ae843c8bb
Cr-Commit-Position: refs/heads/master@{#295992}