Add sync manager classes for backup/rollback:

Add sync manager classes for backup/rollback:
* SyncBackupManager runs before first-sync to save local data in backup DB.
* SyncRollbackManager deletes local data that's not found in backup DB.

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

[Nicolas Zea, 2014-04-14 21:36:31]

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/public/tes...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/public/tes...
sync/internal_api/public/test/test_internal_components_factory.h:26: explicit
TestInternalComponentsFactory(
Comment for both these constructors, and what they're for.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:23: // DB for backup types as
needed. Most of functions are no ops.
I think this would be a good place to go into more detail. How does this behave
differently? Which functions aren't no-ops and why?

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:26: public JsBackend,
Why do we need the JsBackend?

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:28: public
syncable::TransactionObserver {
transaction observer doesn't seem necessary either

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
File sync/internal_api/sync_rollback_manager_unittest.cc (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:26: class
SyncRollbackManagerTest
put this all within an anon namespace?

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:59: protected:
I'm not sure there's any point to distinguish between public and protected.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:73: void
InitManager(SyncManager *manager, ModelTypeSet types) {
nit: SyncManager *manager -> SyncManager* manager (move the asterisk)

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:78:
manager->Init(base::FilePath("test"),
is this going to leave the database behind after the test is run?

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:96: void
PrepopulateDb(ModelType type, const std::string& client_tag) {
Comment for the method.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:103: bool
VerifyEntry(UserShare* user_share, ModelType type,
pass UserShare by const ref?

Also comment for the method and what it's verifying.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:110: protected:
this section is already within a "protected"

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:128: EXPECT_CALL(*this,
OnChangesApplied(_, _, _, _))
In general it's encouraged to avoid mock logic like this that starts performing
actions and such. It's very easy for this mock logic to become overly
complicated and error-prone.

Could you instead just have a separate ChangeDelegate class that you inform of
the expected behavior? That would also clean up your Test class to not be doing
ChangeDelegate work as well.

https://codereview.chromium.org/235053006/diff/1/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/235053006/diff/1/sync/syncable/model_type.cc#...
sync/syncable/model_type.cc:450: result.Put(EXTENSION_SETTINGS);
Extension settings and app settings don't need to be backed up. They only exist
if sync is enabled.

[haitaol1, 2014-04-15 00:19:06]

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/public/tes...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/public/tes...
sync/internal_api/public/test/test_internal_components_factory.h:26: explicit
TestInternalComponentsFactory(
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> Comment for both these constructors, and what they're for.

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:23: // DB for backup types as
needed. Most of functions are no ops.
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> I think this would be a good place to go into more detail. How does this
behave
> differently? Which functions aren't no-ops and why?

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:26: public JsBackend,
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> Why do we need the JsBackend?

This was used to query directory stats, but it may have changed. Removed.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_base.h:28: public
syncable::TransactionObserver {
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> transaction observer doesn't seem necessary either

There's a DCHECK(transaction_observer.IsInitialized()) in constructor of
Directory::Kernel

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
File sync/internal_api/sync_rollback_manager_unittest.cc (right):

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:26: class
SyncRollbackManagerTest
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> put this all within an anon namespace?

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:59: protected:
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> I'm not sure there's any point to distinguish between public and protected.

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:73: void
InitManager(SyncManager *manager, ModelTypeSet types) {
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> nit: SyncManager *manager -> SyncManager* manager (move the asterisk)

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:78:
manager->Init(base::FilePath("test"),
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> is this going to leave the database behind after the test is run?

No, db will be destroyed when test instance is destructed.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:96: void
PrepopulateDb(ModelType type, const std::string& client_tag) {
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> Comment for the method.

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:103: bool
VerifyEntry(UserShare* user_share, ModelType type,
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> pass UserShare by const ref?
> 
> Also comment for the method and what it's verifying.

ReadTransaction doesn't use const share.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:110: protected:
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> this section is already within a "protected"

Done.

https://codereview.chromium.org/235053006/diff/1/sync/internal_api/sync_rollb...
sync/internal_api/sync_rollback_manager_unittest.cc:128: EXPECT_CALL(*this,
OnChangesApplied(_, _, _, _))
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> In general it's encouraged to avoid mock logic like this that starts
performing
> actions and such. It's very easy for this mock logic to become overly
> complicated and error-prone.
> 
> Could you instead just have a separate ChangeDelegate class that you inform of
> the expected behavior? That would also clean up your Test class to not be
doing
> ChangeDelegate work as well.

Done.

https://codereview.chromium.org/235053006/diff/1/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/235053006/diff/1/sync/syncable/model_type.cc#...
sync/syncable/model_type.cc:450: result.Put(EXTENSION_SETTINGS);
On 2014/04/14 21:36:31, Nicolas Zea wrote:
> Extension settings and app settings don't need to be backed up. They only
exist
> if sync is enabled.

Per offline discussion, back them up to simulate setting up extensions using
sync on new device during rollback.

[maniscalco, 2014-04-15 16:29:13]

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
How do you envision backups working with attachments?  Currently, each
GenericChangeProcessor has an AttachmentService which is responsible for
managing and providing access to that datatype's attachments.  Would
SyncBackupManager call out to each AttachmentService and ask it to make a backup
of the datatype's attachments?

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:36: // SyncManager
implementation.
Can you help me understand why this class is a SyncManager?  Seems like that
forces it to implement a bunch of methods that don't really apply (i.e. are
no-op implementations).

[haitaol1, 2014-04-15 16:59:51]

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
Backup is only for local data. Does local model create attachment when sync is
disabled? If not, then there should be no attachments to back up.

On 2014/04/15 16:29:14, maniscalco wrote:
> How do you envision backups working with attachments?  Currently, each
> GenericChangeProcessor has an AttachmentService which is responsible for
> managing and providing access to that datatype's attachments.  Would
> SyncBackupManager call out to each AttachmentService and ask it to make a
backup
> of the datatype's attachments?

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:36: // SyncManager
implementation.
It's sync manager because that's where the sync engine behaves very differently
in backup/rollback mode and in normal sync mode, thus those no-op
implementation. The objects used by normal sync manager, e.g. scheduler, syncer,
session context, etc, are not needed for backup purpose. The backup sync engine
is just a data store for local changes.

On 2014/04/15 16:29:14, maniscalco wrote:
> Can you help me understand why this class is a SyncManager?  Seems like that
> forces it to implement a bunch of methods that don't really apply (i.e. are
> no-op implementations).

[maniscalco, 2014-04-15 17:21:19]

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
Model type code will not create attachments when sync is disabled.  I think I
understand, you're saying that this feature will only backup unsynced data?  If
that's right, can you update this class comment?  When I first read it, I
interpreted it as backing up the current sync db, which isn't what it's doing.

On 2014/04/15 16:59:51, haitaol1 wrote:
> Backup is only for local data. Does local model create attachment when sync is
> disabled? If not, then there should be no attachments to back up.
> 
> On 2014/04/15 16:29:14, maniscalco wrote:
> > How do you envision backups working with attachments?  Currently, each
> > GenericChangeProcessor has an AttachmentService which is responsible for
> > managing and providing access to that datatype's attachments.  Would
> > SyncBackupManager call out to each AttachmentService and ask it to make a
> backup
> > of the datatype's attachments?
>

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:36: // SyncManager
implementation.
What I'm asking is why does this class need to implement the SyncManager
interface?  Which other classes call into this class?  Could (should?) the
SyncManager interface be refactoring into smaller units so that this class could
only implement the parts that make sense?

On 2014/04/15 16:59:51, haitaol1 wrote:
> It's sync manager because that's where the sync engine behaves very
differently
> in backup/rollback mode and in normal sync mode, thus those no-op
> implementation. The objects used by normal sync manager, e.g. scheduler,
syncer,
> session context, etc, are not needed for backup purpose. The backup sync
engine
> is just a data store for local changes.
> 
> On 2014/04/15 16:29:14, maniscalco wrote:
> > Can you help me understand why this class is a SyncManager?  Seems like that
> > forces it to implement a bunch of methods that don't really apply (i.e. are
> > no-op implementations).
>

[haitaol1, 2014-04-15 18:11:20]

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
On 2014/04/15 17:21:19, maniscalco wrote:
> Model type code will not create attachments when sync is disabled.  I think I
> understand, you're saying that this feature will only backup unsynced data? 
If
> that's right, can you update this class comment?  When I first read it, I
> interpreted it as backing up the current sync db, which isn't what it's doing.
> 
> On 2014/04/15 16:59:51, haitaol1 wrote:
> > Backup is only for local data. Does local model create attachment when sync
is
> > disabled? If not, then there should be no attachments to back up.
> > 
> > On 2014/04/15 16:29:14, maniscalco wrote:
> > > How do you envision backups working with attachments?  Currently, each
> > > GenericChangeProcessor has an AttachmentService which is responsible for
> > > managing and providing access to that datatype's attachments.  Would
> > > SyncBackupManager call out to each AttachmentService and ask it to make a
> > backup
> > > of the datatype's attachments?
> > 
> 

Done.

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/60001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:36: // SyncManager
implementation.
SyncBackendHostCore interacts with SyncManager. Refactoring SyncManager is
desired (SyncManagerImpl is more than 1000 lines) but out of the scope of this
change.

On 2014/04/15 17:21:19, maniscalco wrote:
> What I'm asking is why does this class need to implement the SyncManager
> interface?  Which other classes call into this class?  Could (should?) the
> SyncManager interface be refactoring into smaller units so that this class
could
> only implement the parts that make sense?
> 
> On 2014/04/15 16:59:51, haitaol1 wrote:
> > It's sync manager because that's where the sync engine behaves very
> differently
> > in backup/rollback mode and in normal sync mode, thus those no-op
> > implementation. The objects used by normal sync manager, e.g. scheduler,
> syncer,
> > session context, etc, are not needed for backup purpose. The backup sync
> engine
> > is just a data store for local changes.
> > 
> > On 2014/04/15 16:29:14, maniscalco wrote:
> > > Can you help me understand why this class is a SyncManager?  Seems like
that
> > > forces it to implement a bunch of methods that don't really apply (i.e.
are
> > > no-op implementations).
> > 
>

[Nicolas Zea, 2014-04-15 23:05:51]

I find it strange that we'll now have three different types of SyncManager: the
normal one, the one that runs when in backup mode, and one for performing
rollback. I'm not sure this is what a SyncManager is for, although it's true
that it's roll has been overloaded over the years.

As Nick mentions, it seems like there should be a better way to specialize their
logic than entirely new SyncManagers. I think I agree that introducing a new
pure interface that abstracts all the logic which needs to be overloaded is
cleaner.

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
nit: into -> in to

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager_unittest.cc (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager_unittest.cc:34:
scoped_ptr<syncable::DirectoryBackingStore>(
do you have to create this here? Could you instead change the
TestInternalComponentsFactory to support creating a TestDirectoryBackingStore (a
new StorageOption)?

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:21: // Base class of sync
managers used for backup and rollback. Tow major
nit: tow -> two

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:115: protected:
data members should be private, not protected. If they're shared, then there
should be explicit getter/setter methods, but that typically means that layering
isn't clear.

[haitaol1, 2014-04-16 21:52:22]

On 2014/04/15 23:05:51, Nicolas Zea wrote:
> I find it strange that we'll now have three different types of SyncManager:
the
> normal one, the one that runs when in backup mode, and one for performing
> rollback. I'm not sure this is what a SyncManager is for, although it's true
> that it's roll has been overloaded over the years.
> 
> As Nick mentions, it seems like there should be a better way to specialize
their
> logic than entirely new SyncManagers. I think I agree that introducing a new
> pure interface that abstracts all the logic which needs to be overloaded is
> cleaner.
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
> File sync/internal_api/sync_backup_manager.h (right):
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
> sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
> user signs into sync to back up user's data
> nit: into -> in to
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
> File sync/internal_api/sync_backup_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
> sync/internal_api/sync_backup_manager_unittest.cc:34:
> scoped_ptr<syncable::DirectoryBackingStore>(
> do you have to create this here? Could you instead change the
> TestInternalComponentsFactory to support creating a TestDirectoryBackingStore
(a
> new StorageOption)?
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
> File sync/internal_api/sync_rollback_manager_base.h (right):
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
> sync/internal_api/sync_rollback_manager_base.h:21: // Base class of sync
> managers used for backup and rollback. Tow major
> nit: tow -> two
> 
>
https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
> sync/internal_api/sync_rollback_manager_base.h:115: protected:
> data members should be private, not protected. If they're shared, then there
> should be explicit getter/setter methods, but that typically means that
layering
> isn't clear.

[haitaol1, 2014-04-16 21:52:35]

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager.h:14: // SyncBackupManager runs before
user signs into sync to back up user's data
On 2014/04/15 23:05:51, Nicolas Zea wrote:
> nit: into -> in to

Done.

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
File sync/internal_api/sync_backup_manager_unittest.cc (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_b...
sync/internal_api/sync_backup_manager_unittest.cc:34:
scoped_ptr<syncable::DirectoryBackingStore>(
That's not sufficient because I want to verify data is persisted. So I need to
pass in db connection instead of letting store create its own so that I can
still read db after store is closed.

On 2014/04/15 23:05:51, Nicolas Zea wrote:
> do you have to create this here? Could you instead change the
> TestInternalComponentsFactory to support creating a TestDirectoryBackingStore
(a
> new StorageOption)?

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:21: // Base class of sync
managers used for backup and rollback. Tow major
On 2014/04/15 23:05:51, Nicolas Zea wrote:
> nit: tow -> two

Done.

https://codereview.chromium.org/235053006/diff/80001/sync/internal_api/sync_r...
sync/internal_api/sync_rollback_manager_base.h:115: protected:
Done. 


On 2014/04/15 23:05:51, Nicolas Zea wrote:
> data members should be private, not protected. If they're shared, then there
> should be explicit getter/setter methods, but that typically means that
layering
> isn't clear.

[haitaol1, 2014-04-17 16:41:13]

Changed to use ondisk sync db and reverted changes to
TestInternalComponentFactory. PTAL.

[Nicolas Zea, 2014-04-17 18:22:55]

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_backup_manager.cc (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.cc:21: NormalizeEntries();
Is it necessary to call NormalizeEntries on every save changes? Is there any
drawback if you didn't call it at all?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.h:17: class SyncBackupManager : public
SyncRollbackManagerBase {
do these new classes need SYNC_EXPORT?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.h:39: bool in_normalization_;
DISALLOW_COPY_AND_ASSIGN (here and in the other classes)

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager.cc (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:56: change_delegate_ =
change_delegate;
Where do you clear the old backup directory? Or this directory for that matter
(after rollback)?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:76: &unsynced);
fix indent (FYI, you can use git cl format to fix all these issues)

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:99: }
add a TODO to handle any cases where data was merged at association (possibly
referencing a bug)?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager_base.h:116: void
UpdateNewEntryStats(ModelType type);
do these two methods need to be part of the parent class? It seems like they're
only used by the backup manager

[haitaol1, 2014-04-17 19:30:38]

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_backup_manager.cc (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.cc:21: NormalizeEntries();
On 2014/04/17 18:22:56, Nicolas Zea wrote:
> Is it necessary to call NormalizeEntries on every save changes? Is there any
> drawback if you didn't call it at all?

It only checks entries that are changed, so cost is not high. Without doing
this, deleted entry is marked unsynced, which prevents them from being cleared
from memory.

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_backup_manager.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.h:17: class SyncBackupManager : public
SyncRollbackManagerBase {
They are not used by chrome/browser/sync. Looks like SYNC_EXPORT_PRIVATE should
be used according to:
https://sites.google.com/a/google.com/chromesync/sync-client/sync-and-compone...

On 2014/04/17 18:22:56, Nicolas Zea wrote:
> do these new classes need SYNC_EXPORT?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_backup_manager.h:39: bool in_normalization_;
On 2014/04/17 18:22:56, Nicolas Zea wrote:
> DISALLOW_COPY_AND_ASSIGN (here and in the other classes)

Done.

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager.cc (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:56: change_delegate_ =
change_delegate;
SyncBackendHostCore::DoShutdown() deletes the directory.

On 2014/04/17 18:22:56, Nicolas Zea wrote:
> Where do you clear the old backup directory? Or this directory for that matter
> (after rollback)?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:76: &unsynced);
On 2014/04/17 18:22:56, Nicolas Zea wrote:
> fix indent (FYI, you can use git cl format to fix all these issues)

Done.

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager.cc:99: }
Added TODO above.

On 2014/04/17 18:22:56, Nicolas Zea wrote:
> add a TODO to handle any cases where data was merged at association (possibly
> referencing a bug)?

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager_base.h:116: void
UpdateNewEntryStats(ModelType type);
Added to access private status_

On 2014/04/17 18:22:56, Nicolas Zea wrote:
> do these two methods need to be part of the parent class? It seems like
they're
> only used by the backup manager

[Nicolas Zea, 2014-04-17 20:26:44]

mostly LG, with one further comment

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager_base.h:116: void
UpdateNewEntryStats(ModelType type);
On 2014/04/17 19:30:38, haitaol1 wrote:
> Added to access private status_
> 
> On 2014/04/17 18:22:56, Nicolas Zea wrote:
> > do these two methods need to be part of the parent class? It seems like
> they're
> > only used by the backup manager
> 

I think it's cleaner to have the implementations of this class own the status,
and handle any status updates they want. You could maybe pass a pointer to the
status in the init call to let the directory fill the initial status
information.

In general, it seems like this base class should never be instantiated on its
own, and should ideally have only that logic shared by both the backup and
rollback managers

[haitaol1, 2014-04-17 22:21:18]

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
File sync/internal_api/sync_rollback_manager_base.h (right):

https://codereview.chromium.org/235053006/diff/120001/sync/internal_api/sync_...
sync/internal_api/sync_rollback_manager_base.h:116: void
UpdateNewEntryStats(ModelType type);
On 2014/04/17 20:26:44, Nicolas Zea wrote:
> On 2014/04/17 19:30:38, haitaol1 wrote:
> > Added to access private status_
> > 
> > On 2014/04/17 18:22:56, Nicolas Zea wrote:
> > > do these two methods need to be part of the parent class? It seems like
> > they're
> > > only used by the backup manager
> > 
> 
> I think it's cleaner to have the implementations of this class own the status,
> and handle any status updates they want. You could maybe pass a pointer to the
> status in the init call to let the directory fill the initial status
> information.
> 
> In general, it seems like this base class should never be instantiated on its
> own, and should ideally have only that logic shared by both the backup and
> rollback managers

Done.

[Nicolas Zea, 2014-04-17 22:29:34]

LGTM, thanks!

[haitaol1, 2014-04-17 22:50:32]

Thanks for reviewing!

[haitaol1, 2014-04-17 22:50:57]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-17 22:51:29]

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

[commit-bot: I haz the power, 2014-04-18 00:04:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 00:04:47]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[haitaol1, 2014-04-18 16:49:17]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-18 16:49:26]

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

[commit-bot: I haz the power, 2014-04-18 18:46:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 18:46:54]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[haitaol1, 2014-04-18 20:58:16]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-18 21:02:52]

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

[commit-bot: I haz the power, 2014-04-18 22:49:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 22:49:34]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[haitaol1, 2014-04-21 16:45:23]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-04-21 16:45:28]

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

[commit-bot: I haz the power, 2014-04-21 18:35:47]

Change committed as 265027