Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - Part 2

Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - Part 2

This change introduces implementations of UIDataTypeController and
SyncableService for DEVICE_INFO type and corresponding unit tests.
This types are not hooked to Profile Sync Service yet.
The next change (part 3) will move all DeviceInfo
related classes to sync_driver component and the final
change (part 4) will hook everything up and remove the
legacy SyncedDeviceTracker.

BUG=395349
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287297

[stanisc, 2014-07-30 00:14:57]

zea@chromium.org or pavely@chromium.org, please review these changes.

[Nicolas Zea, 2014-07-30 17:44:51]

FYI you only need one owner to review a change. I'll leave it to Pavel, unless
there's a specific question or area for me.

[stanisc, 2014-07-30 18:57:50]

This is all new code - implementation of Syncable Service and DataTypeController
for DEVICE_INFO type. The new code isn't hooked up at the ProfileSyncService
yet.

Richard, please review this only if you have time today. Otherwise I'll get a CR
from Pavel tomorrow.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller.cc:31: // Start the
data type as soon as the local device info gets available
Will add missing period at the end of the sentence.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller.h:29: // available
Will add missing period at the end of the sentence.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:142: // Iterate over all
chanages and merge entries
Will add missing period at the end of the sentence.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:41: // DeviceInfoSyncService
API
Will add missing period at the end of the sentence.

[pavely, 2014-07-31 22:52:41]

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller_unittest.cc:52:
local_device_.reset();
Resetting controller_ and local_device_ is not necessary, dtor will take care of
them.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:81: // synced one?
Today all changes to DeviceInfo originate on local device, if device infos 
don't match then local device info got updated. You just need to overwrite sync
one with local one (which you already do).

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:110: return result;
Do you need to notify callback list here?

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:232:
specifics.signin_scoped_device_id());
There are few formatting suggestions for this change. Please run it through "git
cl format".

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:45: typedef
base::CallbackList<void(void)>::Subscription Subscription;
nit: typedef should be before ctor
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service_unittest.cc:107: return
wrapper_.PassAs<SyncChangeProcessor>();
You can do:
return scoped_ptr<SyncChangeProcessor>(new
SyncChangeProcessorWrapperForTest(...));

[stanisc, 2014-08-01 18:54:52]

Addressed CR feedback and ran the code through "git cl format".
Also moved a few methods from DeviceInfoSyncService to a separate
DeviceInfoTracker interface which DeviceInfoSyncService now implements. I'd need
that later when integrating DeviceInfoSyncService with ProfileSyncService and
other components.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller.cc:31: // Start the
data type as soon as the local device info gets available
On 2014/07/30 18:57:50, stanisc wrote:
> Will add missing period at the end of the sentence.

Done.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller.h:29: // available
On 2014/07/30 18:57:50, stanisc wrote:
> Will add missing period at the end of the sentence.

Done.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_data_type_controller_unittest.cc:52:
local_device_.reset();
On 2014/07/31 22:52:40, pavely wrote:
> Resetting controller_ and local_device_ is not necessary, dtor will take care
of
> them.

Done.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:81: // synced one?
On 2014/07/31 22:52:40, pavely wrote:
> Today all changes to DeviceInfo originate on local device, if device infos 
> don't match then local device info got updated. You just need to overwrite
sync
> one with local one (which you already do). 
OK. I'll remove this comment and keep the current implementation.

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:110: return result;
On 2014/07/31 22:52:40, pavely wrote:
> Do you need to notify callback list here?
The original implementation doesn't notify at this point, but  
probably it won't hurt.

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:142: // Iterate over all
chanages and merge entries
On 2014/07/30 18:57:50, stanisc wrote:
> Will add missing period at the end of the sentence.

Done.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:41: // DeviceInfoSyncService
API
On 2014/07/30 18:57:50, stanisc wrote:
> Will add missing period at the end of the sentence.

Done.

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:45: typedef
base::CallbackList<void(void)>::Subscription Subscription;
On 2014/07/31 22:52:40, pavely wrote:
> nit: typedef should be before ctor
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

Done.

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

https://codereview.chromium.org/430583003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service_unittest.cc:107: return
wrapper_.PassAs<SyncChangeProcessor>();
On 2014/07/31 22:52:40, pavely wrote:
> You can do:
> return scoped_ptr<SyncChangeProcessor>(new
> SyncChangeProcessorWrapperForTest(...));

Done.

[rlarocque, 2014-08-01 20:40:37]

I did a quick read over this CL.  Sorry if any of these comments are duplicates.

The change looks pretty good.  I left a few remarks, but I don't think there's
much in here that needs to change.

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:90: // Update SyncData from
device info if it new or different than
nit: ... if it *is* new ...

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:202: return
callback_list_.Add(callback);
It seems there's no way to unregister a callback.  That's not necessarily wrong,
but it does look a bit suspicious.

The usual pattern is to have an ObserverList and require that the list be empty
by the time this class is destroyed.  This helps to ensure the destruction order
is sane.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:244: void
DeviceInfoSyncService::DeleteSyncData(const std::string& client_id) {
I'm pretty sure this never happens, though it's a good thing that you decided to
support it anyway.  We'll be glad you did if we decide to change this behavior
in the future.

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:47: // Create SyncData from
local DeviceInfo.
nit: I prefer to have a whitespace line before a comment line.  I don't think
the style guide specifies one way or the other, so feel free to leave it as is.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:67: SyncDataMap all_data_;
I think Pavel already pointed this out, but I'll mention it again in case he
didn't.

This is a little bit strange.  Most data types were built first, then had sync
features grafted on afterwards.  We didn't want to rewrite them when adding sync
functionality, so they tend to keep their data in their own format then convert
it to a sync-friendly format at the last minute, just before calling
ProcessSyncChanges.  They also tend to convert from SyncData into their own data
structures as soon as the data is received.

This type was synced before it was a proper data type, so you had the
opportunity to do things differently.  Yours might be one of the few types that
keeps an internal data structure filled with SyncDatas.  I don't see any
problems with this, but I just figured you should be aware that this is slightly
unusual.

As long as we don't make any significant changes SyncData (and I see no reason
why we would), then this won't cause any problems, and it makes it easier to
manage book-keeping for sync.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:70:
base::CallbackList<void(void)> callback_list_;
Why not use base/observer_list.h?

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service_unittest.cc (right):

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service_unittest.cc:56: return
change_list_[index].change_type();
operator[] doesn't do bounds checking.  Maybe there should be a CHECK here to
make sure we crash if this function is misused?

[rlarocque, 2014-08-01 20:56:29]

I almost forgot to publish some comments that apply to the whole CL.

- If you haven't used it yet, try 'git cl format'.  Just make sure all your
outstanding changes are checked in first, since it can be hard to undo if it
goes wrong.

- The git tools work best with commit messages that start with a 50 character
summary on the first line, followed by a blank line, then a series of paragraphs
with line length no longer than 72 characters.  That's just a suggestion,
though.  Many CLs in Chromium do not follow these guidelines.

[stanisc, 2014-08-01 21:37:42]

See my comments below. I'll go ahead and switch to ObserverList. Let me also
know if it makes sense to switch to caching native DeviceInfo data.

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:90: // Update SyncData from
device info if it new or different than
On 2014/08/01 20:40:36, rlarocque wrote:
> nit: ... if it *is* new ...

Done.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:202: return
callback_list_.Add(callback);
On 2014/08/01 20:40:36, rlarocque wrote:
> It seems there's no way to unregister a callback.  That's not necessarily
wrong,
> but it does look a bit suspicious.
> 
> The usual pattern is to have an ObserverList and require that the list be
empty
> by the time this class is destroyed.  This helps to ensure the destruction
order
> is sane.

There is a way to unregister by destroying the returned subscription value. One
of the
tests demostrates / verifies that.
The original implementation used ThreadSafeObserverList. Since the new mechanism
doesn't
need to be thread safe I decided to switch to CallbackList that I was already
familiar with.
But if this pattern seems to be less obvious I'll switch back to ObserverList
(the regular
version, not the thread safe one).

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:67: SyncDataMap all_data_;
On 2014/08/01 20:40:36, rlarocque wrote:
> I think Pavel already pointed this out, but I'll mention it again in case he
> didn't.
> 
> This is a little bit strange.  Most data types were built first, then had sync
> features grafted on afterwards.  We didn't want to rewrite them when adding
sync
> functionality, so they tend to keep their data in their own format then
convert
> it to a sync-friendly format at the last minute, just before calling
> ProcessSyncChanges.  They also tend to convert from SyncData into their own
data
> structures as soon as the data is received.
> 
> This type was synced before it was a proper data type, so you had the
> opportunity to do things differently.  Yours might be one of the few types
that
> keeps an internal data structure filled with SyncDatas.  I don't see any
> problems with this, but I just figured you should be aware that this is
slightly
> unusual.
> 
> As long as we don't make any significant changes SyncData (and I see no reason
> why we would), then this won't cause any problems, and it makes it easier to
> manage book-keeping for sync.

Yeah, I've thought about using native DeviceInfo when implementing this code.
The current implementation of SyncedDeviceTracker actually does the same - it
stores SyncData in the directory and creates native DeviceInfo on demand. I
decided to stick with SyncData mainly for the following two reasons:
1) GetAllSyncData method should probably return the original SyncData - remote
SyncData for sync'ed devices and local SyncData for the local device. I guess it
is possible to recreate SyncData from DeviceInfo based on whether device GUID
matches the local device or not. But I thought it would be simpler to just cache
SyncData that comes from the sync and don't have to worry about recreating it.
2) Copy semantics is disabled on DeviceInfo which makes it unsuitable to be used
with STL collections. I don't know if there is a good reason for that. The type
seems to be simple and safe enough to be copied around. So perhaps the right
approach could to enable copy constructor on DeviceInfo and copy DeviceInfo
values to the cache.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:70:
base::CallbackList<void(void)> callback_list_;
On 2014/08/01 20:40:36, rlarocque wrote:
> Why not use base/observer_list.h?

I thought CallbackList was simpler and more elegant, mainly because it doesn't
require an observer code to implement an Observer interface. I'll go ahead and
move to ObserverList.

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info_sync_service_unittest.cc (right):

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service_unittest.cc:56: return
change_list_[index].change_type();
On 2014/08/01 20:40:36, rlarocque wrote:
> operator[] doesn't do bounds checking.  Maybe there should be a CHECK here to
> make sure we crash if this function is misused?

Done.

[stanisc, 2014-08-01 22:55:50]

Addressed most of CR feedback from Richard.A
Switched notification mechanism to ObserverList.
For now I decided to keep caching Sync specific SyncData
objects.  Let me know if you'd prefer native DeviceInfo data
to be cached instead.  I'll have an opportunity to change
that when integrating this change with ProfileSyncService in the next round of
changes.

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:202: return
callback_list_.Add(callback);
On 2014/08/01 21:37:42, stanisc wrote:
> On 2014/08/01 20:40:36, rlarocque wrote:
> > It seems there's no way to unregister a callback.  That's not necessarily
> wrong,
> > but it does look a bit suspicious.
> > 
> > The usual pattern is to have an ObserverList and require that the list be
> empty
> > by the time this class is destroyed.  This helps to ensure the destruction
> order
> > is sane.
> 
> There is a way to unregister by destroying the returned subscription value.
One
> of the
> tests demostrates / verifies that.
> The original implementation used ThreadSafeObserverList. Since the new
mechanism
> doesn't
> need to be thread safe I decided to switch to CallbackList that I was already
> familiar with.
> But if this pattern seems to be less obvious I'll switch back to ObserverList
> (the regular
> version, not the thread safe one).
OK, switched to ObserverList and added RemoveObserver method.

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:70:
base::CallbackList<void(void)> callback_list_;
On 2014/08/01 20:40:36, rlarocque wrote:
> Why not use base/observer_list.h?

Done.

[rlarocque, 2014-08-01 23:03:23]

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

https://codereview.chromium.org/430583003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.h:67: SyncDataMap all_data_;
On 2014/08/01 21:37:42, stanisc wrote:
> On 2014/08/01 20:40:36, rlarocque wrote:
> > I think Pavel already pointed this out, but I'll mention it again in case he
> > didn't.
> > 
> > This is a little bit strange.  Most data types were built first, then had
sync
> > features grafted on afterwards.  We didn't want to rewrite them when adding
> sync
> > functionality, so they tend to keep their data in their own format then
> convert
> > it to a sync-friendly format at the last minute, just before calling
> > ProcessSyncChanges.  They also tend to convert from SyncData into their own
> data
> > structures as soon as the data is received.
> > 
> > This type was synced before it was a proper data type, so you had the
> > opportunity to do things differently.  Yours might be one of the few types
> that
> > keeps an internal data structure filled with SyncDatas.  I don't see any
> > problems with this, but I just figured you should be aware that this is
> slightly
> > unusual.
> > 
> > As long as we don't make any significant changes SyncData (and I see no
reason
> > why we would), then this won't cause any problems, and it makes it easier to
> > manage book-keeping for sync.
> 
> Yeah, I've thought about using native DeviceInfo when implementing this code.
> The current implementation of SyncedDeviceTracker actually does the same - it
> stores SyncData in the directory and creates native DeviceInfo on demand. I
> decided to stick with SyncData mainly for the following two reasons:
> 1) GetAllSyncData method should probably return the original SyncData - remote
> SyncData for sync'ed devices and local SyncData for the local device. I guess
it
> is possible to recreate SyncData from DeviceInfo based on whether device GUID
> matches the local device or not. But I thought it would be simpler to just
cache
> SyncData that comes from the sync and don't have to worry about recreating it.
> 2) Copy semantics is disabled on DeviceInfo which makes it unsuitable to be
used
> with STL collections. I don't know if there is a good reason for that. The
type
> seems to be simple and safe enough to be copied around. So perhaps the right
> approach could to enable copy constructor on DeviceInfo and copy DeviceInfo
> values to the cache.
> 

SGTM for now.  I see this as an experiment.  Since we might end up doing
something similar in some of the non-blocking sync data types, it would be nice
to see how it works out here.

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

https://codereview.chromium.org/430583003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:29:
DeviceInfoSyncService::~DeviceInfoSyncService() {
It's usually a good idea to DCHECK(observers_.empty()) at this point.  We
wouldn't want some observer to try to unregister itself after this object has
been destructed.

Alternatively, you could just set ObserverList's second template argument,
check_empty, to true.  That should have the same effect.

[stanisc, 2014-08-02 00:53:44]

Added a check that verifies that ObserverList is empty
when DeviceInfoSyncService is destroyed.

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

https://codereview.chromium.org/430583003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info_sync_service.cc:29:
DeviceInfoSyncService::~DeviceInfoSyncService() {
On 2014/08/01 23:03:23, rlarocque wrote:
> It's usually a good idea to DCHECK(observers_.empty()) at this point.  We
> wouldn't want some observer to try to unregister itself after this object has
> been destructed.
> 
> Alternatively, you could just set ObserverList's second template argument,
> check_empty, to true.  That should have the same effect.

Done.

[rlarocque, 2014-08-02 00:58:50]

LGTM, unless Pavel has any other concerns.

[pavely, 2014-08-02 01:10:40]

Oh, sorry...
LGTM

[stanisc, 2014-08-04 05:58:26]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-08-04 05:58:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stanisc@chromium.org/430583003/100001

[commit-bot: I haz the power, 2014-08-04 07:00:03]

Change committed as 287297