diff options
author | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-12-02 18:36:21 +0000 |
---|---|---|
committer | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-12-02 18:36:21 +0000 |
commit | 932a7c831a73253afe68d8368e74f71ae0101e4e (patch) | |
tree | 88339c4bf1a4d7acbf287b52a0ecd0ff4388232e /chrome | |
parent | 64688e49023269bc2cde0d7890dccf665c1195c2 (diff) | |
download | chromium_src-932a7c831a73253afe68d8368e74f71ae0101e4e.zip chromium_src-932a7c831a73253afe68d8368e74f71ae0101e4e.tar.gz chromium_src-932a7c831a73253afe68d8368e74f71ae0101e4e.tar.bz2 |
[Sync] Make syncer commands avoid posting tasks on threads with no work to do
Add abstract GetGroupsToChange() method to ModelChangingSyncerCommand.
Use that to figure out which worker threads to post work on (instead of
posting on all of them).
Implement GetGroupsToChange() for each ModelChangingSyncerCommand.
Add GetEnabledGroups() and GetEnabledGroupsWithConflicts() functions to SyncSession.
Key unapplied updates index by type for ApplyUpdatesCommand.
Make the abstract methods of ModelChangingSyncerCommand protected.
This patch includes a speculative fix for the perf regression introduced by the last time this was landed (112178).
BUG=97832
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112178
Review URL: http://codereview.chromium.org/8637006
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112743 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
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', |