[sync] Add backup time in synced device info so that server can flag device

[sync] Add backup time in synced device info so that server knows which device can be rolled back to when.

Backup DB is checked and backup time in device info is updated periodically
to keep server informed about backup state.

BUG=362679
TBR=gbillock@chromium.org

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

[Nicolas Zea, 2014-06-13 22:36:41]

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

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_device_tracker.cc:210:
specifics.set_backup_timestamp(backup_time.ToInternalValue());
ToInternalValue uses the windows epoch. It's probably better to use the linux
epoch, and convert using the syncer::TimeToProtoTime helper.

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2570: #if defined(OS_WIN) ||
defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
I wonder if it's better to pull out these platform-specific macros and just have
an #if defined(ENABLE_SYNC_BACKUP), which is defined in common.gypi for the
specified platforms. That way if we decide to enable it on e.g. mobile we don't
have to find all the places this guard exists.

See for example ENABLE_EXTENSIONS macro

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2575: // closing backup DB.
Why do we need to serialize on the sync thread in some cases?

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:1123: scoped_ptr<base::Time>
last_backup_time_;
Does this need to be in a scoped ptr? Couldn't you just check for
base::Time::is_null()?

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:43:
base::MessageLoop::current()->Run();
Run is deprecated, RunLoops should be used instead.

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:48:
void CheckDbCallback(base::Time* time_out, base::Time time_in) {
nit: output params should be ordered after input ones

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/public/uti...
File sync/internal_api/public/util/sync_db_util.h (right):

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/public/uti...
sync/internal_api/public/util/sync_db_util.h:20: // modified time if integrity
check passes, with NULL time otherwise.
Mention what thread this should be run on?

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/sync_db_ut...
File sync/internal_api/sync_db_util.cc (right):

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/sync_db_ut...
sync/internal_api/sync_db_util.cc:41: 
remove extra newlines

https://codereview.chromium.org/332923002/diff/1/sync/protocol/device_info_sp...
File sync/protocol/device_info_specifics.proto (right):

https://codereview.chromium.org/332923002/diff/1/sync/protocol/device_info_sp...
sync/protocol/device_info_specifics.proto:39: // restored to state back to this
time.
Mention time format/units

[haitaol1, 2014-06-13 23:28:26]

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

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_device_tracker.cc:210:
specifics.set_backup_timestamp(backup_time.ToInternalValue());
On 2014/06/13 22:36:41, Nicolas Zea wrote:
> ToInternalValue uses the windows epoch. It's probably better to use the linux
> epoch, and convert using the syncer::TimeToProtoTime helper.

Done.

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2570: #if defined(OS_WIN) ||
defined(OS_MACOSX) || (defined(OS_LINUX) && !defined(OS_CHROMEOS))
Will do in follow-up cl.

On 2014/06/13 22:36:41, Nicolas Zea wrote:
> I wonder if it's better to pull out these platform-specific macros and just
have
> an #if defined(ENABLE_SYNC_BACKUP), which is defined in common.gypi for the
> specified platforms. That way if we decide to enable it on e.g. mobile we
don't
> have to find all the places this guard exists.
> 
> See for example ENABLE_EXTENSIONS macro

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2575: // closing backup DB.
Check will fail if sync DB is still opened by backup backend.

On 2014/06/13 22:36:41, Nicolas Zea wrote:
> Why do we need to serialize on the sync thread in some cases?

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:1123: scoped_ptr<base::Time>
last_backup_time_;
Using scoped_ptr makes it 3-state, NULL pointer means unknown, null time means
no backup.

On 2014/06/13 22:36:41, Nicolas Zea wrote:
> Does this need to be in a scoped ptr? Couldn't you just check for
> base::Time::is_null()?

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:43:
base::MessageLoop::current()->Run();
On 2014/06/13 22:36:41, Nicolas Zea wrote:
> Run is deprecated, RunLoops should be used instead.

Done.

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/test/int...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:48:
void CheckDbCallback(base::Time* time_out, base::Time time_in) {
time_out is provided at binding time while time_in is at calling time.

On 2014/06/13 22:36:41, Nicolas Zea wrote:
> nit: output params should be ordered after input ones

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/public/uti...
File sync/internal_api/public/util/sync_db_util.h (right):

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/public/uti...
sync/internal_api/public/util/sync_db_util.h:20: // modified time if integrity
check passes, with NULL time otherwise.
On 2014/06/13 22:36:41, Nicolas Zea wrote:
> Mention what thread this should be run on?

Done.

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/sync_db_ut...
File sync/internal_api/sync_db_util.cc (right):

https://codereview.chromium.org/332923002/diff/1/sync/internal_api/sync_db_ut...
sync/internal_api/sync_db_util.cc:41: 
On 2014/06/13 22:36:41, Nicolas Zea wrote:
> remove extra newlines

Done.

https://codereview.chromium.org/332923002/diff/1/sync/protocol/device_info_sp...
File sync/protocol/device_info_specifics.proto (right):

https://codereview.chromium.org/332923002/diff/1/sync/protocol/device_info_sp...
sync/protocol/device_info_specifics.proto:39: // restored to state back to this
time.
On 2014/06/13 22:36:41, Nicolas Zea wrote:
> Mention time format/units

Done.

[Nicolas Zea, 2014-06-13 23:48:52]

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2575: // closing backup DB.
On 2014/06/13 23:28:25, haitaol1 wrote:
> Check will fail if sync DB is still opened by backup backend.
> 
> On 2014/06/13 22:36:41, Nicolas Zea wrote:
> > Why do we need to serialize on the sync thread in some cases? 
> 

If the backup is running, why not have it do this work automatically and post
the result back? In that case, this CheckSyncBackupIfNeeded does nothing.

https://codereview.chromium.org/332923002/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/332923002/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:43:
base::MessageLoop::current()->RunLoop();
note: RunLoop is a class, not a method (I don't think this will compile?). See
base/run_loop.h

[haitaol1, 2014-06-16 17:35:29]

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/332923002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:2575: // closing backup DB.
Backup may or may not be running at the time. If it's first time sync, it's
likely that backup is running, but not guaranteed because of startup delay or
unrecoverable error. For periodical check after first signin, backup is
definitely not running. 

On 2014/06/13 23:48:52, Nicolas Zea wrote:
> On 2014/06/13 23:28:25, haitaol1 wrote:
> > Check will fail if sync DB is still opened by backup backend.
> > 
> > On 2014/06/13 22:36:41, Nicolas Zea wrote:
> > > Why do we need to serialize on the sync thread in some cases? 
> > 
> 
> If the backup is running, why not have it do this work automatically and post
> the result back? In that case, this CheckSyncBackupIfNeeded does nothing.

https://codereview.chromium.org/332923002/diff/20001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
(right):

https://codereview.chromium.org/332923002/diff/20001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc:43:
base::MessageLoop::current()->RunLoop();
really done

On 2014/06/13 23:48:52, Nicolas Zea wrote:
> note: RunLoop is a class, not a method (I don't think this will compile?). See
> base/run_loop.h

[Nicolas Zea, 2014-06-17 21:46:31]

lgtm

[haitaol1, 2014-06-18 20:14:31]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-06-18 20:15:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/332923002/60001

[haitaol1, 2014-06-19 16:37:25]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-06-19 16:38:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/332923002/80001

[haitaol1, 2014-07-14 21:41:10]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-07-14 21:43:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/332923002/180001

[commit-bot: I haz the power, 2014-07-14 22:41:58]

Change committed as 283060