diff options
| author | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-10-06 00:20:23 +0000 | 
|---|---|---|
| committer | rlarocque@chromium.org <rlarocque@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-10-06 00:20:23 +0000 | 
| commit | ff390b8451249f70326d38fe12ed1166f26164d2 (patch) | |
| tree | ae0854550e3e0e6bdc12c49f28530082c97aa1b0 | |
| parent | 574eedcdeb1f43a7f26f647f2f4c87cb9cc73c35 (diff) | |
| download | chromium_src-ff390b8451249f70326d38fe12ed1166f26164d2.zip chromium_src-ff390b8451249f70326d38fe12ed1166f26164d2.tar.gz chromium_src-ff390b8451249f70326d38fe12ed1166f26164d2.tar.bz2 | |
sync: Remove ConflictProgress
This change removes the ConflictProgress struct from the
PerModelSafeGroupState.  The struct was intended to pass state between
the update application and conflict resolution commands.  This part of
its functionality has been replaced with a vector of simple conflict
item IDs.
The ConflictProgress struct also had an important role to play in stats
gathering and unit tests.  To replace it, the update applicator has been
updated to store its counts directly in the StatusController.  Unlike
ConflictProgress, this state is not thread-local. That's acceptable
because we can guarantee it will only be read or written by one thread
at a time and those threads use appropriate thread-safety mechanisms to
transfer control to each other.
This change is another step towards merging update application and
conflict resolution.
BUG=147681
Review URL: https://chromiumcodereview.appspot.com/11049002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@160530 0039d316-1c4b-4281-b951-d872f2087c98
| -rw-r--r-- | sync/engine/apply_updates_command.cc | 5 | ||||
| -rw-r--r-- | sync/engine/apply_updates_command_unittest.cc | 118 | ||||
| -rw-r--r-- | sync/engine/process_commit_response_command.cc | 5 | ||||
| -rw-r--r-- | sync/engine/resolve_conflicts_command.cc | 8 | ||||
| -rw-r--r-- | sync/engine/resolve_conflicts_command_unittest.cc | 51 | ||||
| -rw-r--r-- | sync/engine/syncer.cc | 5 | ||||
| -rw-r--r-- | sync/engine/syncer_unittest.cc | 24 | ||||
| -rw-r--r-- | sync/engine/update_applicator.cc | 18 | ||||
| -rw-r--r-- | sync/engine/update_applicator.h | 10 | ||||
| -rw-r--r-- | sync/internal_api/public/sessions/model_neutral_state.cc | 4 | ||||
| -rw-r--r-- | sync/internal_api/public/sessions/model_neutral_state.h | 6 | ||||
| -rw-r--r-- | sync/sessions/session_state.cc | 54 | ||||
| -rw-r--r-- | sync/sessions/session_state.h | 49 | ||||
| -rw-r--r-- | sync/sessions/status_controller.cc | 97 | ||||
| -rw-r--r-- | sync/sessions/status_controller.h | 51 | ||||
| -rw-r--r-- | sync/sessions/status_controller_unittest.cc | 22 | ||||
| -rw-r--r-- | sync/sessions/sync_session.cc | 16 | ||||
| -rw-r--r-- | sync/sessions/sync_session_unittest.cc | 21 | ||||
| -rw-r--r-- | sync/sync.gyp | 1 | 
19 files changed, 178 insertions, 387 deletions
| diff --git a/sync/engine/apply_updates_command.cc b/sync/engine/apply_updates_command.cc index 639aef7..e43f071 100644 --- a/sync/engine/apply_updates_command.cc +++ b/sync/engine/apply_updates_command.cc @@ -69,9 +69,10 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl(        dir->GetCryptographer(&trans),        session->routing_info(),        session->status_controller().group_restriction()); -  applicator.AttemptApplications(&trans, handles); +  applicator.AttemptApplications(&trans, handles, +                                 session->mutable_status_controller());    applicator.SaveProgressIntoSessionState( -      session->mutable_status_controller()->mutable_conflict_progress(), +      session->mutable_status_controller()->mutable_simple_conflict_ids(),        session->mutable_status_controller()->mutable_update_progress());    // This might be the first time we've fully completed a sync cycle, for diff --git a/sync/engine/apply_updates_command_unittest.cc b/sync/engine/apply_updates_command_unittest.cc index eee3edb..01ddd93 100644 --- a/sync/engine/apply_updates_command_unittest.cc +++ b/sync/engine/apply_updates_command_unittest.cc @@ -83,17 +83,16 @@ TEST_F(ApplyUpdatesCommandTest, Simple) {    sessions::StatusController* status = session()->mutable_status_controller(); +  EXPECT_EQ(0, status->num_simple_conflicts()) +      << "Simple update shouldn't result in conflicts"; +  EXPECT_EQ(0, status->num_encryption_conflicts()) +      << "Simple update shouldn't result in conflicts"; +  EXPECT_EQ(0, status->num_hierarchy_conflicts()) +      << "Simple update shouldn't result in conflicts";    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);    ASSERT_TRUE(status->update_progress());    EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())        << "All updates should have been attempted"; -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -      << "Simple update shouldn't result in conflicts"; -  EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) -      << "Simple update shouldn't result in conflicts"; -  EXPECT_EQ(0, status->conflict_progress()->HierarchyConflictingItemsSize()) -      << "Simple update shouldn't result in conflicts";    EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount())        << "All items should have been successfully applied";  } @@ -117,13 +116,12 @@ TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); +  EXPECT_EQ(0, status->num_simple_conflicts()) +      << "Simple update shouldn't result in conflicts, even if out-of-order";    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);    ASSERT_TRUE(status->update_progress());    EXPECT_EQ(5, status->update_progress()->AppliedUpdatesSize())        << "All updates should have been attempted"; -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -      << "Simple update shouldn't result in conflicts, even if out-of-order";    EXPECT_EQ(5, status->update_progress()->SuccessfullyAppliedUpdateCount())        << "All updates should have been successfully applied";  } @@ -139,9 +137,7 @@ TEST_F(ApplyUpdatesCommandTest, SimpleConflict) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); -  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(1, status->conflict_progress()->SimpleConflictingItemsSize()) +  EXPECT_EQ(1, status->num_simple_conflicts())        << "Unsynced and unapplied item should be a simple conflict";  } @@ -169,15 +165,14 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); -  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); - -  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());    // An update that is both a simple conflict and a hierarchy conflict should be    // treated as a hierarchy conflict. -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +  EXPECT_EQ(1, status->num_hierarchy_conflicts()); +  EXPECT_EQ(0, status->num_simple_conflicts()); + +  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); +  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());  } @@ -216,14 +211,13 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); -  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); - -  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());    // This should count as a hierarchy conflict. -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +  EXPECT_EQ(1, status->num_hierarchy_conflicts()); +  EXPECT_EQ(0, status->num_simple_conflicts()); + +  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); +  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());  }  // Runs the ApplyUpdatesCommand on a directory where the server sent us an @@ -254,12 +248,8 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); -  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); - -  // This should count as a hierarchy conflict. -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +  EXPECT_EQ(1, status->num_hierarchy_conflicts()); +  EXPECT_EQ(0, status->num_simple_conflicts());  }  // Runs the ApplyUpdatesCommand on a directory where the server is trying to @@ -296,12 +286,9 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); -  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI); -    // This should count as a hierarchy conflict. -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()); +  EXPECT_EQ(1, status->num_hierarchy_conflicts()); +  EXPECT_EQ(0, status->num_simple_conflicts());  }  // Runs the ApplyUpdatesCommand on a server-created item that has a locally @@ -318,16 +305,17 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictUnknownParent) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); + +  EXPECT_EQ(0, status->num_simple_conflicts()) +      << "Updates with unknown parent should not be treated as 'simple'" +      << " conflicts"; +  EXPECT_EQ(2, status->num_hierarchy_conflicts()) +      << "All updates with an unknown ancestors should be in conflict"; +    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);    ASSERT_TRUE(status->update_progress());    EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())        << "All updates should have been attempted"; -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -      << "Updates with unknown parent should not be treated as 'simple'" -      << " conflicts"; -  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize()) -      << "All updates with an unknown ancestors should be in conflict";    EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())        << "No item with an unknown ancestor should be applied";  } @@ -352,13 +340,14 @@ TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); + +  EXPECT_EQ(2, status->num_hierarchy_conflicts()) +      << "The updates with unknown ancestors should be in conflict"; +    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);    ASSERT_TRUE(status->update_progress());    EXPECT_EQ(6, status->update_progress()->AppliedUpdatesSize())        << "All updates should have been attempted"; -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize()) -      << "The updates with unknown ancestors should be in conflict";    EXPECT_EQ(4, status->update_progress()->SuccessfullyAppliedUpdateCount())        << "The updates with known ancestors should be successfully applied";  } @@ -388,13 +377,14 @@ TEST_F(ApplyUpdatesCommandTest, DecryptablePassword) {    apply_updates_command_.ExecuteImpl(session());    sessions::StatusController* status = session()->mutable_status_controller(); + +  EXPECT_EQ(0, status->num_simple_conflicts()) +      << "No update should be in conflict because they're all decryptable"; +    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD);    ASSERT_TRUE(status->update_progress());    EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())        << "All updates should have been attempted"; -  ASSERT_TRUE(status->conflict_progress()); -  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -      << "No update should be in conflict because they're all decryptable";    EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())        << "The updates that can be decrypted should be applied";  } @@ -419,18 +409,15 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) {    EXPECT_TRUE(status->HasConflictingUpdates())      << "Updates that can't be decrypted should trigger the syncer to have "      << "conflicting updates."; +  EXPECT_EQ(0, status->num_simple_conflicts()) +      << "Updates that can't be decrypted should not be in regular conflict"; +  EXPECT_EQ(3, status->num_encryption_conflicts()) +      << "Updates that can't be decrypted should be in encryption conflict";    {      sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);      ASSERT_TRUE(status->update_progress());      EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())          << "All updates should have been attempted"; -    ASSERT_TRUE(status->conflict_progress()); -    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -        << "The updates that can't be decrypted should not be in regular " -        << "conflict"; -    EXPECT_EQ(2, status->conflict_progress()->EncryptionConflictingItemsSize()) -        << "The updates that can't be decrypted should be in encryption " -        << "conflict";      EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())          << "No update that can't be decrypted should be applied";    } @@ -439,13 +426,6 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) {      ASSERT_TRUE(status->update_progress());      EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())          << "All updates should have been attempted"; -    ASSERT_TRUE(status->conflict_progress()); -    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) -        << "The updates that can't be decrypted should not be in regular " -        << "conflict"; -    EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize()) -        << "The updates that can't be decrypted should be in encryption " -        << "conflict";      EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())          << "No update that can't be decrypted should be applied";    } @@ -493,17 +473,17 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {      << "Updates that can't be decrypted should trigger the syncer to have "      << "conflicting updates.";    { -    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD); -    ASSERT_TRUE(status->update_progress()); -    EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) -        << "All updates should have been attempted"; -    ASSERT_TRUE(status->conflict_progress()); -    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) +    EXPECT_EQ(0, status->num_simple_conflicts())          << "The updates that can't be decrypted should not be in regular "          << "conflict"; -    EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize()) +    EXPECT_EQ(1, status->num_encryption_conflicts())          << "The updates that can't be decrypted should be in encryption "          << "conflict"; + +    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD); +    ASSERT_TRUE(status->update_progress()); +    EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) +        << "All updates should have been attempted";      EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())          << "The undecryptable password update shouldn't be applied";    } diff --git a/sync/engine/process_commit_response_command.cc b/sync/engine/process_commit_response_command.cc index 8a00608..2f84619 100644 --- a/sync/engine/process_commit_response_command.cc +++ b/sync/engine/process_commit_response_command.cc @@ -32,7 +32,6 @@ namespace syncer {  using sessions::OrderedCommitSet;  using sessions::StatusController;  using sessions::SyncSession; -using sessions::ConflictProgress;  using syncable::WriteTransaction;  using syncable::MutableEntry;  using syncable::Entry; @@ -110,7 +109,6 @@ SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(    int successes = 0;    set<syncable::Id> deleted_folders; -  ConflictProgress* conflict_progress = status->mutable_conflict_progress();    OrderedCommitSet::Projection proj = status->commit_id_projection(        commit_set_); @@ -129,8 +127,7 @@ SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(            break;          case CommitResponse::CONFLICT:            ++conflicting_commits; -          conflict_progress->AddServerConflictingItemById( -              commit_set_.GetCommitIdAt(proj[i])); +          status->increment_num_server_conflicts();            break;          case CommitResponse::SUCCESS:            // TODO(sync): worry about sync_rate_ rate calc? diff --git a/sync/engine/resolve_conflicts_command.cc b/sync/engine/resolve_conflicts_command.cc index 3adfa9d..5b7f5e2 100644 --- a/sync/engine/resolve_conflicts_command.cc +++ b/sync/engine/resolve_conflicts_command.cc @@ -27,14 +27,14 @@ SyncerError ResolveConflictsCommand::ModelChangingExecuteImpl(    syncable::Directory* dir = session->context()->directory();    sessions::StatusController* status = session->mutable_status_controller(); -  const sessions::ConflictProgress* progress = status->conflict_progress(); -  if (!progress) -    return SYNCER_OK;  // Nothing to do. +  const std::set<syncable::Id>* simple_conflict_ids = +      status->simple_conflict_ids(); +    syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);    const Cryptographer* cryptographer = dir->GetCryptographer(&trans);    status->update_conflicts_resolved(        resolver->ResolveConflicts(&trans, cryptographer, -                                 progress->SimpleConflictingItems(), status)); +                                 *simple_conflict_ids, status));    return SYNCER_OK;  } diff --git a/sync/engine/resolve_conflicts_command_unittest.cc b/sync/engine/resolve_conflicts_command_unittest.cc deleted file mode 100644 index 7a9bc14..0000000 --- a/sync/engine/resolve_conflicts_command_unittest.cc +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "base/basictypes.h" -#include "base/memory/ref_counted.h" -#include "sync/engine/resolve_conflicts_command.h" -#include "sync/internal_api/public/base/model_type.h" -#include "sync/sessions/sync_session.h" -#include "sync/syncable/syncable_id.h" -#include "sync/test/engine/fake_model_worker.h" -#include "sync/test/engine/syncer_command_test.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace syncer { - -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())[BOOKMARKS] = GROUP_UI; -    (*mutable_routing_info())[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)-> -      AddSimpleConflictingItemById(syncable::Id()); -  ExpectGroupToChange(command_, GROUP_PASSWORD); -} - -}  // namespace - -}  // namespace syncer diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index adb34b4..af8e3a9 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -36,7 +36,6 @@ namespace syncer {  using sessions::ScopedSessionContextConflictResolver;  using sessions::StatusController;  using sessions::SyncSession; -using sessions::ConflictProgress;  using syncable::IS_UNAPPLIED_UPDATE;  using syncable::SERVER_CTIME;  using syncable::SERVER_IS_DEL; @@ -197,10 +196,10 @@ void Syncer::SyncShare(sessions::SyncSession* session,          // We only care to resolve conflicts again if we made progress on the          // simple conflicts.          int before_blocking_conflicting_updates = -            status->TotalNumSimpleConflictingItems(); +            status->num_simple_conflicts();          apply_updates.Execute(session);          int after_blocking_conflicting_updates = -            status->TotalNumSimpleConflictingItems(); +            status->num_simple_conflicts();          // If the following call sets the conflicts_resolved value to true,          // SyncSession::HasMoreToSync() will send us into another sync cycle          // after this one completes. diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index da4f01c0..c57def1 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -105,7 +105,6 @@ using syncable::SPECIFICS;  using syncable::SYNCING;  using syncable::UNITTEST; -using sessions::ConflictProgress;  using sessions::ScopedSetSessionWriteTransaction;  using sessions::StatusController;  using sessions::SyncSessionContext; @@ -1583,12 +1582,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) {    // Id 3 should be in conflict now.    EXPECT_EQ(1, status().TotalNumConflictingItems()); -  { -    sessions::ScopedModelSafeGroupRestriction r( -        session_->mutable_status_controller(), GROUP_PASSIVE); -    ASSERT_TRUE(status().conflict_progress()); -    EXPECT_EQ(1, status().conflict_progress()->HierarchyConflictingItemsSize()); -  } +  EXPECT_EQ(1, status().num_hierarchy_conflicts());    // These entries will be used in the second set of updates.    mock_server_->AddUpdateDirectory(4, 0, "newer_version", 20, 10); @@ -1602,12 +1596,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) {    // The three items with an unresolved parent should be unapplied (3, 9, 100).    // The name clash should also still be in conflict.    EXPECT_EQ(3, status().TotalNumConflictingItems()); -  { -    sessions::ScopedModelSafeGroupRestriction r( -        session_->mutable_status_controller(), GROUP_PASSIVE); -    ASSERT_TRUE(status().conflict_progress()); -    EXPECT_EQ(3, status().conflict_progress()->HierarchyConflictingItemsSize()); -  } +  EXPECT_EQ(3, status().num_hierarchy_conflicts());    {      WriteTransaction trans(FROM_HERE, UNITTEST, directory()); @@ -1697,12 +1686,7 @@ TEST_F(SyncerTest, IllegalAndLegalUpdates) {    EXPECT_FALSE(saw_syncer_event_);    EXPECT_EQ(4, status().TotalNumConflictingItems()); -  { -    sessions::ScopedModelSafeGroupRestriction r( -        session_->mutable_status_controller(), GROUP_PASSIVE); -    ASSERT_TRUE(status().conflict_progress()); -    EXPECT_EQ(4, status().conflict_progress()->HierarchyConflictingItemsSize()); -  } +  EXPECT_EQ(4, status().num_hierarchy_conflicts());  }  TEST_F(SyncerTest, CommitTimeRename) { @@ -2392,7 +2376,7 @@ TEST_F(SyncerTest, DeletingEntryInFolder) {      existing.Put(IS_DEL, true);    }    syncer_->SyncShare(session_.get(), SYNCER_BEGIN, SYNCER_END); -  EXPECT_EQ(0, status().TotalNumServerConflictingItems()); +  EXPECT_EQ(0, status().num_server_conflicts());  }  TEST_F(SyncerTest, DeletingEntryWithLocalEdits) { diff --git a/sync/engine/update_applicator.cc b/sync/engine/update_applicator.cc index 3dedc14..cb09fc7 100644 --- a/sync/engine/update_applicator.cc +++ b/sync/engine/update_applicator.cc @@ -45,7 +45,8 @@ UpdateApplicator::~UpdateApplicator() {  // don't bother re-processing them on subsequent passes.  void UpdateApplicator::AttemptApplications(      syncable::WriteTransaction* trans, -    const std::vector<int64>& handles) { +    const std::vector<int64>& handles, +    sessions::StatusController* status) {    std::vector<int64> to_apply = handles;    DVLOG(1) << "UpdateApplicator running over " << to_apply.size() << " items.";    while (!to_apply.empty()) { @@ -64,12 +65,14 @@ void UpdateApplicator::AttemptApplications(        switch (result) {          case SUCCESS:            application_results_.AddSuccess(entry.Get(ID)); +          status->increment_num_updates_applied();            break;          case CONFLICT_SIMPLE:            application_results_.AddSimpleConflict(entry.Get(ID));            break;          case CONFLICT_ENCRYPTION:            application_results_.AddEncryptionConflict(entry.Get(ID)); +          status->increment_num_encryption_conflicts();            break;          case CONFLICT_HIERARCHY:            application_results_.AddHierarchyConflict(entry.Get(ID)); @@ -86,7 +89,7 @@ void UpdateApplicator::AttemptApplications(      if (to_reapply.size() == to_apply.size()) {        // We made no progress.  Must be stubborn hierarchy conflicts. -      // Break out early, leaving some updates unapplied. +      status->set_num_hierarchy_conflicts(to_apply.size());        break;      } @@ -119,9 +122,9 @@ bool UpdateApplicator::SkipUpdate(const syncable::Entry& entry) {  }  void UpdateApplicator::SaveProgressIntoSessionState( -    sessions::ConflictProgress* conflict_progress, +    std::set<syncable::Id>* simple_conflict_ids,      sessions::UpdateProgress* update_progress) { -  application_results_.SaveProgress(conflict_progress, update_progress); +  application_results_.SaveProgress(simple_conflict_ids, update_progress);  }  UpdateApplicator::ResultTracker::ResultTracker() { @@ -147,25 +150,22 @@ void UpdateApplicator::ResultTracker::AddSuccess(syncable::Id id) {  }  void UpdateApplicator::ResultTracker::SaveProgress( -    sessions::ConflictProgress* conflict_progress, +    std::set<syncable::Id>* simple_conflict_ids,      sessions::UpdateProgress* update_progress) {    std::set<syncable::Id>::const_iterator i; +  *simple_conflict_ids = conflicting_ids_;    for (i = conflicting_ids_.begin(); i != conflicting_ids_.end(); ++i) { -    conflict_progress->AddSimpleConflictingItemById(*i);      update_progress->AddAppliedUpdate(CONFLICT_SIMPLE, *i);    }    for (i = encryption_conflict_ids_.begin();         i != encryption_conflict_ids_.end(); ++i) { -    conflict_progress->AddEncryptionConflictingItemById(*i);      update_progress->AddAppliedUpdate(CONFLICT_ENCRYPTION, *i);    }    for (i = hierarchy_conflict_ids_.begin();         i != hierarchy_conflict_ids_.end(); ++i) { -    conflict_progress->AddHierarchyConflictingItemById(*i);      update_progress->AddAppliedUpdate(CONFLICT_HIERARCHY, *i);    }    for (i = successful_ids_.begin(); i != successful_ids_.end(); ++i) { -    conflict_progress->EraseSimpleConflictingItemById(*i);      update_progress->AddAppliedUpdate(SUCCESS, *i);    }  } diff --git a/sync/engine/update_applicator.h b/sync/engine/update_applicator.h index 36a907f..290a76c 100644 --- a/sync/engine/update_applicator.h +++ b/sync/engine/update_applicator.h @@ -17,11 +17,12 @@  #include "base/port.h"  #include "sync/internal_api/public/engine/model_safe_worker.h"  #include "sync/syncable/syncable_id.h" +#include "sync/sessions/status_controller.h"  namespace syncer {  namespace sessions { -class ConflictProgress; +class StatusController;  class UpdateProgress;  } @@ -44,14 +45,15 @@ class UpdateApplicator {    // Attempt to apply the specified updates.    void AttemptApplications(syncable::WriteTransaction* trans, -                           const std::vector<int64>& handles); +                           const std::vector<int64>& handles, +                           sessions::StatusController* status);    // This class does not automatically save its progress into the    // SyncSession -- to get that to happen, call this method after update    // application is finished (i.e., when AttemptOneAllocation stops returning    // true).    void SaveProgressIntoSessionState( -      sessions::ConflictProgress* conflict_progress, +      std::set<syncable::Id>* simple_conflict_ids,        sessions::UpdateProgress* update_progress);   private: @@ -64,7 +66,7 @@ class UpdateApplicator {       void AddEncryptionConflict(syncable::Id);       void AddHierarchyConflict(syncable::Id);       void AddSuccess(syncable::Id); -     void SaveProgress(sessions::ConflictProgress* conflict_progress, +     void SaveProgress(std::set<syncable::Id>* simple_conflict_ids,                         sessions::UpdateProgress* update_progress);       void ClearHierarchyConflicts(); diff --git a/sync/internal_api/public/sessions/model_neutral_state.cc b/sync/internal_api/public/sessions/model_neutral_state.cc index 9ae8436..c9caa75 100644 --- a/sync/internal_api/public/sessions/model_neutral_state.cc +++ b/sync/internal_api/public/sessions/model_neutral_state.cc @@ -13,6 +13,10 @@ ModelNeutralState::ModelNeutralState()        num_updates_downloaded_total(0),        num_tombstone_updates_downloaded_total(0),        num_reflected_updates_downloaded_total(0), +      num_updates_applied(0), +      num_encryption_conflicts(0), +      num_server_conflicts(0), +      num_hierarchy_conflicts(0),        num_local_overwrites(0),        num_server_overwrites(0),        last_get_key_result(UNSET), diff --git a/sync/internal_api/public/sessions/model_neutral_state.h b/sync/internal_api/public/sessions/model_neutral_state.h index c1d8862..e291a25 100644 --- a/sync/internal_api/public/sessions/model_neutral_state.h +++ b/sync/internal_api/public/sessions/model_neutral_state.h @@ -41,6 +41,12 @@ struct ModelNeutralState {    // the client must now "migrate", by purging and re-downloading all updates.    ModelTypeSet types_needing_local_migration; +  // Update application and conflicts. +  int num_updates_applied; +  int num_encryption_conflicts; +  int num_server_conflicts; +  int num_hierarchy_conflicts; +    // Overwrites due to conflict resolution counters.    int num_local_overwrites;    int num_server_overwrites; diff --git a/sync/sessions/session_state.cc b/sync/sessions/session_state.cc index 015b20c..f5d1709 100644 --- a/sync/sessions/session_state.cc +++ b/sync/sessions/session_state.cc @@ -16,60 +16,6 @@ using std::vector;  namespace syncer {  namespace sessions { -ConflictProgress::ConflictProgress() -  : num_server_conflicting_items(0), num_hierarchy_conflicting_items(0), -    num_encryption_conflicting_items(0) { -} - -ConflictProgress::~ConflictProgress() { -} - -bool ConflictProgress::HasSimpleConflictItem(const syncable::Id& id) const { -  return simple_conflicting_item_ids_.count(id) > 0; -} - -const std::set<syncable::Id>& ConflictProgress::SimpleConflictingItems() const { -  return simple_conflicting_item_ids_; -} - -void ConflictProgress::AddSimpleConflictingItemById( -    const syncable::Id& the_id) { -  simple_conflicting_item_ids_.insert(the_id); -} - -void ConflictProgress::EraseSimpleConflictingItemById( -    const syncable::Id& the_id) { -  simple_conflicting_item_ids_.erase(the_id); -} - -void ConflictProgress::AddEncryptionConflictingItemById( -    const syncable::Id& the_id) { -  std::pair<std::set<syncable::Id>::iterator, bool> ret = -      unresolvable_conflicting_item_ids_.insert(the_id); -  if (ret.second) { -    num_encryption_conflicting_items++; -  } -  unresolvable_conflicting_item_ids_.insert(the_id); -} - -void ConflictProgress::AddHierarchyConflictingItemById( -    const syncable::Id& the_id) { -  std::pair<std::set<syncable::Id>::iterator, bool> ret = -      unresolvable_conflicting_item_ids_.insert(the_id); -  if (ret.second) { -    num_hierarchy_conflicting_items++; -  } -} - -void ConflictProgress::AddServerConflictingItemById( -    const syncable::Id& the_id) { -  std::pair<std::set<syncable::Id>::iterator, bool> ret = -      unresolvable_conflicting_item_ids_.insert(the_id); -  if (ret.second) { -    num_server_conflicting_items++; -  } -} -  UpdateProgress::UpdateProgress() {}  UpdateProgress::~UpdateProgress() {} diff --git a/sync/sessions/session_state.h b/sync/sessions/session_state.h index eff9591..548b4ed 100644 --- a/sync/sessions/session_state.h +++ b/sync/sessions/session_state.h @@ -22,53 +22,6 @@  namespace syncer {  namespace sessions { -// Tracks progress of conflicts and their resolutions. -class ConflictProgress { - public: -  explicit ConflictProgress(); -  ~ConflictProgress(); - -  bool HasSimpleConflictItem(const syncable::Id &id) const; - -  // Various mutators for tracking commit conflicts. -  void AddSimpleConflictingItemById(const syncable::Id& the_id); -  void EraseSimpleConflictingItemById(const syncable::Id& the_id); -  const std::set<syncable::Id>& SimpleConflictingItems() const; -  int SimpleConflictingItemsSize() const { -    return simple_conflicting_item_ids_.size(); -  } - -  // Mutators for unresolvable conflicting items (see description below). -  void AddEncryptionConflictingItemById(const syncable::Id& the_id); -  int EncryptionConflictingItemsSize() const { -    return num_encryption_conflicting_items; -  } - -  void AddHierarchyConflictingItemById(const syncable::Id& id); -  int HierarchyConflictingItemsSize() const { -    return num_hierarchy_conflicting_items; -  } - -  void AddServerConflictingItemById(const syncable::Id& id); -  int ServerConflictingItemsSize() const { -    return num_server_conflicting_items; -  } - - private: -  // Conflicts that occur when local and server changes collide and can be -  // resolved locally. -  std::set<syncable::Id> simple_conflicting_item_ids_; - -  // Unresolvable conflicts are not processed by the conflict resolver.  We wait -  // and hope the server will provide us with an update that resolves these -  // conflicts. -  std::set<syncable::Id> unresolvable_conflicting_item_ids_; - -  size_t num_server_conflicting_items; -  size_t num_hierarchy_conflicting_items; -  size_t num_encryption_conflicting_items; -}; -  typedef std::pair<VerifyResult, sync_pb::SyncEntity> VerifiedUpdate;  typedef std::pair<UpdateAttemptResponse, syncable::Id> AppliedUpdate; @@ -123,7 +76,7 @@ struct PerModelSafeGroupState {    ~PerModelSafeGroupState();    UpdateProgress update_progress; -  ConflictProgress conflict_progress; +  std::set<syncable::Id> simple_conflict_ids;  };  }  // namespace sessions diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc index 61b1094..93a37f1 100644 --- a/sync/sessions/status_controller.cc +++ b/sync/sessions/status_controller.cc @@ -33,34 +33,27 @@ UpdateProgress* StatusController::mutable_update_progress() {        true, group_restriction_)->update_progress;  } -const ConflictProgress* StatusController::conflict_progress() const { +const std::set<syncable::Id>* StatusController::simple_conflict_ids() const {    const PerModelSafeGroupState* state =        GetModelSafeGroupState(true, group_restriction_); -  return state ? &state->conflict_progress : NULL; +  return state ? &state->simple_conflict_ids : NULL;  } -ConflictProgress* StatusController::mutable_conflict_progress() { +std::set<syncable::Id>* StatusController::mutable_simple_conflict_ids() {    return &GetOrCreateModelSafeGroupState( -      true, group_restriction_)->conflict_progress; +      true, group_restriction_)->simple_conflict_ids;  } -const ConflictProgress* StatusController::GetUnrestrictedConflictProgress( -    ModelSafeGroup group) const { -  const PerModelSafeGroupState* state = -      GetModelSafeGroupState(false, group); -  return state ? &state->conflict_progress : NULL; -} - -ConflictProgress* -    StatusController::GetUnrestrictedMutableConflictProgressForTest( -        ModelSafeGroup group) { -  return &GetOrCreateModelSafeGroupState(false, group)->conflict_progress; +const std::set<syncable::Id>* +    StatusController::GetUnrestrictedSimpleConflictIds( +        ModelSafeGroup group) const { +  const PerModelSafeGroupState* state = GetModelSafeGroupState(false, group); +  return state ? &state->simple_conflict_ids : NULL;  }  const UpdateProgress* StatusController::GetUnrestrictedUpdateProgress(      ModelSafeGroup group) const { -  const PerModelSafeGroupState* state = -      GetModelSafeGroupState(false, group); +  const PerModelSafeGroupState* state = GetModelSafeGroupState(false, group);    return state ? &state->update_progress : NULL;  } @@ -129,6 +122,22 @@ void StatusController::increment_num_successful_commits() {    model_neutral_.num_successful_commits++;  } +void StatusController::increment_num_updates_applied() { +  model_neutral_.num_updates_applied++; +} + +void StatusController::increment_num_encryption_conflicts() { +  model_neutral_.num_encryption_conflicts++; +} + +void StatusController::increment_num_server_conflicts() { +  model_neutral_.num_server_conflicts++; +} + +void StatusController::set_num_hierarchy_conflicts(int value) { +  model_neutral_.num_hierarchy_conflicts = value; +} +  void StatusController::increment_num_local_overwrites() {    model_neutral_.num_local_overwrites++;  } @@ -189,66 +198,42 @@ bool StatusController::HasConflictingUpdates() const {    return false;  } -int StatusController::TotalNumEncryptionConflictingItems() const { -  DCHECK(!group_restriction_in_effect_) -      << "TotalNumEncryptionConflictingItems applies to all ModelSafeGroups"; -  std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it = -      per_model_group_.begin(); -  int sum = 0; -  for (; it != per_model_group_.end(); ++it) { -    sum += it->second->conflict_progress.EncryptionConflictingItemsSize(); -  } -  return sum; +int StatusController::num_encryption_conflicts() const { +  return model_neutral_.num_encryption_conflicts;  } -int StatusController::TotalNumHierarchyConflictingItems() const { +int StatusController::num_hierarchy_conflicts() const {    DCHECK(!group_restriction_in_effect_) -      << "TotalNumHierarchyConflictingItems applies to all ModelSafeGroups"; -  std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it = -      per_model_group_.begin(); -  int sum = 0; -  for (; it != per_model_group_.end(); ++it) { -    sum += it->second->conflict_progress.HierarchyConflictingItemsSize(); -  } -  return sum; +      << "num_hierarchy_conflicts applies to all ModelSafeGroups"; +  return model_neutral_.num_hierarchy_conflicts;  } -int StatusController::TotalNumSimpleConflictingItems() const { +int StatusController::num_simple_conflicts() const {    DCHECK(!group_restriction_in_effect_) -      << "TotalNumSimpleConflictingItems applies to all ModelSafeGroups"; +   << "num_simple_conflicts applies to all ModelSafeGroups";    std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it =        per_model_group_.begin();    int sum = 0;    for (; it != per_model_group_.end(); ++it) { -    sum += it->second->conflict_progress.SimpleConflictingItemsSize(); +    sum += it->second->simple_conflict_ids.size();    }    return sum;  } -int StatusController::TotalNumServerConflictingItems() const { +int StatusController::num_server_conflicts() const {    DCHECK(!group_restriction_in_effect_) -      << "TotalNumServerConflictingItems applies to all ModelSafeGroups"; -  std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it = -      per_model_group_.begin(); -  int sum = 0; -  for (; it != per_model_group_.end(); ++it) { -    sum += it->second->conflict_progress.ServerConflictingItemsSize(); -  } -  return sum; +      << "num_server_conflicts applies to all ModelSafeGroups"; +  return model_neutral_.num_server_conflicts;  }  int StatusController::TotalNumConflictingItems() const {    DCHECK(!group_restriction_in_effect_)        << "TotalNumConflictingItems applies to all ModelSafeGroups"; -  std::map<ModelSafeGroup, PerModelSafeGroupState*>::const_iterator it = -    per_model_group_.begin();    int sum = 0; -  for (; it != per_model_group_.end(); ++it) { -    sum += it->second->conflict_progress.SimpleConflictingItemsSize(); -    sum += it->second->conflict_progress.EncryptionConflictingItemsSize(); -    sum += it->second->conflict_progress.HierarchyConflictingItemsSize(); -    sum += it->second->conflict_progress.ServerConflictingItemsSize(); -  } +  sum += num_simple_conflicts(); +  sum += num_encryption_conflicts(); +  sum += num_hierarchy_conflicts(); +  sum += num_server_conflicts();    return sum;  } diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h index 752d07d..c18fc7d 100644 --- a/sync/sessions/status_controller.h +++ b/sync/sessions/status_controller.h @@ -53,14 +53,12 @@ class StatusController {    // Progress counters.  All const methods may return NULL if the    // progress structure doesn't exist, but all non-const methods    // auto-create. -  const ConflictProgress* conflict_progress() const; -  ConflictProgress* mutable_conflict_progress(); +  const std::set<syncable::Id>* simple_conflict_ids() const; +  std::set<syncable::Id>* mutable_simple_conflict_ids();    const UpdateProgress* update_progress() const;    UpdateProgress* mutable_update_progress(); -  const ConflictProgress* GetUnrestrictedConflictProgress( +  const std::set<syncable::Id>* GetUnrestrictedSimpleConflictIds(        ModelSafeGroup group) const; -  ConflictProgress* GetUnrestrictedMutableConflictProgressForTest( -      ModelSafeGroup group);    const UpdateProgress* GetUnrestrictedUpdateProgress(        ModelSafeGroup group) const;    UpdateProgress* GetUnrestrictedMutableUpdateProgressForTest( @@ -102,16 +100,14 @@ class StatusController {    // Note: this includes unresolvable conflicts.    bool HasConflictingUpdates() const; -  // Aggregate sums of various types of conflict counters accross all -  // ConflictProgress objects (one for each ModelSafeGroup currently in-use). -  int TotalNumEncryptionConflictingItems() const; -  int TotalNumHierarchyConflictingItems() const; -  int TotalNumServerConflictingItems() const; -  int TotalNumSimpleConflictingItems() const; +  // Various conflict counters. +  int num_encryption_conflicts() const; +  int num_hierarchy_conflicts() const; +  int num_server_conflicts() const; + +  int num_simple_conflicts() const; -  // Aggregate sum of SimpleConflictingItemSize() and other -  // ${Type}ConflictingItemSize() methods over all ConflictProgress objects (one -  // for each ModelSafeGroup currently in-use). +  // Aggregate sum of all conflicting items over all conflict types.    int TotalNumConflictingItems() const;    // Returns the number of updates received from the sync server. @@ -151,24 +147,37 @@ class StatusController {    SyncerError last_get_key_result() const; -  // A toolbelt full of methods for updating counters and flags. +  // Download counters.    void set_num_server_changes_remaining(int64 changes_remaining); -  void set_num_successful_bookmark_commits(int value); -  void increment_num_successful_commits(); -  void increment_num_successful_bookmark_commits();    void increment_num_updates_downloaded_by(int value);    void increment_num_tombstone_updates_downloaded_by(int value);    void increment_num_reflected_updates_downloaded_by(int value); -  void set_types_needing_local_migration(ModelTypeSet types); + +  // Update application and conflict resolution counters. +  void increment_num_updates_applied(); +  void increment_num_encryption_conflicts(); +  void increment_num_server_conflicts(); +  void set_num_hierarchy_conflicts(int value);    void increment_num_local_overwrites();    void increment_num_server_overwrites(); + +  // TODO(rlarocque): Remove these after conflict resolution refactor. +  void update_conflicts_resolved(bool resolved); +  void reset_conflicts_resolved(); + +  // Commit counters. +  void increment_num_successful_commits(); +  void increment_num_successful_bookmark_commits(); +  void set_num_successful_bookmark_commits(int value); + +  // Server communication status tracking.    void set_sync_protocol_error(const SyncProtocolError& error);    void set_last_get_key_result(const SyncerError result);    void set_last_download_updates_result(const SyncerError result);    void set_commit_result(const SyncerError result); -  void update_conflicts_resolved(bool resolved); -  void reset_conflicts_resolved(); +  // A very important flag used to inform frontend of need to migrate. +  void set_types_needing_local_migration(ModelTypeSet types);    void UpdateStartTime(); diff --git a/sync/sessions/status_controller_unittest.cc b/sync/sessions/status_controller_unittest.cc index 8fec64d..619e1fa 100644 --- a/sync/sessions/status_controller_unittest.cc +++ b/sync/sessions/status_controller_unittest.cc @@ -95,24 +95,22 @@ TEST_F(StatusControllerTest, TotalNumConflictingItems) {    TestIdFactory f;    {      ScopedModelSafeGroupRestriction r(&status, GROUP_UI); -    EXPECT_FALSE(status.conflict_progress()); -    status.mutable_conflict_progress()-> -        AddSimpleConflictingItemById(f.NewLocalId()); -    status.mutable_conflict_progress()-> -        AddSimpleConflictingItemById(f.NewLocalId()); -    EXPECT_EQ(2, status.conflict_progress()->SimpleConflictingItemsSize()); +    status.mutable_simple_conflict_ids()->insert(f.NewLocalId()); +    status.mutable_simple_conflict_ids()->insert(f.NewLocalId()); +    EXPECT_EQ(static_cast<size_t>(2), status.simple_conflict_ids()->size());    }    EXPECT_EQ(2, status.TotalNumConflictingItems());    {      ScopedModelSafeGroupRestriction r(&status, GROUP_DB); -    EXPECT_FALSE(status.conflict_progress()); -    status.mutable_conflict_progress()-> -        AddSimpleConflictingItemById(f.NewLocalId()); -    status.mutable_conflict_progress()-> -        AddSimpleConflictingItemById(f.NewLocalId()); -    EXPECT_EQ(2, status.conflict_progress()->SimpleConflictingItemsSize()); +    status.mutable_simple_conflict_ids()->insert(f.NewLocalId()); +    status.mutable_simple_conflict_ids()->insert(f.NewLocalId()); +    EXPECT_EQ(static_cast<size_t>(2), status.simple_conflict_ids()->size());    }    EXPECT_EQ(4, status.TotalNumConflictingItems()); + +  status.increment_num_server_conflicts(); +  status.set_num_hierarchy_conflicts(3); +  EXPECT_EQ(8, status.TotalNumConflictingItems());  }  // Basic test that non group-restricted state accessors don't cause violations. diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc index 9806425..b6a9ff3 100644 --- a/sync/sessions/sync_session.cc +++ b/sync/sessions/sync_session.cc @@ -179,10 +179,10 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {        download_progress_markers,        HasMoreToSync(),        delegate_->IsSyncingCurrentlySilenced(), -      status_controller_->TotalNumEncryptionConflictingItems(), -      status_controller_->TotalNumHierarchyConflictingItems(), -      status_controller_->TotalNumSimpleConflictingItems(), -      status_controller_->TotalNumServerConflictingItems(), +      status_controller_->num_encryption_conflicts(), +      status_controller_->num_hierarchy_conflicts(), +      status_controller_->num_simple_conflicts(), +      status_controller_->num_server_conflicts(),        source_,        context_->notifications_enabled(),        dir->GetEntriesCount(), @@ -207,15 +207,15 @@ const std::set<ModelSafeGroup>& SyncSession::GetEnabledGroups() const {    return enabled_groups_;  } +// TODO(rlarocque): Delete this function after refactoring conflict resolution.  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->SimpleConflictingItemsSize() > 0) { +    const std::set<syncable::Id>* ids = +        status_controller_->GetUnrestrictedSimpleConflictIds(*it); +    if (ids && ids->size() > 0) {        enabled_groups_with_conflicts.insert(*it);      }    } diff --git a/sync/sessions/sync_session_unittest.cc b/sync/sessions/sync_session_unittest.cc index d187ce6..285f33a 100644 --- a/sync/sessions/sync_session_unittest.cc +++ b/sync/sessions/sync_session_unittest.cc @@ -162,27 +162,6 @@ TEST_F(SyncSessionTest, EnabledGroups) {    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)-> -      AddSimpleConflictingItemById(syncable::Id()); -  std::set<ModelSafeGroup> expected_enabled_groups_with_conflicts; -  expected_enabled_groups_with_conflicts.insert(GROUP_UI); -  EXPECT_EQ(expected_enabled_groups_with_conflicts, -            session->GetEnabledGroupsWithConflicts()); -} -  TEST_F(SyncSessionTest, ScopedContextHelpers) {    ConflictResolver resolver;    EXPECT_FALSE(context_->resolver()); diff --git a/sync/sync.gyp b/sync/sync.gyp index 4358eed..5fa6417 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -605,7 +605,6 @@            'engine/model_changing_syncer_command_unittest.cc',            'engine/process_commit_response_command_unittest.cc',            'engine/process_updates_command_unittest.cc', -          'engine/resolve_conflicts_command_unittest.cc',            'engine/sync_scheduler_unittest.cc',            'engine/sync_scheduler_whitebox_unittest.cc',            'engine/syncer_proto_util_unittest.cc', | 
