diff options
35 files changed, 811 insertions, 104 deletions
diff --git a/chrome/browser/sync/engine/apply_updates_command.cc b/chrome/browser/sync/engine/apply_updates_command.cc index 8b5a6b8..57472b5 100644 --- a/chrome/browser/sync/engine/apply_updates_command.cc +++ b/chrome/browser/sync/engine/apply_updates_command.cc @@ -17,6 +17,35 @@ using sessions::SyncSession; ApplyUpdatesCommand::ApplyUpdatesCommand() {} ApplyUpdatesCommand::~ApplyUpdatesCommand() {} +std::set<ModelSafeGroup> ApplyUpdatesCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + std::set<ModelSafeGroup> groups_with_unapplied_updates; + + syncable::ModelTypeBitSet server_types_with_unapplied_updates; + { + syncable::ScopedDirLookup dir(session.context()->directory_manager(), + session.context()->account_name()); + if (!dir.good()) { + LOG(ERROR) << "Scoped dir lookup failed!"; + return groups_with_unapplied_updates; + } + + syncable::ReadTransaction trans(FROM_HERE, dir); + server_types_with_unapplied_updates = + dir->GetServerTypesWithUnappliedUpdates(&trans); + } + + for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) { + const syncable::ModelType type = syncable::ModelTypeFromInt(i); + if (server_types_with_unapplied_updates.test(type)) { + groups_with_unapplied_updates.insert( + GetGroupForModelType(type, session.routing_info())); + } + } + + return groups_with_unapplied_updates; +} + void ApplyUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) { syncable::ScopedDirLookup dir(session->context()->directory_manager(), session->context()->account_name()); @@ -24,9 +53,27 @@ void ApplyUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) { LOG(ERROR) << "Scoped dir lookup failed!"; return; } + syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir); + + // Compute server types with unapplied updates that fall under our + // group restriction. + const syncable::ModelTypeBitSet server_types_with_unapplied_updates = + dir->GetServerTypesWithUnappliedUpdates(&trans); + syncable::ModelTypeBitSet server_type_restriction; + for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) { + const syncable::ModelType server_type = syncable::ModelTypeFromInt(i); + if (server_types_with_unapplied_updates.test(server_type)) { + if (GetGroupForModelType(server_type, session->routing_info()) == + session->status_controller().group_restriction()) { + server_type_restriction.set(server_type); + } + } + } + syncable::Directory::UnappliedUpdateMetaHandles handles; - dir->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir->GetUnappliedUpdateMetaHandles( + &trans, server_type_restriction, &handles); UpdateApplicator applicator( session->context()->resolver(), @@ -42,13 +89,6 @@ void ApplyUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) { // some subset of the currently synced datatypes. const sessions::StatusController& status(session->status_controller()); if (status.ServerSaysNothingMoreToDownload()) { - syncable::ScopedDirLookup dir(session->context()->directory_manager(), - session->context()->account_name()); - if (!dir.good()) { - LOG(ERROR) << "Scoped dir lookup failed!"; - return; - } - for (int i = syncable::FIRST_REAL_MODEL_TYPE; i < syncable::MODEL_TYPE_COUNT; ++i) { syncable::ModelType model_type = syncable::ModelTypeFromInt(i); diff --git a/chrome/browser/sync/engine/apply_updates_command.h b/chrome/browser/sync/engine/apply_updates_command.h index a6c9d2b..d75754f 100644 --- a/chrome/browser/sync/engine/apply_updates_command.h +++ b/chrome/browser/sync/engine/apply_updates_command.h @@ -16,7 +16,10 @@ class ApplyUpdatesCommand : public ModelChangingSyncerCommand { ApplyUpdatesCommand(); virtual ~ApplyUpdatesCommand(); + protected: // ModelChangingSyncerCommand implementation. + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc index b951e4f..734f9207 100644 --- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc +++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc @@ -57,12 +57,11 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest { make_scoped_refptr(new FakeModelWorker(GROUP_UI))); workers()->push_back( make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD))); - workers()->push_back( - make_scoped_refptr(new FakeModelWorker(GROUP_PASSIVE))); (*mutable_routing_info())[syncable::BOOKMARKS] = GROUP_UI; (*mutable_routing_info())[syncable::PASSWORDS] = GROUP_PASSWORD; (*mutable_routing_info())[syncable::NIGORI] = GROUP_PASSIVE; SyncerCommandTest::SetUp(); + ExpectNoGroupsToChange(apply_updates_command_); } // Create a new unapplied folder node with a parent. @@ -160,6 +159,7 @@ TEST_F(ApplyUpdatesCommandTest, Simple) { DefaultBookmarkSpecifics(), "parent"); + ExpectGroupToChange(apply_updates_command_, GROUP_UI); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -195,6 +195,7 @@ TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) { DefaultBookmarkSpecifics(), "parent"); + ExpectGroupToChange(apply_updates_command_, GROUP_UI); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -218,6 +219,7 @@ TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) { DefaultBookmarkSpecifics(), "some_item"); + ExpectGroupToChange(apply_updates_command_, GROUP_UI); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -254,6 +256,7 @@ TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) { DefaultBookmarkSpecifics(), root_server_id); + ExpectGroupToChange(apply_updates_command_, GROUP_UI); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -292,6 +295,7 @@ TEST_F(ApplyUpdatesCommandTest, DecryptablePassword) { specifics.MutableExtension(sync_pb::password)->mutable_encrypted()); CreateUnappliedNewItem("item", specifics, false); + ExpectGroupToChange(apply_updates_command_, GROUP_PASSWORD); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -320,6 +324,7 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) { encrypted_password.MutableExtension(sync_pb::password); CreateUnappliedNewItem("item3", encrypted_password, false); + ExpectGroupsToChange(apply_updates_command_, GROUP_UI, GROUP_PASSWORD); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -394,6 +399,7 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { CreateUnappliedNewItem("item2", specifics, false); } + ExpectGroupToChange(apply_updates_command_, GROUP_PASSWORD); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -448,6 +454,7 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) { specifics, true); EXPECT_FALSE(cryptographer->has_pending_keys()); + ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -499,6 +506,7 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { specifics, true); EXPECT_FALSE(cryptographer->has_pending_keys()); + ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -586,6 +594,7 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { EXPECT_EQ(2*batch_s+1, handles.size()); } + ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); @@ -688,6 +697,7 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { EXPECT_EQ(2*batch_s+1, handles.size()); } + ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); apply_updates_command_.ExecuteImpl(session()); sessions::StatusController* status = session()->mutable_status_controller(); 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 index 9810225..065f021 100644 --- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc +++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc @@ -30,6 +30,11 @@ using std::vector; BuildAndProcessConflictSetsCommand::BuildAndProcessConflictSetsCommand() {} BuildAndProcessConflictSetsCommand::~BuildAndProcessConflictSetsCommand() {} +std::set<ModelSafeGroup> BuildAndProcessConflictSetsCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + return session.GetEnabledGroupsWithConflicts(); +} + void BuildAndProcessConflictSetsCommand::ModelChangingExecuteImpl( SyncSession* session) { session->mutable_status_controller()->update_conflict_sets_built( 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 index a446e7e..8ef7b8d 100644 --- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h +++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.h @@ -35,7 +35,10 @@ class BuildAndProcessConflictSetsCommand : public ModelChangingSyncerCommand { BuildAndProcessConflictSetsCommand(); virtual ~BuildAndProcessConflictSetsCommand(); + protected: // ModelChangingSyncerCommand implementation. + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; 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 new file mode 100644 index 0000000..3152484 --- /dev/null +++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command_unittest.cc @@ -0,0 +1,52 @@ +// 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/model_changing_syncer_command.cc b/chrome/browser/sync/engine/model_changing_syncer_command.cc index a8df65a..2ea2e71 100644 --- a/chrome/browser/sync/engine/model_changing_syncer_command.cc +++ b/chrome/browser/sync/engine/model_changing_syncer_command.cc @@ -6,7 +6,6 @@ #include "base/basictypes.h" #include "base/callback_old.h" -#include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/sessions/status_controller.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/util/unrecoverable_error_info.h" @@ -19,27 +18,13 @@ void ModelChangingSyncerCommand::ExecuteImpl(sessions::SyncSession* session) { return; } - // Project the list of active types (i.e., types in the routing - // info) to a list of groups. - // - // TODO(akalin): Make this overrideable by subclasses (who might be - // working on a subset of |active_groups|). (See - // http://crbug.com/97832.) - std::set<ModelSafeGroup> active_groups; - const ModelSafeRoutingInfo& routing_info = session->routing_info(); - for (ModelSafeRoutingInfo::const_iterator it = routing_info.begin(); - it != routing_info.end(); ++it) { - active_groups.insert(it->second); - } - // Always work on GROUP_PASSIVE, since that's the group that - // top-level folders map to. - active_groups.insert(GROUP_PASSIVE); - + const std::set<ModelSafeGroup>& groups_to_change = + GetGroupsToChange(*work_session_); for (size_t i = 0; i < session->workers().size(); ++i) { - ModelSafeWorker* worker = session->workers()[i]; + ModelSafeWorker* worker = work_session_->workers()[i]; ModelSafeGroup group = worker->GetModelSafeGroup(); // Skip workers whose group isn't active. - if (active_groups.find(group) == active_groups.end()) { + if (groups_to_change.count(group) == 0u) { DVLOG(2) << "Skipping worker for group " << ModelSafeGroupToString(group); continue; diff --git a/chrome/browser/sync/engine/model_changing_syncer_command.h b/chrome/browser/sync/engine/model_changing_syncer_command.h index 9a2311e..e6f6ca1 100644 --- a/chrome/browser/sync/engine/model_changing_syncer_command.h +++ b/chrome/browser/sync/engine/model_changing_syncer_command.h @@ -7,6 +7,7 @@ #pragma once #include "base/compiler_specific.h" +#include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/engine/syncer_command.h" #include "chrome/browser/sync/util/unrecoverable_error_info.h" @@ -41,6 +42,22 @@ class ModelChangingSyncerCommand : public SyncerCommand { return UnrecoverableErrorInfo(); } + std::set<ModelSafeGroup> GetGroupsToChangeForTest( + const sessions::SyncSession& session) const { + return GetGroupsToChange(session); + } + + protected: + // This should return the set of groups in |session| that need to be + // changed. The returned set should be a subset of + // session.GetEnabledGroups(). Subclasses can guarantee this either + // by calling one of the session.GetEnabledGroups*() functions and + // filtering that, or using GetGroupForModelType() (which handles + // top-level/unspecified nodes) to project from model types to + // groups. + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const = 0; + // Sometimes, a command has work to do that needs to touch global state // belonging to multiple ModelSafeGroups, but in a way that is known to be // safe. This will be called once, prior to ModelChangingExecuteImpl, diff --git a/chrome/browser/sync/engine/model_changing_syncer_command_unittest.cc b/chrome/browser/sync/engine/model_changing_syncer_command_unittest.cc new file mode 100644 index 0000000..5f07d8a --- /dev/null +++ b/chrome/browser/sync/engine/model_changing_syncer_command_unittest.cc @@ -0,0 +1,74 @@ +// 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/model_changing_syncer_command.h" +#include "chrome/browser/sync/sessions/sync_session.h" +#include "chrome/browser/sync/syncable/model_type.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 FakeModelChangingSyncerCommand : public ModelChangingSyncerCommand { + public: + FakeModelChangingSyncerCommand() {} + virtual ~FakeModelChangingSyncerCommand() {} + + const std::set<ModelSafeGroup>& changed_groups() const { + return changed_groups_; + } + + protected: + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const { + return session.GetEnabledGroups(); + } + + virtual void ModelChangingExecuteImpl(sessions::SyncSession* session) { + changed_groups_.insert(session->status_controller().group_restriction()); + } + + private: + std::set<ModelSafeGroup> changed_groups_; + + DISALLOW_COPY_AND_ASSIGN(FakeModelChangingSyncerCommand); +}; + +class ModelChangingSyncerCommandTest : public SyncerCommandTest { + protected: + ModelChangingSyncerCommandTest() {} + virtual ~ModelChangingSyncerCommandTest() {} + + 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(); + } + + FakeModelChangingSyncerCommand command_; + + private: + DISALLOW_COPY_AND_ASSIGN(ModelChangingSyncerCommandTest); +}; + +TEST_F(ModelChangingSyncerCommandTest, Basic) { + ExpectGroupsToChange(command_, GROUP_UI, GROUP_PASSWORD, GROUP_PASSIVE); + EXPECT_TRUE(command_.changed_groups().empty()); + command_.ExecuteImpl(session()); + EXPECT_EQ(command_.GetGroupsToChangeForTest(*session()), + command_.changed_groups()); +} + +} // namespace + +} // 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 83e0d30..a4c3291 100644 --- a/chrome/browser/sync/engine/process_commit_response_command.cc +++ b/chrome/browser/sync/engine/process_commit_response_command.cc @@ -4,6 +4,7 @@ #include "chrome/browser/sync/engine/process_commit_response_command.h" +#include <cstddef> #include <set> #include <string> #include <vector> @@ -59,6 +60,27 @@ void ResetErrorCounters(StatusController* status) { ProcessCommitResponseCommand::ProcessCommitResponseCommand() {} ProcessCommitResponseCommand::~ProcessCommitResponseCommand() {} +std::set<ModelSafeGroup> ProcessCommitResponseCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + std::set<ModelSafeGroup> groups_with_commits; + syncable::ScopedDirLookup dir(session.context()->directory_manager(), + session.context()->account_name()); + if (!dir.good()) { + LOG(ERROR) << "Scoped dir lookup failed!"; + return groups_with_commits; + } + + syncable::ReadTransaction trans(FROM_HERE, dir); + const StatusController& status = session.status_controller(); + for (size_t i = 0; i < status.commit_ids().size(); ++i) { + groups_with_commits.insert( + GetGroupForModelType(status.GetUnrestrictedCommitModelTypeAt(i), + session.routing_info())); + } + + return groups_with_commits; +} + bool ProcessCommitResponseCommand::ModelNeutralExecuteImpl( sessions::SyncSession* session) { ScopedDirLookup dir(session->context()->directory_manager(), diff --git a/chrome/browser/sync/engine/process_commit_response_command.h b/chrome/browser/sync/engine/process_commit_response_command.h index dd13806..bac2a31 100644 --- a/chrome/browser/sync/engine/process_commit_response_command.h +++ b/chrome/browser/sync/engine/process_commit_response_command.h @@ -28,8 +28,12 @@ class ProcessCommitResponseCommand : public ModelChangingSyncerCommand { ProcessCommitResponseCommand(); virtual ~ProcessCommitResponseCommand(); + protected: // ModelChangingSyncerCommand implementation. - virtual bool ModelNeutralExecuteImpl(sessions::SyncSession* session) OVERRIDE; + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; + virtual bool ModelNeutralExecuteImpl( + sessions::SyncSession* session) OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; diff --git a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc b/chrome/browser/sync/engine/process_commit_response_command_unittest.cc index 83b8885..b22963c 100644 --- a/chrome/browser/sync/engine/process_commit_response_command_unittest.cc +++ b/chrome/browser/sync/engine/process_commit_response_command_unittest.cc @@ -53,6 +53,9 @@ class ProcessCommitResponseCommandTestWithParam commit_set_.reset(new sessions::OrderedCommitSet(routing_info())); SyncerCommandTestWithParam<T>::SetUp(); + // Need to explicitly use this-> to avoid obscure template + // warning. + this->ExpectNoGroupsToChange(command_); } protected: @@ -233,6 +236,7 @@ TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) { CreateUnprocessedCommitResult(autofill_id2, id_factory_.root(), "Autofill 2", syncable::AUTOFILL); + ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB); command_.ExecuteImpl(session()); ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); @@ -324,6 +328,7 @@ TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) { // 25 items. This should apply the values indicated by // each CommitResponse_EntryResponse to the syncable Entries. All new // items in the commit batch should have their IDs changed to server IDs. + ExpectGroupToChange(command_, GROUP_UI); command_.ExecuteImpl(session()); ScopedDirLookup dir(syncdb()->manager(), syncdb()->name()); @@ -428,6 +433,7 @@ TEST_P(MixedResult, ExtensionActivity) { (*records)["xyz"].extension_id = "xyz"; (*records)["xyz"].bookmark_write_count = 4U; } + ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB); command_.ExecuteImpl(session()); ExtensionsActivityMonitor::Records final_monitor_records; diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc index e6763bd..08cba00 100644 --- a/chrome/browser/sync/engine/process_updates_command.cc +++ b/chrome/browser/sync/engine/process_updates_command.cc @@ -22,10 +22,20 @@ namespace browser_sync { using sessions::SyncSession; using sessions::StatusController; +using sessions::UpdateProgress; ProcessUpdatesCommand::ProcessUpdatesCommand() {} ProcessUpdatesCommand::~ProcessUpdatesCommand() {} +std::set<ModelSafeGroup> ProcessUpdatesCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + // TODO(akalin): Figure out why using + // GetEnabledGroupsWithVerifiedUpdates() regresses the perf tests + // for delete_typed_urls, add_autofill_profiles, and + // delete_autofill_profiles. + return session.GetEnabledGroups(); +} + bool ProcessUpdatesCommand::ModelNeutralExecuteImpl(SyncSession* session) { const GetUpdatesResponse& updates = session->status_controller().updates_response().get_updates(); diff --git a/chrome/browser/sync/engine/process_updates_command.h b/chrome/browser/sync/engine/process_updates_command.h index 26e4993..1f12706 100644 --- a/chrome/browser/sync/engine/process_updates_command.h +++ b/chrome/browser/sync/engine/process_updates_command.h @@ -34,8 +34,12 @@ class ProcessUpdatesCommand : public ModelChangingSyncerCommand { ProcessUpdatesCommand(); virtual ~ProcessUpdatesCommand(); + protected: // ModelChangingSyncerCommand implementation. - virtual bool ModelNeutralExecuteImpl(sessions::SyncSession* session) OVERRIDE; + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; + virtual bool ModelNeutralExecuteImpl( + sessions::SyncSession* session) OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; diff --git a/chrome/browser/sync/engine/process_updates_command_unittest.cc b/chrome/browser/sync/engine/process_updates_command_unittest.cc new file mode 100644 index 0000000..bd1bdbb --- /dev/null +++ b/chrome/browser/sync/engine/process_updates_command_unittest.cc @@ -0,0 +1,52 @@ +// 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/process_updates_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 ProcessUpdatesCommandTest : public SyncerCommandTest { + protected: + ProcessUpdatesCommandTest() {} + virtual ~ProcessUpdatesCommandTest() {} + + virtual void SetUp() { + workers()->push_back( + make_scoped_refptr(new FakeModelWorker(GROUP_UI))); + workers()->push_back( + make_scoped_refptr(new FakeModelWorker(GROUP_DB))); + (*mutable_routing_info())[syncable::BOOKMARKS] = GROUP_UI; + (*mutable_routing_info())[syncable::AUTOFILL] = GROUP_DB; + SyncerCommandTest::SetUp(); + } + + ProcessUpdatesCommand command_; + + private: + DISALLOW_COPY_AND_ASSIGN(ProcessUpdatesCommandTest); +}; + +TEST_F(ProcessUpdatesCommandTest, GetGroupsToChange) { + ExpectNoGroupsToChange(command_); + // Add a verified update for GROUP_DB. + session()->mutable_status_controller()-> + GetUnrestrictedMutableUpdateProgressForTest(GROUP_DB)-> + AddVerifyResult(VerifyResult(), sync_pb::SyncEntity()); + ExpectGroupToChange(command_, GROUP_DB); +} + +} // namespace + +} // namespace browser_sync diff --git a/chrome/browser/sync/engine/resolve_conflicts_command.cc b/chrome/browser/sync/engine/resolve_conflicts_command.cc index 71351c4..b55f9ad 100644 --- a/chrome/browser/sync/engine/resolve_conflicts_command.cc +++ b/chrome/browser/sync/engine/resolve_conflicts_command.cc @@ -14,6 +14,11 @@ namespace browser_sync { ResolveConflictsCommand::ResolveConflictsCommand() {} ResolveConflictsCommand::~ResolveConflictsCommand() {} +std::set<ModelSafeGroup> ResolveConflictsCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + return session.GetEnabledGroupsWithConflicts(); +} + void ResolveConflictsCommand::ModelChangingExecuteImpl( sessions::SyncSession* session) { ConflictResolver* resolver = session->context()->resolver(); diff --git a/chrome/browser/sync/engine/resolve_conflicts_command.h b/chrome/browser/sync/engine/resolve_conflicts_command.h index 1fcdec9..2407593 100644 --- a/chrome/browser/sync/engine/resolve_conflicts_command.h +++ b/chrome/browser/sync/engine/resolve_conflicts_command.h @@ -17,7 +17,10 @@ class ResolveConflictsCommand : public ModelChangingSyncerCommand { ResolveConflictsCommand(); virtual ~ResolveConflictsCommand(); + protected: // ModelChangingSyncerCommand implementation. + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; diff --git a/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc b/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc new file mode 100644 index 0000000..a36d5e2 --- /dev/null +++ b/chrome/browser/sync/engine/resolve_conflicts_command_unittest.cc @@ -0,0 +1,51 @@ +// 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/resolve_conflicts_command.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 ResolveConflictsCommandTest : public SyncerCommandTest { + protected: + ResolveConflictsCommandTest() {} + virtual ~ResolveConflictsCommandTest() {} + + 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(); + } + + ResolveConflictsCommand command_; + + private: + DISALLOW_COPY_AND_ASSIGN(ResolveConflictsCommandTest); +}; + +TEST_F(ResolveConflictsCommandTest, GetGroupsToChange) { + ExpectNoGroupsToChange(command_); + // Put GROUP_PASSWORD in conflict. + session()->mutable_status_controller()-> + GetUnrestrictedMutableConflictProgressForTest(GROUP_PASSWORD)-> + AddConflictingItemById(syncable::Id()); + ExpectGroupToChange(command_, GROUP_PASSWORD); +} + +} // namespace + +} // namespace browser_sync diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc index ce05fb1..c21749b 100644 --- a/chrome/browser/sync/engine/sync_scheduler.cc +++ b/chrome/browser/sync/engine/sync_scheduler.cc @@ -762,7 +762,7 @@ void SyncScheduler::DoSyncSessionJob(const SyncSessionJob& job) { // and update any disabled or modified entries in the job. scoped_ptr<SyncSession> session(CreateSyncSession(job.session->source())); - job.session->RebaseRoutingInfoWithLatest(session.get()); + job.session->RebaseRoutingInfoWithLatest(*session); } SDVLOG(2) << "DoSyncSessionJob with " << SyncSessionJob::GetPurposeString(job.purpose) << " job"; diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc index 582655e..50c30fc 100644 --- a/chrome/browser/sync/engine/syncer_unittest.cc +++ b/chrome/browser/sync/engine/syncer_unittest.cc @@ -2162,6 +2162,8 @@ TEST_F(SyncerTest, CommitsUpdateDoesntAlterEntry) { } TEST_F(SyncerTest, ParentAndChildBothMatch) { + syncable::ModelTypeBitSet all_types; + all_types.set(); ScopedDirLookup dir(syncdb_.manager(), syncdb_.name()); CHECK(dir.good()); syncable::Id parent_id = ids_.NewServerId(); @@ -2198,7 +2200,7 @@ TEST_F(SyncerTest, ParentAndChildBothMatch) { dir->GetChildHandlesById(&trans, parent_id, &children); EXPECT_EQ(1u, children.size()); Directory::UnappliedUpdateMetaHandles unapplied; - dir->GetUnappliedUpdateMetaHandles(&trans, &unapplied); + dir->GetUnappliedUpdateMetaHandles(&trans, all_types, &unapplied); EXPECT_EQ(0u, unapplied.size()); syncable::Directory::UnsyncedMetaHandles unsynced; dir->GetUnsyncedMetaHandles(&trans, &unsynced); diff --git a/chrome/browser/sync/engine/update_applicator.cc b/chrome/browser/sync/engine/update_applicator.cc index 6246836..e44083c 100644 --- a/chrome/browser/sync/engine/update_applicator.cc +++ b/chrome/browser/sync/engine/update_applicator.cc @@ -97,12 +97,12 @@ void UpdateApplicator::Advance() { bool UpdateApplicator::SkipUpdate(const syncable::Entry& entry) { syncable::ModelType type = entry.GetServerModelType(); ModelSafeGroup g = GetGroupForModelType(type, routing_info_); - // The extra routing_info count check here is to support GetUpdateses for - // a subset of the globally enabled types, and not attempt to update items - // if their type isn't permitted in the current run. These would typically - // be unapplied items from a previous sync. - if (g != group_filter_) + // The set of updates passed to the UpdateApplicator should already + // be group-filtered. + if (g != group_filter_) { + NOTREACHED(); return true; + } if (g == GROUP_PASSIVE && !routing_info_.count(type) && type != syncable::UNSPECIFIED && diff --git a/chrome/browser/sync/engine/verify_updates_command.cc b/chrome/browser/sync/engine/verify_updates_command.cc index 81f1de2..96dd4e7 100644 --- a/chrome/browser/sync/engine/verify_updates_command.cc +++ b/chrome/browser/sync/engine/verify_updates_command.cc @@ -27,6 +27,21 @@ using syncable::SYNCER; VerifyUpdatesCommand::VerifyUpdatesCommand() {} VerifyUpdatesCommand::~VerifyUpdatesCommand() {} +std::set<ModelSafeGroup> VerifyUpdatesCommand::GetGroupsToChange( + const sessions::SyncSession& session) const { + std::set<ModelSafeGroup> groups_with_updates; + + const GetUpdatesResponse& updates = + session.status_controller().updates_response().get_updates(); + for (int i = 0; i < updates.entries().size(); i++) { + groups_with_updates.insert( + GetGroupForModelType(syncable::GetModelType(updates.entries(i)), + session.routing_info())); + } + + return groups_with_updates; +} + void VerifyUpdatesCommand::ModelChangingExecuteImpl( sessions::SyncSession* session) { DVLOG(1) << "Beginning Update Verification"; diff --git a/chrome/browser/sync/engine/verify_updates_command.h b/chrome/browser/sync/engine/verify_updates_command.h index b612e18..26b0b37 100644 --- a/chrome/browser/sync/engine/verify_updates_command.h +++ b/chrome/browser/sync/engine/verify_updates_command.h @@ -26,7 +26,10 @@ class VerifyUpdatesCommand : public ModelChangingSyncerCommand { VerifyUpdatesCommand(); virtual ~VerifyUpdatesCommand(); - // SyncerCommand implementation. + protected: + // ModelChangingSyncerCommand implementation. + virtual std::set<ModelSafeGroup> GetGroupsToChange( + const sessions::SyncSession& session) const OVERRIDE; virtual void ModelChangingExecuteImpl( sessions::SyncSession* session) OVERRIDE; diff --git a/chrome/browser/sync/engine/verify_updates_command_unittest.cc b/chrome/browser/sync/engine/verify_updates_command_unittest.cc index 20aab01..7de5cf2 100644 --- a/chrome/browser/sync/engine/verify_updates_command_unittest.cc +++ b/chrome/browser/sync/engine/verify_updates_command_unittest.cc @@ -5,6 +5,7 @@ #include "base/location.h" #include "chrome/browser/sync/engine/verify_updates_command.h" #include "chrome/browser/sync/protocol/bookmark_specifics.pb.h" +#include "chrome/browser/sync/sessions/session_state.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/syncable.h" @@ -81,6 +82,8 @@ TEST_F(VerifyUpdatesCommandTest, AllVerified) { CreateLocalItem("p1", root, syncable::PREFERENCES); CreateLocalItem("a1", root, syncable::AUTOFILL); + ExpectNoGroupsToChange(command_); + GetUpdatesResponse* updates = session()->mutable_status_controller()-> mutable_updates_response()->mutable_get_updates(); AddUpdate(updates, "b1", root, syncable::BOOKMARKS); @@ -88,6 +91,8 @@ TEST_F(VerifyUpdatesCommandTest, AllVerified) { AddUpdate(updates, "p1", root, syncable::PREFERENCES); AddUpdate(updates, "a1", root, syncable::AUTOFILL); + ExpectGroupsToChange(command_, GROUP_UI, GROUP_DB); + command_.ExecuteImpl(session()); StatusController* status = session()->mutable_status_controller(); diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc index 26f5c62..4b82aa8 100644 --- a/chrome/browser/sync/sessions/status_controller.cc +++ b/chrome/browser/sync/sessions/status_controller.cc @@ -62,6 +62,12 @@ const ConflictProgress* StatusController::GetUnrestrictedConflictProgress( return state ? &state->conflict_progress : NULL; } +ConflictProgress* + StatusController::GetUnrestrictedMutableConflictProgressForTest( + ModelSafeGroup group) { + return &GetOrCreateModelSafeGroupState(false, group)->conflict_progress; +} + const UpdateProgress* StatusController::GetUnrestrictedUpdateProgress( ModelSafeGroup group) const { const PerModelSafeGroupState* state = @@ -69,6 +75,12 @@ const UpdateProgress* StatusController::GetUnrestrictedUpdateProgress( return state ? &state->update_progress : NULL; } +UpdateProgress* + StatusController::GetUnrestrictedMutableUpdateProgressForTest( + ModelSafeGroup group) { + return &GetOrCreateModelSafeGroupState(false, group)->update_progress; +} + const PerModelSafeGroupState* StatusController::GetModelSafeGroupState( bool restrict, ModelSafeGroup group) const { DCHECK_EQ(restrict, group_restriction_in_effect_); diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h index c0782cb..70aed96 100644 --- a/chrome/browser/sync/sessions/status_controller.h +++ b/chrome/browser/sync/sessions/status_controller.h @@ -63,8 +63,12 @@ class StatusController { UpdateProgress* mutable_update_progress(); const ConflictProgress* GetUnrestrictedConflictProgress( ModelSafeGroup group) const; + ConflictProgress* GetUnrestrictedMutableConflictProgressForTest( + ModelSafeGroup group); const UpdateProgress* GetUnrestrictedUpdateProgress( ModelSafeGroup group) const; + UpdateProgress* GetUnrestrictedMutableUpdateProgressForTest( + ModelSafeGroup group); // ClientToServer messages. const ClientToServerMessage& commit_message() { diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc index f399d7d..94bcd85 100644 --- a/chrome/browser/sync/sessions/sync_session.cc +++ b/chrome/browser/sync/sessions/sync_session.cc @@ -6,12 +6,48 @@ #include <algorithm> +#include "base/logging.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type.h" namespace browser_sync { namespace sessions { +namespace { + +std::set<ModelSafeGroup> ComputeEnabledGroups( + const ModelSafeRoutingInfo& routing_info, + const std::vector<ModelSafeWorker*>& workers) { + std::set<ModelSafeGroup> enabled_groups; + // Project the list of enabled types (i.e., types in the routing + // info) to a list of enabled groups. + for (ModelSafeRoutingInfo::const_iterator it = routing_info.begin(); + it != routing_info.end(); ++it) { + enabled_groups.insert(it->second); + } + // GROUP_PASSIVE is always enabled, since that's the group that + // top-level folders map to. + enabled_groups.insert(GROUP_PASSIVE); + if (DCHECK_IS_ON()) { + // We sometimes create dummy SyncSession objects (see + // SyncScheduler::InitialSnapshot) so don't check in that case. + if (!routing_info.empty() || !workers.empty()) { + std::set<ModelSafeGroup> groups_with_workers; + for (std::vector<ModelSafeWorker*>::const_iterator it = workers.begin(); + it != workers.end(); ++it) { + groups_with_workers.insert((*it)->GetModelSafeGroup()); + } + // All enabled groups should have a corresponding worker. + DCHECK(std::includes( + groups_with_workers.begin(), groups_with_workers.end(), + enabled_groups.begin(), enabled_groups.end())); + } + } + return enabled_groups; +} + +} // namesepace + SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate, const SyncSourceInfo& source, const ModelSafeRoutingInfo& routing_info, @@ -21,7 +57,8 @@ SyncSession::SyncSession(SyncSessionContext* context, Delegate* delegate, write_transaction_(NULL), delegate_(delegate), workers_(workers), - routing_info_(routing_info) { + routing_info_(routing_info), + enabled_groups_(ComputeEnabledGroups(routing_info_, workers_)) { status_controller_.reset(new StatusController(routing_info_)); std::sort(workers_.begin(), workers_.end()); } @@ -53,15 +90,18 @@ void SyncSession::Coalesce(const SyncSession& session) { ++it) { routing_info_[it->first] = it->second; } + + // Now update enabled groups. + enabled_groups_ = ComputeEnabledGroups(routing_info_, workers_); } -void SyncSession::RebaseRoutingInfoWithLatest(SyncSession* session) { +void SyncSession::RebaseRoutingInfoWithLatest(const SyncSession& session) { ModelSafeRoutingInfo temp_routing_info; // Take the intersecion and also set the routing info(it->second) from the // passed in session. for (ModelSafeRoutingInfo::const_iterator it = - session->routing_info_.begin(); it != session->routing_info_.end(); + session.routing_info_.begin(); it != session.routing_info_.end(); ++it) { if (routing_info_.find(it->first) != routing_info_.end()) { temp_routing_info[it->first] = it->second; @@ -72,14 +112,17 @@ void SyncSession::RebaseRoutingInfoWithLatest(SyncSession* session) { routing_info_.swap(temp_routing_info); // Now update the payload map. - PurgeStalePayload(&source_.types, session->routing_info_); + PurgeStalePayload(&source_.types, session.routing_info_); // Now update the workers. std::vector<ModelSafeWorker*> temp; std::set_intersection(workers_.begin(), workers_.end(), - session->workers_.begin(), session->workers_.end(), + session.workers_.begin(), session.workers_.end(), std::back_inserter(temp)); workers_.swap(temp); + + // Now update enabled groups. + enabled_groups_ = ComputeEnabledGroups(routing_info_, workers_); } void SyncSession::ResetTransientState() { @@ -141,7 +184,46 @@ bool SyncSession::HasMoreToSync() const { status->conflicts_resolved(); // Or, we have conflicting updates, but we're making progress on // resolving them... +} + +const std::set<ModelSafeGroup>& SyncSession::GetEnabledGroups() const { + return enabled_groups_; +} + +std::set<ModelSafeGroup> SyncSession::GetEnabledGroupsWithConflicts() const { + const std::set<ModelSafeGroup>& enabled_groups = GetEnabledGroups(); + std::set<ModelSafeGroup> enabled_groups_with_conflicts; + for (std::set<ModelSafeGroup>::const_iterator it = + enabled_groups.begin(); it != enabled_groups.end(); ++it) { + const sessions::ConflictProgress* conflict_progress = + status_controller_->GetUnrestrictedConflictProgress(*it); + if (conflict_progress && + (conflict_progress->ConflictingItemsBegin() != + conflict_progress->ConflictingItemsEnd())) { + enabled_groups_with_conflicts.insert(*it); + } } + return enabled_groups_with_conflicts; +} + +std::set<ModelSafeGroup> + SyncSession::GetEnabledGroupsWithVerifiedUpdates() const { + const std::set<ModelSafeGroup>& enabled_groups = GetEnabledGroups(); + std::set<ModelSafeGroup> enabled_groups_with_verified_updates; + for (std::set<ModelSafeGroup>::const_iterator it = + enabled_groups.begin(); it != enabled_groups.end(); ++it) { + const UpdateProgress* update_progress = + status_controller_->GetUnrestrictedUpdateProgress(*it); + if (update_progress && + (update_progress->VerifiedUpdatesBegin() != + update_progress->VerifiedUpdatesEnd())) { + enabled_groups_with_verified_updates.insert(*it); + } + } + + return enabled_groups_with_verified_updates; +} + } // namespace sessions } // namespace browser_sync diff --git a/chrome/browser/sync/sessions/sync_session.h b/chrome/browser/sync/sessions/sync_session.h index 2762057..135e6d7 100644 --- a/chrome/browser/sync/sessions/sync_session.h +++ b/chrome/browser/sync/sessions/sync_session.h @@ -16,6 +16,7 @@ #pragma once #include <map> +#include <set> #include <string> #include <utility> #include <vector> @@ -119,7 +120,7 @@ class SyncSession { // session. Purges types from the above 3 which are not in session. Useful // to update the sync session when the user has disabled some types from // syncing. - void RebaseRoutingInfoWithLatest(SyncSession* session); + void RebaseRoutingInfoWithLatest(const SyncSession& session); // Should be called any time |this| is being re-used in a new call to // SyncShare (e.g., HasMoreToSync returned true). @@ -152,6 +153,15 @@ class SyncSession { const ModelSafeRoutingInfo& routing_info() const { return routing_info_; } const SyncSourceInfo& source() const { return source_; } + // Returns the set of groups which have enabled types. + const std::set<ModelSafeGroup>& GetEnabledGroups() const; + + // Returns the set of enabled groups that have conflicts. + std::set<ModelSafeGroup> GetEnabledGroupsWithConflicts() const; + + // Returns the set of enabled groups that have verified updates. + std::set<ModelSafeGroup> GetEnabledGroupsWithVerifiedUpdates() const; + private: // Extend the encapsulation boundary to utilities for internal member // assignments. This way, the scope of these actions is explicit, they can't @@ -171,7 +181,7 @@ class SyncSession { syncable::WriteTransaction* write_transaction_; // The delegate for this session, must never be NULL. - Delegate* delegate_; + Delegate* const delegate_; // Our controller for various status and error counters. scoped_ptr<StatusController> status_controller_; @@ -185,6 +195,10 @@ class SyncSession { // on those datatypes. ModelSafeRoutingInfo routing_info_; + // The set of groups with enabled types. Computed from + // |routing_info_|. + std::set<ModelSafeGroup> enabled_groups_; + DISALLOW_COPY_AND_ASSIGN(SyncSession); }; diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc index d914b76..64c2cec 100644 --- a/chrome/browser/sync/sessions/sync_session_unittest.cc +++ b/chrome/browser/sync/sessions/sync_session_unittest.cc @@ -10,9 +10,12 @@ #include "base/message_loop.h" #include "chrome/browser/sync/engine/conflict_resolver.h" #include "chrome/browser/sync/engine/syncer_types.h" +#include "chrome/browser/sync/sessions/session_state.h" +#include "chrome/browser/sync/sessions/status_controller.h" #include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/syncable/syncable.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/test_directory_setter_upper.h" #include "testing/gtest/include/gtest/gtest.h" @@ -27,13 +30,13 @@ class SyncSessionTest : public testing::Test, public SyncSession::Delegate, public ModelSafeWorkerRegistrar { public: - SyncSessionTest() : controller_invocations_allowed_(false) { - GetModelSafeRoutingInfo(&routes_); - } + SyncSessionTest() : controller_invocations_allowed_(false) {} SyncSession* MakeSession() { - return new SyncSession(context_.get(), this, SyncSourceInfo(), routes_, - std::vector<ModelSafeWorker*>()); + std::vector<ModelSafeWorker*> workers; + GetWorkers(&workers); + return new SyncSession(context_.get(), this, SyncSourceInfo(), + routes_, workers); } virtual void SetUp() { @@ -41,7 +44,17 @@ class SyncSessionTest : public testing::Test, std::vector<SyncEngineEventListener*>(), NULL)); routes_.clear(); routes_[syncable::BOOKMARKS] = GROUP_UI; - routes_[syncable::AUTOFILL] = GROUP_UI; + routes_[syncable::AUTOFILL] = GROUP_DB; + scoped_refptr<ModelSafeWorker> passive_worker( + new FakeModelWorker(GROUP_PASSIVE)); + scoped_refptr<ModelSafeWorker> ui_worker( + new FakeModelWorker(GROUP_UI)); + scoped_refptr<ModelSafeWorker> db_worker( + new FakeModelWorker(GROUP_DB)); + workers_.clear(); + workers_.push_back(passive_worker); + workers_.push_back(ui_worker); + workers_.push_back(db_worker); session_.reset(MakeSession()); } virtual void TearDown() { @@ -77,9 +90,15 @@ class SyncSessionTest : public testing::Test, } // ModelSafeWorkerRegistrar implementation. - virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) OVERRIDE {} + virtual void GetWorkers(std::vector<ModelSafeWorker*>* out) OVERRIDE { + out->clear(); + for (std::vector<scoped_refptr<ModelSafeWorker> >::const_iterator it = + workers_.begin(); it != workers_.end(); ++it) { + out->push_back(it->get()); + } + } virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) OVERRIDE { - out->swap(routes_); + *out = routes_; } StatusController* status() { return session_->mutable_status_controller(); } @@ -106,9 +125,49 @@ class SyncSessionTest : public testing::Test, bool controller_invocations_allowed_; scoped_ptr<SyncSession> session_; scoped_ptr<SyncSessionContext> context_; + std::vector<scoped_refptr<ModelSafeWorker> > workers_; ModelSafeRoutingInfo routes_; }; +TEST_F(SyncSessionTest, EnabledGroupsEmpty) { + routes_.clear(); + workers_.clear(); + scoped_ptr<SyncSession> session(MakeSession()); + std::set<ModelSafeGroup> expected_enabled_groups; + expected_enabled_groups.insert(GROUP_PASSIVE); + EXPECT_EQ(expected_enabled_groups, session->GetEnabledGroups()); +} + +TEST_F(SyncSessionTest, EnabledGroups) { + scoped_ptr<SyncSession> session(MakeSession()); + std::set<ModelSafeGroup> expected_enabled_groups; + expected_enabled_groups.insert(GROUP_PASSIVE); + expected_enabled_groups.insert(GROUP_UI); + expected_enabled_groups.insert(GROUP_DB); + EXPECT_EQ(expected_enabled_groups, session->GetEnabledGroups()); +} + +TEST_F(SyncSessionTest, EnabledGroupsWithConflictsEmpty) { + scoped_ptr<SyncSession> session(MakeSession()); + // Auto-create conflict progress. This shouldn't put that group in + // conflict. + session->mutable_status_controller()-> + GetUnrestrictedMutableConflictProgressForTest(GROUP_PASSIVE); + EXPECT_TRUE(session->GetEnabledGroupsWithConflicts().empty()); +} + +TEST_F(SyncSessionTest, EnabledGroupsWithConflicts) { + scoped_ptr<SyncSession> session(MakeSession()); + // Put GROUP_UI in conflict. + session->mutable_status_controller()-> + GetUnrestrictedMutableConflictProgressForTest(GROUP_UI)-> + AddConflictingItemById(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, + session->GetEnabledGroupsWithConflicts()); +} + TEST_F(SyncSessionTest, ScopedContextHelpers) { ConflictResolver resolver; EXPECT_FALSE(context_->resolver()); @@ -288,9 +347,13 @@ TEST_F(SyncSessionTest, Coalesce) { SyncSourceInfo source_one(sync_pb::GetUpdatesCallerInfo::PERIODIC, one_type); SyncSourceInfo source_two(sync_pb::GetUpdatesCallerInfo::LOCAL, all_types); + scoped_refptr<ModelSafeWorker> passive_worker( + new FakeModelWorker(GROUP_PASSIVE)); scoped_refptr<ModelSafeWorker> db_worker(new FakeModelWorker(GROUP_DB)); scoped_refptr<ModelSafeWorker> ui_worker(new FakeModelWorker(GROUP_UI)); + workers_one.push_back(passive_worker); workers_one.push_back(db_worker); + workers_two.push_back(passive_worker); workers_two.push_back(db_worker); workers_two.push_back(ui_worker); routes_one[syncable::AUTOFILL] = GROUP_DB; @@ -299,8 +362,23 @@ TEST_F(SyncSessionTest, Coalesce) { SyncSession one(context_.get(), this, source_one, routes_one, workers_one); SyncSession two(context_.get(), this, source_two, routes_two, workers_two); + std::set<ModelSafeGroup> expected_enabled_groups_one; + expected_enabled_groups_one.insert(GROUP_PASSIVE); + expected_enabled_groups_one.insert(GROUP_DB); + + std::set<ModelSafeGroup> expected_enabled_groups_two; + expected_enabled_groups_two.insert(GROUP_PASSIVE); + expected_enabled_groups_two.insert(GROUP_DB); + expected_enabled_groups_two.insert(GROUP_UI); + + EXPECT_EQ(expected_enabled_groups_one, one.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups_two, two.GetEnabledGroups()); + one.Coalesce(two); + EXPECT_EQ(expected_enabled_groups_two, one.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups_two, two.GetEnabledGroups()); + EXPECT_EQ(two.source().updates_source, one.source().updates_source); EXPECT_EQ(all_types, one.source().types); std::vector<ModelSafeWorker*>::const_iterator it_db = @@ -326,18 +404,37 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestRemoveOneType) { SyncSourceInfo source_one(sync_pb::GetUpdatesCallerInfo::PERIODIC, one_type); SyncSourceInfo source_two(sync_pb::GetUpdatesCallerInfo::LOCAL, all_types); + scoped_refptr<ModelSafeWorker> passive_worker( + new FakeModelWorker(GROUP_PASSIVE)); scoped_refptr<ModelSafeWorker> db_worker(new FakeModelWorker(GROUP_DB)); scoped_refptr<ModelSafeWorker> ui_worker(new FakeModelWorker(GROUP_UI)); + workers_one.push_back(passive_worker); workers_one.push_back(db_worker); + workers_two.push_back(passive_worker); workers_two.push_back(db_worker); workers_two.push_back(ui_worker); routes_one[syncable::AUTOFILL] = GROUP_DB; - routes_two[syncable::AUTOFILL] = GROUP_UI; + routes_two[syncable::AUTOFILL] = GROUP_DB; routes_two[syncable::BOOKMARKS] = GROUP_UI; SyncSession one(context_.get(), this, source_one, routes_one, workers_one); SyncSession two(context_.get(), this, source_two, routes_two, workers_two); - two.RebaseRoutingInfoWithLatest(&one); + std::set<ModelSafeGroup> expected_enabled_groups_one; + expected_enabled_groups_one.insert(GROUP_PASSIVE); + expected_enabled_groups_one.insert(GROUP_DB); + + std::set<ModelSafeGroup> expected_enabled_groups_two; + expected_enabled_groups_two.insert(GROUP_PASSIVE); + expected_enabled_groups_two.insert(GROUP_DB); + expected_enabled_groups_two.insert(GROUP_UI); + + EXPECT_EQ(expected_enabled_groups_one, one.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups_two, two.GetEnabledGroups()); + + two.RebaseRoutingInfoWithLatest(one); + + EXPECT_EQ(expected_enabled_groups_one, one.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups_one, two.GetEnabledGroups()); // Make sure the source has not been touched. EXPECT_EQ(two.source().updates_source, @@ -353,7 +450,7 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestRemoveOneType) { std::find(two.workers().begin(), two.workers().end(), ui_worker); EXPECT_NE(it_db, two.workers().end()); EXPECT_EQ(it_ui, two.workers().end()); - EXPECT_EQ(two.workers().size(), 1U); + EXPECT_EQ(two.workers().size(), 2U); // Make sure the model safe routing info is reduced to one type. ModelSafeRoutingInfo::const_iterator it = @@ -375,10 +472,14 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestWithSameType) { SyncSourceInfo source_second(sync_pb::GetUpdatesCallerInfo::LOCAL, all_types); + scoped_refptr<ModelSafeWorker> passive_worker( + new FakeModelWorker(GROUP_PASSIVE)); scoped_refptr<FakeModelWorker> db_worker(new FakeModelWorker(GROUP_DB)); scoped_refptr<FakeModelWorker> ui_worker(new FakeModelWorker(GROUP_UI)); + workers_first.push_back(passive_worker); workers_first.push_back(db_worker); workers_first.push_back(ui_worker); + workers_second.push_back(passive_worker); workers_second.push_back(db_worker); workers_second.push_back(ui_worker); routes_first[syncable::AUTOFILL] = GROUP_DB; @@ -390,7 +491,18 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestWithSameType) { SyncSession second(context_.get(), this, source_second, routes_second, workers_second); - second.RebaseRoutingInfoWithLatest(&first); + std::set<ModelSafeGroup> expected_enabled_groups; + expected_enabled_groups.insert(GROUP_PASSIVE); + expected_enabled_groups.insert(GROUP_DB); + expected_enabled_groups.insert(GROUP_UI); + + EXPECT_EQ(expected_enabled_groups, first.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups, second.GetEnabledGroups()); + + second.RebaseRoutingInfoWithLatest(first); + + EXPECT_EQ(expected_enabled_groups, first.GetEnabledGroups()); + EXPECT_EQ(expected_enabled_groups, second.GetEnabledGroups()); // Make sure the source has not been touched. EXPECT_EQ(second.source().updates_source, @@ -400,13 +512,17 @@ TEST_F(SyncSessionTest, RebaseRoutingInfoWithLatestWithSameType) { EXPECT_EQ(all_types, second.source().types); // Make sure the workers are still the same. + std::vector<ModelSafeWorker*>::const_iterator it_passive = + std::find(second.workers().begin(), second.workers().end(), + passive_worker); std::vector<ModelSafeWorker*>::const_iterator it_db = std::find(second.workers().begin(), second.workers().end(), db_worker); std::vector<ModelSafeWorker*>::const_iterator it_ui = std::find(second.workers().begin(), second.workers().end(), ui_worker); + EXPECT_NE(it_passive, second.workers().end()); EXPECT_NE(it_db, second.workers().end()); EXPECT_NE(it_ui, second.workers().end()); - EXPECT_EQ(second.workers().size(), 2U); + EXPECT_EQ(second.workers().size(), 3U); // Make sure the model safe routing info is reduced to first type. ModelSafeRoutingInfo::const_iterator it1 = diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc index 0050fbf..6a931f3 100644 --- a/chrome/browser/sync/syncable/syncable.cc +++ b/chrome/browser/sync/syncable/syncable.cc @@ -258,6 +258,20 @@ EntryKernel::EntryKernel() : dirty_(false) { EntryKernel::~EntryKernel() {} +syncable::ModelType EntryKernel::GetServerModelType() const { + ModelType specifics_type = GetModelTypeFromSpecifics(ref(SERVER_SPECIFICS)); + if (specifics_type != UNSPECIFIED) + return specifics_type; + if (ref(ID).IsRoot()) + return TOP_LEVEL_FOLDER; + // Loose check for server-created top-level folders that aren't + // bound to a particular model type. + if (!ref(UNIQUE_SERVER_TAG).empty() && ref(SERVER_IS_DIR)) + return TOP_LEVEL_FOLDER; + + return UNSPECIFIED; +} + bool EntryKernel::ContainsString(const std::string& lowercase_query) const { // TODO(lipalani) - figure out what to do if the node is encrypted. const sync_pb::EntitySpecifics& specifics = ref(SPECIFICS); @@ -328,6 +342,7 @@ StringValue* IdToValue(const Id& id) { DictionaryValue* EntryKernel::ToValue() const { DictionaryValue* kernel_info = new DictionaryValue(); kernel_info->SetBoolean("isDirty", is_dirty()); + kernel_info->Set("serverModelType", ModelTypeToValue(GetServerModelType())); // Int64 fields. SetFieldValues(*this, kernel_info, @@ -433,7 +448,6 @@ Directory::Kernel::Kernel( ids_index(new Directory::IdsIndex), parent_id_child_index(new Directory::ParentIdChildIndex), client_tag_index(new Directory::ClientTagIndex), - unapplied_update_metahandles(new MetahandleSet), unsynced_metahandles(new MetahandleSet), dirty_metahandles(new MetahandleSet), metahandles_to_purge(new MetahandleSet), @@ -459,7 +473,6 @@ void Directory::Kernel::Release() { Directory::Kernel::~Kernel() { CHECK_EQ(0, refcount); delete unsynced_metahandles; - delete unapplied_update_metahandles; delete dirty_metahandles; delete metahandles_to_purge; delete parent_id_child_index; @@ -496,10 +509,13 @@ void Directory::InitializeIndices() { kernel_->parent_id_child_index); InitializeIndexEntry<IdIndexer>(entry, kernel_->ids_index); InitializeIndexEntry<ClientTagIndexer>(entry, kernel_->client_tag_index); + const int64 metahandle = entry->ref(META_HANDLE); if (entry->ref(IS_UNSYNCED)) - kernel_->unsynced_metahandles->insert(entry->ref(META_HANDLE)); - if (entry->ref(IS_UNAPPLIED_UPDATE)) - kernel_->unapplied_update_metahandles->insert(entry->ref(META_HANDLE)); + kernel_->unsynced_metahandles->insert(metahandle); + if (entry->ref(IS_UNAPPLIED_UPDATE)) { + const ModelType type = entry->GetServerModelType(); + kernel_->unapplied_update_metahandles[type].insert(metahandle); + } DCHECK(!entry->is_dirty()); } } @@ -702,11 +718,12 @@ bool Directory::SafeToPurgeFromMemory(const EntryKernel* const entry) const { !entry->ref(IS_UNSYNCED); if (safe) { - int64 handle = entry->ref(META_HANDLE); + const int64 handle = entry->ref(META_HANDLE); + const ModelType type = entry->GetServerModelType(); CHECK_EQ(kernel_->dirty_metahandles->count(handle), 0U); // TODO(tim): Bug 49278. CHECK(!kernel_->unsynced_metahandles->count(handle)); - CHECK(!kernel_->unapplied_update_metahandles->count(handle)); + CHECK(!kernel_->unapplied_update_metahandles[type].count(handle)); } return safe; @@ -837,7 +854,8 @@ void Directory::PurgeEntriesWithTypeIn(const std::set<ModelType>& types) { DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased); num_erased = kernel_->unsynced_metahandles->erase(handle); DCHECK_EQ(entry->ref(IS_UNSYNCED), num_erased > 0); - num_erased = kernel_->unapplied_update_metahandles->erase(handle); + num_erased = + kernel_->unapplied_update_metahandles[server_type].erase(handle); DCHECK_EQ(entry->ref(IS_UNAPPLIED_UPDATE), num_erased > 0); num_erased = kernel_->parent_id_child_index->erase(entry); DCHECK_EQ(entry->ref(IS_DEL), !num_erased); @@ -1007,13 +1025,33 @@ int64 Directory::unsynced_entity_count() const { return kernel_->unsynced_metahandles->size(); } -void Directory::GetUnappliedUpdateMetaHandles(BaseTransaction* trans, +syncable::ModelTypeBitSet + Directory::GetServerTypesWithUnappliedUpdates( + BaseTransaction* trans) const { + syncable::ModelTypeBitSet server_types; + ScopedKernelLock lock(this); + for (int i = 0; i < MODEL_TYPE_COUNT; ++i) { + if (!kernel_->unapplied_update_metahandles[i].empty()) { + server_types.set(i); + } + } + return server_types; +} + +void Directory::GetUnappliedUpdateMetaHandles( + BaseTransaction* trans, + syncable::ModelTypeBitSet server_types, UnappliedUpdateMetaHandles* result) { result->clear(); ScopedKernelLock lock(this); - copy(kernel_->unapplied_update_metahandles->begin(), - kernel_->unapplied_update_metahandles->end(), - back_inserter(*result)); + for (int i = 0; i < MODEL_TYPE_COUNT; ++i) { + const ModelType type = ModelTypeFromInt(i); + if (server_types.test(type)) { + std::copy(kernel_->unapplied_update_metahandles[type].begin(), + kernel_->unapplied_update_metahandles[type].end(), + back_inserter(*result)); + } + } } @@ -1370,8 +1408,6 @@ DictionaryValue* Entry::ToValue() const { entry_info->SetBoolean("good", good()); if (good()) { entry_info->Set("kernel", kernel_->ToValue()); - entry_info->Set("serverModelType", - ModelTypeToValue(GetServerModelTypeHelper())); entry_info->Set("modelType", ModelTypeToValue(GetModelType())); entry_info->SetBoolean("existsOnClientBecauseNameIsNonEmpty", @@ -1387,7 +1423,7 @@ const string& Entry::Get(StringField field) const { } syncable::ModelType Entry::GetServerModelType() const { - ModelType specifics_type = GetServerModelTypeHelper(); + ModelType specifics_type = kernel_->GetServerModelType(); if (specifics_type != UNSPECIFIED) return specifics_type; @@ -1403,20 +1439,6 @@ syncable::ModelType Entry::GetServerModelType() const { return UNSPECIFIED; } -syncable::ModelType Entry::GetServerModelTypeHelper() const { - ModelType specifics_type = GetModelTypeFromSpecifics(Get(SERVER_SPECIFICS)); - if (specifics_type != UNSPECIFIED) - return specifics_type; - if (IsRoot()) - return TOP_LEVEL_FOLDER; - // Loose check for server-created top-level folders that aren't - // bound to a particular model type. - if (!Get(UNIQUE_SERVER_TAG).empty() && Get(SERVER_IS_DIR)) - return TOP_LEVEL_FOLDER; - - return UNSPECIFIED; -} - syncable::ModelType Entry::GetModelType() const { ModelType specifics_type = GetModelTypeFromSpecifics(Get(SPECIFICS)); if (specifics_type != UNSPECIFIED) @@ -1603,8 +1625,32 @@ bool MutableEntry::Put(ProtoField field, // TODO(ncarter): This is unfortunately heavyweight. Can we do // better? if (kernel_->ref(field).SerializeAsString() != value.SerializeAsString()) { + const bool update_unapplied_updates_index = + (field == SERVER_SPECIFICS) && kernel_->ref(IS_UNAPPLIED_UPDATE); + if (update_unapplied_updates_index) { + // Remove ourselves from unapplied_update_metahandles with our + // old server type. + const syncable::ModelType old_server_type = + kernel_->GetServerModelType(); + const int64 metahandle = kernel_->ref(META_HANDLE); + size_t erase_count = + dir()->kernel_->unapplied_update_metahandles[old_server_type] + .erase(metahandle); + DCHECK_EQ(erase_count, 1u); + } + kernel_->put(field, value); kernel_->mark_dirty(dir()->kernel_->dirty_metahandles); + + if (update_unapplied_updates_index) { + // Add ourselves back into unapplied_update_metahandles with our + // new server type. + const syncable::ModelType new_server_type = + kernel_->GetServerModelType(); + const int64 metahandle = kernel_->ref(META_HANDLE); + dir()->kernel_->unapplied_update_metahandles[new_server_type] + .insert(metahandle); + } } return true; } @@ -1667,10 +1713,16 @@ bool MutableEntry::Put(IndexedBitField field, bool value) { DCHECK(kernel_); if (kernel_->ref(field) != value) { MetahandleSet* index; - if (IS_UNSYNCED == field) + if (IS_UNSYNCED == field) { index = dir()->kernel_->unsynced_metahandles; - else - index = dir()->kernel_->unapplied_update_metahandles; + } else { + // Use kernel_->GetServerModelType() instead of + // GetServerModelType() as we may trigger some DCHECKs in the + // latter. + index = + &dir()->kernel_->unapplied_update_metahandles[ + kernel_->GetServerModelType()]; + } ScopedKernelLock lock(dir()); if (value) diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h index 64c518b..73e71d5 100644 --- a/chrome/browser/sync/syncable/syncable.h +++ b/chrome/browser/sync/syncable/syncable.h @@ -371,6 +371,8 @@ struct EntryKernel { return id_fields[field - ID_FIELDS_BEGIN]; } + syncable::ModelType GetServerModelType() const; + // Does a case in-sensitive search for a given string, which must be // lower case. bool ContainsString(const std::string& lowercase_query) const; @@ -487,9 +489,6 @@ class Entry { EntryKernel* kernel_; private: - // Like GetServerModelType() but without the DCHECKs. - ModelType GetServerModelTypeHelper() const; - DISALLOW_COPY_AND_ASSIGN(Entry); }; @@ -969,9 +968,17 @@ class Directory { void GetUnsyncedMetaHandles(BaseTransaction* trans, UnsyncedMetaHandles* result); - // Get all the metahandles for unapplied updates + // Returns all server types with unapplied updates. A subset of + // those types can then be passed into + // GetUnappliedUpdateMetaHandles() below. + syncable::ModelTypeBitSet GetServerTypesWithUnappliedUpdates( + BaseTransaction* trans) const; + + // Get all the metahandles for unapplied updates for a given set of + // server types. typedef std::vector<int64> UnappliedUpdateMetaHandles; void GetUnappliedUpdateMetaHandles(BaseTransaction* trans, + syncable::ModelTypeBitSet server_types, UnappliedUpdateMetaHandles* result); // Checks tree metadata consistency. @@ -1103,7 +1110,8 @@ class Directory { EntryKernel needle; // 3 in-memory indices on bits used extremely frequently by the syncer. - MetahandleSet* const unapplied_update_metahandles; + // |unapplied_update_metahandles| is keyed by the server model type. + MetahandleSet unapplied_update_metahandles[MODEL_TYPE_COUNT]; MetahandleSet* const unsynced_metahandles; // Contains metahandles that are most likely dirty (though not // necessarily). Dirtyness is confirmed in TakeSnapshotForSaveChanges(). diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc index 1b6fc38..b7fcb6e 100644 --- a/chrome/browser/sync/syncable/syncable_unittest.cc +++ b/chrome/browser/sync/syncable/syncable_unittest.cc @@ -47,8 +47,8 @@ TEST_F(SyncableKernelTest, ToValue) { if (value.get()) { // Not much to check without repeating the ToValue() code. EXPECT_TRUE(value->HasKey("isDirty")); - // The extra +1 is for "isDirty". - EXPECT_EQ(BIT_TEMPS_END - BEGIN_FIELDS + 1, + // The extra +2 is for "isDirty" and "serverModelType". + EXPECT_EQ(BIT_TEMPS_END - BEGIN_FIELDS + 2, static_cast<int>(value->size())); } else { ADD_FAILURE(); @@ -370,7 +370,6 @@ TEST_F(SyncableGeneralTest, ToValue) { scoped_ptr<DictionaryValue> value(me.ToValue()); ExpectDictBooleanValue(true, *value, "good"); EXPECT_TRUE(value->HasKey("kernel")); - ExpectDictStringValue("Unspecified", *value, "serverModelType"); ExpectDictStringValue("Unspecified", *value, "modelType"); ExpectDictBooleanValue(true, *value, "existsOnClientBecauseNameIsNonEmpty"); ExpectDictBooleanValue(false, *value, "isRoot"); @@ -880,10 +879,12 @@ TEST_F(SyncableDirectoryTest, TestGetUnsynced) { TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) { Directory::UnappliedUpdateMetaHandles handles; int64 handle1, handle2; + syncable::ModelTypeBitSet all_types; + all_types.set(); { WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get()); - dir_->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles); ASSERT_TRUE(0 == handles.size()); MutableEntry e1(&trans, CREATE, trans.root_id(), "abba"); @@ -905,7 +906,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) { { WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get()); - dir_->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles); ASSERT_TRUE(0 == handles.size()); MutableEntry e3(&trans, GET_BY_HANDLE, handle1); @@ -915,7 +916,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) { dir_->SaveChanges(); { WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get()); - dir_->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles); ASSERT_TRUE(1 == handles.size()); ASSERT_TRUE(handle1 == handles[0]); @@ -926,7 +927,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) { dir_->SaveChanges(); { WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get()); - dir_->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles); ASSERT_TRUE(2 == handles.size()); if (handle1 == handles[0]) { ASSERT_TRUE(handle2 == handles[1]); @@ -942,7 +943,7 @@ TEST_F(SyncableDirectoryTest, TestGetUnappliedUpdates) { dir_->SaveChanges(); { WriteTransaction trans(FROM_HERE, UNITTEST, dir_.get()); - dir_->GetUnappliedUpdateMetaHandles(&trans, &handles); + dir_->GetUnappliedUpdateMetaHandles(&trans, all_types, &handles); ASSERT_TRUE(1 == handles.size()); ASSERT_TRUE(handle2 == handles[0]); } diff --git a/chrome/browser/sync/test/engine/fake_model_safe_worker_registrar.cc b/chrome/browser/sync/test/engine/fake_model_safe_worker_registrar.cc index 15a4d45f9..21ab061 100644 --- a/chrome/browser/sync/test/engine/fake_model_safe_worker_registrar.cc +++ b/chrome/browser/sync/test/engine/fake_model_safe_worker_registrar.cc @@ -15,6 +15,8 @@ FakeModelSafeWorkerRegistrar::FakeModelSafeWorkerRegistrar( it != routes_.end(); ++it) { groups.insert(it->second); } + // Sessions always expect a passive worker to be present. + groups.insert(GROUP_PASSIVE); for (std::set<ModelSafeGroup>::const_iterator it = groups.begin(); it != groups.end(); ++it) { diff --git a/chrome/browser/sync/test/engine/syncer_command_test.h b/chrome/browser/sync/test/engine/syncer_command_test.h index c6fccbb..2b4e17a 100644 --- a/chrome/browser/sync/test/engine/syncer_command_test.h +++ b/chrome/browser/sync/test/engine/syncer_command_test.h @@ -11,12 +11,15 @@ #include <vector> #include "base/compiler_specific.h" +#include "base/memory/ref_counted.h" #include "base/message_loop.h" +#include "chrome/browser/sync/engine/model_changing_syncer_command.h" #include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/sessions/debug_info_getter.h" #include "chrome/browser/sync/sessions/sync_session.h" #include "chrome/browser/sync/sessions/sync_session_context.h" #include "chrome/browser/sync/test/engine/mock_connection_manager.h" +#include "chrome/browser/sync/test/engine/fake_model_worker.h" #include "chrome/browser/sync/test/engine/test_directory_setter_upper.h" #include "testing/gtest/include/gtest/gtest.h" #include "testing/gmock/include/gmock/gmock.h" @@ -93,6 +96,9 @@ class SyncerCommandTestWithParam : public testing::TestWithParam<T>, virtual void SetUp() { syncdb_->SetUp(); ResetContext(); + // The session always expects there to be a passive worker. + workers()->push_back( + make_scoped_refptr(new FakeModelWorker(GROUP_PASSIVE))); } virtual void TearDown() { syncdb_->TearDown(); @@ -157,6 +163,41 @@ class SyncerCommandTestWithParam : public testing::TestWithParam<T>, return &mock_debug_info_getter_; } + // Helper functions to check command.GetGroupsToChange(). + + void ExpectNoGroupsToChange(const ModelChangingSyncerCommand& command) { + EXPECT_TRUE(command.GetGroupsToChangeForTest(*session()).empty()); + } + + void ExpectGroupToChange( + const ModelChangingSyncerCommand& command, ModelSafeGroup group) { + std::set<ModelSafeGroup> expected_groups_to_change; + expected_groups_to_change.insert(group); + EXPECT_EQ(expected_groups_to_change, + command.GetGroupsToChangeForTest(*session())); + } + + void ExpectGroupsToChange( + const ModelChangingSyncerCommand& command, + ModelSafeGroup group1, ModelSafeGroup group2) { + std::set<ModelSafeGroup> expected_groups_to_change; + expected_groups_to_change.insert(group1); + expected_groups_to_change.insert(group2); + EXPECT_EQ(expected_groups_to_change, + command.GetGroupsToChangeForTest(*session())); + } + + void ExpectGroupsToChange( + const ModelChangingSyncerCommand& command, + ModelSafeGroup group1, ModelSafeGroup group2, ModelSafeGroup group3) { + std::set<ModelSafeGroup> expected_groups_to_change; + expected_groups_to_change.insert(group1); + expected_groups_to_change.insert(group2); + expected_groups_to_change.insert(group3); + EXPECT_EQ(expected_groups_to_change, + command.GetGroupsToChangeForTest(*session())); + } + private: MessageLoop message_loop_; scoped_ptr<TestDirectorySetterUpper> syncdb_; diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 63137cf..07a82a3 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -3231,12 +3231,16 @@ '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', + 'browser/sync/engine/model_changing_syncer_command_unittest.cc', 'browser/sync/engine/model_safe_worker_unittest.cc', 'browser/sync/engine/nigori_util_unittest.cc', 'browser/sync/engine/process_commit_response_command_unittest.cc', + 'browser/sync/engine/process_updates_command_unittest.cc', + 'browser/sync/engine/resolve_conflicts_command_unittest.cc', 'browser/sync/engine/syncer_proto_util_unittest.cc', 'browser/sync/engine/sync_scheduler_unittest.cc', 'browser/sync/engine/sync_scheduler_whitebox_unittest.cc', |