diff options
28 files changed, 564 insertions, 792 deletions
diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc index 19bc480..40d9df1 100644 --- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc +++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -143,6 +143,88 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest { *metahandle_out = entry.Get(syncable::META_HANDLE); } + // Creates an item that is both unsynced an an unapplied update. Returns the + // metahandle of the created item. + int64 CreateUnappliedAndUnsyncedItem(const string& name, + syncable::ModelType model_type) { + int64 metahandle = 0; + CreateUnsyncedItem(id_factory_.MakeServer(name), id_factory_.root(), name, + false, model_type, &metahandle); + + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + if (!dir.good()) { + ADD_FAILURE(); + return syncable::kInvalidMetaHandle; + } + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + MutableEntry entry(&trans, syncable::GET_BY_HANDLE, metahandle); + if (!entry.good()) { + ADD_FAILURE(); + return syncable::kInvalidMetaHandle; + } + + entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); + entry.Put(syncable::SERVER_VERSION, GetNextRevision()); + + return metahandle; + } + + + // Creates an item that has neither IS_UNSYNED or IS_UNAPPLIED_UPDATE. The + // item is known to both the server and client. Returns the metahandle of + // the created item. + int64 CreateSyncedItem(const std::string& name, syncable::ModelType + model_type, bool is_folder) { + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + if (!dir.good()) { + ADD_FAILURE(); + return syncable::kInvalidMetaHandle; + } + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + + syncable::Id parent_id(id_factory_.root()); + syncable::Id item_id(id_factory_.MakeServer(name)); + int64 version = GetNextRevision(); + + sync_pb::EntitySpecifics default_specifics; + syncable::AddDefaultExtensionValue(model_type, &default_specifics); + + MutableEntry entry(&trans, syncable::CREATE, parent_id, name); + if (!entry.good()) { + ADD_FAILURE(); + return syncable::kInvalidMetaHandle; + } + + entry.Put(syncable::ID, item_id); + entry.Put(syncable::BASE_VERSION, version); + entry.Put(syncable::IS_UNSYNCED, false); + entry.Put(syncable::NON_UNIQUE_NAME, name); + entry.Put(syncable::IS_DIR, is_folder); + entry.Put(syncable::IS_DEL, false); + entry.Put(syncable::PARENT_ID, parent_id); + + if (!entry.PutPredecessor(id_factory_.root())) { + ADD_FAILURE(); + return syncable::kInvalidMetaHandle; + } + entry.Put(syncable::SPECIFICS, default_specifics); + + entry.Put(syncable::SERVER_VERSION, GetNextRevision()); + entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); + entry.Put(syncable::SERVER_NON_UNIQUE_NAME, "X"); + entry.Put(syncable::SERVER_PARENT_ID, id_factory_.MakeServer("Y")); + entry.Put(syncable::SERVER_IS_DIR, is_folder); + entry.Put(syncable::SERVER_IS_DEL, false); + entry.Put(syncable::SERVER_SPECIFICS, default_specifics); + entry.Put(syncable::SERVER_PARENT_ID, parent_id); + + return entry.Get(syncable::META_HANDLE); + } + + int64 GetNextRevision() { + return next_revision_++; + } + ApplyUpdatesCommand apply_updates_command_; TestIdFactory id_factory_; private: @@ -169,7 +251,11 @@ TEST_F(ApplyUpdatesCommandTest, Simple) { EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) + << "Simple update shouldn't result in conflicts"; + EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) + << "Simple update shouldn't result in conflicts"; + EXPECT_EQ(0, status->conflict_progress()->HierarchyConflictingItemsSize()) << "Simple update shouldn't result in conflicts"; EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "All items should have been successfully applied"; @@ -204,13 +290,197 @@ TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) { EXPECT_EQ(5, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "Simple update shouldn't result in conflicts, even if out-of-order"; EXPECT_EQ(5, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "All updates should have been successfully applied"; } -TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) { +// Runs the ApplyUpdatesCommand on an item that has both local and remote +// modifications (IS_UNSYNCED and IS_UNAPPLIED_UPDATE). We expect the command +// to detect that this update can't be applied because it is in a CONFLICT +// state. +TEST_F(ApplyUpdatesCommandTest, SimpleConflict) { + CreateUnappliedAndUnsyncedItem("item", syncable::BOOKMARKS); + + ExpectGroupToChange(apply_updates_command_, GROUP_UI); + apply_updates_command_.ExecuteImpl(session()); + + sessions::StatusController* status = session()->mutable_status_controller(); + sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); + ASSERT_TRUE(status->conflict_progress()); + EXPECT_EQ(1, status->conflict_progress()->SimpleConflictingItemsSize()) + << "Unsynced and unapplied item should be a simple conflict"; +} + +// Runs the ApplyUpdatesCommand on an item that has both local and remote +// modifications *and* the remote modification cannot be applied without +// violating the tree constraints. We expect the command to detect that this +// update can't be applied and that this situation can't be resolved with the +// simple conflict processing logic; it is in a CONFLICT_HIERARCHY state. +TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) { + // Create a simply-conflicting item. It will start with valid parent ids. + int64 handle = CreateUnappliedAndUnsyncedItem("orphaned_by_server", + syncable::BOOKMARKS); + { + // Manually set the SERVER_PARENT_ID to bad value. + // A bad parent indicates a hierarchy conflict. + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + ASSERT_TRUE(dir.good()); + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(entry.good()); + + entry.Put(syncable::SERVER_PARENT_ID, + id_factory_.MakeServer("bogus_parent")); + } + + ExpectGroupToChange(apply_updates_command_, GROUP_UI); + apply_updates_command_.ExecuteImpl(session()); + + sessions::StatusController* status = session()->mutable_status_controller(); + sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); + + EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()); + + // An update that is both a simple conflict and a hierarchy conflict should be + // treated as a hierarchy conflict. + ASSERT_TRUE(status->conflict_progress()); + EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +} + + +// Runs the ApplyUpdatesCommand on an item with remote modifications that would +// create a directory loop if the update were applied. We expect the command to +// detect that this update can't be applied because it is in a +// CONFLICT_HIERARCHY state. +TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) { + // Item 'X' locally has parent of 'root'. Server is updating it to have + // parent of 'Y'. + { + // Create it as a child of root node. + int64 handle = CreateSyncedItem("X", syncable::BOOKMARKS, true); + + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + ASSERT_TRUE(dir.good()); + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(entry.good()); + + // Re-parent from root to "Y" + entry.Put(syncable::SERVER_VERSION, GetNextRevision()); + entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); + entry.Put(syncable::SERVER_PARENT_ID, id_factory_.MakeServer("Y")); + } + + // Item 'Y' is child of 'X'. + CreateUnsyncedItem(id_factory_.MakeServer("Y"), id_factory_.MakeServer("X"), + "Y", true, syncable::BOOKMARKS, NULL); + + // If the server's update were applied, we would have X be a child of Y, and Y + // as a child of X. That's a directory loop. The UpdateApplicator should + // prevent the update from being applied and note that this is a hierarchy + // conflict. + + ExpectGroupToChange(apply_updates_command_, GROUP_UI); + apply_updates_command_.ExecuteImpl(session()); + + sessions::StatusController* status = session()->mutable_status_controller(); + sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); + + EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()); + + // This should count as a hierarchy conflict. + ASSERT_TRUE(status->conflict_progress()); + EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +} + +// Runs the ApplyUpdatesCommand on a directory where the server sent us an +// update to add a child to a locally deleted (and unsynced) parent. We expect +// the command to not apply the update and to indicate the update is in a +// CONFLICT_HIERARCHY state. +TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) { + // Create a locally deleted parent item. + int64 parent_handle; + CreateUnsyncedItem(Id::CreateFromServerId("parent"), id_factory_.root(), + "parent", true, syncable::BOOKMARKS, &parent_handle); + { + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + ASSERT_TRUE(dir.good()); + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + MutableEntry entry(&trans, syncable::GET_BY_HANDLE, parent_handle); + entry.Put(syncable::IS_DEL, true); + } + + // Create an incoming child from the server. + CreateUnappliedNewItemWithParent("child", DefaultBookmarkSpecifics(), + "parent"); + + // The server's update may seem valid to some other client, but on this client + // that new item's parent no longer exists. The update should not be applied + // and the update applicator should indicate this is a hierarchy conflict. + + ExpectGroupToChange(apply_updates_command_, GROUP_UI); + apply_updates_command_.ExecuteImpl(session()); + + sessions::StatusController* status = session()->mutable_status_controller(); + sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); + + // This should count as a hierarchy conflict. + ASSERT_TRUE(status->conflict_progress()); + EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +} + +// Runs the ApplyUpdatesCommand on a directory where the server is trying to +// delete a folder that has a recently added (and unsynced) child. We expect +// the command to not apply the update because it is in a CONFLICT_HIERARCHY +// state. +TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) { + // Create a server-deleted directory. + { + // Create it as a child of root node. + int64 handle = CreateSyncedItem("parent", syncable::BOOKMARKS, true); + + ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); + ASSERT_TRUE(dir.good()); + WriteTransaction trans(FROM_HERE, UNITTEST, dir); + MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle); + ASSERT_TRUE(entry.good()); + + // Delete it on the server. + entry.Put(syncable::SERVER_VERSION, GetNextRevision()); + entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); + entry.Put(syncable::SERVER_PARENT_ID, id_factory_.root()); + entry.Put(syncable::SERVER_IS_DEL, true); + } + + // Create a local child of the server-deleted directory. + CreateUnsyncedItem(id_factory_.MakeServer("child"), + id_factory_.MakeServer("parent"), "child", false, + syncable::BOOKMARKS, NULL); + + // The server's request to delete the directory must be ignored, otherwise our + // unsynced new child would be orphaned. This is a hierarchy conflict. + + ExpectGroupToChange(apply_updates_command_, GROUP_UI); + apply_updates_command_.ExecuteImpl(session()); + + sessions::StatusController* status = session()->mutable_status_controller(); + sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); + + // This should count as a hierarchy conflict. + ASSERT_TRUE(status->conflict_progress()); + EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +} + +// Runs the ApplyUpdatesCommand on a server-created item that has a locally +// unknown parent. We expect the command to not apply the update because the +// item is in a CONFLICT_HIERARCHY state. +TEST_F(ApplyUpdatesCommandTest, HierarchyConflictUnknownParent) { // We shouldn't be able to do anything with either of these items. CreateUnappliedNewItemWithParent("some_item", DefaultBookmarkSpecifics(), @@ -228,7 +498,10 @@ TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) { EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) + << "Updates with unknown parent should not be treated as 'simple'" + << " conflicts"; + EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize()) << "All updates with an unknown ancestors should be in conflict"; EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "No item with an unknown ancestor should be applied"; @@ -265,7 +538,7 @@ TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) { EXPECT_EQ(6, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize()) << "The updates with unknown ancestors should be in conflict"; EXPECT_EQ(4, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The updates with known ancestors should be successfully applied"; @@ -304,7 +577,7 @@ TEST_F(ApplyUpdatesCommandTest, DecryptablePassword) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "No update should be in conflict because they're all decryptable"; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The updates that can be decrypted should be applied"; @@ -337,11 +610,11 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) { EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The updates that can't be decrypted should not be in regular " << "conflict"; - EXPECT_EQ(2, status->conflict_progress()->NonblockingConflictingItemsSize()) - << "The updates that can't be decrypted should be in nonblocking " + EXPECT_EQ(2, status->conflict_progress()->EncryptionConflictingItemsSize()) + << "The updates that can't be decrypted should be in encryption " << "conflict"; EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "No update that can't be decrypted should be applied"; @@ -352,11 +625,11 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The updates that can't be decrypted should not be in regular " << "conflict"; - EXPECT_EQ(1, status->conflict_progress()->NonblockingConflictingItemsSize()) - << "The updates that can't be decrypted should be in nonblocking " + EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize()) + << "The updates that can't be decrypted should be in encryption " << "conflict"; EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "No update that can't be decrypted should be applied"; @@ -412,11 +685,11 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The updates that can't be decrypted should not be in regular " << "conflict"; - EXPECT_EQ(1, status->conflict_progress()->NonblockingConflictingItemsSize()) - << "The updates that can't be decrypted should be in nonblocking " + EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize()) + << "The updates that can't be decrypted should be in encryption " << "conflict"; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The undecryptable password update shouldn't be applied"; @@ -463,7 +736,7 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The nigori update shouldn't be in conflict"; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The nigori update should be applied"; @@ -517,7 +790,7 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The nigori update shouldn't be in conflict"; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The nigori update should be applied"; @@ -607,9 +880,9 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "No updates should be in conflict"; - EXPECT_EQ(0, status->conflict_progress()->NonblockingConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) << "No updates should be in conflict"; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The nigori update should be applied"; @@ -710,11 +983,11 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) << "All updates should have been attempted"; ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize()) + EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) << "The unsynced changes don't trigger a blocking conflict with the " << "nigori update."; - EXPECT_EQ(0, status->conflict_progress()->NonblockingConflictingItemsSize()) - << "The unsynced changes don't trigger a non-blocking conflict with the " + EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) + << "The unsynced changes don't trigger an encryption conflict with the " << "nigori update."; EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) << "The nigori update should be applied"; diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc deleted file mode 100644 index 3db89f0..0000000 --- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc +++ /dev/null @@ -1,238 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "chrome/browser/sync/engine/build_and_process_conflict_sets_command.h" - -#include <set> -#include <string> -#include <sstream> -#include <vector> - -#include "base/basictypes.h" -#include "base/format_macros.h" -#include "base/location.h" -#include "chrome/browser/sync/engine/syncer_util.h" -#include "chrome/browser/sync/engine/update_applicator.h" -#include "chrome/browser/sync/sessions/sync_session.h" -#include "chrome/browser/sync/syncable/directory_manager.h" - -namespace browser_sync { - -using sessions::ConflictProgress; -using sessions::StatusController; -using sessions::SyncSession; -using sessions::UpdateProgress; -using std::set; -using std::string; -using std::vector; - -BuildAndProcessConflictSetsCommand::BuildAndProcessConflictSetsCommand() {} -BuildAndProcessConflictSetsCommand::~BuildAndProcessConflictSetsCommand() {} - -std::set<ModelSafeGroup> BuildAndProcessConflictSetsCommand::GetGroupsToChange( - const sessions::SyncSession& session) const { - return session.GetEnabledGroupsWithConflicts(); -} - -SyncerError BuildAndProcessConflictSetsCommand::ModelChangingExecuteImpl( - SyncSession* session) { - syncable::ScopedDirLookup dir(session->context()->directory_manager(), - session->context()->account_name()); - if (!dir.good()) - return DIRECTORY_LOOKUP_FAILED; - - syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir); - BuildConflictSets(&trans, - session->mutable_status_controller()->mutable_conflict_progress()); - - return SYNCER_OK; -} - -void BuildAndProcessConflictSetsCommand::BuildConflictSets( - syncable::BaseTransaction* trans, - ConflictProgress* conflict_progress) { - conflict_progress->CleanupSets(); - set<syncable::Id>::const_iterator i = - conflict_progress->ConflictingItemsBegin(); - while (i != conflict_progress->ConflictingItemsEnd()) { - syncable::Entry entry(trans, syncable::GET_BY_ID, *i); - if (!entry.good() || - (!entry.Get(syncable::IS_UNSYNCED) && - !entry.Get(syncable::IS_UNAPPLIED_UPDATE))) { - // This can happen very rarely. It means we had a simply conflicting item - // that randomly committed; its ID could have changed during the commit. - // We drop the entry as it's no longer conflicting. - conflict_progress->EraseConflictingItemById(*(i++)); - continue; - } - if (entry.ExistsOnClientBecauseNameIsNonEmpty() && - (entry.Get(syncable::IS_DEL) || entry.Get(syncable::SERVER_IS_DEL))) { - // If we're deleted on client or server we can't be in a complex set. - ++i; - continue; - } - bool new_parent = - entry.Get(syncable::PARENT_ID) != entry.Get(syncable::SERVER_PARENT_ID); - if (new_parent) - MergeSetsForIntroducedLoops(trans, &entry, conflict_progress); - MergeSetsForNonEmptyDirectories(trans, &entry, conflict_progress); - ++i; - } -} - -void BuildAndProcessConflictSetsCommand::MergeSetsForIntroducedLoops( - syncable::BaseTransaction* trans, syncable::Entry* entry, - ConflictProgress* conflict_progress) { - // This code crawls up from the item in question until it gets to the root - // or itself. If it gets to the root it does nothing. If it finds a loop all - // moved unsynced entries in the list of crawled entries have their sets - // merged with the entry. - // TODO(sync): Build test cases to cover this function when the argument list - // has settled. - syncable::Id parent_id = entry->Get(syncable::SERVER_PARENT_ID); - syncable::Entry parent(trans, syncable::GET_BY_ID, parent_id); - if (!parent.good()) { - return; - } - // Don't check for loop if the server parent is deleted. - if (parent.Get(syncable::IS_DEL)) - return; - vector<syncable::Id> conflicting_entries; - while (!parent_id.IsRoot()) { - syncable::Entry parent(trans, syncable::GET_BY_ID, parent_id); - if (!parent.good()) { - DVLOG(1) << "Bad parent in loop check, skipping. Bad parent id: " - << parent_id << " entry: " << *entry; - return; - } - if (parent.Get(syncable::IS_UNSYNCED) && - entry->Get(syncable::PARENT_ID) != - entry->Get(syncable::SERVER_PARENT_ID)) - conflicting_entries.push_back(parent_id); - parent_id = parent.Get(syncable::PARENT_ID); - if (parent_id == entry->Get(syncable::ID)) - break; - } - if (parent_id.IsRoot()) - return; - for (size_t i = 0; i < conflicting_entries.size(); i++) { - conflict_progress->MergeSets(entry->Get(syncable::ID), - conflicting_entries[i]); - } -} - -namespace { - -class ServerDeletedPathChecker { - public: - static bool CausingConflict(const syncable::Entry& e, - const syncable::Entry& log_entry) { - CHECK(e.good()) << "Missing parent in path of: " << log_entry; - if (e.Get(syncable::IS_UNAPPLIED_UPDATE) && - e.Get(syncable::SERVER_IS_DEL)) { - CHECK(!e.Get(syncable::IS_DEL)) << " Inconsistency in local tree. " - "syncable::Entry: " << e << " Leaf: " << log_entry; - return true; - } else { - CHECK(!e.Get(syncable::IS_DEL)) << " Deleted entry has children. " - "syncable::Entry: " << e << " Leaf: " << log_entry; - return false; - } - } - - // returns 0 if we should stop investigating the path. - static syncable::Id GetAndExamineParent(syncable::BaseTransaction* trans, - const syncable::Id& id, - const syncable::Id& check_id, - const syncable::Entry& log_entry) { - syncable::Entry parent(trans, syncable::GET_BY_ID, id); - CHECK(parent.good()) << "Tree inconsitency, missing id" << id << " " - << log_entry; - syncable::Id parent_id = parent.Get(syncable::PARENT_ID); - CHECK(parent_id != check_id) << "Loop in dir tree! " - << log_entry << " " << parent; - return parent_id; - } -}; - -class LocallyDeletedPathChecker { - public: - static bool CausingConflict(const syncable::Entry& e, - const syncable::Entry& log_entry) { - return e.good() && e.Get(syncable::IS_DEL) && e.Get(syncable::IS_UNSYNCED); - } - - // returns 0 if we should stop investigating the path. - static syncable::Id GetAndExamineParent(syncable::BaseTransaction* trans, - const syncable::Id& id, - const syncable::Id& check_id, - const syncable::Entry& log_entry) { - syncable::Entry parent(trans, syncable::GET_BY_ID, id); - if (!parent.good()) - return syncable::GetNullId(); - syncable::Id parent_id = parent.Get(syncable::PARENT_ID); - if (parent_id == check_id) - return syncable::GetNullId(); - return parent_id; - } -}; - -template <typename Checker> -void CrawlDeletedTreeMergingSets(syncable::BaseTransaction* trans, - const syncable::Entry& entry, - ConflictProgress* conflict_progress, - Checker checker) { - syncable::Id parent_id = entry.Get(syncable::PARENT_ID); - syncable::Id double_step_parent_id = parent_id; - // This block builds sets where we've got an entry in a directory the server - // wants to delete. - // - // Here we're walking up the tree to find all entries that the pass checks - // deleted. We can be extremely strict here as anything unexpected means - // invariants in the local hierarchy have been broken. - while (!parent_id.IsRoot()) { - if (!double_step_parent_id.IsRoot()) { - // Checks to ensure we don't loop. - double_step_parent_id = checker.GetAndExamineParent( - trans, double_step_parent_id, parent_id, entry); - double_step_parent_id = checker.GetAndExamineParent( - trans, double_step_parent_id, parent_id, entry); - } - syncable::Entry parent(trans, syncable::GET_BY_ID, parent_id); - if (checker.CausingConflict(parent, entry)) { - conflict_progress->MergeSets(entry.Get(syncable::ID), - parent.Get(syncable::ID)); - } else { - break; - } - parent_id = parent.Get(syncable::PARENT_ID); - } -} - -} // namespace - -void BuildAndProcessConflictSetsCommand::MergeSetsForNonEmptyDirectories( - syncable::BaseTransaction* trans, syncable::Entry* entry, - ConflictProgress* conflict_progress) { - if (entry->Get(syncable::IS_UNSYNCED) && !entry->Get(syncable::IS_DEL)) { - ServerDeletedPathChecker checker; - CrawlDeletedTreeMergingSets(trans, *entry, conflict_progress, checker); - } - if (entry->Get(syncable::IS_UNAPPLIED_UPDATE) && - !entry->Get(syncable::SERVER_IS_DEL)) { - syncable::Entry parent(trans, syncable::GET_BY_ID, - entry->Get(syncable::SERVER_PARENT_ID)); - syncable::Id parent_id = entry->Get(syncable::SERVER_PARENT_ID); - if (!parent.good()) - return; - LocallyDeletedPathChecker checker; - if (!checker.CausingConflict(parent, *entry)) - return; - conflict_progress->MergeSets(entry->Get(syncable::ID), - parent.Get(syncable::ID)); - CrawlDeletedTreeMergingSets(trans, parent, conflict_progress, checker); - } -} - -} // namespace browser_sync diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h deleted file mode 100644 index 99f130d..0000000 --- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CHROME_BROWSER_SYNC_ENGINE_BUILD_AND_PROCESS_CONFLICT_SETS_COMMAND_H_ -#define CHROME_BROWSER_SYNC_ENGINE_BUILD_AND_PROCESS_CONFLICT_SETS_COMMAND_H_ -#pragma once - -#include <vector> - -#include "base/basictypes.h" -#include "base/compiler_specific.h" -#include "chrome/browser/sync/engine/model_changing_syncer_command.h" -#include "chrome/browser/sync/engine/model_safe_worker.h" - -namespace syncable { -class BaseTransaction; -class Entry; -class Id; -class WriteTransaction; -} // namespace syncable - -namespace browser_sync { - -class ConflictResolver; -class Cryptographer; - -namespace sessions { -class ConflictProgress; -class StatusController; -} - -class BuildAndProcessConflictSetsCommand : public ModelChangingSyncerCommand { - public: - BuildAndProcessConflictSetsCommand(); - virtual ~BuildAndProcessConflictSetsCommand(); - - protected: - // ModelChangingSyncerCommand implementation. - virtual std::set<ModelSafeGroup> GetGroupsToChange( - const sessions::SyncSession& session) const OVERRIDE; - virtual SyncerError ModelChangingExecuteImpl( - sessions::SyncSession* session) OVERRIDE; - - private: - void BuildConflictSets(syncable::BaseTransaction* trans, - sessions::ConflictProgress* conflict_progress); - - void MergeSetsForIntroducedLoops(syncable::BaseTransaction* trans, - syncable::Entry* entry, - sessions::ConflictProgress* conflict_progress); - void MergeSetsForNonEmptyDirectories(syncable::BaseTransaction* trans, - syncable::Entry* entry, - sessions::ConflictProgress* conflict_progress); - void MergeSetsForPositionUpdate(syncable::BaseTransaction* trans, - syncable::Entry* entry, - sessions::ConflictProgress* conflict_progress); - - DISALLOW_COPY_AND_ASSIGN(BuildAndProcessConflictSetsCommand); -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_ENGINE_BUILD_AND_PROCESS_CONFLICT_SETS_COMMAND_H_ diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command_unittest.cc b/chrome/browser/sync/engine/build_and_process_conflict_sets_command_unittest.cc deleted file mode 100644 index 3152484..0000000 --- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command_unittest.cc +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/basictypes.h" -#include "base/memory/ref_counted.h" -#include "chrome/browser/sync/engine/build_and_process_conflict_sets_command.h" -#include "chrome/browser/sync/sessions/session_state.h" -#include "chrome/browser/sync/sessions/sync_session.h" -#include "chrome/browser/sync/syncable/model_type.h" -#include "chrome/browser/sync/syncable/syncable_id.h" -#include "chrome/browser/sync/test/engine/fake_model_worker.h" -#include "chrome/browser/sync/test/engine/syncer_command_test.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace browser_sync { - -namespace { - -class BuildAndProcessConflictSetsCommandTest : public SyncerCommandTest { - protected: - BuildAndProcessConflictSetsCommandTest() {} - virtual ~BuildAndProcessConflictSetsCommandTest() {} - - virtual void SetUp() { - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_UI))); - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD))); - (*mutable_routing_info())[syncable::BOOKMARKS] = GROUP_UI; - (*mutable_routing_info())[syncable::PASSWORDS] = GROUP_PASSWORD; - SyncerCommandTest::SetUp(); - } - - BuildAndProcessConflictSetsCommand command_; - - private: - DISALLOW_COPY_AND_ASSIGN(BuildAndProcessConflictSetsCommandTest); -}; - -TEST_F(BuildAndProcessConflictSetsCommandTest, GetGroupsToChange) { - ExpectNoGroupsToChange(command_); - // Put GROUP_UI in conflict. - session()->mutable_status_controller()-> - GetUnrestrictedMutableConflictProgressForTest(GROUP_UI)-> - AddConflictingItemById(syncable::Id()); - ExpectGroupToChange(command_, GROUP_UI); -} - -} // namespace - -} // namespace browser_sync diff --git a/chrome/browser/sync/engine/conflict_resolver.cc b/chrome/browser/sync/engine/conflict_resolver.cc index a262ff7..60651e6 100644 --- a/chrome/browser/sync/engine/conflict_resolver.cc +++ b/chrome/browser/sync/engine/conflict_resolver.cc @@ -77,21 +77,13 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, MutableEntry entry(trans, syncable::GET_BY_ID, id); // Must be good as the entry won't have been cleaned up. CHECK(entry.good()); - // If an update fails, locally we have to be in a set or unsynced. We're not - // in a set here, so we must be unsynced. - if (!entry.Get(syncable::IS_UNSYNCED)) { - return NO_SYNC_PROGRESS; - } - if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE)) { - if (!entry.Get(syncable::PARENT_ID).ServerKnows()) { - DVLOG(1) << "Item conflicting because its parent not yet committed. Id: " - << id; - } else { - DVLOG(1) << "No set for conflicting entry id " << id << ". There should " - << "be an update/commit that will fix this soon. This message " - << "should not repeat."; - } + // This function can only resolve simple conflicts. Simple conflicts have + // both IS_UNSYNCED and IS_UNAPPLIED_UDPATE set. + if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE) || + !entry.Get(syncable::IS_UNSYNCED)) { + // This is very unusual, but it can happen in tests. We may be able to + // assert NOTREACHED() here when those tests are updated. return NO_SYNC_PROGRESS; } @@ -313,7 +305,10 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, entry.Put(syncable::BASE_SERVER_SPECIFICS, sync_pb::EntitySpecifics()); return SYNC_PROGRESS; } else { // SERVER_IS_DEL is true - // If a server deleted folder has local contents we should be in a set. + // If a server deleted folder has local contents it should be a hierarchy + // conflict. Hierarchy conflicts should not be processed by this function. + // We could end up here if a change was made since we last tried to detect + // conflicts, which was during update application. if (entry.Get(syncable::IS_DIR)) { Directory::ChildHandles children; trans->directory()->GetChildHandlesById(trans, @@ -321,7 +316,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, &children); if (0 != children.size()) { DVLOG(1) << "Entry is a server deleted directory with local contents, " - << "should be in a set. (race condition)."; + << "should be a hierarchy conflict. (race condition)."; return NO_SYNC_PROGRESS; } } @@ -361,79 +356,53 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, } } -bool ConflictResolver::ResolveSimpleConflicts( - syncable::WriteTransaction* trans, - const Cryptographer* cryptographer, - const ConflictProgress& progress, - sessions::StatusController* status) { +bool ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans, + const Cryptographer* cryptographer, + const ConflictProgress& progress, + sessions::StatusController* status) { bool forward_progress = false; - // First iterate over simple conflict items (those that belong to no set). + // Iterate over simple conflict items. set<Id>::const_iterator conflicting_item_it; set<Id> processed_items; - for (conflicting_item_it = progress.ConflictingItemsBegin(); - conflicting_item_it != progress.ConflictingItemsEnd(); + for (conflicting_item_it = progress.SimpleConflictingItemsBegin(); + conflicting_item_it != progress.SimpleConflictingItemsEnd(); ++conflicting_item_it) { Id id = *conflicting_item_it; if (processed_items.count(id) > 0) continue; - map<Id, ConflictSet*>::const_iterator item_set_it = - progress.IdToConflictSetFind(id); - if (item_set_it == progress.IdToConflictSetEnd() || - 0 == item_set_it->second) { - // We have a simple conflict. In order check if positions have changed, - // we need to process conflicting predecessors before successors. Traverse - // backwards through all continuous conflicting predecessors, building a - // stack of items to resolve in predecessor->successor order, then process - // each item individually. - list<Id> predecessors; - Id prev_id = id; - do { - predecessors.push_back(prev_id); - Entry entry(trans, syncable::GET_BY_ID, prev_id); - // Any entry in conflict must be valid. - CHECK(entry.good()); - Id new_prev_id = entry.Get(syncable::PREV_ID); - if (new_prev_id == prev_id) + + // We have a simple conflict. In order check if positions have changed, + // we need to process conflicting predecessors before successors. Traverse + // backwards through all continuous conflicting predecessors, building a + // stack of items to resolve in predecessor->successor order, then process + // each item individually. + list<Id> predecessors; + Id prev_id = id; + do { + predecessors.push_back(prev_id); + Entry entry(trans, syncable::GET_BY_ID, prev_id); + // Any entry in conflict must be valid. + CHECK(entry.good()); + Id new_prev_id = entry.Get(syncable::PREV_ID); + if (new_prev_id == prev_id) + break; + prev_id = new_prev_id; + } while (processed_items.count(prev_id) == 0 && + progress.HasSimpleConflictItem(prev_id)); // Excludes root. + while (!predecessors.empty()) { + id = predecessors.back(); + predecessors.pop_back(); + switch (ProcessSimpleConflict(trans, id, cryptographer, status)) { + case NO_SYNC_PROGRESS: + break; + case SYNC_PROGRESS: + forward_progress = true; break; - prev_id = new_prev_id; - } while (processed_items.count(prev_id) == 0 && - progress.HasSimpleConflictItem(prev_id)); // Excludes root. - while (!predecessors.empty()) { - id = predecessors.back(); - predecessors.pop_back(); - switch (ProcessSimpleConflict(trans, id, cryptographer, status)) { - case NO_SYNC_PROGRESS: - break; - case SYNC_PROGRESS: - forward_progress = true; - break; - } - processed_items.insert(id); } + processed_items.insert(id); } } return forward_progress; } -bool ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans, - const Cryptographer* cryptographer, - const ConflictProgress& progress, - sessions::StatusController* status) { - // TODO(rlarocque): A good amount of code related to the resolution of - // conflict sets has been deleted here. This was done because the code had - // not been used in years. An unrelated bug fix accidentally re-enabled the - // code, forcing us to make a decision about what we should do with the code. - // We decided to do the safe thing and delete it for now. This restores the - // behaviour we've relied on for quite some time. We should think about what - // that code was trying to do and consider re-enabling parts of it. - - if (progress.ConflictSetsSize() > 0) { - DVLOG(1) << "Detected " << progress.IdToConflictSetSize() - << " non-simple conflicting entries in " << progress.ConflictSetsSize() - << " unprocessed conflict sets."; - } - - return ResolveSimpleConflicts(trans, cryptographer, progress, status); -} - } // namespace browser_sync diff --git a/chrome/browser/sync/engine/process_commit_response_command.cc b/chrome/browser/sync/engine/process_commit_response_command.cc index 536db0e..6135ef6 100644 --- a/chrome/browser/sync/engine/process_commit_response_command.cc +++ b/chrome/browser/sync/engine/process_commit_response_command.cc @@ -165,7 +165,7 @@ SyncerError ProcessCommitResponseCommand::ProcessCommitResponse( case CommitResponse::CONFLICT: ++conflicting_commits; // Only server CONFLICT responses will activate conflict resolution. - conflict_progress->AddConflictingItemById( + conflict_progress->AddServerConflictingItemById( status->GetCommitIdAt(proj[i])); break; case CommitResponse::SUCCESS: diff --git a/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc b/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc index a36d5e2..7f7ddfd 100644 --- a/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc +++ b/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -42,7 +42,7 @@ TEST_F(ResolveConflictsCommandTest, GetGroupsToChange) { // Put GROUP_PASSWORD in conflict. session()->mutable_status_controller()-> GetUnrestrictedMutableConflictProgressForTest(GROUP_PASSWORD)-> - AddConflictingItemById(syncable::Id()); + AddSimpleConflictingItemById(syncable::Id()); ExpectGroupToChange(command_, GROUP_PASSWORD); } diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc index dbab2fb..f64b030 100644 --- a/chrome/browser/sync/engine/syncer.cc +++ b/chrome/browser/sync/engine/syncer.cc @@ -10,7 +10,6 @@ #include "base/message_loop.h" #include "base/time.h" #include "chrome/browser/sync/engine/apply_updates_command.h" -#include "chrome/browser/sync/engine/build_and_process_conflict_sets_command.h" #include "chrome/browser/sync/engine/build_commit_command.h" #include "chrome/browser/sync/engine/cleanup_disabled_types_command.h" #include "chrome/browser/sync/engine/clear_data_command.h" @@ -70,7 +69,6 @@ const char* SyncerStepToString(const SyncerStep step) ENUM_CASE(BUILD_COMMIT_REQUEST); ENUM_CASE(POST_COMMIT_MESSAGE); ENUM_CASE(PROCESS_COMMIT_RESPONSE); - ENUM_CASE(BUILD_AND_PROCESS_CONFLICT_SETS); ENUM_CASE(RESOLVE_CONFLICTS); ENUM_CASE(APPLY_UPDATES_TO_RESOLVE_CONFLICTS); ENUM_CASE(CLEAR_PRIVATE_DATA); @@ -216,7 +214,7 @@ void Syncer::SyncShare(sessions::SyncSession* session, next_step = POST_COMMIT_MESSAGE; } else { - next_step = BUILD_AND_PROCESS_CONFLICT_SETS; + next_step = RESOLVE_CONFLICTS; } break; @@ -234,12 +232,6 @@ void Syncer::SyncShare(sessions::SyncSession* session, session->mutable_status_controller()-> set_last_process_commit_response_result( process_response_command.Execute(session)); - next_step = BUILD_AND_PROCESS_CONFLICT_SETS; - break; - } - case BUILD_AND_PROCESS_CONFLICT_SETS: { - BuildAndProcessConflictSetsCommand build_process_conflict_sets; - build_process_conflict_sets.Execute(session); next_step = RESOLVE_CONFLICTS; break; } @@ -249,7 +241,7 @@ void Syncer::SyncShare(sessions::SyncSession* session, ResolveConflictsCommand resolve_conflicts_command; resolve_conflicts_command.Execute(session); - // Has ConflictingUpdates includes both blocking and non-blocking + // Has ConflictingUpdates includes both resolvable and unresolvable // conflicts. If we have either, we want to attempt to reapply. if (status->HasConflictingUpdates()) next_step = APPLY_UPDATES_TO_RESOLVE_CONFLICTS; @@ -263,13 +255,12 @@ void Syncer::SyncShare(sessions::SyncSession* session, ApplyUpdatesCommand apply_updates; // We only care to resolve conflicts again if we made progress on the - // blocking conflicts. Whether or not we made progress on the - // non-blocking doesn't matter. + // simple conflicts. int before_blocking_conflicting_updates = - status->TotalNumBlockingConflictingItems(); + status->TotalNumSimpleConflictingItems(); apply_updates.Execute(session); int after_blocking_conflicting_updates = - status->TotalNumBlockingConflictingItems(); + status->TotalNumSimpleConflictingItems(); // If the following call sets the conflicts_resolved value to true, // SyncSession::HasMoreToSync() will send us into another sync cycle // after this one completes. diff --git a/chrome/browser/sync/engine/syncer.h b/chrome/browser/sync/engine/syncer.h index 9a17025..f9f3dbe 100644 --- a/chrome/browser/sync/engine/syncer.h +++ b/chrome/browser/sync/engine/syncer.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -40,7 +40,6 @@ enum SyncerStep { BUILD_COMMIT_REQUEST, POST_COMMIT_MESSAGE, PROCESS_COMMIT_RESPONSE, - BUILD_AND_PROCESS_CONFLICT_SETS, RESOLVE_CONFLICTS, APPLY_UPDATES_TO_RESOLVE_CONFLICTS, CLEAR_PRIVATE_DATA, // TODO(tim): Rename 'private' to 'user'. diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h index e4fde71..4948884 100644 --- a/chrome/browser/sync/engine/syncer_types.h +++ b/chrome/browser/sync/engine/syncer_types.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -30,15 +30,36 @@ enum UpdateAttemptResponse { // Update was applied or safely ignored. SUCCESS, - // Conflicts with the local data representation. This can also mean that the - // entry doesn't currently make sense if we applied it. - CONFLICT, + // The conditions described by the following enum values are not mutually + // exclusive. The list has been ordered according to priority in the case of + // overlap, with highest priority first. + // + // For example, in the case where an item had both the IS_UNSYCNED and + // IS_UNAPPLIED_UPDATE flags set (CONFLICT_SIMPLE), and a SERVER_PARENT_ID + // that, if applied, would cause a directory loop (CONFLICT_HIERARCHY), and + // specifics that we can't decrypt right now (CONFLICT_ENCRYPTION), the + // UpdateApplicator would return CONFLICT_ENCRYPTION when attempting to + // process the item. + // + // We do not attempt to resolve CONFLICT_HIERARCHY or CONFLICT_ENCRYPTION + // items. We will leave these updates unapplied and wait for the server + // to send us newer updates that will resolve the conflict. // We were unable to decrypt/encrypt this server data. As such, we can't make // forward progress on this node, but because the passphrase may not arrive // until later we don't want to get the syncer stuck. See UpdateApplicator // for how this is handled. - CONFLICT_ENCRYPTION + CONFLICT_ENCRYPTION, + + // These are some updates that, if applied, would violate the tree's + // invariants. Examples of this include the server adding children to locally + // deleted directories and directory moves that would create loops. + CONFLICT_HIERARCHY, + + // This indicates that item was modified both remotely (IS_UNAPPLIED_UPDATE) + // and locally (IS_UNSYNCED). We use the ConflictResolver to decide which of + // the changes should take priority, or if we can possibly merge the data. + CONFLICT_SIMPLE }; enum ServerUpdateProcessingResult { @@ -132,14 +153,6 @@ class SyncEngineEventListener { virtual ~SyncEngineEventListener() {} }; -// This struct is passed between parts of the syncer during the processing of -// one sync loop. It lives on the stack. We don't expose the number of -// conflicts during SyncShare as the conflicts may be solved automatically -// by the conflict resolver. -typedef std::vector<syncable::Id> ConflictSet; - -typedef std::map<syncable::Id, ConflictSet*> IdToConflictSetMap; - } // namespace browser_sync #endif // CHROME_BROWSER_SYNC_ENGINE_SYNCER_TYPES_H_ diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc index 098c1b4..8933ba8 100644 --- a/chrome/browser/sync/engine/syncer_unittest.cc +++ b/chrome/browser/sync/engine/syncer_unittest.cc @@ -1728,7 +1728,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) { { sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(1, status->conflict_progress()->ConflictingItemsSize()); + EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); } // These entries will be used in the second set of updates. @@ -1746,7 +1746,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) { { sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(3, status->conflict_progress()->ConflictingItemsSize()); + EXPECT_EQ(3, status->conflict_progress()->HierarchyConflictingItemsSize()); } { @@ -1840,7 +1840,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) { { sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(4, status->conflict_progress()->ConflictingItemsSize()); + EXPECT_EQ(4, status->conflict_progress()->HierarchyConflictingItemsSize()); } } @@ -2526,7 +2526,7 @@ TEST_F(SyncerTest, UnappliedUpdateDuringCommit) { } syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END); syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END); - EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems()); + EXPECT_EQ(1, session_->status_controller().TotalNumConflictingItems()); saw_syncer_event_ = false; } @@ -2939,31 +2939,6 @@ TEST_F(SyncerTest, SiblingDirectoriesBecomeCircular) { } } -TEST_F(SyncerTest, ConflictSetClassificationError) { - // This code used to cause a CHECK failure because we incorrectly thought - // a set was only unapplied updates. - ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); - CHECK(dir.good()); - mock_server_->AddUpdateDirectory(1, 0, "A", 10, 10); - mock_server_->AddUpdateDirectory(2, 0, "B", 10, 10); - mock_server_->set_conflict_all_commits(true); - SyncShareAsDelegate(); - { - WriteTransaction wtrans(FROM_HERE, UNITTEST, dir); - MutableEntry A(&wtrans, GET_BY_ID, ids_.FromNumber(1)); - ASSERT_TRUE(A.good()); - A.Put(IS_UNSYNCED, true); - A.Put(IS_UNAPPLIED_UPDATE, true); - A.Put(SERVER_NON_UNIQUE_NAME, "B"); - MutableEntry B(&wtrans, GET_BY_ID, ids_.FromNumber(2)); - ASSERT_TRUE(B.good()); - B.Put(IS_UNAPPLIED_UPDATE, true); - B.Put(SERVER_NON_UNIQUE_NAME, "A"); - } - SyncShareAsDelegate(); - saw_syncer_event_ = false; -} - TEST_F(SyncerTest, SwapEntryNames) { // Simple transaction test. ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); @@ -3168,39 +3143,6 @@ TEST_F(SyncerTest, UpdateFlipsTheFolderBit) { } } -TEST(SyncerSyncProcessState, MergeSetsTest) { - TestIdFactory id_factory; - syncable::Id id[7]; - for (int i = 1; i < 7; i++) { - id[i] = id_factory.NewServerId(); - } - bool is_dirty = false; - ConflictProgress c(&is_dirty); - c.MergeSets(id[1], id[2]); - c.MergeSets(id[2], id[3]); - c.MergeSets(id[4], id[5]); - c.MergeSets(id[5], id[6]); - EXPECT_EQ(6u, c.IdToConflictSetSize()); - EXPECT_FALSE(is_dirty); - for (int i = 1; i < 7; i++) { - EXPECT_TRUE(NULL != c.IdToConflictSetGet(id[i])); - EXPECT_TRUE(c.IdToConflictSetGet(id[(i & ~3) + 1]) == - c.IdToConflictSetGet(id[i])); - } - c.MergeSets(id[1], id[6]); - for (int i = 1; i < 7; i++) { - EXPECT_TRUE(NULL != c.IdToConflictSetGet(id[i])); - EXPECT_TRUE(c.IdToConflictSetGet(id[1]) == c.IdToConflictSetGet(id[i])); - } - - // Check dupes don't cause double sets. - ConflictProgress identical_set(&is_dirty); - identical_set.MergeSets(id[1], id[1]); - EXPECT_TRUE(identical_set.IdToConflictSetSize() == 1); - EXPECT_TRUE(identical_set.IdToConflictSetGet(id[1])->size() == 1); - EXPECT_FALSE(is_dirty); -} - // Bug Synopsis: // Merge conflict resolution will merge a new local entry with another entry // that needs updates, resulting in CHECK. @@ -3453,31 +3395,6 @@ TEST_F(SyncerTest, DirectoryCommitTest) { } } -TEST_F(SyncerTest, ConflictSetSizeReducedToOne) { - ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); - CHECK(dir.good()); - - syncable::Id in_root_id = ids_.NewServerId(); - - mock_server_->AddUpdateBookmark(in_root_id, TestIdFactory::root(), - "in_root", 1, 1); - SyncShareAsDelegate(); - { - WriteTransaction trans(FROM_HERE, UNITTEST, dir); - MutableEntry oentry(&trans, GET_BY_ID, in_root_id); - ASSERT_TRUE(oentry.good()); - oentry.Put(NON_UNIQUE_NAME, "old_in_root"); - WriteTestDataToEntry(&trans, &oentry); - MutableEntry entry(&trans, CREATE, trans.root_id(), "in_root"); - ASSERT_TRUE(entry.good()); - WriteTestDataToEntry(&trans, &entry); - } - mock_server_->set_conflict_all_commits(true); - // This SyncShare call used to result in a CHECK failure. - SyncShareAsDelegate(); - saw_syncer_event_ = false; -} - TEST_F(SyncerTest, TestClientCommand) { ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); CHECK(dir.good()); diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc index bf1456e..b3557f4 100644 --- a/chrome/browser/sync/engine/syncer_util.cc +++ b/chrome/browser/sync/engine/syncer_util.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -311,7 +311,7 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry( // passphrase may not arrive within this GetUpdates, we can't just return // conflict, else we try to perform normal conflict resolution prematurely or // the syncer may get stuck. As such, we return CONFLICT_ENCRYPTION, which is - // treated as a non-blocking conflict. See the description in syncer_types.h. + // treated as an unresolvable conflict. See the description in syncer_types.h. // This prevents any unsynced changes from commiting and postpones conflict // resolution until all data can be decrypted. if (specifics.has_encrypted() && @@ -333,11 +333,6 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry( } } - if (entry->Get(IS_UNSYNCED)) { - DVLOG(1) << "Skipping update, returning conflict for: " << id - << " ; it's unsynced."; - return CONFLICT; - } if (!entry->Get(SERVER_IS_DEL)) { syncable::Id new_parent = entry->Get(SERVER_PARENT_ID); Entry parent(trans, GET_BY_ID, new_parent); @@ -348,13 +343,13 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry( // different ways we deal with it once here to reduce the amount of code and // potential errors. if (!parent.good() || parent.Get(IS_DEL) || !parent.Get(IS_DIR)) { - return CONFLICT; + return CONFLICT_HIERARCHY; } if (entry->Get(PARENT_ID) != new_parent) { if (!entry->Get(IS_DEL) && !IsLegalNewParent(trans, id, new_parent)) { DVLOG(1) << "Not updating item " << id << ", illegal new parent (would cause loop)."; - return CONFLICT; + return CONFLICT_HIERARCHY; } } } else if (entry->Get(IS_DIR)) { @@ -364,10 +359,16 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry( // If we have still-existing children, then we need to deal with // them before we can process this change. DVLOG(1) << "Not deleting directory; it's not empty " << *entry; - return CONFLICT; + return CONFLICT_HIERARCHY; } } + if (entry->Get(IS_UNSYNCED)) { + DVLOG(1) << "Skipping update, returning conflict for: " << id + << " ; it's unsynced."; + return CONFLICT_SIMPLE; + } + if (specifics.has_encrypted()) { DVLOG(2) << "Received a decryptable " << syncable::ModelTypeToString(entry->GetServerModelType()) diff --git a/chrome/browser/sync/engine/update_applicator.cc b/chrome/browser/sync/engine/update_applicator.cc index e44083c..0c8122a 100644 --- a/chrome/browser/sync/engine/update_applicator.cc +++ b/chrome/browser/sync/engine/update_applicator.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -71,14 +71,18 @@ bool UpdateApplicator::AttemptOneApplication( progress_ = true; application_results_.AddSuccess(entry.Get(syncable::ID)); break; - case CONFLICT: + case CONFLICT_SIMPLE: pointer_++; - application_results_.AddConflict(entry.Get(syncable::ID)); + application_results_.AddSimpleConflict(entry.Get(syncable::ID)); break; case CONFLICT_ENCRYPTION: pointer_++; application_results_.AddEncryptionConflict(entry.Get(syncable::ID)); break; + case CONFLICT_HIERARCHY: + pointer_++; + application_results_.AddHierarchyConflict(entry.Get(syncable::ID)); + break; default: NOTREACHED(); break; @@ -133,7 +137,7 @@ UpdateApplicator::ResultTracker::ResultTracker(size_t num_results) { UpdateApplicator::ResultTracker::~ResultTracker() { } -void UpdateApplicator::ResultTracker::AddConflict(syncable::Id id) { +void UpdateApplicator::ResultTracker::AddSimpleConflict(syncable::Id id) { conflicting_ids_.push_back(id); } @@ -141,6 +145,10 @@ void UpdateApplicator::ResultTracker::AddEncryptionConflict(syncable::Id id) { encryption_conflict_ids_.push_back(id); } +void UpdateApplicator::ResultTracker::AddHierarchyConflict(syncable::Id id) { + hierarchy_conflict_ids_.push_back(id); +} + void UpdateApplicator::ResultTracker::AddSuccess(syncable::Id id) { successful_ids_.push_back(id); } @@ -150,19 +158,21 @@ void UpdateApplicator::ResultTracker::SaveProgress( sessions::UpdateProgress* update_progress) { vector<syncable::Id>::const_iterator i; for (i = conflicting_ids_.begin(); i != conflicting_ids_.end(); ++i) { - conflict_progress->AddConflictingItemById(*i); - update_progress->AddAppliedUpdate(CONFLICT, *i); + conflict_progress->AddSimpleConflictingItemById(*i); + update_progress->AddAppliedUpdate(CONFLICT_SIMPLE, *i); } for (i = encryption_conflict_ids_.begin(); i != encryption_conflict_ids_.end(); ++i) { - // Encryption conflicts should not put the syncer into a stuck state. We - // mark as conflict, so that we reattempt to apply updates, but add it to - // the list of nonblocking conflicts instead of normal conflicts. - conflict_progress->AddNonblockingConflictingItemById(*i); - update_progress->AddAppliedUpdate(CONFLICT, *i); + conflict_progress->AddEncryptionConflictingItemById(*i); + update_progress->AddAppliedUpdate(CONFLICT_ENCRYPTION, *i); + } + for (i = hierarchy_conflict_ids_.begin(); + i != hierarchy_conflict_ids_.end(); ++i) { + conflict_progress->AddHierarchyConflictingItemById(*i); + update_progress->AddAppliedUpdate(CONFLICT_HIERARCHY, *i); } for (i = successful_ids_.begin(); i != successful_ids_.end(); ++i) { - conflict_progress->EraseConflictingItemById(*i); + conflict_progress->EraseSimpleConflictingItemById(*i); update_progress->AddAppliedUpdate(SUCCESS, *i); } } @@ -170,6 +180,7 @@ void UpdateApplicator::ResultTracker::SaveProgress( void UpdateApplicator::ResultTracker::ClearConflicts() { conflicting_ids_.clear(); encryption_conflict_ids_.clear(); + hierarchy_conflict_ids_.clear(); } bool UpdateApplicator::ResultTracker::no_conflicts() const { diff --git a/chrome/browser/sync/engine/update_applicator.h b/chrome/browser/sync/engine/update_applicator.h index 1196fcc..bcc928d 100644 --- a/chrome/browser/sync/engine/update_applicator.h +++ b/chrome/browser/sync/engine/update_applicator.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. // @@ -57,14 +57,13 @@ class UpdateApplicator { private: // Track the status of all applications. - // We treat encryption conflicts as nonblocking conflict items when we save - // progress. class ResultTracker { public: explicit ResultTracker(size_t num_results); virtual ~ResultTracker(); - void AddConflict(syncable::Id); + void AddSimpleConflict(syncable::Id); void AddEncryptionConflict(syncable::Id); + void AddHierarchyConflict(syncable::Id); void AddSuccess(syncable::Id); void SaveProgress(sessions::ConflictProgress* conflict_progress, sessions::UpdateProgress* update_progress); @@ -77,6 +76,7 @@ class UpdateApplicator { std::vector<syncable::Id> conflicting_ids_; std::vector<syncable::Id> successful_ids_; std::vector<syncable::Id> encryption_conflict_ids_; + std::vector<syncable::Id> hierarchy_conflict_ids_; }; // If true, AttemptOneApplication will skip over |entry| and return true. diff --git a/chrome/browser/sync/internal_api/debug_info_event_listener.cc b/chrome/browser/sync/internal_api/debug_info_event_listener.cc index d783dee..8c49eb5 100644 --- a/chrome/browser/sync/internal_api/debug_info_event_listener.cc +++ b/chrome/browser/sync/internal_api/debug_info_event_listener.cc @@ -23,10 +23,11 @@ void DebugInfoEventListener::OnSyncCycleCompleted( sync_pb::SyncCycleCompletedEventInfo* sync_completed_event_info = event_info.mutable_sync_cycle_completed_event_info(); + // TODO(rlarocque): These counters are reversed. We should fix this bug. sync_completed_event_info->set_num_blocking_conflicts( snapshot->num_conflicting_updates); sync_completed_event_info->set_num_non_blocking_conflicts( - snapshot->num_blocking_conflicting_updates); + snapshot->num_simple_conflicting_updates); AddEventToQueue(event_info); } diff --git a/chrome/browser/sync/profile_sync_service_harness.cc b/chrome/browser/sync/profile_sync_service_harness.cc index 16303c6..48c2159 100644 --- a/chrome/browser/sync/profile_sync_service_harness.cc +++ b/chrome/browser/sync/profile_sync_service_harness.cc @@ -776,7 +776,7 @@ ProfileSyncService::Status ProfileSyncServiceHarness::GetStatus() { bool ProfileSyncServiceHarness::IsDataSyncedImpl( const browser_sync::sessions::SyncSessionSnapshot *snap) { return snap && - snap->num_blocking_conflicting_updates == 0 && + snap->num_simple_conflicting_updates == 0 && ServiceIsPushingChanges() && GetStatus().notifications_enabled && !service()->HasUnsyncedItems() && @@ -1017,8 +1017,8 @@ std::string ProfileSyncServiceHarness::GetClientInfoString( << snap->unsynced_count << ", num_conflicting_updates: " << snap->num_conflicting_updates - << ", num_blocking_conflicting_updates: " - << snap->num_blocking_conflicting_updates + << ", num_simple_conflicting_updates: " + << snap->num_simple_conflicting_updates << ", num_updates_downloaded : " << snap->syncer_status.num_updates_downloaded_total << ", passphrase_required_reason: " diff --git a/chrome/browser/sync/protocol/client_debug_info.proto b/chrome/browser/sync/protocol/client_debug_info.proto index 6d67b72..d58f61c 100644 --- a/chrome/browser/sync/protocol/client_debug_info.proto +++ b/chrome/browser/sync/protocol/client_debug_info.proto @@ -15,6 +15,8 @@ package sync_pb; // SYNC_CYCLE_COMPLETED is sent. message SyncCycleCompletedEventInfo { // optional bool syncer_stuck = 1; // Was always false, now obsolete. + + // TODO(rlarocque): rename these counters. optional int32 num_blocking_conflicts = 2; optional int32 num_non_blocking_conflicts = 3; } diff --git a/chrome/browser/sync/sessions/session_state.cc b/chrome/browser/sync/sessions/session_state.cc index 18f4e59..eff0a22 100644 --- a/chrome/browser/sync/sessions/session_state.cc +++ b/chrome/browser/sync/sessions/session_state.cc @@ -114,7 +114,7 @@ SyncSessionSnapshot::SyncSessionSnapshot( bool more_to_sync, bool is_silenced, int64 unsynced_count, - int num_blocking_conflicting_updates, + int num_simple_conflicting_updates, int num_conflicting_updates, bool did_commit_items, const SyncSourceInfo& source, @@ -130,7 +130,7 @@ SyncSessionSnapshot::SyncSessionSnapshot( has_more_to_sync(more_to_sync), is_silenced(is_silenced), unsynced_count(unsynced_count), - num_blocking_conflicting_updates(num_blocking_conflicting_updates), + num_simple_conflicting_updates(num_simple_conflicting_updates), num_conflicting_updates(num_conflicting_updates), did_commit_items(did_commit_items), source(source), @@ -163,8 +163,8 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const { // We don't care too much if we lose precision here, also. value->SetInteger("unsyncedCount", static_cast<int>(unsynced_count)); - value->SetInteger("numBlockingConflictingUpdates", - num_blocking_conflicting_updates); + value->SetInteger("numSimpleConflictingUpdates", + num_simple_conflicting_updates); value->SetInteger("numConflictingUpdates", num_conflicting_updates); value->SetBoolean("didCommitItems", did_commit_items); value->SetInteger("numEntries", num_entries); @@ -179,144 +179,70 @@ std::string SyncSessionSnapshot::ToString() const { return json; } -ConflictProgress::ConflictProgress(bool* dirty_flag) : dirty_(dirty_flag) {} +ConflictProgress::ConflictProgress(bool* dirty_flag) + : num_server_conflicting_items(0), num_hierarchy_conflicting_items(0), + num_encryption_conflicting_items(0), dirty_(dirty_flag) { +} ConflictProgress::~ConflictProgress() { - CleanupSets(); } bool ConflictProgress::HasSimpleConflictItem(const syncable::Id& id) const { - return conflicting_item_ids_.count(id) > 0 && - (IdToConflictSetFind(id) == IdToConflictSetEnd()); -} - -IdToConflictSetMap::const_iterator ConflictProgress::IdToConflictSetFind( - const syncable::Id& the_id) const { - return id_to_conflict_set_.find(the_id); -} - -IdToConflictSetMap::const_iterator -ConflictProgress::IdToConflictSetBegin() const { - return id_to_conflict_set_.begin(); -} - -IdToConflictSetMap::const_iterator -ConflictProgress::IdToConflictSetEnd() const { - return id_to_conflict_set_.end(); -} - -IdToConflictSetMap::size_type ConflictProgress::IdToConflictSetSize() const { - return id_to_conflict_set_.size(); -} - -const ConflictSet* ConflictProgress::IdToConflictSetGet( - const syncable::Id& the_id) { - return id_to_conflict_set_[the_id]; -} - -std::set<ConflictSet*>::const_iterator -ConflictProgress::ConflictSetsBegin() const { - return conflict_sets_.begin(); -} - -std::set<ConflictSet*>::const_iterator -ConflictProgress::ConflictSetsEnd() const { - return conflict_sets_.end(); -} - -std::set<ConflictSet*>::size_type -ConflictProgress::ConflictSetsSize() const { - return conflict_sets_.size(); + return simple_conflicting_item_ids_.count(id) > 0; } std::set<syncable::Id>::const_iterator -ConflictProgress::ConflictingItemsBegin() const { - return conflicting_item_ids_.begin(); +ConflictProgress::SimpleConflictingItemsBegin() const { + return simple_conflicting_item_ids_.begin(); } std::set<syncable::Id>::const_iterator -ConflictProgress::ConflictingItemsEnd() const { - return conflicting_item_ids_.end(); +ConflictProgress::SimpleConflictingItemsEnd() const { + return simple_conflicting_item_ids_.end(); } -void ConflictProgress::AddConflictingItemById(const syncable::Id& the_id) { +void ConflictProgress::AddSimpleConflictingItemById( + const syncable::Id& the_id) { std::pair<std::set<syncable::Id>::iterator, bool> ret = - conflicting_item_ids_.insert(the_id); + simple_conflicting_item_ids_.insert(the_id); if (ret.second) *dirty_ = true; } -void ConflictProgress::EraseConflictingItemById(const syncable::Id& the_id) { - int items_erased = conflicting_item_ids_.erase(the_id); +void ConflictProgress::EraseSimpleConflictingItemById( + const syncable::Id& the_id) { + int items_erased = simple_conflicting_item_ids_.erase(the_id); if (items_erased != 0) *dirty_ = true; } -void ConflictProgress::AddNonblockingConflictingItemById( +void ConflictProgress::AddEncryptionConflictingItemById( const syncable::Id& the_id) { std::pair<std::set<syncable::Id>::iterator, bool> ret = - nonblocking_conflicting_item_ids_.insert(the_id); - if (ret.second) + unresolvable_conflicting_item_ids_.insert(the_id); + if (ret.second) { + num_encryption_conflicting_items++; *dirty_ = true; + } } -void ConflictProgress::EraseNonblockingConflictingItemById( +void ConflictProgress::AddHierarchyConflictingItemById( const syncable::Id& the_id) { - int items_erased = nonblocking_conflicting_item_ids_.erase(the_id); - if (items_erased != 0) + std::pair<std::set<syncable::Id>::iterator, bool> ret = + unresolvable_conflicting_item_ids_.insert(the_id); + if (ret.second) { + num_hierarchy_conflicting_items++; *dirty_ = true; -} - -void ConflictProgress::MergeSets(const syncable::Id& id1, - const syncable::Id& id2) { - // There are no single item sets, we just leave those entries == 0 - vector<syncable::Id>* set1 = id_to_conflict_set_[id1]; - vector<syncable::Id>* set2 = id_to_conflict_set_[id2]; - vector<syncable::Id>* rv = 0; - if (0 == set1 && 0 == set2) { - // Neither item currently has a set so we build one. - rv = new vector<syncable::Id>(); - rv->push_back(id1); - if (id1 != id2) { - rv->push_back(id2); - } else { - LOG(WARNING) << "[BUG] Attempting to merge two identical conflict ids."; - } - conflict_sets_.insert(rv); - } else if (0 == set1) { - // Add the item to the existing set. - rv = set2; - rv->push_back(id1); - } else if (0 == set2) { - // Add the item to the existing set. - rv = set1; - rv->push_back(id2); - } else if (set1 == set2) { - // It's the same set already. - return; - } else { - // Merge the two sets. - rv = set1; - // Point all the second sets id's back to the first. - vector<syncable::Id>::iterator i; - for (i = set2->begin() ; i != set2->end() ; ++i) { - id_to_conflict_set_[*i] = rv; - } - // Copy the second set to the first. - rv->insert(rv->end(), set2->begin(), set2->end()); - conflict_sets_.erase(set2); - delete set2; } - id_to_conflict_set_[id1] = id_to_conflict_set_[id2] = rv; } -void ConflictProgress::CleanupSets() { - // Clean up all the sets. - set<ConflictSet*>::iterator i; - for (i = conflict_sets_.begin(); i != conflict_sets_.end(); i++) { - delete *i; +void ConflictProgress::AddServerConflictingItemById( + const syncable::Id& the_id) { + std::pair<std::set<syncable::Id>::iterator, bool> ret = + unresolvable_conflicting_item_ids_.insert(the_id); + if (ret.second) { + num_server_conflicting_items++; + *dirty_ = true; } - conflict_sets_.clear(); - id_to_conflict_set_.clear(); } UpdateProgress::UpdateProgress() {} @@ -369,7 +295,7 @@ int UpdateProgress::SuccessfullyAppliedUpdateCount() const { bool UpdateProgress::HasConflictingUpdates() const { std::vector<AppliedUpdate>::const_iterator it; for (it = applied_updates_.begin(); it != applied_updates_.end(); ++it) { - if (it->first == CONFLICT) { + if (it->first != SUCCESS) { return true; } } diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h index 7b96ec8..f98a94f 100644 --- a/chrome/browser/sync/sessions/session_state.h +++ b/chrome/browser/sync/sessions/session_state.h @@ -120,7 +120,7 @@ struct SyncSessionSnapshot { bool more_to_sync, bool is_silenced, int64 unsynced_count, - int num_blocking_conflicting_updates, + int num_simple_conflicting_updates, int num_conflicting_updates, bool did_commit_items, const SyncSourceInfo& source, @@ -143,7 +143,7 @@ struct SyncSessionSnapshot { const bool has_more_to_sync; const bool is_silenced; const int64 unsynced_count; - const int num_blocking_conflicting_updates; + const int num_simple_conflicting_updates; const int num_conflicting_updates; const bool did_commit_items; const SyncSourceInfo source; @@ -152,49 +152,52 @@ struct SyncSessionSnapshot { const bool retry_scheduled; }; -// Tracks progress of conflicts and their resolution using conflict sets. +// Tracks progress of conflicts and their resolutions. class ConflictProgress { public: explicit ConflictProgress(bool* dirty_flag); ~ConflictProgress(); - // Various iterators, size, and retrieval functions for conflict sets. - IdToConflictSetMap::const_iterator IdToConflictSetBegin() const; - IdToConflictSetMap::const_iterator IdToConflictSetEnd() const; - IdToConflictSetMap::size_type IdToConflictSetSize() const; - IdToConflictSetMap::const_iterator IdToConflictSetFind( - const syncable::Id& the_id) const; - const ConflictSet* IdToConflictSetGet(const syncable::Id& the_id); - std::set<ConflictSet*>::const_iterator ConflictSetsBegin() const; - std::set<ConflictSet*>::const_iterator ConflictSetsEnd() const; - std::set<ConflictSet*>::size_type ConflictSetsSize() const; - bool HasSimpleConflictItem(const syncable::Id& id) const; + + bool HasSimpleConflictItem(const syncable::Id &id) const; // Various mutators for tracking commit conflicts. - void AddConflictingItemById(const syncable::Id& the_id); - void EraseConflictingItemById(const syncable::Id& the_id); - int ConflictingItemsSize() const { return conflicting_item_ids_.size(); } - std::set<syncable::Id>::const_iterator ConflictingItemsBegin() const; - std::set<syncable::Id>::const_iterator ConflictingItemsEnd() const; - - // Mutators for nonblocking conflicting items (see description below). - void AddNonblockingConflictingItemById(const syncable::Id& the_id); - void EraseNonblockingConflictingItemById(const syncable::Id& the_id); - int NonblockingConflictingItemsSize() const { - return nonblocking_conflicting_item_ids_.size(); + void AddSimpleConflictingItemById(const syncable::Id& the_id); + void EraseSimpleConflictingItemById(const syncable::Id& the_id); + std::set<syncable::Id>::const_iterator SimpleConflictingItemsBegin() const; + std::set<syncable::Id>::const_iterator SimpleConflictingItemsEnd() const; + int SimpleConflictingItemsSize() const { + return simple_conflicting_item_ids_.size(); + } + + // Mutators for unresolvable conflicting items (see description below). + void AddEncryptionConflictingItemById(const syncable::Id& the_id); + int EncryptionConflictingItemsSize() const { + return num_encryption_conflicting_items; } - void MergeSets(const syncable::Id& set1, const syncable::Id& set2); - void CleanupSets(); + void AddHierarchyConflictingItemById(const syncable::Id& id); + int HierarchyConflictingItemsSize() const { + return num_hierarchy_conflicting_items; + } + + void AddServerConflictingItemById(const syncable::Id& id); + int ServerConflictingItemsSize() const { + return num_server_conflicting_items; + } private: - // TODO(sync): move away from sets if it makes more sense. - std::set<syncable::Id> conflicting_item_ids_; - std::map<syncable::Id, ConflictSet*> id_to_conflict_set_; - std::set<ConflictSet*> conflict_sets_; - - // Nonblocking conflicts are not processed by the conflict resolver, but - // they will be processed in the APPLY_UDPATES_TO_RESOLVE_CONFLICTS step. - std::set<syncable::Id> nonblocking_conflicting_item_ids_; + // Conflicts that occur when local and server changes collide and can be + // resolved locally. + std::set<syncable::Id> simple_conflicting_item_ids_; + + // Unresolvable conflicts are not processed by the conflict resolver. We wait + // and hope the server will provide us with an update that resolves these + // conflicts. + std::set<syncable::Id> unresolvable_conflicting_item_ids_; + + size_t num_server_conflicting_items; + size_t num_hierarchy_conflicting_items; + size_t num_encryption_conflicting_items; // Whether a conflicting item was added or removed since // the last call to reset_progress_changed(), if any. In practice this diff --git a/chrome/browser/sync/sessions/session_state_unittest.cc b/chrome/browser/sync/sessions/session_state_unittest.cc index 244ece8e..5a373247 100644 --- a/chrome/browser/sync/sessions/session_state_unittest.cc +++ b/chrome/browser/sync/sessions/session_state_unittest.cc @@ -130,7 +130,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) { const bool kHasMoreToSync = false; const bool kIsSilenced = true; const int kUnsyncedCount = 1053; - const int kNumBlockingConflictingUpdates = 1054; + const int kNumSimpleConflictingUpdates = 1054; const int kNumConflictingUpdates = 1055; const bool kDidCommitItems = true; @@ -146,7 +146,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) { kHasMoreToSync, kIsSilenced, kUnsyncedCount, - kNumBlockingConflictingUpdates, + kNumSimpleConflictingUpdates, kNumConflictingUpdates, kDidCommitItems, source, @@ -168,8 +168,8 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) { ExpectDictBooleanValue(kHasMoreToSync, *value, "hasMoreToSync"); ExpectDictBooleanValue(kIsSilenced, *value, "isSilenced"); ExpectDictIntegerValue(kUnsyncedCount, *value, "unsyncedCount"); - ExpectDictIntegerValue(kNumBlockingConflictingUpdates, *value, - "numBlockingConflictingUpdates"); + ExpectDictIntegerValue(kNumSimpleConflictingUpdates, *value, + "numSimpleConflictingUpdates"); ExpectDictIntegerValue(kNumConflictingUpdates, *value, "numConflictingUpdates"); ExpectDictBooleanValue(kDidCommitItems, *value, diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc index ca9903d..bfd22732 100644 --- a/chrome/browser/sync/sessions/status_controller.cc +++ b/chrome/browser/sync/sessions/status_controller.cc @@ -233,14 +233,14 @@ bool StatusController::HasConflictingUpdates() const { return false; } -int StatusController::TotalNumBlockingConflictingItems() const { +int StatusController::TotalNumSimpleConflictingItems() const { DCHECK(!group_restriction_in_effect_) - << "TotalNumBlockingConflictingItems applies to all ModelSafeGroups"; + << "TotalNumSimpleConflictingItems applies to all ModelSafeGroups"; std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it = per_model_group_.begin(); int sum = 0; for (; it != per_model_group_.end(); ++it) { - sum += it->second->conflict_progress.ConflictingItemsSize(); + sum += it->second->conflict_progress.SimpleConflictingItemsSize(); } return sum; } @@ -252,8 +252,10 @@ int StatusController::TotalNumConflictingItems() const { per_model_group_.begin(); int sum = 0; for (; it != per_model_group_.end(); ++it) { - sum += it->second->conflict_progress.ConflictingItemsSize(); - sum += it->second->conflict_progress.NonblockingConflictingItemsSize(); + sum += it->second->conflict_progress.SimpleConflictingItemsSize(); + sum += it->second->conflict_progress.EncryptionConflictingItemsSize(); + sum += it->second->conflict_progress.HierarchyConflictingItemsSize(); + sum += it->second->conflict_progress.ServerConflictingItemsSize(); } return sum; } diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h index 651e90c6..a1287fd 100644 --- a/chrome/browser/sync/sessions/status_controller.h +++ b/chrome/browser/sync/sessions/status_controller.h @@ -147,17 +147,17 @@ class StatusController { // If a GetUpdates for any data type resulted in downloading an update that // is in conflict, this method returns true. - // Note: this includes non-blocking conflicts. + // Note: this includes unresolvable conflicts. bool HasConflictingUpdates() const; - // Aggregate sum of ConflictingItemSize() over all ConflictProgress objects - // (one for each ModelSafeGroup currently in-use). - // Note: this does not include non-blocking conflicts. - int TotalNumBlockingConflictingItems() const; + // Aggregate sum of SimpleConflictingItemSize() over all ConflictProgress + // objects (one for each ModelSafeGroup currently in-use). + // Note: this does not include unresolvable conflicts. + int TotalNumSimpleConflictingItems() const; - // Aggregate sum of ConflictingItemSize() and NonblockingConflictingItemsSize - // over all ConflictProgress objects (one for each ModelSafeGroup currently - // in-use). + // Aggregate sum of SimpleConflictingItemSize() and other + // ${Type}ConflictingItemSize() methods over all ConflictProgress objects (one + // for each ModelSafeGroup currently in-use). int TotalNumConflictingItems() const; // Returns the number of updates received from the sync server. diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc index a207893..c640e53 100644 --- a/chrome/browser/sync/sessions/status_controller_unittest.cc +++ b/chrome/browser/sync/sessions/status_controller_unittest.cc @@ -43,7 +43,8 @@ TEST_F(StatusControllerTest, GetsDirty) { { ScopedModelSafeGroupRestriction r(&status, GROUP_UI); - status.mutable_conflict_progress()->AddConflictingItemById(syncable::Id()); + status.mutable_conflict_progress()-> + AddSimpleConflictingItemById(syncable::Id()); } EXPECT_TRUE(status.TestAndClearIsDirty()); @@ -118,7 +119,7 @@ TEST_F(StatusControllerTest, HasConflictingUpdates) { EXPECT_FALSE(status.update_progress()); status.mutable_update_progress()->AddAppliedUpdate(SUCCESS, syncable::Id()); - status.mutable_update_progress()->AddAppliedUpdate(CONFLICT, + status.mutable_update_progress()->AddAppliedUpdate(CONFLICT_SIMPLE, syncable::Id()); EXPECT_TRUE(status.update_progress()->HasConflictingUpdates()); } @@ -131,6 +132,22 @@ TEST_F(StatusControllerTest, HasConflictingUpdates) { } } +TEST_F(StatusControllerTest, HasConflictingUpdates_NonBlockingUpdates) { + StatusController status(routes_); + EXPECT_FALSE(status.HasConflictingUpdates()); + { + ScopedModelSafeGroupRestriction r(&status, GROUP_UI); + EXPECT_FALSE(status.update_progress()); + status.mutable_update_progress()->AddAppliedUpdate(SUCCESS, + syncable::Id()); + status.mutable_update_progress()->AddAppliedUpdate(CONFLICT_HIERARCHY, + syncable::Id()); + EXPECT_TRUE(status.update_progress()->HasConflictingUpdates()); + } + + EXPECT_TRUE(status.HasConflictingUpdates()); +} + TEST_F(StatusControllerTest, CountUpdates) { StatusController status(routes_); EXPECT_EQ(0, status.CountUpdates()); @@ -148,17 +165,21 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) { { ScopedModelSafeGroupRestriction r(&status, GROUP_UI); EXPECT_FALSE(status.conflict_progress()); - status.mutable_conflict_progress()->AddConflictingItemById(f.NewLocalId()); - status.mutable_conflict_progress()->AddConflictingItemById(f.NewLocalId()); - EXPECT_EQ(2, status.conflict_progress()->ConflictingItemsSize()); + status.mutable_conflict_progress()-> + AddSimpleConflictingItemById(f.NewLocalId()); + status.mutable_conflict_progress()-> + AddSimpleConflictingItemById(f.NewLocalId()); + EXPECT_EQ(2, status.conflict_progress()->SimpleConflictingItemsSize()); } EXPECT_EQ(2, status.TotalNumConflictingItems()); { ScopedModelSafeGroupRestriction r(&status, GROUP_DB); EXPECT_FALSE(status.conflict_progress()); - status.mutable_conflict_progress()->AddConflictingItemById(f.NewLocalId()); - status.mutable_conflict_progress()->AddConflictingItemById(f.NewLocalId()); - EXPECT_EQ(2, status.conflict_progress()->ConflictingItemsSize()); + status.mutable_conflict_progress()-> + AddSimpleConflictingItemById(f.NewLocalId()); + status.mutable_conflict_progress()-> + AddSimpleConflictingItemById(f.NewLocalId()); + EXPECT_EQ(2, status.conflict_progress()->SimpleConflictingItemsSize()); } EXPECT_EQ(4, status.TotalNumConflictingItems()); } diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc index a1cea48..5ba19ebe 100644 --- a/chrome/browser/sync/sessions/sync_session.cc +++ b/chrome/browser/sync/sessions/sync_session.cc @@ -160,7 +160,7 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const { HasMoreToSync(), delegate_->IsSyncingCurrentlySilenced(), status_controller_->unsynced_handles().size(), - status_controller_->TotalNumBlockingConflictingItems(), + status_controller_->TotalNumSimpleConflictingItems(), status_controller_->TotalNumConflictingItems(), status_controller_->did_commit_items(), source_, @@ -207,8 +207,8 @@ std::set<ModelSafeGroup> SyncSession::GetEnabledGroupsWithConflicts() const { const sessions::ConflictProgress* conflict_progress = status_controller_->GetUnrestrictedConflictProgress(*it); if (conflict_progress && - (conflict_progress->ConflictingItemsBegin() != - conflict_progress->ConflictingItemsEnd())) { + (conflict_progress->SimpleConflictingItemsBegin() != + conflict_progress->SimpleConflictingItemsEnd())) { enabled_groups_with_conflicts.insert(*it); } } diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc index 830aa01..f41d9d2 100644 --- a/chrome/browser/sync/sessions/sync_session_unittest.cc +++ b/chrome/browser/sync/sessions/sync_session_unittest.cc @@ -162,7 +162,7 @@ TEST_F(SyncSessionTest, EnabledGroupsWithConflicts) { // Put GROUP_UI in conflict. session->mutable_status_controller()-> GetUnrestrictedMutableConflictProgressForTest(GROUP_UI)-> - AddConflictingItemById(syncable::Id()); + AddSimpleConflictingItemById(syncable::Id()); std::set<ModelSafeGroup> expected_enabled_groups_with_conflicts; expected_enabled_groups_with_conflicts.insert(GROUP_UI); EXPECT_EQ(expected_enabled_groups_with_conflicts, diff --git a/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc index 00dd5f8..e04e5ae 100644 --- a/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc +++ b/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc @@ -168,7 +168,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(GetClient(1)->AwaitPassphraseRequired()); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); + num_simple_conflicting_updates); // We have two meta nodes (one for each client), the one tab node, plus the // basic preference/themes/search engines items. ASSERT_EQ(NumberOfDefaultSyncItems() + 3, @@ -218,9 +218,9 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(GetClient(1)->AwaitPassphraseRequired()); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); - // We have nine non-blocking conflicts due to the two meta nodes (one for - // each client), plus the basic preference/themes/search engines nodes. + num_simple_conflicting_updates); + // We have nine encryption conflicts due to the two meta nodes (one for each + // client), plus the basic preference/themes/search engines nodes. ASSERT_EQ(NumberOfDefaultSyncItems() + 2, GetClient(1)->GetLastSessionSnapshot()-> num_conflicting_updates); // The encrypted nodes. @@ -230,7 +230,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, client0_windows.GetMutable())); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); + num_simple_conflicting_updates); ASSERT_EQ(NumberOfDefaultSyncItems() + 3, GetClient(1)->GetLastSessionSnapshot()-> num_conflicting_updates); // The encrypted nodes. @@ -270,8 +270,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_TRUE(GetClient(1)->AwaitPassphraseRequired()); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); - // We have two non-blocking conflicts due to the two meta nodes (one for each + num_simple_conflicting_updates); + // We have two encryption conflicts due to the two meta nodes (one for each // client), plus the basic preference/themes/search engines nodes. ASSERT_EQ(NumberOfDefaultSyncItems() + 2, GetClient(1)->GetLastSessionSnapshot()-> @@ -335,8 +335,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ASSERT_TRUE(AwaitQuiescence()); ASSERT_TRUE(GetClient(1)->AwaitPassphraseRequired()); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); - // We have three non-blocking conflicts due to the two meta nodes (one for + num_simple_conflicting_updates); + // We have three encryption conflicts due to the two meta nodes (one for // each client), the one tab node, plus the basic preference/themes/search // engines nodes. ASSERT_GE(NumberOfDefaultSyncItems() + 3, @@ -394,8 +394,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, ASSERT_TRUE(EnableEncryption(0, syncable::SESSIONS)); ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1))); ASSERT_EQ(0, GetClient(1)->GetLastSessionSnapshot()-> - num_blocking_conflicting_updates); - // We have three non-blocking conflicts due to the two meta nodes (one for + num_simple_conflicting_updates); + // We have three encryption conflicts due to the two meta nodes (one for // each client), the one tab node, plus the basic preference/themes/search // engines nodes. ASSERT_EQ(NumberOfDefaultSyncItems() + 3, diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index fbe388c..5930235 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -367,8 +367,6 @@ 'browser/sync/engine/all_status.h', 'browser/sync/engine/apply_updates_command.cc', 'browser/sync/engine/apply_updates_command.h', - 'browser/sync/engine/build_and_process_conflict_sets_command.cc', - 'browser/sync/engine/build_and_process_conflict_sets_command.h', 'browser/sync/engine/build_commit_command.cc', 'browser/sync/engine/build_commit_command.h', 'browser/sync/engine/cleanup_disabled_types_command.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index cad1005..37dd9e4 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -3567,7 +3567,6 @@ 'browser/sync/api/sync_error_unittest.cc', 'browser/sync/engine/apply_updates_command_unittest.cc', 'browser/sync/engine/build_commit_command_unittest.cc', - 'browser/sync/engine/build_and_process_conflict_sets_command_unittest.cc', 'browser/sync/engine/clear_data_command_unittest.cc', 'browser/sync/engine/cleanup_disabled_types_command_unittest.cc', 'browser/sync/engine/download_updates_command_unittest.cc', |