Merge 281926 "Two fixes regarding to bookmark backup"

> Two fixes regarding to bookmark backup
> 
> * Only change parent id when normalizing entry. Use PutParentIdPropertyOnly() instead of PutParentId() because the latter resets predecessor and changes ordering.
> * Don't create sync node for mobile bookmarks, which only exists with sync enabled.
> 
> BUG=385731
> 
> Review URL: https://codereview.chromium.org/374183003

TBR=haitaol@chromium.org

Review URL: https://codereview.chromium.org/419283004

git-svn-id: svn://svn.chromium.org/chrome/branches/2062/src@285663 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 131 insertions, 4 deletions

diff --git a/chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc b/chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
index 5dec3a1..1571d66 100644
--- a/chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_backup_rollback_test.cc
@@ -21,6 +21,12 @@ using bookmarks_helper::Move;
 using bookmarks_helper::Remove;
 using sync_integration_test_util::AwaitCommitActivityCompletion;
 
+namespace {
+const char kUrl1[] = "http://www.google.com";
+const char kUrl2[] = "http://map.google.com";
+const char kUrl3[] = "http://plus.google.com";
+}  // anonymous namespace
+
 class SingleClientBackupRollbackTest : public SyncTest {
  public:
   SingleClientBackupRollbackTest() : SyncTest(SINGLE_CLIENT) {}
@@ -131,3 +137,125 @@ IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
   const BookmarkNode* url2 = tier1_b->GetChild(0);
   ASSERT_EQ(GURL("http://www.nhl.com"), url2->url());
 }
+
+#if defined(ENABLE_PRE_SYNC_BACKUP)
+#define MAYBE_TestPrefBackupRollback TestPrefBackupRollback
+#else
+#define MAYBE_TestPrefBackupRollback DISABLED_TestPrefBackupRollback
+#endif
+// Verify local preferences are not affected by preferences in backup DB under
+// backup mode.
+IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
+                       MAYBE_TestPrefBackupRollback) {
+  EnableRollback();
+
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl1);
+
+  BackupModeChecker checker(GetSyncService(0),
+                            base::TimeDelta::FromSeconds(15));
+  ASSERT_TRUE(checker.Wait());
+
+  // Shut down backup, then change preference.
+  GetSyncService(0)->StartStopBackupForTesting();
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl2);
+
+  // Restart backup. Preference shouldn't change after backup starts.
+  GetSyncService(0)->StartStopBackupForTesting();
+  ASSERT_TRUE(checker.Wait());
+  ASSERT_EQ(kUrl2,
+            preferences_helper::GetPrefs(0)->GetString(prefs::kHomePage));
+
+  // Start sync and change preference.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl3);
+  ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
+  ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Let server return rollback command on next sync request.
+  GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
+
+  // Make another change to trigger downloading of rollback command.
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, "");
+
+  // Wait for sync to switch to backup mode after finishing rollback.
+  ASSERT_TRUE(checker.Wait());
+
+  // Verify preference is restored.
+  ASSERT_EQ(kUrl2,
+            preferences_helper::GetPrefs(0)->GetString(prefs::kHomePage));
+}
+
+#if defined(ENABLE_PRE_SYNC_BACKUP)
+#define MAYBE_RollbackNoBackup RollbackNoBackup
+#else
+#define MAYBE_RollbackNoBackup DISABLED_RollbackNoBackup
+#endif
+IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
+                       MAYBE_RollbackNoBackup) {
+  EnableRollback();
+
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Setup sync, wait for its completion, and make sure changes were synced.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  // Starting state:
+  // other_node
+  //    -> http://mail.google.com  "url0"
+  //    -> http://www.nhl.com "url1"
+  ASSERT_TRUE(AddURL(0, GetOtherNode(0), 0, "url0",
+                     GURL("http://mail.google.com")));
+  ASSERT_TRUE(AddURL(0, GetOtherNode(0), 1, "url1",
+                     GURL("http://www.nhl.com")));
+
+  ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
+  ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Let server to return rollback command on next sync request.
+  GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
+
+  // Make another change to trigger downloading of rollback command.
+  Remove(0, GetOtherNode(0), 0);
+
+  // Wait for sync to switch to backup mode after finishing rollback.
+  BackupModeChecker checker(GetSyncService(0),
+                            base::TimeDelta::FromSeconds(15));
+  ASSERT_TRUE(checker.Wait());
+
+  // Without backup DB, bookmarks added during sync shouldn't be removed.
+  ASSERT_EQ(1, GetOtherNode(0)->child_count());
+  ASSERT_EQ(GURL("http://www.nhl.com"),
+            GetOtherNode(0)->GetChild(0)->url());
+}
+
+#if defined(ENABLE_PRE_SYNC_BACKUP)
+#define MAYBE_DontChangeBookmarkOrdering DontChangeBookmarkOrdering
+#else
+#define MAYBE_DontChangeBookmarkOrdering DISABLED_DontChangeBookmarkOrdering
+#endif
+IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
+                       MAYBE_DontChangeBookmarkOrdering) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  const BookmarkNode* sub_folder = AddFolder(0, GetOtherNode(0), 0, "test");
+  ASSERT_TRUE(AddURL(0, sub_folder, 0, "", GURL(kUrl1)));
+  ASSERT_TRUE(AddURL(0, sub_folder, 1, "", GURL(kUrl2)));
+  ASSERT_TRUE(AddURL(0, sub_folder, 2, "", GURL(kUrl3)));
+
+  BackupModeChecker checker(GetSyncService(0),
+                            base::TimeDelta::FromSeconds(15));
+  ASSERT_TRUE(checker.Wait());
+
+  // Restart backup.
+  GetSyncService(0)->StartStopBackupForTesting();
+  GetSyncService(0)->StartStopBackupForTesting();
+  ASSERT_TRUE(checker.Wait());
+
+  // Verify bookmarks are unchanged.
+  ASSERT_EQ(3, sub_folder->child_count());
+  ASSERT_EQ(GURL(kUrl1), sub_folder->GetChild(0)->url());
+  ASSERT_EQ(GURL(kUrl2), sub_folder->GetChild(1)->url());
+  ASSERT_EQ(GURL(kUrl3), sub_folder->GetChild(2)->url());
+}
diff --git a/sync/internal_api/sync_backup_manager.cc b/sync/internal_api/sync_backup_manager.cc
index 295f79a..4f16fbf 100644
--- a/sync/internal_api/sync_backup_manager.cc
+++ b/sync/internal_api/sync_backup_manager.cc
@@ -104,7 +104,7 @@ void SyncBackupManager::NormalizeEntries() {
     if (!entry.GetId().ServerKnows())
       entry.PutId(syncable::Id::CreateFromServerId(entry.GetId().value()));
     if (!entry.GetParentId().ServerKnows()) {
-      entry.PutParentId(syncable::Id::CreateFromServerId(
+      entry.PutParentIdPropertyOnly(syncable::Id::CreateFromServerId(
           entry.GetParentId().value()));
     }
     entry.PutBaseVersion(1);
diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc
index ade0422..96d3b74 100644
--- a/sync/internal_api/sync_rollback_manager_base.cc
+++ b/sync/internal_api/sync_rollback_manager_base.cc
@@ -15,8 +15,8 @@
 namespace {
 
 // Permanent bookmark folders as defined in bookmark_model_associator.cc.
+// No mobile bookmarks because they only exists with sync enabled.
 const char kBookmarkBarTag[] = "bookmark_bar";
-const char kMobileBookmarksTag[] = "synced_bookmarks";
 const char kOtherBookmarksTag[] = "other_bookmarks";
 
 class DummyEntryptionHandler : public syncer::SyncEncryptionHandler {
@@ -118,7 +118,6 @@ void SyncRollbackManagerBase::ConfigureSyncer(
     if (InitTypeRootNode(type.Get())) {
       if (type.Get() == BOOKMARKS) {
         InitBookmarkFolder(kBookmarkBarTag);
-        InitBookmarkFolder(kMobileBookmarksTag);
         InitBookmarkFolder(kOtherBookmarksTag);
       }
     }
diff --git a/sync/internal_api/sync_rollback_manager_base_unittest.cc b/sync/internal_api/sync_rollback_manager_base_unittest.cc
index 79075d2..39217f5 100644
--- a/sync/internal_api/sync_rollback_manager_base_unittest.cc
+++ b/sync/internal_api/sync_rollback_manager_base_unittest.cc
@@ -58,7 +58,7 @@ TEST_F(SyncRollbackManagerBaseTest, InitTypeOnConfiguration) {
   EXPECT_EQ(BaseNode::INIT_OK,
             bookmark_bar.InitByTagLookupForBookmarks("bookmark_bar"));
   ReadNode bookmark_mobile(&trans);
-  EXPECT_EQ(BaseNode::INIT_OK,
+  EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD,
             bookmark_mobile.InitByTagLookupForBookmarks("synced_bookmarks"));
   ReadNode bookmark_other(&trans);
   EXPECT_EQ(BaseNode::INIT_OK,