diff options
| -rw-r--r-- | sync/engine/build_commit_command.cc | 33 | ||||
| -rw-r--r-- | sync/engine/build_commit_command.h | 16 | ||||
| -rw-r--r-- | sync/engine/build_commit_command_unittest.cc | 112 | ||||
| -rw-r--r-- | sync/engine/commit.cc | 43 | ||||
| -rw-r--r-- | sync/engine/get_commit_ids_command.cc | 41 | ||||
| -rw-r--r-- | sync/engine/get_commit_ids_command.h | 17 | ||||
| -rw-r--r-- | sync/engine/process_commit_response_command.cc | 19 | ||||
| -rw-r--r-- | sync/engine/process_commit_response_command.h | 3 | ||||
| -rw-r--r-- | sync/engine/process_commit_response_command_unittest.cc | 78 | ||||
| -rw-r--r-- | sync/engine/syncer_unittest.cc | 79 | ||||
| -rw-r--r-- | sync/sessions/status_controller.cc | 5 | ||||
| -rw-r--r-- | sync/sessions/status_controller.h | 19 | ||||
| -rw-r--r-- | sync/sessions/status_controller_unittest.cc | 17 | ||||
| -rw-r--r-- | sync/sessions/sync_session.cc | 3 | ||||
| -rw-r--r-- | sync/sessions/sync_session.h | 42 | ||||
| -rw-r--r-- | sync/sessions/sync_session_unittest.cc | 14 | ||||
| -rw-r--r-- | sync/test/engine/mock_connection_manager.cc | 41 | ||||
| -rw-r--r-- | sync/test/engine/mock_connection_manager.h | 9 |
18 files changed, 274 insertions, 317 deletions
diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc index fb49553..ea0346c 100644 --- a/sync/engine/build_commit_command.cc +++ b/sync/engine/build_commit_command.cc @@ -16,10 +16,10 @@ #include "sync/sessions/ordered_commit_set.h" #include "sync/sessions/sync_session.h" #include "sync/syncable/directory.h" -#include "sync/syncable/mutable_entry.h" +#include "sync/syncable/entry.h" +#include "sync/syncable/syncable_base_transaction.h" #include "sync/syncable/syncable_changes_version.h" #include "sync/syncable/syncable_proto_util.h" -#include "sync/syncable/syncable_write_transaction.h" #include "sync/util/time.h" // TODO(vishwath): Remove this include after node positions have @@ -40,7 +40,6 @@ using syncable::SERVER_ORDINAL_IN_PARENT; using syncable::IS_UNAPPLIED_UPDATE; using syncable::IS_UNSYNCED; using syncable::Id; -using syncable::MutableEntry; using syncable::SPECIFICS; // static @@ -59,9 +58,14 @@ int64 BuildCommitCommand::GetGap() { } BuildCommitCommand::BuildCommitCommand( + syncable::BaseTransaction* trans, const sessions::OrderedCommitSet& batch_commit_set, - sync_pb::ClientToServerMessage* commit_message) - : batch_commit_set_(batch_commit_set), commit_message_(commit_message) { + sync_pb::ClientToServerMessage* commit_message, + ExtensionsActivityMonitor::Records* extensions_activity_buffer) + : trans_(trans), + batch_commit_set_(batch_commit_set), + commit_message_(commit_message), + extensions_activity_buffer_(extensions_activity_buffer) { } BuildCommitCommand::~BuildCommitCommand() {} @@ -81,10 +85,10 @@ void BuildCommitCommand::AddExtensionsActivityToMessage( // We will push this list of extensions activity back into the // ExtensionsActivityMonitor if this commit fails. That's why we must keep // a copy of these records in the session. - monitor->GetAndClearRecords(session->mutable_extensions_activity()); + monitor->GetAndClearRecords(extensions_activity_buffer_); const ExtensionsActivityMonitor::Records& records = - session->extensions_activity(); + *extensions_activity_buffer_; for (ExtensionsActivityMonitor::Records::const_iterator it = records.begin(); it != records.end(); ++it) { @@ -111,7 +115,7 @@ void BuildCommitCommand::AddClientConfigParamsToMessage( } namespace { -void SetEntrySpecifics(MutableEntry* meta_entry, +void SetEntrySpecifics(Entry* meta_entry, sync_pb::SyncEntity* sync_entry) { // Add the new style extension and the folder bit. sync_entry->mutable_specifics()->CopyFrom(meta_entry->Get(SPECIFICS)); @@ -126,8 +130,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) { commit_message_->set_message_contents(sync_pb::ClientToServerMessage::COMMIT); sync_pb::CommitMessage* commit_message = commit_message_->mutable_commit(); - commit_message->set_cache_guid( - session->write_transaction()->directory()->cache_guid()); + commit_message->set_cache_guid(trans_->directory()->cache_guid()); AddExtensionsActivityToMessage(session, commit_message); AddClientConfigParamsToMessage(session, commit_message); @@ -143,8 +146,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) { Id id = batch_commit_set_.GetCommitIdAt(i); sync_pb::SyncEntity* sync_entry = commit_message->add_entries(); sync_entry->set_id_string(SyncableIdToProto(id)); - MutableEntry meta_entry(session->write_transaction(), - syncable::GET_BY_ID, id); + Entry meta_entry(trans_, syncable::GET_BY_ID, id); CHECK(meta_entry.good()); DCHECK_NE(0UL, @@ -173,7 +175,7 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) { Id new_parent_id; if (meta_entry.Get(syncable::IS_DEL) && !meta_entry.Get(syncable::PARENT_ID).ServerKnows()) { - new_parent_id = session->write_transaction()->root_id(); + new_parent_id = trans_->root_id(); } else { new_parent_id = meta_entry.Get(syncable::PARENT_ID); } @@ -231,8 +233,8 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) { FindAnchorPosition(syncable::PREV_ID, meta_entry), FindAnchorPosition(syncable::NEXT_ID, meta_entry)); } - position_block.first = InterpolatePosition(position_block.first, - position_block.second); + position_block.first = BuildCommitCommand::InterpolatePosition( + position_block.first, position_block.second); position_map[id] = position_block; sync_entry->set_position_in_parent(position_block.first); @@ -261,6 +263,7 @@ int64 BuildCommitCommand::FindAnchorPosition(syncable::IdField direction, GetFirstPosition() : GetLastPosition(); } +// static int64 BuildCommitCommand::InterpolatePosition(const int64 lo, const int64 hi) { DCHECK_LE(lo, hi); diff --git a/sync/engine/build_commit_command.h b/sync/engine/build_commit_command.h index 253b6c0..a1b101d 100644 --- a/sync/engine/build_commit_command.h +++ b/sync/engine/build_commit_command.h @@ -11,6 +11,7 @@ #include "sync/base/sync_export.h" #include "sync/engine/syncer_command.h" #include "sync/syncable/entry_kernel.h" +#include "sync/util/extensions_activity_monitor.h" namespace syncer { @@ -20,6 +21,7 @@ class OrderedCommitSet; namespace syncable { class Entry; +class BaseTransaction; } // A class that contains the code used to serialize a set of sync items into a @@ -36,8 +38,11 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand { // // The commit_message parameter is an output parameter which will contain the // fully initialized commit message once ExecuteImpl() has been called. - BuildCommitCommand(const sessions::OrderedCommitSet& batch_commit_set, - sync_pb::ClientToServerMessage* commit_message); + BuildCommitCommand( + syncable::BaseTransaction* trans, + const sessions::OrderedCommitSet& batch_commit_set, + sync_pb::ClientToServerMessage* commit_message, + ExtensionsActivityMonitor::Records* extensions_activity_buffer); virtual ~BuildCommitCommand(); // SyncerCommand implementation. @@ -67,15 +72,20 @@ class SYNC_EXPORT_PRIVATE BuildCommitCommand : public SyncerCommand { const syncable::Entry& entry); // Given two values of the type returned by FindAnchorPosition, // compute a third value in between the two ranges. - int64 InterpolatePosition(int64 lo, int64 hi); + static int64 InterpolatePosition(int64 lo, int64 hi); DISALLOW_COPY_AND_ASSIGN(BuildCommitCommand); + // A pointer to a valid transaction not owned by this class. + syncable::BaseTransaction* trans_; + // Input parameter; see constructor comment. const sessions::OrderedCommitSet& batch_commit_set_; // Output parameter; see constructor comment. sync_pb::ClientToServerMessage* commit_message_; + + ExtensionsActivityMonitor::Records* extensions_activity_buffer_; }; } // namespace syncer diff --git a/sync/engine/build_commit_command_unittest.cc b/sync/engine/build_commit_command_unittest.cc index 6ad3c55..1ad8667 100644 --- a/sync/engine/build_commit_command_unittest.cc +++ b/sync/engine/build_commit_command_unittest.cc @@ -8,98 +8,98 @@ namespace syncer { // A test fixture for tests exercising ClearDataCommandTest. -class BuildCommitCommandTest : public SyncerCommandTest { - protected: - BuildCommitCommandTest() - : batch_commit_set_(ModelSafeRoutingInfo()), - command_(batch_commit_set_, &commit_message_) { - } - - private: - sessions::OrderedCommitSet batch_commit_set_; - sync_pb::ClientToServerMessage commit_message_; - - protected: - BuildCommitCommand command_; - - private: - DISALLOW_COPY_AND_ASSIGN(BuildCommitCommandTest); -}; +class BuildCommitCommandTest : public SyncerCommandTest { }; TEST_F(BuildCommitCommandTest, InterpolatePosition) { EXPECT_LT(BuildCommitCommand::GetFirstPosition(), BuildCommitCommand::GetLastPosition()); // Dense ranges. - EXPECT_EQ(10, command_.InterpolatePosition(10, 10)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 11)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 12)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 13)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 14)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 15)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 16)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 17)); - EXPECT_EQ(11, command_.InterpolatePosition(10, 18)); - EXPECT_EQ(12, command_.InterpolatePosition(10, 19)); - EXPECT_EQ(12, command_.InterpolatePosition(10, 20)); + EXPECT_EQ(10, BuildCommitCommand::InterpolatePosition(10, 10)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 11)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 12)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 13)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 14)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 15)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 16)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 17)); + EXPECT_EQ(11, BuildCommitCommand::InterpolatePosition(10, 18)); + EXPECT_EQ(12, BuildCommitCommand::InterpolatePosition(10, 19)); + EXPECT_EQ(12, BuildCommitCommand::InterpolatePosition(10, 20)); // Sparse ranges. EXPECT_EQ(0x32535ffe3dc97LL + BuildCommitCommand::GetGap(), - command_.InterpolatePosition(0x32535ffe3dc97LL, 0x61abcd323122cLL)); + BuildCommitCommand::InterpolatePosition( + 0x32535ffe3dc97LL, 0x61abcd323122cLL)); EXPECT_EQ(~0x61abcd323122cLL + BuildCommitCommand::GetGap(), - command_.InterpolatePosition(~0x61abcd323122cLL, ~0x32535ffe3dc97LL)); + BuildCommitCommand::InterpolatePosition( + ~0x61abcd323122cLL, ~0x32535ffe3dc97LL)); // Lower limits EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 0x20, - command_.InterpolatePosition( - BuildCommitCommand::GetFirstPosition(), - BuildCommitCommand::GetFirstPosition() + 0x100)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), + BuildCommitCommand::GetFirstPosition() + 0x100)); EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 2, - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1, - BuildCommitCommand::GetFirstPosition() + 2)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition() + 1, + BuildCommitCommand::GetFirstPosition() + 2)); EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + BuildCommitCommand::GetGap()/8 + 1, - command_.InterpolatePosition( - BuildCommitCommand::GetFirstPosition() + 1, - BuildCommitCommand::GetFirstPosition() + 1 + - BuildCommitCommand::GetGap())); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition() + 1, + BuildCommitCommand::GetFirstPosition() + 1 + + BuildCommitCommand::GetGap())); // Extremal cases. EXPECT_EQ(0, - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), - BuildCommitCommand::GetLastPosition())); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), + BuildCommitCommand::GetLastPosition())); EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 1 + BuildCommitCommand::GetGap(), - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1, - BuildCommitCommand::GetLastPosition())); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition() + 1, + BuildCommitCommand::GetLastPosition())); EXPECT_EQ(BuildCommitCommand::GetFirstPosition() + 1 + BuildCommitCommand::GetGap(), - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition() + 1, - BuildCommitCommand::GetLastPosition() - 1)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition() + 1, + BuildCommitCommand::GetLastPosition() - 1)); EXPECT_EQ(BuildCommitCommand::GetLastPosition() - 1 - BuildCommitCommand::GetGap(), - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), - BuildCommitCommand::GetLastPosition() - 1)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), + BuildCommitCommand::GetLastPosition() - 1)); // Edge cases around zero. EXPECT_EQ(BuildCommitCommand::GetGap(), - command_.InterpolatePosition(0, BuildCommitCommand::GetLastPosition())); + BuildCommitCommand::InterpolatePosition( + 0, BuildCommitCommand::GetLastPosition())); EXPECT_EQ(BuildCommitCommand::GetGap() + 1, - command_.InterpolatePosition(1, BuildCommitCommand::GetLastPosition())); + BuildCommitCommand::InterpolatePosition( + 1, BuildCommitCommand::GetLastPosition())); EXPECT_EQ(BuildCommitCommand::GetGap() - 1, - command_.InterpolatePosition(-1, BuildCommitCommand::GetLastPosition())); + BuildCommitCommand::InterpolatePosition( + -1, BuildCommitCommand::GetLastPosition())); EXPECT_EQ(-BuildCommitCommand::GetGap(), - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), 0)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), 0)); EXPECT_EQ(-BuildCommitCommand::GetGap() + 1, - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), 1)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), 1)); EXPECT_EQ(-BuildCommitCommand::GetGap() - 1, - command_.InterpolatePosition(BuildCommitCommand::GetFirstPosition(), -1)); + BuildCommitCommand::InterpolatePosition( + BuildCommitCommand::GetFirstPosition(), -1)); EXPECT_EQ(BuildCommitCommand::GetGap() / 8, - command_.InterpolatePosition(0, BuildCommitCommand::GetGap())); + BuildCommitCommand::InterpolatePosition( + 0, BuildCommitCommand::GetGap())); EXPECT_EQ(BuildCommitCommand::GetGap() / 4, - command_.InterpolatePosition(0, BuildCommitCommand::GetGap()*2)); + BuildCommitCommand::InterpolatePosition( + 0, BuildCommitCommand::GetGap()*2)); EXPECT_EQ(BuildCommitCommand::GetGap(), - command_.InterpolatePosition(0, BuildCommitCommand::GetGap()*2 + 1)); + BuildCommitCommand::InterpolatePosition( + 0, BuildCommitCommand::GetGap()*2 + 1)); } } // namespace syncer diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc index f2728cd..55fe4f5 100644 --- a/sync/engine/commit.cc +++ b/sync/engine/commit.cc @@ -61,20 +61,21 @@ void ClearSyncingBits(syncable::Directory* dir, // The ClientToServerMessage parameter is an output parameter which will contain // the commit message which should be sent to the server. It is valid iff the // return value of this function is true. -bool PrepareCommitMessage(sessions::SyncSession* session, - sessions::OrderedCommitSet* commit_set, - sync_pb::ClientToServerMessage* commit_message) { +bool PrepareCommitMessage( + sessions::SyncSession* session, + sessions::OrderedCommitSet* commit_set, + sync_pb::ClientToServerMessage* commit_message, + ExtensionsActivityMonitor::Records* extensions_activity_buffer) { TRACE_EVENT0("sync", "PrepareCommitMessage"); commit_set->Clear(); commit_message->Clear(); WriteTransaction trans(FROM_HERE, SYNCER, session->context()->directory()); - sessions::ScopedSetSessionWriteTransaction set_trans(session, &trans); // Fetch the items to commit. const size_t batch_size = session->context()->max_commit_batch_size(); - GetCommitIdsCommand get_commit_ids_command(batch_size, commit_set); + GetCommitIdsCommand get_commit_ids_command(&trans, batch_size, commit_set); get_commit_ids_command.Execute(session); DVLOG(1) << "Commit message will contain " << commit_set->Size() << " items."; @@ -83,19 +84,30 @@ bool PrepareCommitMessage(sessions::SyncSession* session, } // Serialize the message. - BuildCommitCommand build_commit_command(*commit_set, commit_message); + BuildCommitCommand build_commit_command(&trans, + *commit_set, + commit_message, + extensions_activity_buffer); build_commit_command.Execute(session); - SetSyncingBits(session->write_transaction(), *commit_set); + SetSyncingBits(&trans, *commit_set); return true; } SyncerError BuildAndPostCommitsImpl(Syncer* syncer, sessions::SyncSession* session, sessions::OrderedCommitSet* commit_set) { - sync_pb::ClientToServerMessage commit_message; - while (!syncer->ExitRequested() && - PrepareCommitMessage(session, commit_set, &commit_message)) { + while (!syncer->ExitRequested()) { + sync_pb::ClientToServerMessage commit_message; + ExtensionsActivityMonitor::Records extensions_activity_buffer; + + if (!PrepareCommitMessage(session, + commit_set, + &commit_message, + &extensions_activity_buffer)) { + break; + } + sync_pb::ClientToServerResponse commit_response; DVLOG(1) << "Sending commit message."; @@ -104,6 +116,9 @@ SyncerError BuildAndPostCommitsImpl(Syncer* syncer, &commit_message, &commit_response, session); TRACE_EVENT_END0("sync", "PostCommit"); + // TODO(rlarocque): Put all the post-commit logic in one place. + // See crbug.com/196338. + if (post_result != SYNCER_OK) { LOG(WARNING) << "Post commit failed"; return post_result; @@ -130,6 +145,14 @@ SyncerError BuildAndPostCommitsImpl(Syncer* syncer, process_response_command.Execute(session); TRACE_EVENT_END0("sync", "ProcessCommitResponse"); + // If the commit failed, return the data to the ExtensionsActivityMonitor. + if (session->status_controller(). + model_neutral_state().num_successful_bookmark_commits == 0) { + ExtensionsActivityMonitor* extensions_activity_monitor = + session->context()->extensions_monitor(); + extensions_activity_monitor->PutRecords(extensions_activity_buffer); + } + if (processing_result != SYNCER_OK) { return processing_result; } diff --git a/sync/engine/get_commit_ids_command.cc b/sync/engine/get_commit_ids_command.cc index a84024a..6b2325f 100644 --- a/sync/engine/get_commit_ids_command.cc +++ b/sync/engine/get_commit_ids_command.cc @@ -11,11 +11,10 @@ #include "sync/engine/syncer_util.h" #include "sync/engine/throttled_data_type_tracker.h" #include "sync/syncable/entry.h" -#include "sync/syncable/mutable_entry.h" #include "sync/syncable/nigori_handler.h" #include "sync/syncable/nigori_util.h" +#include "sync/syncable/syncable_base_transaction.h" #include "sync/syncable/syncable_util.h" -#include "sync/syncable/syncable_write_transaction.h" #include "sync/util/cryptographer.h" using std::set; @@ -28,9 +27,11 @@ using sessions::SyncSession; using sessions::StatusController; GetCommitIdsCommand::GetCommitIdsCommand( + syncable::BaseTransaction* trans, const size_t commit_batch_size, sessions::OrderedCommitSet* commit_set) - : requested_commit_batch_size_(commit_batch_size), + : trans_(trans), + requested_commit_batch_size_(commit_batch_size), commit_set_(commit_set) { } @@ -41,17 +42,17 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) { // are not in the correct order for commit. std::set<int64> ready_unsynced_set; syncable::Directory::UnsyncedMetaHandles all_unsynced_handles; - GetUnsyncedEntries(session->write_transaction(), + GetUnsyncedEntries(trans_, &all_unsynced_handles); ModelTypeSet encrypted_types; bool passphrase_missing = false; Cryptographer* cryptographer = session->context()-> - directory()->GetCryptographer(session->write_transaction()); + directory()->GetCryptographer(trans_); if (cryptographer) { encrypted_types = session->context()->directory()->GetNigoriHandler()-> - GetEncryptedTypes(session->write_transaction()); + GetEncryptedTypes(trans_); passphrase_missing = cryptographer->has_pending_keys(); }; @@ -60,14 +61,14 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) { // We filter out all unready entries from the set of unsynced handles. This // new set of ready and unsynced items (which excludes throttled items as // well) is then what we use to determine what is a candidate for commit. - FilterUnreadyEntries(session->write_transaction(), + FilterUnreadyEntries(trans_, throttled_types, encrypted_types, passphrase_missing, all_unsynced_handles, &ready_unsynced_set); - BuildCommitIds(session->write_transaction(), + BuildCommitIds(trans_, session->context()->routing_info(), ready_unsynced_set); @@ -292,7 +293,7 @@ bool GetCommitIdsCommand::IsCommitBatchFull() const { } void GetCommitIdsCommand::AddCreatesAndMoves( - syncable::WriteTransaction* write_transaction, + syncable::BaseTransaction* trans, const ModelSafeRoutingInfo& routes, const std::set<int64>& ready_unsynced_set) { // Add moves and creates, and prepend their uncommitted parents. @@ -302,7 +303,7 @@ void GetCommitIdsCommand::AddCreatesAndMoves( if (commit_set_->HaveCommitItem(metahandle)) continue; - syncable::Entry entry(write_transaction, + syncable::Entry entry(trans, syncable::GET_BY_HANDLE, metahandle); if (!entry.Get(syncable::IS_DEL)) { @@ -310,12 +311,12 @@ void GetCommitIdsCommand::AddCreatesAndMoves( // dependencies are not in conflict. OrderedCommitSet item_dependencies(routes); if (AddUncommittedParentsAndTheirPredecessors( - write_transaction, + trans, routes, ready_unsynced_set, entry, &item_dependencies) && - AddPredecessorsThenItem(write_transaction, + AddPredecessorsThenItem(trans, routes, ready_unsynced_set, entry, @@ -331,7 +332,7 @@ void GetCommitIdsCommand::AddCreatesAndMoves( } void GetCommitIdsCommand::AddDeletes( - syncable::WriteTransaction* write_transaction, + syncable::BaseTransaction* trans, const std::set<int64>& ready_unsynced_set) { set<syncable::Id> legal_delete_parents; @@ -341,11 +342,11 @@ void GetCommitIdsCommand::AddDeletes( if (commit_set_->HaveCommitItem(metahandle)) continue; - syncable::Entry entry(write_transaction, syncable::GET_BY_HANDLE, + syncable::Entry entry(trans, syncable::GET_BY_HANDLE, metahandle); if (entry.Get(syncable::IS_DEL)) { - syncable::Entry parent(write_transaction, syncable::GET_BY_ID, + syncable::Entry parent(trans, syncable::GET_BY_ID, entry.Get(syncable::PARENT_ID)); // If the parent is deleted and unsynced, then any children of that // parent don't need to be added to the delete queue. @@ -402,8 +403,8 @@ void GetCommitIdsCommand::AddDeletes( int64 metahandle = *iter; if (commit_set_->HaveCommitItem(metahandle)) continue; - syncable::MutableEntry entry(write_transaction, syncable::GET_BY_HANDLE, - metahandle); + syncable::Entry entry(trans, syncable::GET_BY_HANDLE, + metahandle); if (entry.Get(syncable::IS_DEL)) { syncable::Id parent_id = entry.Get(syncable::PARENT_ID); if (legal_delete_parents.count(parent_id)) { @@ -415,7 +416,7 @@ void GetCommitIdsCommand::AddDeletes( } void GetCommitIdsCommand::BuildCommitIds( - syncable::WriteTransaction* write_transaction, + syncable::BaseTransaction* trans, const ModelSafeRoutingInfo& routes, const std::set<int64>& ready_unsynced_set) { // Commits follow these rules: @@ -428,10 +429,10 @@ void GetCommitIdsCommand::BuildCommitIds( // delete trees. // Add moves and creates, and prepend their uncommitted parents. - AddCreatesAndMoves(write_transaction, routes, ready_unsynced_set); + AddCreatesAndMoves(trans, routes, ready_unsynced_set); // Add all deletes. - AddDeletes(write_transaction, ready_unsynced_set); + AddDeletes(trans, ready_unsynced_set); } } // namespace syncer diff --git a/sync/engine/get_commit_ids_command.h b/sync/engine/get_commit_ids_command.h index 86abf24..a41dd00 100644 --- a/sync/engine/get_commit_ids_command.h +++ b/sync/engine/get_commit_ids_command.h @@ -12,7 +12,6 @@ #include "sync/base/sync_export.h" #include "sync/engine/syncer_command.h" #include "sync/engine/syncer_util.h" -#include "sync/sessions/ordered_commit_set.h" #include "sync/sessions/sync_session.h" #include "sync/syncable/directory.h" @@ -21,6 +20,10 @@ using std::vector; namespace syncer { +namespace sessions { +class OrderedCommitSet; +} + // A class that contains the code used to search the syncable::Directory for // locally modified items that are ready to be committed to the server. // @@ -36,7 +39,8 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand { // set of items that are ready to commit. Its size shall not exceed the // provided batch_size. This contents of this "set" will be ordered; see the // comments in this class' implementation for details. - GetCommitIdsCommand(const size_t commit_batch_size, + GetCommitIdsCommand(syncable::BaseTransaction* trans, + const size_t commit_batch_size, sessions::OrderedCommitSet* ordered_commit_set); virtual ~GetCommitIdsCommand(); @@ -44,7 +48,7 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand { virtual SyncerError ExecuteImpl(sessions::SyncSession* session) OVERRIDE; // Builds a vector of IDs that should be committed. - void BuildCommitIds(syncable::WriteTransaction* write_transaction, + void BuildCommitIds(syncable::BaseTransaction* write_transaction, const ModelSafeRoutingInfo& routes, const std::set<int64>& ready_unsynced_set); @@ -119,13 +123,16 @@ class SYNC_EXPORT_PRIVATE GetCommitIdsCommand : public SyncerCommand { bool IsCommitBatchFull() const; - void AddCreatesAndMoves(syncable::WriteTransaction* write_transaction, + void AddCreatesAndMoves(syncable::BaseTransaction* write_transaction, const ModelSafeRoutingInfo& routes, const std::set<int64>& ready_unsynced_set); - void AddDeletes(syncable::WriteTransaction* write_transaction, + void AddDeletes(syncable::BaseTransaction* write_transaction, const std::set<int64>& ready_unsynced_set); + // A pointer to a valid transaction not owned by this class. + syncable::BaseTransaction* trans_; + // Input parameter; see constructor comment. const size_t requested_commit_batch_size_; diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc index 49b1a4d..96a82bd 100644 --- a/sync/engine/process_commit_response_command.cc +++ b/sync/engine/process_commit_response_command.cc @@ -84,25 +84,6 @@ std::set<ModelSafeGroup> ProcessCommitResponseCommand::GetGroupsToChange( SyncerError ProcessCommitResponseCommand::ModelChangingExecuteImpl( SyncSession* session) { - SyncerError result = ProcessCommitResponse(session); - ExtensionsActivityMonitor* monitor = session->context()->extensions_monitor(); - - // This is to be run on one model only: the bookmark model. - if (session->status_controller().HasBookmarkCommitActivity()) { - // If the commit failed, return the data to the ExtensionsActivityMonitor. - if (session->status_controller(). - model_neutral_state().num_successful_bookmark_commits == 0) { - monitor->PutRecords(session->extensions_activity()); - } - // Clear our cached data in either case. - session->mutable_extensions_activity()->clear(); - } - - return result; -} - -SyncerError ProcessCommitResponseCommand::ProcessCommitResponse( - SyncSession* session) { syncable::Directory* dir = session->context()->directory(); StatusController* status = session->mutable_status_controller(); const CommitResponse& cr = commit_response_.commit(); diff --git a/sync/engine/process_commit_response_command.h b/sync/engine/process_commit_response_command.h index f7efcfc..155a47d 100644 --- a/sync/engine/process_commit_response_command.h +++ b/sync/engine/process_commit_response_command.h @@ -73,9 +73,6 @@ class SYNC_EXPORT_PRIVATE ProcessCommitResponseCommand const syncable::Id& pre_commit_id, std::set<syncable::Id>* deleted_folders); - // Actually does the work of execute. - SyncerError ProcessCommitResponse(sessions::SyncSession* session); - void ProcessSuccessfulCommitResponse( const sync_pb::SyncEntity& committed_entry, const sync_pb::CommitResponse_EntryResponse& entry_response, diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc index 3fb7017..5c96424 100644 --- a/sync/engine/process_commit_response_command_unittest.cc +++ b/sync/engine/process_commit_response_command_unittest.cc @@ -349,82 +349,4 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) { << "Too few or too many children in parent folder after commit."; } -// This test fixture runs across a Cartesian product of per-type fail/success -// possibilities. -enum { - TEST_PARAM_BOOKMARK_ENABLE_BIT, - TEST_PARAM_AUTOFILL_ENABLE_BIT, - TEST_PARAM_BIT_COUNT -}; -class MixedResult : - public ProcessCommitResponseCommandTest, - public ::testing::WithParamInterface<int> { - protected: - bool ShouldFailBookmarkCommit() { - return (GetParam() & (1 << TEST_PARAM_BOOKMARK_ENABLE_BIT)) == 0; - } - bool ShouldFailAutofillCommit() { - return (GetParam() & (1 << TEST_PARAM_AUTOFILL_ENABLE_BIT)) == 0; - } -}; -INSTANTIATE_TEST_CASE_P(ProcessCommitResponse, - MixedResult, - testing::Range(0, 1 << TEST_PARAM_BIT_COUNT)); - -// This test commits 2 items (one bookmark, one autofill) and validates what -// happens to the extensions activity records. Commits could fail or succeed, -// depending on the test parameter. -TEST_P(MixedResult, ExtensionActivity) { - sessions::OrderedCommitSet commit_set(session()->context()->routing_info()); - sync_pb::ClientToServerMessage request; - sync_pb::ClientToServerResponse response; - - EXPECT_NE(routing_info().find(BOOKMARKS)->second, - routing_info().find(AUTOFILL)->second) - << "To not be lame, this test requires more than one active group."; - - // Bookmark item setup. - CreateUnprocessedCommitResult( - id_factory_.NewServerId(), - id_factory_.root(), "Some bookmark", false, - BOOKMARKS, &commit_set, &request, &response); - if (ShouldFailBookmarkCommit()) - SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response); - // Autofill item setup. - CreateUnprocessedCommitResult( - id_factory_.NewServerId(), - id_factory_.root(), "Some autofill", false, - AUTOFILL, &commit_set, &request, &response); - if (ShouldFailAutofillCommit()) - SetLastErrorCode(CommitResponse::TRANSIENT_ERROR, &response); - - // Put some extensions activity in the session. - { - ExtensionsActivityMonitor::Records* records = - session()->mutable_extensions_activity(); - (*records)["ABC"].extension_id = "ABC"; - (*records)["ABC"].bookmark_write_count = 2049U; - (*records)["xyz"].extension_id = "xyz"; - (*records)["xyz"].bookmark_write_count = 4U; - } - ProcessCommitResponseCommand command(commit_set, request, response); - command.ExecuteImpl(session()); - ExpectGroupsToChange(command, GROUP_UI, GROUP_DB); - - ExtensionsActivityMonitor::Records final_monitor_records; - context()->extensions_monitor()->GetAndClearRecords(&final_monitor_records); - - if (ShouldFailBookmarkCommit()) { - ASSERT_EQ(2U, final_monitor_records.size()) - << "Should restore records after unsuccessful bookmark commit."; - EXPECT_EQ("ABC", final_monitor_records["ABC"].extension_id); - EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id); - EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count); - EXPECT_EQ(4U, final_monitor_records["xyz"].bookmark_write_count); - } else { - EXPECT_TRUE(final_monitor_records.empty()) - << "Should not restore records after successful bookmark commit."; - } -} - } // namespace syncer diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 304dd5a..1bdc13a 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -105,7 +105,6 @@ using syncable::SPECIFICS; using syncable::SYNCING; using syncable::UNITTEST; -using sessions::ScopedSetSessionWriteTransaction; using sessions::StatusController; using sessions::SyncSessionContext; using sessions::SyncSession; @@ -409,18 +408,16 @@ class SyncerTest : public testing::Test, const vector<syncable::Id>& expected_id_order) { for (size_t limit = expected_id_order.size() + 2; limit > 0; --limit) { WriteTransaction wtrans(FROM_HERE, UNITTEST, directory()); - ScopedSetSessionWriteTransaction set_trans(session_.get(), &wtrans); ModelSafeRoutingInfo routes; GetModelSafeRoutingInfo(&routes); sessions::OrderedCommitSet output_set(routes); - GetCommitIdsCommand command(limit, &output_set); + GetCommitIdsCommand command(&wtrans, limit, &output_set); std::set<int64> ready_unsynced_set; command.FilterUnreadyEntries(&wtrans, ModelTypeSet(), ModelTypeSet(), false, unsynced_handle_view, &ready_unsynced_set); - command.BuildCommitIds(session_->write_transaction(), routes, - ready_unsynced_set); + command.BuildCommitIds(&wtrans, routes, ready_unsynced_set); size_t truncated_size = std::min(limit, expected_id_order.size()); ASSERT_EQ(truncated_size, output_set.Size()); for (size_t i = 0; i < truncated_size; ++i) { @@ -4924,4 +4921,76 @@ TEST_F(SyncerPositionTiebreakingTest, MidLowHigh) { ExpectLocalOrderIsByServerId(); } +enum { + TEST_PARAM_BOOKMARK_ENABLE_BIT, + TEST_PARAM_AUTOFILL_ENABLE_BIT, + TEST_PARAM_BIT_COUNT +}; + +class MixedResult : + public SyncerTest, + public ::testing::WithParamInterface<int> { + protected: + bool ShouldFailBookmarkCommit() { + return (GetParam() & (1 << TEST_PARAM_BOOKMARK_ENABLE_BIT)) == 0; + } + bool ShouldFailAutofillCommit() { + return (GetParam() & (1 << TEST_PARAM_AUTOFILL_ENABLE_BIT)) == 0; + } +}; + +INSTANTIATE_TEST_CASE_P(ExtensionsActivity, + MixedResult, + testing::Range(0, 1 << TEST_PARAM_BIT_COUNT)); + +TEST_P(MixedResult, ExtensionsActivity) { + { + WriteTransaction wtrans(FROM_HERE, UNITTEST, directory()); + + MutableEntry pref(&wtrans, CREATE, PREFERENCES, wtrans.root_id(), "pref"); + ASSERT_TRUE(pref.good()); + pref.Put(syncable::IS_UNSYNCED, true); + + MutableEntry bookmark( + &wtrans, CREATE, BOOKMARKS, wtrans.root_id(), "bookmark"); + ASSERT_TRUE(bookmark.good()); + bookmark.Put(syncable::IS_UNSYNCED, true); + + if (ShouldFailBookmarkCommit()) { + mock_server_->SetTransientErrorId(bookmark.Get(ID)); + } + + if (ShouldFailAutofillCommit()) { + mock_server_->SetTransientErrorId(pref.Get(ID)); + } + } + + + // Put some extenions activity records into the monitor. + { + ExtensionsActivityMonitor::Records records; + records["ABC"].extension_id = "ABC"; + records["ABC"].bookmark_write_count = 2049U; + records["xyz"].extension_id = "xyz"; + records["xyz"].bookmark_write_count = 4U; + context_->extensions_monitor()->PutRecords(records); + } + + SyncShareNudge(); + + ExtensionsActivityMonitor::Records final_monitor_records; + context_->extensions_monitor()->GetAndClearRecords(&final_monitor_records); + if (ShouldFailBookmarkCommit()) { + ASSERT_EQ(2U, final_monitor_records.size()) + << "Should restore records after unsuccessful bookmark commit."; + EXPECT_EQ("ABC", final_monitor_records["ABC"].extension_id); + EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id); + EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count); + EXPECT_EQ(4U, final_monitor_records["xyz"].bookmark_write_count); + } else { + EXPECT_TRUE(final_monitor_records.empty()) + << "Should not restore records after successful bookmark commit."; + } +} + } // namespace syncer diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc index 2287a51..abd6f1e 100644 --- a/sync/sessions/status_controller.cc +++ b/sync/sessions/status_controller.cc @@ -13,10 +13,9 @@ namespace syncer { namespace sessions { -StatusController::StatusController(const ModelSafeRoutingInfo& routes) +StatusController::StatusController() : group_restriction_in_effect_(false), - group_restriction_(GROUP_PASSIVE), - routing_info_(routes) { + group_restriction_(GROUP_PASSIVE) { } StatusController::~StatusController() {} diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h index bf100b0..5f8de13 100644 --- a/sync/sessions/status_controller.h +++ b/sync/sessions/status_controller.h @@ -43,7 +43,7 @@ namespace sessions { class SYNC_EXPORT_PRIVATE StatusController { public: - explicit StatusController(const ModelSafeRoutingInfo& routes); + explicit StatusController(); ~StatusController(); // ClientToServer messages. @@ -112,10 +112,6 @@ class SYNC_EXPORT_PRIVATE StatusController { return sync_start_time_; } - bool HasBookmarkCommitActivity() const { - return ActiveGroupRestrictionIncludesModel(BOOKMARKS); - } - const ModelNeutralState& model_neutral_state() const { return model_neutral_; } @@ -159,17 +155,6 @@ class SYNC_EXPORT_PRIVATE StatusController { private: friend class ScopedModelSafeGroupRestriction; - // Check whether a particular model is included by the active group - // restriction. - bool ActiveGroupRestrictionIncludesModel(ModelType model) const { - if (!group_restriction_in_effect_) - return true; - ModelSafeRoutingInfo::const_iterator it = routing_info_.find(model); - if (it == routing_info_.end()) - return false; - return group_restriction() == it->second; - } - ModelNeutralState model_neutral_; // Used to fail read/write operations on state that don't obey the current @@ -177,8 +162,6 @@ class SYNC_EXPORT_PRIVATE StatusController { bool group_restriction_in_effect_; ModelSafeGroup group_restriction_; - const ModelSafeRoutingInfo routing_info_; - base::Time sync_start_time_; DISALLOW_COPY_AND_ASSIGN(StatusController); diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc index d361a6e..e6b59e8 100644 --- a/sync/sessions/status_controller_unittest.cc +++ b/sync/sessions/status_controller_unittest.cc @@ -9,20 +9,13 @@ namespace syncer { namespace sessions { -class StatusControllerTest : public testing::Test { - public: - virtual void SetUp() { - routes_[BOOKMARKS] = GROUP_UI; - } - protected: - ModelSafeRoutingInfo routes_; -}; +class StatusControllerTest : public testing::Test { }; // This test is useful, as simple as it sounds, due to the copy-paste prone // nature of status_controller.cc (we have had bugs in the past where a set_foo // method was actually setting |bar_| instead!). TEST_F(StatusControllerTest, ReadYourWrites) { - StatusController status(routes_); + StatusController status; status.set_num_server_changes_remaining(13); EXPECT_EQ(13, status.num_server_changes_remaining()); @@ -39,7 +32,7 @@ TEST_F(StatusControllerTest, ReadYourWrites) { } TEST_F(StatusControllerTest, CountUpdates) { - StatusController status(routes_); + StatusController status; EXPECT_EQ(0, status.CountUpdates()); sync_pb::ClientToServerResponse* response(status.mutable_updates_response()); sync_pb::SyncEntity* entity1 = response->mutable_get_updates()->add_entries(); @@ -50,7 +43,7 @@ TEST_F(StatusControllerTest, CountUpdates) { // Test TotalNumConflictingItems TEST_F(StatusControllerTest, TotalNumConflictingItems) { - StatusController status(routes_); + StatusController status; EXPECT_EQ(0, status.TotalNumConflictingItems()); status.increment_num_server_conflicts(); @@ -61,7 +54,7 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) { // Basic test that non group-restricted state accessors don't cause violations. TEST_F(StatusControllerTest, Unrestricted) { - StatusController status(routes_); + StatusController status; status.model_neutral_state(); status.download_updates_succeeded(); status.ServerSaysNothingMoreToDownload(); diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc index b7ff433..58cf11a 100644 --- a/sync/sessions/sync_session.cc +++ b/sync/sessions/sync_session.cc @@ -21,9 +21,8 @@ SyncSession::SyncSession( const SyncSourceInfo& source) : context_(context), source_(source), - write_transaction_(NULL), delegate_(delegate) { - status_controller_.reset(new StatusController(context_->routing_info())); + status_controller_.reset(new StatusController()); debug_info_sources_list_.push_back(source_); } diff --git a/sync/sessions/sync_session.h b/sync/sessions/sync_session.h index 9272bae..126d57f 100644 --- a/sync/sessions/sync_session.h +++ b/sync/sessions/sync_session.h @@ -30,15 +30,10 @@ #include "sync/sessions/ordered_commit_set.h" #include "sync/sessions/status_controller.h" #include "sync/sessions/sync_session_context.h" -#include "sync/util/extensions_activity_monitor.h" namespace syncer { class ModelSafeWorker; -namespace syncable { -class WriteTransaction; -} - namespace sessions { class SYNC_EXPORT_PRIVATE SyncSession { @@ -113,7 +108,6 @@ class SYNC_EXPORT_PRIVATE SyncSession { // TODO(akalin): Split this into context() and mutable_context(). SyncSessionContext* context() const { return context_; } Delegate* delegate() const { return delegate_; } - syncable::WriteTransaction* write_transaction() { return write_transaction_; } const StatusController& status_controller() const { return *status_controller_.get(); } @@ -121,21 +115,9 @@ class SYNC_EXPORT_PRIVATE SyncSession { return status_controller_.get(); } - const ExtensionsActivityMonitor::Records& extensions_activity() const { - return extensions_activity_; - } - ExtensionsActivityMonitor::Records* mutable_extensions_activity() { - return &extensions_activity_; - } - const SyncSourceInfo& source() const { return source_; } private: - // Extend the encapsulation boundary to utilities for internal member - // assignments. This way, the scope of these actions is explicit, they can't - // be overridden, and assigning is always accompanied by unassigning. - friend class ScopedSetSessionWriteTransaction; - // The context for this session, guaranteed to outlive |this|. SyncSessionContext* const context_; @@ -146,12 +128,6 @@ class SYNC_EXPORT_PRIVATE SyncSession { // Currently used only for logging. std::vector<SyncSourceInfo> debug_info_sources_list_; - // Information about extensions activity since the last successful commit. - ExtensionsActivityMonitor::Records extensions_activity_; - - // Used to allow various steps to share a transaction. Can be NULL. - syncable::WriteTransaction* write_transaction_; - // The delegate for this session, must never be NULL. Delegate* const delegate_; @@ -161,24 +137,6 @@ class SYNC_EXPORT_PRIVATE SyncSession { DISALLOW_COPY_AND_ASSIGN(SyncSession); }; -// Installs a WriteTransaction to a given session and later clears it when the -// utility falls out of scope. Transactions are not nestable, so it is an error -// to try and use one of these if the session already has a transaction. -class ScopedSetSessionWriteTransaction { - public: - ScopedSetSessionWriteTransaction(SyncSession* session, - syncable::WriteTransaction* trans) - : session_(session) { - DCHECK(!session_->write_transaction_); - session_->write_transaction_ = trans; - } - ~ScopedSetSessionWriteTransaction() { session_->write_transaction_ = NULL; } - - private: - SyncSession* session_; - DISALLOW_COPY_AND_ASSIGN(ScopedSetSessionWriteTransaction); -}; - } // namespace sessions } // namespace syncer diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc index bb4a3d5..c3c4d69 100644 --- a/sync/sessions/sync_session_unittest.cc +++ b/sync/sessions/sync_session_unittest.cc @@ -140,20 +140,6 @@ class SyncSessionTest : public testing::Test, scoped_ptr<ThrottledDataTypeTracker> throttled_data_type_tracker_; }; -TEST_F(SyncSessionTest, SetWriteTransaction) { - TestDirectorySetterUpper dir_maker; - dir_maker.SetUp(); - syncable::Directory* directory = dir_maker.directory(); - - scoped_ptr<SyncSession> session(MakeSession()); - EXPECT_TRUE(NULL == session->write_transaction()); - { - WriteTransaction trans(FROM_HERE, syncable::UNITTEST, directory); - sessions::ScopedSetSessionWriteTransaction set_trans(session.get(), &trans); - EXPECT_TRUE(&trans == session->write_transaction()); - } -} - TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) { status()->set_updates_request_types(ParamsMeaningAllEnabledTypes()); diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc index 1718699..3fd4a32 100644 --- a/sync/test/engine/mock_connection_manager.cc +++ b/sync/test/engine/mock_connection_manager.cc @@ -18,6 +18,7 @@ #include "sync/test/engine/test_id_factory.h" #include "testing/gtest/include/gtest/gtest.h" +using std::find; using std::map; using std::string; using sync_pb::ClientToServerMessage; @@ -224,6 +225,10 @@ void MockConnectionManager::SetCommitClientCommand( commit_client_command_.reset(command); } +void MockConnectionManager::SetTransientErrorId(syncable::Id id) { + transient_error_ids_.push_back(id); +} + sync_pb::SyncEntity* MockConnectionManager::AddUpdateBookmark( int id, int parent_id, string name, int64 version, @@ -545,6 +550,11 @@ bool MockConnectionManager::ShouldConflictThisCommit() { return conflict; } +bool MockConnectionManager::ShouldTransientErrorThisId(syncable::Id id) { + return find(transient_error_ids_.begin(), transient_error_ids_.end(), id) + != transient_error_ids_.end(); +} + void MockConnectionManager::ProcessCommit( sync_pb::ClientToServerMessage* csm, sync_pb::ClientToServerResponse* response_buffer) { @@ -559,40 +569,47 @@ void MockConnectionManager::ProcessCommit( for (int i = 0; i < commit_message.entries_size() ; i++) { const sync_pb::SyncEntity& entry = commit_message.entries(i); CHECK(entry.has_id_string()); - string id = entry.id_string(); + string id_string = entry.id_string(); ASSERT_LT(entry.name().length(), 256ul) << " name probably too long. True " "server name checking not implemented"; + syncable::Id id; if (entry.version() == 0) { // Relies on our new item string id format. (string representation of a // negative number). - committed_ids_.push_back(syncable::Id::CreateFromClientString(id)); + id = syncable::Id::CreateFromClientString(id_string); } else { - committed_ids_.push_back(syncable::Id::CreateFromServerId(id)); + id = syncable::Id::CreateFromServerId(id_string); } - if (response_map.end() == response_map.find(id)) - response_map[id] = commit_response->add_entryresponse(); - sync_pb::CommitResponse_EntryResponse* er = response_map[id]; + committed_ids_.push_back(id); + + if (response_map.end() == response_map.find(id_string)) + response_map[id_string] = commit_response->add_entryresponse(); + sync_pb::CommitResponse_EntryResponse* er = response_map[id_string]; if (ShouldConflictThisCommit()) { er->set_response_type(CommitResponse::CONFLICT); continue; } + if (ShouldTransientErrorThisId(id)) { + er->set_response_type(CommitResponse::TRANSIENT_ERROR); + continue; + } er->set_response_type(CommitResponse::SUCCESS); er->set_version(entry.version() + 1); if (!commit_time_rename_prepended_string_.empty()) { // Commit time rename sent down from the server. er->set_name(commit_time_rename_prepended_string_ + entry.name()); } - string parent_id = entry.parent_id_string(); + string parent_id_string = entry.parent_id_string(); // Remap id's we've already assigned. - if (changed_ids.end() != changed_ids.find(parent_id)) { - parent_id = changed_ids[parent_id]; - er->set_parent_id_string(parent_id); + if (changed_ids.end() != changed_ids.find(parent_id_string)) { + parent_id_string = changed_ids[parent_id_string]; + er->set_parent_id_string(parent_id_string); } if (entry.has_version() && 0 != entry.version()) { - er->set_id_string(id); // Allows verification. + er->set_id_string(id_string); // Allows verification. } else { string new_id = base::StringPrintf("mock_server:%d", next_new_id_++); - changed_ids[id] = new_id; + changed_ids[id_string] = new_id; er->set_id_string(new_id); } } diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h index 0818b72..53b950bd 100644 --- a/sync/test/engine/mock_connection_manager.h +++ b/sync/test/engine/mock_connection_manager.h @@ -174,6 +174,8 @@ class MockConnectionManager : public ServerConnectionManager { void SetGUClientCommand(sync_pb::ClientCommand* command); void SetCommitClientCommand(sync_pb::ClientCommand* command); + void SetTransientErrorId(syncable::Id); + const std::vector<syncable::Id>& committed_ids() const { return committed_ids_; } @@ -284,6 +286,10 @@ class MockConnectionManager : public ServerConnectionManager { // Determine if one entry in a commit should be rejected with a conflict. bool ShouldConflictThisCommit(); + // Determine if the given item's commit request should be refused with + // a TRANSIENT_ERROR response. + bool ShouldTransientErrorThisId(syncable::Id id); + // Generate a numeric position_in_parent value. We use a global counter // that only decreases; this simulates new objects always being added to the // front of the ordering. @@ -315,6 +321,9 @@ class MockConnectionManager : public ServerConnectionManager { // All IDs that have been committed. std::vector<syncable::Id> committed_ids_; + // List of IDs which should return a transient error. + std::vector<syncable::Id> transient_error_ids_; + // Control of when/if we return conflicts. bool conflict_all_commits_; int conflict_n_commits_; |