Sync: Add DeviceInfo's ChangeProcessor

Sync: Add DeviceInfo's ChangeProcessor

This is the long-awaited change to enable DeviceInfo tracking.

Long ago (r161496) we committed code to support the DeviceInfo type, but
we had to leave this code disabled until the server was ready to support
the new type.  That support has now been added.

This change includes a very special kind of ChangeProcessor named
SyncedDeviceTracker.  It is the only ChangeProcessor that is owned by
the sync thread.  The SyncBackendHost creates and initializes it during
backend intialization and deletes it before the UserShare it references
is destroyed.  As part of its initialization, the SyncedDeviceTracker
will update the DeviceInfo entry for the current device.

The SyncedDeviceTracker could support sending notifications to any
interested listeners when new or update DeviceInfo information arrives,
but this functionality has not been implemented yet.

Also included are:
- Tests for the SyncedDeviceTracker
- A MockTransactionObserver to support these tests
- A cache_guid() accessor on the SyncManager, used to initialize the
  SyncedDeviceTracker
- Remove support for default constructing a DeviceInfo object

BUG=122825
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168972

[rlarocque, 2012-11-15 01:08:53]

Please review.

This has been sitting in my git tree for months, so it could be a bit stale.  I
tried to be careful when rebasing, but it's still possible I made a mistake
somewhere.

[Nicolas Zea, 2012-11-16 19:42:27]

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/devi...
File chrome/browser/sync/glue/device_info.h (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/devi...
chrome/browser/sync/glue/device_info.h:57: const sync_pb::SyncEnums::DeviceType
device_type_;
disallow copy and assign? Or if you do allow them, add a comment mentioning
default copy and assign welcome

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:34: #include
"chrome/browser/sync/glue/device_info.h"
abc order

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:175: void
FinishInitialProcessControlTypes();
Comment about why these two are separated?

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1181:
registrar_->ActivateDataType(syncer::DEVICE_INFO,
Why not activate the datatype after initialization succeeds? Or is it that we
expect it to never fail?

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1194: void
SyncBackendHost::Core::FinishInitialProcessControlTypes() {
I think the naming convention prefers DoFinishInitialProcessControlTypes?
Although, given that this is only calling
HandleInitializationCompletedOnFrontendLoop, DoHandleInitialization... might be
more appropriate.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:7: #include
"base/compiler_specific.h"
is this used?

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:21: // Return the DeviceInfo
UNIQUE_CLIENT_TAG value for given sync cache_guid.
DeviceInfo's
"for the given"

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:37:
weak_factory_.InvalidateWeakPtrs();
This isn't necessary (destroying the factory invalidates weak pointers)

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:64: !=
syncer::BaseNode::INIT_OK) {
!= on previous line, then indent four spaces
(chromium style guide: http://www.chromium.org/developers/coding-style)

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:100: ==
syncer::BaseNode::INIT_OK) {
newline after boolean operator here too

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.h:14: #include
"sync/protocol/sync.pb.h"
is this necessary?

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:147:
EXPECT_EQ(device_info_v2.sync_user_agent(),
device_info_v2.equals(result_device_info)?

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/base_transa...
File sync/internal_api/base_transaction.cc (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/base_transa...
sync/internal_api/base_transaction.cc:16: user_share_(share) {
colon on this line if full initializer doesn't fit above:
"  :  user_share(share) {"

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base_transaction.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base_transaction.h:35: if (!user_share_) {
remove curly braces?

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/test...
File sync/internal_api/public/test/test_user_share.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/test...
sync/internal_api/public/test/test_user_share.h:76: 
remove extra newline

https://codereview.chromium.org/11360259/diff/1/sync/test/mock_transaction_ob...
File sync/test/mock_transaction_observer.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/test/mock_transaction_ob...
sync/test/mock_transaction_observer.h:18: class MockTransactionObserver :
Given that this isn't actually a mock, I'd prefer calling this something else.
Perhaps TestTransactionObserver?

[rlarocque, 2012-11-17 00:07:20]

Patch updated.  PTAL.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/devi...
File chrome/browser/sync/glue/device_info.h (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/devi...
chrome/browser/sync/glue/device_info.h:57: const sync_pb::SyncEnums::DeviceType
device_type_;
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> disallow copy and assign? Or if you do allow them, add a comment mentioning
> default copy and assign welcome

Disallowed.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:34: #include
"chrome/browser/sync/glue/device_info.h"
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> abc order

That file was included twice, actually.  Must have been a bad merge.  Fixed.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:175: void
FinishInitialProcessControlTypes();
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> Comment about why these two are separated?

Done.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1181:
registrar_->ActivateDataType(syncer::DEVICE_INFO,
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> Why not activate the datatype after initialization succeeds? Or is it that we
> expect it to never fail?

It made more sense to me to do it after the activation was complete.  I see
InitLocalDeviceInfo() as an operation that can only be performed on an active
data type, in the same way that only an active BookmarkChangeProcessor can
respond to bookmark change notifications.

Also, I think it relies on some the ChangeProcessor::share_handle() function,
which I believe won't work until the type is activated.

Maybe InitLocalDeviceInfo() is a bad name?

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1194: void
SyncBackendHost::Core::FinishInitialProcessControlTypes() {
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> I think the naming convention prefers DoFinishInitialProcessControlTypes?
> Although, given that this is only calling
> HandleInitializationCompletedOnFrontendLoop, DoHandleInitialization... might
be
> more appropriate.

I think you're right.  I renamed it.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:7: #include
"base/compiler_specific.h"
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> is this used?

Nope.  Fixed.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:21: // Return the DeviceInfo
UNIQUE_CLIENT_TAG value for given sync cache_guid.
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> DeviceInfo's
> "for the given"

Done.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:37:
weak_factory_.InvalidateWeakPtrs();
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> This isn't necessary (destroying the factory invalidates weak pointers)

Done.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:64: !=
syncer::BaseNode::INIT_OK) {
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> != on previous line, then indent four spaces
> (chromium style guide: http://www.chromium.org/developers/coding-style)

Done.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.cc:100: ==
syncer::BaseNode::INIT_OK) {
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> newline after boolean operator here too

Done.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker.h (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker.h:14: #include
"sync/protocol/sync.pb.h"
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> is this necessary?

Not anymore.  Good catch.

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/synced_device_tracker_unittest.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/synced_device_tracker_unittest.cc:147:
EXPECT_EQ(device_info_v2.sync_user_agent(),
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> device_info_v2.equals(result_device_info)?

Yes, it could make that assertion.  Done.

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/base_transa...
File sync/internal_api/base_transaction.cc (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/base_transa...
sync/internal_api/base_transaction.cc:16: user_share_(share) {
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> colon on this line if full initializer doesn't fit above:
> "  :  user_share(share) {"

Done.

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/base...
File sync/internal_api/public/base_transaction.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/base...
sync/internal_api/public/base_transaction.h:35: if (!user_share_) {
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> remove curly braces?

I'd prefer not to.  I believe the style guide does not specify either way, as
longs as the style is consistent within the file.

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/test...
File sync/internal_api/public/test/test_user_share.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/internal_api/public/test...
sync/internal_api/public/test/test_user_share.h:76: 
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> remove extra newline

Done.

https://codereview.chromium.org/11360259/diff/1/sync/test/mock_transaction_ob...
File sync/test/mock_transaction_observer.h (right):

https://codereview.chromium.org/11360259/diff/1/sync/test/mock_transaction_ob...
sync/test/mock_transaction_observer.h:18: class MockTransactionObserver :
On 2012/11/16 19:42:27, Nicolas Zea wrote:
> Given that this isn't actually a mock, I'd prefer calling this something else.
> Perhaps TestTransactionObserver?

That's a much better name.  Thanks!

[Nicolas Zea, 2012-11-17 00:31:01]

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11360259/diff/1/chrome/browser/sync/glue/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1181:
registrar_->ActivateDataType(syncer::DEVICE_INFO,
On 2012/11/17 00:07:21, rlarocque wrote:
> On 2012/11/16 19:42:27, Nicolas Zea wrote:
> > Why not activate the datatype after initialization succeeds? Or is it that
we
> > expect it to never fail?
> 
> It made more sense to me to do it after the activation was complete.  I see
> InitLocalDeviceInfo() as an operation that can only be performed on an active
> data type, in the same way that only an active BookmarkChangeProcessor can
> respond to bookmark change notifications.
> 
> Also, I think it relies on some the ChangeProcessor::share_handle() function,
> which I believe won't work until the type is activated.
> 
> Maybe InitLocalDeviceInfo() is a bad name?

InitLocalDeviceInfo seems to me like an "AssociateModels" step, which typically
is what must succeed before we activate a datatype. If there's a chance of that
failing we shouldn't be auto-activating the type.

At the moment, it seems that's not the case. But in case we start doing
something fancier in the future, I think I'd prefer moving ActivateDatatype to
DoFinishInitialProcessControlTypes, which you could eventually extend with a
success bool if you need to.

Given that it's both (effectively) a model associator and a change processor, I
think it's fine to pass in the user share into the device tracker directly at
construction time (which would happen here). This is also how the sync
encryption handler works.

[rlarocque, 2012-11-17 00:58:38]

Patch updated.  

I don't have access to a real machine right now, so I've only run the unit
tests, not the browser itself.  I'm pretty sure this will work, though.

PTAL.

[Nicolas Zea, 2012-11-20 02:43:34]

lgtm

[commit-bot: I haz the power, 2012-11-20 02:56:40]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-11-20 19:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/11360259/3005

[commit-bot: I haz the power, 2012-11-20 19:51:12]

Presubmit check for 11360259-3005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      chrome/browser/sync/glue/synced_device_tracker.h:12 \
      sync/internal_api/test/fake_sync_manager.cc:24

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.8s to calculate.

[rlarocque, 2012-11-20 20:05:51]

+thestig: Please review for chrome/ changes.

[Lei Zhang, 2012-11-20 22:56:21]

chrome gypi files lgtm

[commit-bot: I haz the power, 2012-11-20 22:59:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/11360259/2006

[commit-bot: I haz the power, 2012-11-20 23:31:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-21 00:58:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/11360259/7041

[commit-bot: I haz the power, 2012-11-21 01:34:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-21 01:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/11360259/3010

[commit-bot: I haz the power, 2012-11-21 04:16:34]

Change committed as 168972