diff options
36 files changed, 723 insertions, 332 deletions
diff --git a/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc b/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc index b783276..59044af0 100644 --- a/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc @@ -171,7 +171,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, StartManagerReady) { SetActivateExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, app_notif_dtc_->state()); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); EXPECT_EQ(DataTypeController::RUNNING, app_notif_dtc_->state()); } @@ -204,7 +204,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, StartFirstRun) { SetStartExpectations(); InitAndLoadManager(); SetActivateExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _, _)); change_processor_->set_sync_model_has_user_created_nodes(false); Start(); @@ -216,7 +216,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); InitAndLoadManager(); EXPECT_CALL(start_callback_, - Run(DataTypeController::ASSOCIATION_FAILED, _)); + Run(DataTypeController::ASSOCIATION_FAILED, _, _)); syncable_service_.set_merge_data_and_start_syncing_error( syncer::SyncError(FROM_HERE, "Error", syncer::APP_NOTIFICATIONS)); @@ -230,7 +230,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, SetStartExpectations(); InitAndLoadManager(); EXPECT_CALL(start_callback_, - Run(DataTypeController::UNRECOVERABLE_ERROR, _)); + Run(DataTypeController::UNRECOVERABLE_ERROR, _, _)); // Set up association to fail with an unrecoverable error. change_processor_->set_sync_model_has_user_created_nodes_success(false); @@ -244,7 +244,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, Stop) { InitAndLoadManager(); SetActivateExpectations(); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, app_notif_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); @@ -266,7 +266,7 @@ TEST_F(SyncAppNotificationDataTypeControllerTest, &AppNotificationDataTypeController::Stop)); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); EXPECT_EQ(DataTypeController::RUNNING, app_notif_dtc_->state()); EXPECT_TRUE(syncable_service_.syncing()); diff --git a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc index 3476a17..91de7eb 100644 --- a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc @@ -96,9 +96,10 @@ class SyncAutofillDataTypeControllerTest : public testing::Test { // Passed to AutofillDTC::Start(). void OnStartFinished(DataTypeController::StartResult result, - const syncer::SyncError& error) { + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { last_start_result_ = result; - last_start_error_ = error; + last_start_error_ = local_merge_result.error(); } void OnLoadFinished(syncer::ModelType type, syncer::SyncError error) { diff --git a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc index 2cc0a1c..21961f1 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc @@ -150,7 +150,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartDependentsReady) { EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); EXPECT_EQ(DataTypeController::RUNNING, bookmark_dtc_->state()); } @@ -160,7 +160,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartBookmarkModelNotReady) { EXPECT_CALL(*bookmark_model_, IsLoaded()).WillRepeatedly(Return(false)); SetAssociateExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); bookmark_dtc_->LoadModels( base::Bind(&ModelLoadCallbackMock::Run, base::Unretained(&model_load_callback_))); @@ -208,7 +208,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartFirstRun) { SetAssociateExpectations(); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(true))); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _, _)); Start(); } @@ -231,7 +231,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartOk) { EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); } @@ -249,7 +249,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, StartAssociationFailed) { syncer::BOOKMARKS))); EXPECT_CALL(start_callback_, - Run(DataTypeController::ASSOCIATION_FAILED, _)); + Run(DataTypeController::ASSOCIATION_FAILED, _, _)); Start(); EXPECT_EQ(DataTypeController::DISABLED, bookmark_dtc_->state()); } @@ -264,7 +264,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(false), Return(false))); EXPECT_CALL(start_callback_, - Run(DataTypeController::UNRECOVERABLE_ERROR, _)); + Run(DataTypeController::UNRECOVERABLE_ERROR, _, _)); Start(); EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); } @@ -289,7 +289,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, Stop) { EXPECT_EQ(DataTypeController::NOT_RUNNING, bookmark_dtc_->state()); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); EXPECT_EQ(DataTypeController::RUNNING, bookmark_dtc_->state()); bookmark_dtc_->Stop(); @@ -306,7 +306,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) { &BookmarkDataTypeController::Stop)); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); // This should cause bookmark_dtc_->Stop() to be called. bookmark_dtc_->OnSingleDatatypeUnrecoverableError(FROM_HERE, "Test"); diff --git a/chrome/browser/sync/glue/data_type_controller.cc b/chrome/browser/sync/glue/data_type_controller.cc index 2cd7340..d2dcc21 100644 --- a/chrome/browser/sync/glue/data_type_controller.cc +++ b/chrome/browser/sync/glue/data_type_controller.cc @@ -13,6 +13,10 @@ bool DataTypeController::IsUnrecoverableResult(StartResult result) { return (result == UNRECOVERABLE_ERROR); } +bool DataTypeController::IsSuccessfulResult(StartResult result) { + return (result == OK || result == OK_FIRST_RUN); +} + syncer::SyncError DataTypeController::CreateAndUploadError( const tracked_objects::Location& location, const std::string& message, diff --git a/chrome/browser/sync/glue/data_type_controller.h b/chrome/browser/sync/glue/data_type_controller.h index 2df59fa..7df8eed 100644 --- a/chrome/browser/sync/glue/data_type_controller.h +++ b/chrome/browser/sync/glue/data_type_controller.h @@ -13,6 +13,7 @@ #include "base/sequenced_task_runner_helpers.h" #include "chrome/browser/sync/glue/data_type_error_handler.h" #include "content/public/browser/browser_thread.h" +#include "sync/api/sync_merge_result.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/engine/model_safe_worker.h" #include "sync/internal_api/public/util/unrecoverable_error_handler.h" @@ -63,7 +64,8 @@ class DataTypeController }; typedef base::Callback<void(StartResult, - const syncer::SyncError&)> StartCallback; + const syncer::SyncMergeResult&, + const syncer::SyncMergeResult&)> StartCallback; typedef base::Callback<void(syncer::ModelType, syncer::SyncError)> ModelLoadCallback; @@ -76,6 +78,9 @@ class DataTypeController // Public so unit tests can use this function as well. static bool IsUnrecoverableResult(StartResult result); + // Returns true if the datatype started successfully. + static bool IsSuccessfulResult(StartResult result); + // Begins asynchronous operation of loading the model to get it ready for // model association. Once the models are loaded the callback will be invoked // with the result. If the models are already loaded it is safe to call the diff --git a/chrome/browser/sync/glue/data_type_controller_mock.h b/chrome/browser/sync/glue/data_type_controller_mock.h index 796fb88..40ab896 100644 --- a/chrome/browser/sync/glue/data_type_controller_mock.h +++ b/chrome/browser/sync/glue/data_type_controller_mock.h @@ -16,8 +16,9 @@ class StartCallbackMock { StartCallbackMock(); virtual ~StartCallbackMock(); - MOCK_METHOD2(Run, void(DataTypeController::StartResult result, - const syncer::SyncError& error)); + MOCK_METHOD3(Run, void(DataTypeController::StartResult result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result)); }; class ModelLoadCallbackMock { diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc index f3d7730..fed635f 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -37,7 +37,8 @@ DataTypeManagerImpl::DataTypeManagerImpl( needs_reconfigure_(false), last_configure_reason_(syncer::CONFIGURE_REASON_UNKNOWN), weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), - model_association_manager_(controllers, + model_association_manager_(debug_info_listener, + controllers, ALLOW_THIS_IN_INITIALIZER_LIST(this)), observer_(observer) { DCHECK(configurer_); diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc index 7176123..a5aac50 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -14,7 +14,6 @@ #include "content/public/test/test_browser_thread.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/configure_reason.h" -#include "sync/internal_api/public/util/weak_handle.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/chrome/browser/sync/glue/fake_data_type_controller.cc b/chrome/browser/sync/glue/fake_data_type_controller.cc index 09feff2..b0316bfa 100644 --- a/chrome/browser/sync/glue/fake_data_type_controller.cc +++ b/chrome/browser/sync/glue/fake_data_type_controller.cc @@ -57,17 +57,22 @@ void FakeDataTypeController::FinishStart(StartResult result) { } // Set |state_| first below since the callback may call state(). - syncer::SyncError error; + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); if (result <= OK_FIRST_RUN) { state_ = RUNNING; } else if (result == ASSOCIATION_FAILED) { state_ = DISABLED; - error.Reset(FROM_HERE, "Association failed", type_); + local_merge_result.set_error( + syncer::SyncError(FROM_HERE, "Association failed", type())); } else { state_ = NOT_RUNNING; - error.Reset(FROM_HERE, "Fake error", type_); + local_merge_result.set_error( + syncer::SyncError(FROM_HERE, "Fake error", type())); } - last_start_callback_.Run(result, error); + last_start_callback_.Run(result, + local_merge_result, + syncer_merge_result); last_start_callback_.Reset(); } @@ -84,7 +89,11 @@ void FakeDataTypeController::Stop() { // The DTM still expects |last_start_callback_| to be called back. if (!last_start_callback_.is_null()) { syncer::SyncError error(FROM_HERE, "Fake error", type_); - last_start_callback_.Run(ABORTED, error); + syncer::SyncMergeResult local_merge_result(type_); + local_merge_result.set_error(error); + last_start_callback_.Run(ABORTED, + local_merge_result, + syncer::SyncMergeResult(type_)); } } diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.cc b/chrome/browser/sync/glue/frontend_data_type_controller.cc index 234dd17..704063c 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/frontend_data_type_controller.cc @@ -160,26 +160,31 @@ bool FrontendDataTypeController::StartModels() { bool FrontendDataTypeController::Associate() { DCHECK_EQ(state_, ASSOCIATING); + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); CreateSyncComponents(); if (!model_associator()->CryptoReadyIfNecessary()) { - StartFailed(NEEDS_CRYPTO, syncer::SyncError()); + StartDone(NEEDS_CRYPTO, local_merge_result, syncer_merge_result); return false; } bool sync_has_nodes = false; if (!model_associator()->SyncModelHasUserCreatedNodes(&sync_has_nodes)) { syncer::SyncError error(FROM_HERE, "Failed to load sync nodes", type()); - StartFailed(UNRECOVERABLE_ERROR, error); + local_merge_result.set_error(error); + StartDone(UNRECOVERABLE_ERROR, local_merge_result, syncer_merge_result); return false; } + // TODO(zea): Have AssociateModels fill the local and syncer merge results. base::TimeTicks start_time = base::TimeTicks::Now(); syncer::SyncError error; error = model_associator()->AssociateModels(); // TODO(lipalani): crbug.com/122690 - handle abort. RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, local_merge_result, syncer_merge_result); return false; } @@ -189,7 +194,9 @@ bool FrontendDataTypeController::Associate() { // FinishStart() invokes the DataTypeManager callback, which can lead to a // call to Stop() if one of the other data types being started generates an // error. - FinishStart(!sync_has_nodes ? OK_FIRST_RUN : OK); + StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, + local_merge_result, + syncer_merge_result); // Return false if we're not in the RUNNING state (due to Stop() being called // from FinishStart()). // TODO(zea/atwilson): Should we maybe move the call to FinishStart() out of @@ -208,29 +215,6 @@ void FrontendDataTypeController::CleanUp() { change_processor_.reset(); } -void FrontendDataTypeController::StartFailed(StartResult result, - const syncer::SyncError& error) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - - if (IsUnrecoverableResult(result)) - RecordUnrecoverableError(FROM_HERE, "StartFailed"); - - CleanUp(); - if (result == ASSOCIATION_FAILED) { - state_ = DISABLED; - } else { - state_ = NOT_RUNNING; - } - RecordStartFailure(result); - - // We have to release the callback before we call it, since it's possible - // invoking the callback will trigger a call to STOP(), which will get - // confused by the non-NULL start_callback_. - StartCallback callback = start_callback_; - start_callback_.Reset(); - callback.Run(result, error); -} - void FrontendDataTypeController::AbortModelLoad() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); CleanUp(); @@ -242,15 +226,30 @@ void FrontendDataTypeController::AbortModelLoad() { type())); } -void FrontendDataTypeController::FinishStart(StartResult result) { +void FrontendDataTypeController::StartDone( + StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (!IsSuccessfulResult(start_result)) { + if (IsUnrecoverableResult(start_result)) + RecordUnrecoverableError(FROM_HERE, "StartFailed"); + + CleanUp(); + if (start_result == ASSOCIATION_FAILED) { + state_ = DISABLED; + } else { + state_ = NOT_RUNNING; + } + RecordStartFailure(start_result); + } // We have to release the callback before we call it, since it's possible // invoking the callback will trigger a call to STOP(), which will get // confused by the non-NULL start_callback_. StartCallback callback = start_callback_; start_callback_.Reset(); - callback.Run(result, syncer::SyncError()); + callback.Run(start_result, local_merge_result, syncer_merge_result); } void FrontendDataTypeController::RecordAssociationTime(base::TimeDelta time) { diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.h b/chrome/browser/sync/glue/frontend_data_type_controller.h index e18288f..aec7348 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/frontend_data_type_controller.h @@ -87,9 +87,11 @@ class FrontendDataTypeController : public DataTypeController { // the datatype controller. The default implementation is a no-op. virtual void CleanUpState(); - // Helper methods for cleaning up state an running the start callback. - virtual void StartFailed(StartResult result, const syncer::SyncError& error); - virtual void FinishStart(StartResult result); + // Helper method for cleaning up state and running the start callback. + virtual void StartDone( + StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result); // Record association time. virtual void RecordAssociationTime(base::TimeDelta time); diff --git a/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc index ec34e50..40e9864 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc @@ -119,7 +119,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test { void SetActivateExpectations(DataTypeController::StartResult result) { EXPECT_CALL(service_, ActivateDataType(_, _, _)); - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result, _, _)); } void SetStopExpectations() { @@ -134,7 +134,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test { EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _)); EXPECT_CALL(*dtc_mock_, CleanUpState()); EXPECT_CALL(*dtc_mock_, RecordStartFailure(result)); - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result, _, _)); } void Start() { @@ -209,7 +209,7 @@ TEST_F(SyncFrontendDataTypeControllerTest, StartAssociationFailed) { EXPECT_CALL(*model_associator_, AssociateModels()). WillOnce(Return(syncer::SyncError(FROM_HERE, "error", - syncer::PREFERENCES))); + syncer::BOOKMARKS))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED); diff --git a/chrome/browser/sync/glue/model_association_manager.cc b/chrome/browser/sync/glue/model_association_manager.cc index 433c212..d8bd5b3 100644 --- a/chrome/browser/sync/glue/model_association_manager.cc +++ b/chrome/browser/sync/glue/model_association_manager.cc @@ -2,23 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include <algorithm> -#include <functional> - -#include "base/debug/trace_event.h" - -#include "base/logging.h" -#include "base/message_loop.h" -#include "base/metrics/histogram.h" - #include "chrome/browser/sync/glue/model_association_manager.h" #include <algorithm> #include <functional> -#include "content/public/browser/browser_thread.h" #include "base/debug/trace_event.h" - #include "base/logging.h" #include "base/message_loop.h" #include "base/metrics/histogram.h" @@ -77,15 +66,49 @@ class SortComparator : public std::binary_function<DataTypeController*, std::map<syncer::ModelType, int>* order_; }; +syncer::DataTypeAssociationStats BuildAssociationStatsFromMergeResults( + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { + DCHECK_EQ(local_merge_result.model_type(), syncer_merge_result.model_type()); + syncer::DataTypeAssociationStats stats; + stats.model_type = local_merge_result.model_type(); + stats.had_error = local_merge_result.error().IsSet() || + syncer_merge_result.error().IsSet(); + stats.num_local_items_before_association = + local_merge_result.num_items_before_association(); + stats.num_sync_items_before_association = + syncer_merge_result.num_items_before_association(); + stats.num_local_items_after_association = + local_merge_result.num_items_after_association(); + stats.num_sync_items_after_association = + syncer_merge_result.num_items_after_association(); + stats.num_local_items_added = + local_merge_result.num_items_added(); + stats.num_local_items_deleted = + local_merge_result.num_items_deleted(); + stats.num_local_items_modified = + local_merge_result.num_items_modified(); + stats.num_sync_items_added = + syncer_merge_result.num_items_added(); + stats.num_sync_items_deleted = + syncer_merge_result.num_items_deleted(); + stats.num_sync_items_modified = + syncer_merge_result.num_items_modified(); + return stats; +} + } // namespace ModelAssociationManager::ModelAssociationManager( + const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>& + debug_info_listener, const DataTypeController::TypeMap* controllers, ModelAssociationResultProcessor* processor) : state_(IDLE), currently_associating_(NULL), controllers_(controllers), result_processor_(processor), + debug_info_listener_(debug_info_listener), weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { // Ensure all data type controllers are stopped. @@ -276,8 +299,9 @@ void ModelAssociationManager::AppendToFailedDatatypesAndLogError( } void ModelAssociationManager::TypeStartCallback( - DataTypeController::StartResult result, - const syncer::SyncError& error) { + DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); TRACE_EVENT_END0("sync", "ModelAssociation"); @@ -297,32 +321,47 @@ void ModelAssociationManager::TypeStartCallback( DataTypeController* started_dtc = currently_associating_; currently_associating_ = NULL; - if (result == DataTypeController::ASSOCIATION_FAILED) { + if (start_result == DataTypeController::ASSOCIATION_FAILED) { DVLOG(1) << "ModelAssociationManager: Encountered a failed type"; - AppendToFailedDatatypesAndLogError(result, error); + AppendToFailedDatatypesAndLogError(start_result, + local_merge_result.error()); + } + + // Track the merge results if we succeeded or an association failure + // occurred. + if ((DataTypeController::IsSuccessfulResult(start_result) || + start_result == DataTypeController::ASSOCIATION_FAILED) && + debug_info_listener_.IsInitialized()) { + syncer::DataTypeAssociationStats stats = + BuildAssociationStatsFromMergeResults(local_merge_result, + syncer_merge_result); + debug_info_listener_.Call( + FROM_HERE, + &syncer::DataTypeDebugInfoListener::OnDataTypeAssociationComplete, + stats); } // If the type started normally, continue to the next type. // If the type is waiting for the cryptographer, continue to the next type. // Once the cryptographer is ready, we'll attempt to restart this type. // If this type encountered a type specific error continue to the next type. - if (result == DataTypeController::NEEDS_CRYPTO || - result == DataTypeController::OK || - result == DataTypeController::OK_FIRST_RUN || - result == DataTypeController::ASSOCIATION_FAILED) { + if (start_result == DataTypeController::NEEDS_CRYPTO || + DataTypeController::IsSuccessfulResult(start_result) || + start_result == DataTypeController::ASSOCIATION_FAILED) { + DVLOG(1) << "ModelAssociationManager: type start callback returned " - << result << " so calling LoadModelForNextType"; + << start_result << " so calling LoadModelForNextType"; LoadModelForNextType(); return; } // Any other result requires reconfiguration. Pass it on through the callback. LOG(ERROR) << "Failed to configure " << started_dtc->name(); - DCHECK(error.IsSet()); - DCHECK_EQ(started_dtc->type(), error.type()); + DCHECK(local_merge_result.error().IsSet()); + DCHECK_EQ(started_dtc->type(), local_merge_result.error().type()); DataTypeManager::ConfigureStatus configure_status = DataTypeManager::ABORTED; - switch (result) { + switch (start_result) { case DataTypeController::ABORTED: configure_status = DataTypeManager::ABORTED; break; @@ -335,7 +374,7 @@ void ModelAssociationManager::TypeStartCallback( } std::list<syncer::SyncError> errors; - errors.push_back(error); + errors.push_back(local_merge_result.error()); // Put our state to idle. state_ = IDLE; diff --git a/chrome/browser/sync/glue/model_association_manager.h b/chrome/browser/sync/glue/model_association_manager.h index 520fdde..082289a 100644 --- a/chrome/browser/sync/glue/model_association_manager.h +++ b/chrome/browser/sync/glue/model_association_manager.h @@ -11,6 +11,8 @@ #include "base/timer.h" #include "chrome/browser/sync/glue/data_type_manager.h" +#include "sync/internal_api/public/data_type_debug_info_listener.h" +#include "sync/internal_api/public/util/weak_handle.h" // |ModelAssociationManager| does the heavy lifting for doing the actual model // association. It instructs DataTypeControllers to load models, start @@ -39,8 +41,11 @@ class ModelAssociationResultProcessor { // The class that is responsible for model association. class ModelAssociationManager { public: - ModelAssociationManager(const DataTypeController::TypeMap* controllers, - ModelAssociationResultProcessor* processor); + ModelAssociationManager( + const syncer::WeakHandle<syncer::DataTypeDebugInfoListener>& + debug_info_listener, + const DataTypeController::TypeMap* controllers, + ModelAssociationResultProcessor* processor); virtual ~ModelAssociationManager(); // Initializes the state to do the model association in future. This @@ -95,8 +100,9 @@ class ModelAssociationManager { // Callback passed to each data type controller on starting association. This // callback will be invoked when the model association is done. - void TypeStartCallback(DataTypeController::StartResult result, - const syncer::SyncError& error); + void TypeStartCallback(DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result); // Callback that will be invoked when the models finish loading. This callback // will be passed to |LoadModels| function. @@ -159,9 +165,20 @@ class ModelAssociationManager { // Set of all registered controllers. const DataTypeController::TypeMap* controllers_; + + // The processor in charge of handling model association results. ModelAssociationResultProcessor* result_processor_; - base::WeakPtrFactory<ModelAssociationManager> weak_ptr_factory_; + + // Timer to track and limit how long a datatype takes to model associate. base::OneShotTimer<ModelAssociationManager> timer_; + + // Sync's datatype debug info listener, which we pass model association + // statistics to. + const syncer::WeakHandle<syncer::DataTypeDebugInfoListener> + debug_info_listener_; + + base::WeakPtrFactory<ModelAssociationManager> weak_ptr_factory_; + DISALLOW_COPY_AND_ASSIGN(ModelAssociationManager); }; } // namespace browser_sync diff --git a/chrome/browser/sync/glue/model_association_manager_unittest.cc b/chrome/browser/sync/glue/model_association_manager_unittest.cc index e8a13d9..8d38176 100644 --- a/chrome/browser/sync/glue/model_association_manager_unittest.cc +++ b/chrome/browser/sync/glue/model_association_manager_unittest.cc @@ -53,9 +53,9 @@ ACTION_P(VerifyResult, expected_result) { EXPECT_TRUE(arg0.waiting_to_start.Equals(expected_result.waiting_to_start)); } -class ModelAssociationManagerTest : public testing::Test { +class SyncModelAssociationManagerTest : public testing::Test { public: - ModelAssociationManagerTest() : + SyncModelAssociationManagerTest() : ui_thread_(content::BrowserThread::UI, &ui_loop_) { } @@ -68,11 +68,13 @@ class ModelAssociationManagerTest : public testing::Test { // Start a type and make sure ModelAssociationManager callst the |Start| // method and calls the callback when it is done. -TEST_F(ModelAssociationManagerTest, SimpleModelStart) { +TEST_F(SyncModelAssociationManagerTest, SimpleModelStart) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); DataTypeManager::ConfigureResult expected_result( @@ -94,11 +96,13 @@ TEST_F(ModelAssociationManagerTest, SimpleModelStart) { } // Start a type and call stop before it finishes associating. -TEST_F(ModelAssociationManagerTest, StopModelBeforeFinish) { +TEST_F(SyncModelAssociationManagerTest, StopModelBeforeFinish) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); @@ -124,11 +128,13 @@ TEST_F(ModelAssociationManagerTest, StopModelBeforeFinish) { } // Start a type, let it finish and then call stop. -TEST_F(ModelAssociationManagerTest, StopAfterFinish) { +TEST_F(SyncModelAssociationManagerTest, StopAfterFinish) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); DataTypeManager::ConfigureResult expected_result( @@ -154,11 +160,13 @@ TEST_F(ModelAssociationManagerTest, StopAfterFinish) { } // Make a type fail model association and verify correctness. -TEST_F(ModelAssociationManagerTest, TypeFailModelAssociation) { +TEST_F(SyncModelAssociationManagerTest, TypeFailModelAssociation) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); std::list<syncer::SyncError> errors; @@ -183,11 +191,13 @@ TEST_F(ModelAssociationManagerTest, TypeFailModelAssociation) { } // Ensure configuring stops when a type returns a unrecoverable error. -TEST_F(ModelAssociationManagerTest, TypeReturnUnrecoverableError) { +TEST_F(SyncModelAssociationManagerTest, TypeReturnUnrecoverableError) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); std::list<syncer::SyncError> errors; @@ -211,15 +221,17 @@ TEST_F(ModelAssociationManagerTest, TypeReturnUnrecoverableError) { DataTypeController::UNRECOVERABLE_ERROR); } -TEST_F(ModelAssociationManagerTest, InitializeAbortsLoad) { +TEST_F(SyncModelAssociationManagerTest, InitializeAbortsLoad) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); controllers_[syncer::THEMES] = new FakeDataTypeController(syncer::THEMES); GetController(controllers_, syncer::BOOKMARKS)->SetDelayModelLoad(); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types(syncer::BOOKMARKS, syncer::THEMES); syncer::ModelTypeSet expected_types_waiting_to_load; @@ -283,14 +295,16 @@ TEST_F(ModelAssociationManagerTest, InitializeAbortsLoad) { // Start 2 types. One of which timeout loading. Ensure that type is // fully configured eventually. -TEST_F(ModelAssociationManagerTest, ModelStartWithSlowLoadingType) { +TEST_F(SyncModelAssociationManagerTest, ModelStartWithSlowLoadingType) { controllers_[syncer::BOOKMARKS] = new FakeDataTypeController(syncer::BOOKMARKS); controllers_[syncer::APPS] = new FakeDataTypeController(syncer::APPS); GetController(controllers_, syncer::BOOKMARKS)->SetDelayModelLoad(); - ModelAssociationManager model_association_manager(&controllers_, - &result_processor_); + ModelAssociationManager model_association_manager( + syncer::WeakHandle<syncer::DataTypeDebugInfoListener>(), + &controllers_, + &result_processor_); syncer::ModelTypeSet types; types.Put(syncer::BOOKMARKS); types.Put(syncer::APPS); diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc index 627684a..f510c99 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.cc @@ -5,6 +5,7 @@ #include "chrome/browser/sync/glue/new_non_frontend_data_type_controller.h" #include "base/logging.h" +#include "base/memory/weak_ptr.h" #include "chrome/browser/sync/glue/shared_change_processor_ref.h" #include "chrome/browser/sync/profile_sync_components_factory.h" #include "chrome/browser/sync/profile_sync_service.h" @@ -77,7 +78,12 @@ void NewNonFrontendDataTypeController::StartAssociating( if (!StartAssociationAsync()) { syncer::SyncError error( FROM_HERE, "Failed to post StartAssociation", type()); - StartDoneImpl(ASSOCIATION_FAILED, NOT_RUNNING, error); + syncer::SyncMergeResult local_merge_result(type()); + local_merge_result.set_error(error); + StartDoneImpl(ASSOCIATION_FAILED, + NOT_RUNNING, + local_merge_result, + syncer::SyncMergeResult(type())); // StartDoneImpl should have called ClearSharedChangeProcessor(); DCHECK(!shared_change_processor_.get()); return; @@ -105,7 +111,10 @@ void NewNonFrontendDataTypeController::Stop() { return; // The datatype was never activated, we're done. case ASSOCIATING: set_state(STOPPING); - StartDoneImpl(ABORTED, NOT_RUNNING, syncer::SyncError()); + StartDoneImpl(ABORTED, + NOT_RUNNING, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(type())); // We continue on to deactivate the datatype and stop the local service. break; case DISABLED: @@ -140,31 +149,49 @@ NewNonFrontendDataTypeController::NewNonFrontendDataTypeController() {} NewNonFrontendDataTypeController::~NewNonFrontendDataTypeController() {} void NewNonFrontendDataTypeController::StartDone( - DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error) { + DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); + + DataTypeController::State new_state; + if (IsSuccessfulResult(start_result)) { + new_state = RUNNING; + } else { + new_state = (start_result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING); + } + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind( &NewNonFrontendDataTypeController::StartDoneImpl, this, - result, + start_result, new_state, - error)); + local_merge_result, + syncer_merge_result)); } void NewNonFrontendDataTypeController::StartDoneImpl( - DataTypeController::StartResult result, + DataTypeController::StartResult start_result, DataTypeController::State new_state, - const syncer::SyncError& error) { + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + + if (IsUnrecoverableResult(start_result)) + RecordUnrecoverableError(FROM_HERE, "StartFailed"); + // If we failed to start up, and we haven't been stopped yet, we need to // ensure we clean up the local service and shared change processor properly. if (new_state != RUNNING && state() != NOT_RUNNING && state() != STOPPING) { ClearSharedChangeProcessor(); StopLocalServiceAsync(); } - NonFrontendDataTypeController::StartDoneImpl(result, new_state, error); + + NonFrontendDataTypeController::StartDoneImpl(start_result, + new_state, + local_merge_result, + syncer_merge_result); } void NewNonFrontendDataTypeController::AbortModelStarting() { @@ -199,53 +226,79 @@ void NewNonFrontendDataTypeController:: const scoped_refptr<SharedChangeProcessor>& shared_change_processor) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(shared_change_processor.get()); + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); + base::WeakPtrFactory<syncer::SyncMergeResult> weak_ptr_factory( + &syncer_merge_result); // Connect |shared_change_processor| to the syncer and get the // syncer::SyncableService associated with type(). // Note that it's possible the shared_change_processor has already been // disconnected at this point, so all our accesses to the syncer from this // point on are through it. - local_service_ = shared_change_processor->Connect(profile_sync_factory(), - profile_sync_service(), - this, - type()); + local_service_ = shared_change_processor->Connect( + profile_sync_factory(), + profile_sync_service(), + this, + type(), + weak_ptr_factory.GetWeakPtr()); if (!local_service_.get()) { syncer::SyncError error(FROM_HERE, "Failed to connect to syncer.", type()); - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } if (!shared_change_processor->CryptoReadyIfNecessary()) { - StartFailed(NEEDS_CRYPTO, syncer::SyncError()); + StartDone(NEEDS_CRYPTO, + local_merge_result, + syncer_merge_result); return; } bool sync_has_nodes = false; if (!shared_change_processor->SyncModelHasUserCreatedNodes(&sync_has_nodes)) { syncer::SyncError error(FROM_HERE, "Failed to load sync nodes", type()); - StartFailed(UNRECOVERABLE_ERROR, error); + local_merge_result.set_error(error); + StartDone(UNRECOVERABLE_ERROR, + local_merge_result, + syncer_merge_result); return; } base::TimeTicks start_time = base::TimeTicks::Now(); - syncer::SyncError error; syncer::SyncDataList initial_sync_data; - error = shared_change_processor->GetSyncData(&initial_sync_data); + syncer::SyncError error = + shared_change_processor->GetSyncData(&initial_sync_data); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } // Passes a reference to |shared_change_processor|. - error = local_service_->MergeDataAndStartSyncing( - type(), - initial_sync_data, - scoped_ptr<syncer::SyncChangeProcessor>( - new SharedChangeProcessorRef(shared_change_processor)), - scoped_ptr<syncer::SyncErrorFactory>( - new SharedChangeProcessorRef(shared_change_processor))); + // TODO(zea): have SyncableService return a SyncMergeResult and pass that on + // to the ModelAssociationManager. + // TODO(zea): Call shared_change_processor_->GetAllSyncData(..) before + // and after MergeDataAndStartSyncing and store the item counts into + // syncer_merge_result. + error = + local_service_->MergeDataAndStartSyncing( + type(), + initial_sync_data, + scoped_ptr<syncer::SyncChangeProcessor>( + new SharedChangeProcessorRef(shared_change_processor)), + scoped_ptr<syncer::SyncErrorFactory>( + new SharedChangeProcessorRef(shared_change_processor))); RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } @@ -256,7 +309,9 @@ void NewNonFrontendDataTypeController:: // doesn't start trying to push changes from its thread before we activate // the datatype. shared_change_processor->ActivateDataType(model_safe_group()); - StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, RUNNING, syncer::SyncError()); + StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, + local_merge_result, + syncer_merge_result); } void NewNonFrontendDataTypeController::ClearSharedChangeProcessor() { diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h index fd45115..e8180b0 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller.h @@ -40,12 +40,15 @@ class NewNonFrontendDataTypeController : public NonFrontendDataTypeController { virtual void OnModelLoaded() OVERRIDE; // Overrides of NonFrontendDataTypeController methods. - virtual void StartDone(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error) OVERRIDE; - virtual void StartDoneImpl(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error) OVERRIDE; + virtual void StartDone( + DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) OVERRIDE; + virtual void StartDoneImpl( + DataTypeController::StartResult start_result, + DataTypeController::State new_state, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) OVERRIDE; private: // This overrides the same method in |NonFrontendDataTypeController|. diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h index b57ea1d..9841378 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_mock.h @@ -38,14 +38,15 @@ class NewNonFrontendDataTypeControllerMock const base::Closure&)); MOCK_METHOD0(StartAssociation, void()); MOCK_METHOD0(CreateSyncComponents, void()); - MOCK_METHOD2(StartFailed, void(StartResult result, - const syncer::SyncError& error)); - MOCK_METHOD3(StartDone, void(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error)); - MOCK_METHOD3(StartDoneImpl, void(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error)); + MOCK_METHOD3(StartDone, + void(DataTypeController::StartResult result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result)); + MOCK_METHOD4(StartDoneImpl, + void(DataTypeController::StartResult result, + DataTypeController::State new_state, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result)); MOCK_METHOD0(StopModels, void()); MOCK_METHOD0(StopAssociationAsync, bool()); MOCK_METHOD0(StopAssociation, void()); diff --git a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc index 4005e41..b66644d 100644 --- a/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/new_non_frontend_data_type_controller_unittest.cc @@ -197,7 +197,7 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { } void SetAssociateExpectations() { - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillOnce(Return(true)); @@ -210,7 +210,7 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { } void SetActivateExpectations(DataTypeController::StartResult result) { - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result,_,_)); } void SetStopExpectations() { @@ -224,7 +224,7 @@ class SyncNewNonFrontendDataTypeControllerTest : public testing::Test { if (DataTypeController::IsUnrecoverableResult(result)) EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _)); EXPECT_CALL(*dtc_mock_, RecordStartFailure(result)); - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result,_,_)); } void Start() { @@ -268,7 +268,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartOk) { TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartFirstRun) { SetStartExpectations(); - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillOnce(Return(true)); @@ -308,7 +308,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, AbortDuringStartModels) { // cleanly. TEST_F(SyncNewNonFrontendDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillOnce(Return(true)); @@ -335,7 +335,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, SetStartExpectations(); SetStartFailExpectations(DataTypeController::UNRECOVERABLE_ERROR); // Set up association to fail with an unrecoverable error. - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillRepeatedly(Return(true)); @@ -352,7 +352,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, SetStartExpectations(); SetStartFailExpectations(DataTypeController::NEEDS_CRYPTO); // Set up association to fail with a NEEDS_CRYPTO error. - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillRepeatedly(Return(false)); @@ -370,7 +370,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, AbortDuringAssociation) { SetStartExpectations(); SetStartFailExpectations(DataTypeController::ABORTED); - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(GetWeakPtrToSyncableService(&syncable_service_)); EXPECT_CALL(*change_processor_, CryptoReadyIfNecessary()). WillOnce(Return(true)); @@ -409,7 +409,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, FLAKY_StartAfterSyncShutdown) { EXPECT_CALL(*dtc_mock_, StopModels()); EXPECT_CALL(service_, DeactivateDataType(_)); EXPECT_CALL(*dtc_mock_, RecordStartFailure(DataTypeController::ABORTED)); - EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, new_non_frontend_dtc_->state()); Start(); new_non_frontend_dtc_->Stop(); @@ -419,7 +419,7 @@ TEST_F(SyncNewNonFrontendDataTypeControllerTest, FLAKY_StartAfterSyncShutdown) { Mock::VerifyAndClearExpectations(change_processor_); Mock::VerifyAndClearExpectations(dtc_mock_); - EXPECT_CALL(*change_processor_, Connect(_,_,_,_)). + EXPECT_CALL(*change_processor_, Connect(_,_,_,_,_)). WillOnce(Return(base::WeakPtr<syncer::SyncableService>())); new_non_frontend_dtc_->UnblockBackendTasks(); EXPECT_CALL(*dtc_mock_, RecordUnrecoverableError(_, _)); diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc index 1c3ddeb..938b817 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc @@ -89,7 +89,12 @@ void NonFrontendDataTypeController::StartAssociating( if (!StartAssociationAsync()) { syncer::SyncError error( FROM_HERE, "Failed to post StartAssociation", type()); - StartDoneImpl(ASSOCIATION_FAILED, DISABLED, error); + syncer::SyncMergeResult local_merge_result(type()); + local_merge_result.set_error(error); + StartDoneImpl(ASSOCIATION_FAILED, + DISABLED, + local_merge_result, + syncer::SyncMergeResult(type())); } } @@ -101,7 +106,10 @@ void NonFrontendDataTypeController::StopWhileAssociating() { if (model_associator_.get()) model_associator_->AbortAssociation(); if (!start_association_called_.IsSignaled()) { - StartDoneImpl(ABORTED, NOT_RUNNING, syncer::SyncError()); + StartDoneImpl(ABORTED, + NOT_RUNNING, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(type())); return; // There is nothing more for us to do. } } @@ -128,7 +136,10 @@ void NonFrontendDataTypeController::StopWhileAssociating() { } - StartDoneImpl(ABORTED, STOPPING, syncer::SyncError()); + StartDoneImpl(ABORTED, + STOPPING, + syncer::SyncMergeResult(type()), + syncer::SyncMergeResult(type())); } namespace { @@ -154,55 +165,7 @@ void NonFrontendDataTypeController::Stop() { // thread to finish the StartImpl() task. switch (state_) { case ASSOCIATING: - if (type() == syncer::PASSWORDS) { - LOG(INFO) << " Type is Passwords"; - StopWhileAssociating(); - } else if (type() == syncer::TYPED_URLS) { - LOG(INFO) << " Type is TypedUrl"; - StopWhileAssociating(); - } else if (type() == syncer::APPS) { - LOG(INFO) << "Type is Apps"; - StopWhileAssociating(); - } else if (type() == syncer::EXTENSIONS) { - LOG(INFO) << "Type is Extension"; - StopWhileAssociating(); - } else if (type() == syncer::PREFERENCES) { - LOG(INFO) << "Type is Preferences(Does not belong to non-frontend)"; - StopWhileAssociating(); - } else if (type() == syncer::EXTENSION_SETTINGS) { - LOG(INFO) << "Type is Extension Settings"; - StopWhileAssociating(); - } else if (type() == syncer::APP_SETTINGS) { - LOG(INFO) << "Type is App Settings."; - StopWhileAssociating(); - } else if (type() == syncer::BOOKMARKS) { - LOG(INFO) << "Type is BOOKMARKS."; - StopWhileAssociating(); - } else if (type() == syncer::AUTOFILL_PROFILE) { - LOG(INFO) << "Type is AUTOFILL_PROFILE."; - StopWhileAssociating(); - } else if (type() == syncer::AUTOFILL) { - LOG(INFO) << "Type is AUTOFILL."; - StopWhileAssociating(); - } else if (type() == syncer::THEMES) { - LOG(INFO) << "Type is THEMES."; - StopWhileAssociating(); - } else if (type() == syncer::NIGORI) { - LOG(INFO) << "Type is NIGORI."; - StopWhileAssociating(); - } else if (type() == syncer::SEARCH_ENGINES) { - LOG(INFO) << "Type is SEARCH_ENGINES."; - StopWhileAssociating(); - } else if (type() == syncer::SESSIONS) { - LOG(INFO) << "Type is SESSIONS."; - StopWhileAssociating(); - } else if (type() == syncer::APP_NOTIFICATIONS) { - LOG(INFO) << "Type is APP_NOTIFICATIONS."; - StopWhileAssociating(); - } else { - LOG(INFO) << "Type is unknown"; - StopWhileAssociating(); - } + StopWhileAssociating(); // TODO(sync) : This should be cleaned up. Once we move to the new api // this should not be a problem. @@ -284,40 +247,40 @@ bool NonFrontendDataTypeController::StartModels() { return true; } -void NonFrontendDataTypeController::StartFailed( - StartResult result, - const syncer::SyncError& error) { +void NonFrontendDataTypeController::StartDone( + DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); + DataTypeController::State new_state; - if (IsUnrecoverableResult(result)) - RecordUnrecoverableError(FROM_HERE, "StartFailed"); - StopAssociation(); - StartDone(result, - result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING, - error); -} + if (IsSuccessfulResult(start_result)) { + new_state = RUNNING; + } else { + new_state = (start_result == ASSOCIATION_FAILED ? DISABLED : NOT_RUNNING); + if (IsUnrecoverableResult(start_result)) + RecordUnrecoverableError(FROM_HERE, "StartFailed"); + StopAssociation(); + } -void NonFrontendDataTypeController::StartDone( - DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error) { - DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); abort_association_complete_.Signal(); base::AutoLock lock(abort_association_lock_); if (!abort_association_) { BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(&NonFrontendDataTypeController::StartDoneImpl, this, - result, + start_result, new_state, - error)); + local_merge_result, + syncer_merge_result)); } } void NonFrontendDataTypeController::StartDoneImpl( - DataTypeController::StartResult result, + DataTypeController::StartResult start_result, DataTypeController::State new_state, - const syncer::SyncError& error) { + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // It's possible to have StartDoneImpl called first from the UI thread // (due to Stop being called) and then posted from the non-UI thread. In @@ -331,7 +294,7 @@ void NonFrontendDataTypeController::StartDoneImpl( if (state_ != RUNNING) { // Start failed. StopModels(); - RecordStartFailure(result); + RecordStartFailure(start_result); } // We have to release the callback before we call it, since it's possible @@ -339,7 +302,7 @@ void NonFrontendDataTypeController::StartDoneImpl( // confused by the non-NULL start_callback_. StartCallback callback = start_callback_; start_callback_.Reset(); - callback.Run(result, error); + callback.Run(start_result, local_merge_result, syncer_merge_result); } void NonFrontendDataTypeController::StopModels() { @@ -427,6 +390,8 @@ void NonFrontendDataTypeController::set_change_processor( void NonFrontendDataTypeController::StartAssociation() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); { base::AutoLock lock(abort_association_lock_); @@ -441,30 +406,40 @@ void NonFrontendDataTypeController::StartAssociation() { DCHECK_EQ(state_, ASSOCIATING); if (!model_associator_->CryptoReadyIfNecessary()) { - StartFailed(NEEDS_CRYPTO, syncer::SyncError()); + StartDone(NEEDS_CRYPTO, + local_merge_result, + syncer_merge_result); return; } bool sync_has_nodes = false; if (!model_associator_->SyncModelHasUserCreatedNodes(&sync_has_nodes)) { syncer::SyncError error(FROM_HERE, "Failed to load sync nodes", type()); - StartFailed(UNRECOVERABLE_ERROR, error); + local_merge_result.set_error(error); + StartDone(UNRECOVERABLE_ERROR, + local_merge_result, + syncer_merge_result); return; } + // TODO(zea): Have AssociateModels fill the local and syncer merge results. base::TimeTicks start_time = base::TimeTicks::Now(); - syncer::SyncError error; - error = model_associator_->AssociateModels(); + syncer::SyncError error = model_associator_->AssociateModels(); // TODO(lipalani): crbug.com/122690 - handle abort. RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } profile_sync_service_->ActivateDataType(type(), model_safe_group(), change_processor()); - StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, RUNNING, syncer::SyncError()); + StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, + local_merge_result, + syncer_merge_result); } bool NonFrontendDataTypeController::StopAssociationAsync() { diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller.h b/chrome/browser/sync/glue/non_frontend_data_type_controller.h index 52ceba1..60507cf 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.h @@ -97,21 +97,19 @@ class NonFrontendDataTypeController : public DataTypeController { // Note: this is performed on the datatype's thread. virtual void CreateSyncComponents() = 0; - // Start failed, make sure we record it, clean up state, and invoke the - // callback on the frontend thread. - // Note: this is performed on the datatype's thread. - virtual void StartFailed(StartResult result, const syncer::SyncError& error); - // Start up complete, update the state and invoke the callback. // Note: this is performed on the datatype's thread. - virtual void StartDone(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error); + virtual void StartDone( + DataTypeController::StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result); // UI thread implementation of StartDone. - virtual void StartDoneImpl(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error); + virtual void StartDoneImpl( + DataTypeController::StartResult start_result, + DataTypeController::State new_state, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result); // Perform any DataType controller specific state cleanup before stopping // the datatype controller. The default implementation is a no-op. diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h b/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h index df07dff..a9cbfaf 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h @@ -37,14 +37,15 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController { const base::Closure&)); MOCK_METHOD0(StartAssociation, void()); MOCK_METHOD0(CreateSyncComponents, void()); - MOCK_METHOD2(StartFailed, void(StartResult result, - const syncer::SyncError& error)); - MOCK_METHOD3(StartDone, void(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error)); - MOCK_METHOD3(StartDoneImpl, void(DataTypeController::StartResult result, - DataTypeController::State new_state, - const syncer::SyncError& error)); + MOCK_METHOD3(StartDone, + void(DataTypeController::StartResult result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result)); + MOCK_METHOD4(StartDoneImpl, + void(DataTypeController::StartResult result, + DataTypeController::State new_state, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result)); MOCK_METHOD0(StopModels, void()); MOCK_METHOD0(StopAssociation, void()); MOCK_METHOD2(OnUnrecoverableErrorImpl, void(const tracked_objects::Location&, diff --git a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc index 6c43830..d1878ae 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc @@ -155,7 +155,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { void SetActivateExpectations(DataTypeController::StartResult result) { EXPECT_CALL(service_, ActivateDataType(_, _, _)); - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result, _, _)); } void SetStopExpectations() { @@ -174,7 +174,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test { WillOnce(Return(syncer::SyncError())); } EXPECT_CALL(*dtc_mock_, RecordStartFailure(result)); - EXPECT_CALL(start_callback_, Run(result,_)); + EXPECT_CALL(start_callback_, Run(result, _, _)); } static void SignalDone(WaitableEvent* done) { @@ -249,7 +249,7 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, StartAssociationFailed) { WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); EXPECT_CALL(*model_associator_, AssociateModels()). WillOnce( - Return(syncer::SyncError(FROM_HERE, "Error", syncer::AUTOFILL))); + Return(syncer::SyncError(FROM_HERE, "Error", syncer::BOOKMARKS))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED); // Set up association to fail with an association failed error. @@ -307,7 +307,7 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationInactive) { WillOnce(Return(syncer::SyncError())); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); EXPECT_CALL(service_, ActivateDataType(_, _, _)); - EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_)); + EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_,_)); EXPECT_CALL(*dtc_mock_, RecordStartFailure(DataTypeController::ABORTED)); SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); @@ -338,7 +338,7 @@ TEST_F(SyncNonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { EXPECT_CALL(service_, ActivateDataType(_, _, _)).WillOnce(DoAll( SignalEvent(&wait_for_db_thread_pause), WaitOnEvent(&pause_db_thread))); - EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_)); + EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_,_)); EXPECT_CALL(*dtc_mock_, RecordStartFailure(DataTypeController::ABORTED)); SetStopExpectations(); EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state()); diff --git a/chrome/browser/sync/glue/password_data_type_controller.cc b/chrome/browser/sync/glue/password_data_type_controller.cc index d071cb9..c2c244c 100644 --- a/chrome/browser/sync/glue/password_data_type_controller.cc +++ b/chrome/browser/sync/glue/password_data_type_controller.cc @@ -57,7 +57,12 @@ bool PasswordDataTypeController::StartModels() { FROM_HERE, "PasswordStore not initialized, password datatype controller aborting.", type()); - StartDoneImpl(ASSOCIATION_FAILED, DISABLED, error); + syncer::SyncMergeResult local_merge_result(type()); + local_merge_result.set_error(error); + StartDoneImpl(ASSOCIATION_FAILED, + DISABLED, + local_merge_result, + syncer::SyncMergeResult(type())); return false; } return true; diff --git a/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc b/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc index 77456ef..0db4f7a 100644 --- a/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/search_engine_data_type_controller_unittest.cc @@ -113,7 +113,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, StartURLServiceReady) { // We want to start ready. PreloadTemplateURLService(); SetActivateExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, search_engine_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); @@ -148,7 +148,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, StartFirstRun) { PreloadTemplateURLService(); SetActivateExpectations(); change_processor_->set_sync_model_has_user_created_nodes(false); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _, _)); Start(); EXPECT_TRUE(syncable_service_.syncing()); @@ -159,7 +159,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, StartAssociationFailed) { PreloadTemplateURLService(); SetStopExpectations(); EXPECT_CALL(start_callback_, - Run(DataTypeController::ASSOCIATION_FAILED, _)); + Run(DataTypeController::ASSOCIATION_FAILED, _, _)); syncable_service_.set_merge_data_and_start_syncing_error( syncer::SyncError(FROM_HERE, "Error", syncer::SEARCH_ENGINES)); @@ -176,7 +176,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, SetStartExpectations(); PreloadTemplateURLService(); EXPECT_CALL(start_callback_, - Run(DataTypeController::UNRECOVERABLE_ERROR, _)); + Run(DataTypeController::UNRECOVERABLE_ERROR, _, _)); // Set up association to fail with an unrecoverable error. change_processor_->set_sync_model_has_user_created_nodes_success(false); @@ -190,7 +190,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, Stop) { PreloadTemplateURLService(); SetActivateExpectations(); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, search_engine_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); @@ -212,7 +212,7 @@ TEST_F(SyncSearchEngineDataTypeControllerTest, &SearchEngineDataTypeController::Stop)); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); Start(); // This should cause search_engine_dtc_->Stop() to be called. search_engine_dtc_->OnSingleDatatypeUnrecoverableError(FROM_HERE, "Test"); diff --git a/chrome/browser/sync/glue/shared_change_processor.cc b/chrome/browser/sync/glue/shared_change_processor.cc index 1e536d1..324e7a4 100644 --- a/chrome/browser/sync/glue/shared_change_processor.cc +++ b/chrome/browser/sync/glue/shared_change_processor.cc @@ -49,7 +49,8 @@ base::WeakPtr<syncer::SyncableService> SharedChangeProcessor::Connect( ProfileSyncComponentsFactory* sync_factory, ProfileSyncService* sync_service, DataTypeErrorHandler* error_handler, - syncer::ModelType type) { + syncer::ModelType type, + const base::WeakPtr<syncer::SyncMergeResult>& merge_result) { DCHECK(sync_factory); DCHECK(sync_service); DCHECK(error_handler); @@ -68,6 +69,8 @@ base::WeakPtr<syncer::SyncableService> SharedChangeProcessor::Connect( disconnected_ = true; return base::WeakPtr<syncer::SyncableService>(); } + + // TODO(zea): Pass |merge_result| to the generic change processor. generic_change_processor_ = sync_factory->CreateGenericChangeProcessor(sync_service_, error_handler, diff --git a/chrome/browser/sync/glue/shared_change_processor.h b/chrome/browser/sync/glue/shared_change_processor.h index 7bc5af0..a775fae 100644 --- a/chrome/browser/sync/glue/shared_change_processor.h +++ b/chrome/browser/sync/glue/shared_change_processor.h @@ -14,6 +14,7 @@ #include "sync/api/sync_change_processor.h" #include "sync/api/sync_error.h" #include "sync/api/sync_error_factory.h" +#include "sync/api/sync_merge_result.h" #include "sync/internal_api/public/engine/model_safe_worker.h" class ProfileSyncComponentsFactory; @@ -63,7 +64,8 @@ class SharedChangeProcessor ProfileSyncComponentsFactory* sync_factory, ProfileSyncService* sync_service, DataTypeErrorHandler* error_handler, - syncer::ModelType type); + syncer::ModelType type, + const base::WeakPtr<syncer::SyncMergeResult>& merge_result); // Disconnects from the generic change processor. May be called from any // thread. After this, all attempts to interact with the change processor by diff --git a/chrome/browser/sync/glue/shared_change_processor_mock.h b/chrome/browser/sync/glue/shared_change_processor_mock.h index e19c19f..f0b05ca 100644 --- a/chrome/browser/sync/glue/shared_change_processor_mock.h +++ b/chrome/browser/sync/glue/shared_change_processor_mock.h @@ -16,11 +16,12 @@ class SharedChangeProcessorMock : public SharedChangeProcessor { public: SharedChangeProcessorMock(); - MOCK_METHOD4(Connect, base::WeakPtr<syncer::SyncableService>( + MOCK_METHOD5(Connect, base::WeakPtr<syncer::SyncableService>( ProfileSyncComponentsFactory*, ProfileSyncService*, DataTypeErrorHandler*, - syncer::ModelType)); + syncer::ModelType, + const base::WeakPtr<syncer::SyncMergeResult>&)); MOCK_METHOD0(Disconnect, bool()); MOCK_METHOD2(ProcessSyncChanges, syncer::SyncError(const tracked_objects::Location&, diff --git a/chrome/browser/sync/glue/shared_change_processor_unittest.cc b/chrome/browser/sync/glue/shared_change_processor_unittest.cc index 16e0c7c..755990c 100644 --- a/chrome/browser/sync/glue/shared_change_processor_unittest.cc +++ b/chrome/browser/sync/glue/shared_change_processor_unittest.cc @@ -104,10 +104,12 @@ class SyncSharedChangeProcessorTest : public testing::Test { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); EXPECT_CALL(sync_factory_, GetSyncableServiceForType(syncer::AUTOFILL)). WillOnce(GetWeakPtrToSyncableService(db_syncable_service_.get())); - EXPECT_TRUE(shared_change_processor->Connect(&sync_factory_, - &sync_service_, - &error_handler_, - syncer::AUTOFILL)); + EXPECT_TRUE(shared_change_processor->Connect( + &sync_factory_, + &sync_service_, + &error_handler_, + syncer::AUTOFILL, + base::WeakPtr<syncer::SyncMergeResult>())); } MessageLoopForUI ui_loop_; diff --git a/chrome/browser/sync/glue/ui_data_type_controller.cc b/chrome/browser/sync/glue/ui_data_type_controller.cc index 0b5f755..51fbf2d 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller.cc +++ b/chrome/browser/sync/glue/ui_data_type_controller.cc @@ -5,6 +5,7 @@ #include "chrome/browser/sync/glue/ui_data_type_controller.h" #include "base/logging.h" +#include "base/memory/weak_ptr.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/glue/shared_change_processor_ref.h" #include "chrome/browser/sync/profile_sync_components_factory.h" @@ -116,21 +117,32 @@ bool UIDataTypeController::StartModels() { void UIDataTypeController::Associate() { DCHECK_EQ(state_, ASSOCIATING); + syncer::SyncMergeResult local_merge_result(type()); + syncer::SyncMergeResult syncer_merge_result(type()); + base::WeakPtrFactory<syncer::SyncMergeResult> weak_ptr_factory( + &syncer_merge_result); // Connect |shared_change_processor_| to the syncer and get the // syncer::SyncableService associated with type(). - local_service_ = shared_change_processor_->Connect(profile_sync_factory_, - sync_service_, - this, - type()); + local_service_ = shared_change_processor_->Connect( + profile_sync_factory_, + sync_service_, + this, + type(), + weak_ptr_factory.GetWeakPtr()); if (!local_service_.get()) { syncer::SyncError error(FROM_HERE, "Failed to connect to syncer.", type()); - StartFailed(UNRECOVERABLE_ERROR, error); + local_merge_result.set_error(error); + StartDone(UNRECOVERABLE_ERROR, + local_merge_result, + syncer_merge_result); return; } if (!shared_change_processor_->CryptoReadyIfNecessary()) { - StartFailed(NEEDS_CRYPTO, syncer::SyncError()); + StartDone(NEEDS_CRYPTO, + local_merge_result, + syncer_merge_result); return; } @@ -138,7 +150,10 @@ void UIDataTypeController::Associate() { if (!shared_change_processor_->SyncModelHasUserCreatedNodes( &sync_has_nodes)) { syncer::SyncError error(FROM_HERE, "Failed to load sync nodes", type()); - StartFailed(UNRECOVERABLE_ERROR, error); + local_merge_result.set_error(error); + StartDone(UNRECOVERABLE_ERROR, + local_merge_result, + syncer_merge_result); return; } @@ -147,11 +162,19 @@ void UIDataTypeController::Associate() { syncer::SyncDataList initial_sync_data; error = shared_change_processor_->GetSyncData(&initial_sync_data); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } // Passes a reference to |shared_change_processor_|. + // TODO(zea): have SyncableService return a SyncMergeResult and pass that on + // to the ModelAssociationManager. + // TODO(zea): Call shared_change_processor_->GetAllSyncData(..) before + // and after MergeDataAndStartSyncing and store the item counts into + // syncer_merge_result. error = local_service_->MergeDataAndStartSyncing( type(), initial_sync_data, @@ -161,39 +184,18 @@ void UIDataTypeController::Associate() { new SharedChangeProcessorRef(shared_change_processor_))); RecordAssociationTime(base::TimeTicks::Now() - start_time); if (error.IsSet()) { - StartFailed(ASSOCIATION_FAILED, error); + local_merge_result.set_error(error); + StartDone(ASSOCIATION_FAILED, + local_merge_result, + syncer_merge_result); return; } shared_change_processor_->ActivateDataType(model_safe_group()); state_ = RUNNING; - StartDone(sync_has_nodes ? OK : OK_FIRST_RUN); -} - -void UIDataTypeController::StartFailed(StartResult result, - const syncer::SyncError& error) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (IsUnrecoverableResult(result)) - RecordUnrecoverableError(FROM_HERE, "StartFailed"); - StopModels(); - if (result == ASSOCIATION_FAILED) { - state_ = DISABLED; - } else { - state_ = NOT_RUNNING; - } - RecordStartFailure(result); - - if (shared_change_processor_.get()) { - shared_change_processor_->Disconnect(); - shared_change_processor_ = NULL; - } - - // We have to release the callback before we call it, since it's possible - // invoking the callback will trigger a call to Stop(), which will get - // confused by the non-NULL start_callback_. - StartCallback callback = start_callback_; - start_callback_.Reset(); - callback.Run(result, error); + StartDone(sync_has_nodes ? OK : OK_FIRST_RUN, + local_merge_result, + syncer_merge_result); } void UIDataTypeController::AbortModelLoad() { @@ -211,15 +213,36 @@ void UIDataTypeController::AbortModelLoad() { type())); } -void UIDataTypeController::StartDone(StartResult result) { +void UIDataTypeController::StartDone( + StartResult start_result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (!IsSuccessfulResult(start_result)) { + if (IsUnrecoverableResult(start_result)) + RecordUnrecoverableError(FROM_HERE, "StartFailed"); + + StopModels(); + if (start_result == ASSOCIATION_FAILED) { + state_ = DISABLED; + } else { + state_ = NOT_RUNNING; + } + RecordStartFailure(start_result); + + if (shared_change_processor_.get()) { + shared_change_processor_->Disconnect(); + shared_change_processor_ = NULL; + } + } + // We have to release the callback before we call it, since it's possible // invoking the callback will trigger a call to Stop(), which will get // confused by the non-NULL start_callback_. StartCallback callback = start_callback_; start_callback_.Reset(); - callback.Run(result, syncer::SyncError()); + callback.Run(start_result, local_merge_result, syncer_merge_result); } void UIDataTypeController::Stop() { diff --git a/chrome/browser/sync/glue/ui_data_type_controller.h b/chrome/browser/sync/glue/ui_data_type_controller.h index 40dd418..dbd6f08 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller.h +++ b/chrome/browser/sync/glue/ui_data_type_controller.h @@ -77,9 +77,10 @@ class UIDataTypeController : public DataTypeController { // DataTypeController interface. virtual void OnModelLoaded() OVERRIDE; - // Helper methods for cleaning up state and invoking the start callback. - virtual void StartFailed(StartResult result, const syncer::SyncError& error); - virtual void StartDone(StartResult result); + // Helper method for cleaning up state and invoking the start callback. + virtual void StartDone(StartResult result, + const syncer::SyncMergeResult& local_merge_result, + const syncer::SyncMergeResult& syncer_merge_result); // Record association time. virtual void RecordAssociationTime(base::TimeDelta time); diff --git a/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc b/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc index 042c72f..b1a5d55 100644 --- a/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/ui_data_type_controller_unittest.cc @@ -113,7 +113,7 @@ class SyncUIDataTypeControllerTest : public testing::Test { // state. TEST_F(SyncUIDataTypeControllerTest, Start) { SetActivateExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); @@ -127,7 +127,7 @@ TEST_F(SyncUIDataTypeControllerTest, Start) { TEST_F(SyncUIDataTypeControllerTest, StartStop) { SetActivateExpectations(); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); @@ -144,7 +144,7 @@ TEST_F(SyncUIDataTypeControllerTest, StartStop) { TEST_F(SyncUIDataTypeControllerTest, StartStopFirstRun) { SetActivateExpectations(); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK_FIRST_RUN, _, _)); change_processor_->set_sync_model_has_user_created_nodes(false); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); @@ -163,7 +163,7 @@ TEST_F(SyncUIDataTypeControllerTest, StartStopFirstRun) { TEST_F(SyncUIDataTypeControllerTest, StartAssociationFailed) { SetStopExpectations(); EXPECT_CALL(start_callback_, - Run(DataTypeController::ASSOCIATION_FAILED, _)); + Run(DataTypeController::ASSOCIATION_FAILED, _, _)); syncable_service_.set_merge_data_and_start_syncing_error( syncer::SyncError(FROM_HERE, "Error", type_)); @@ -183,7 +183,7 @@ TEST_F(SyncUIDataTypeControllerTest, StartAssociationFailed) { TEST_F(SyncUIDataTypeControllerTest, StartAssociationTriggersUnrecoverableError) { EXPECT_CALL(start_callback_, - Run(DataTypeController::UNRECOVERABLE_ERROR, _)); + Run(DataTypeController::UNRECOVERABLE_ERROR, _, _)); change_processor_->set_sync_model_has_user_created_nodes_success(false); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); @@ -201,7 +201,7 @@ TEST_F(SyncUIDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) { WillOnce(InvokeWithoutArgs(preference_dtc_.get(), &UIDataTypeController::Stop)); SetStopExpectations(); - EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _)); + EXPECT_CALL(start_callback_, Run(DataTypeController::OK, _, _)); EXPECT_EQ(DataTypeController::NOT_RUNNING, preference_dtc_->state()); EXPECT_FALSE(syncable_service_.syncing()); diff --git a/sync/api/sync_merge_result.cc b/sync/api/sync_merge_result.cc new file mode 100644 index 0000000..c2253b3 --- /dev/null +++ b/sync/api/sync_merge_result.cc @@ -0,0 +1,77 @@ +// 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 "sync/api/sync_merge_result.h" + +namespace syncer { + +SyncMergeResult::SyncMergeResult(ModelType type) + : model_type_(type), + num_items_before_association_(0), + num_items_after_association_(0), + num_items_added_(0), + num_items_deleted_(0), + num_items_modified_(0) { +} + +SyncMergeResult::~SyncMergeResult() { +} + +// Setters. +void SyncMergeResult::set_error(SyncError error) { + DCHECK_EQ(model_type_, error.type()); + error_ = error; +} + +void SyncMergeResult::set_num_items_before_association( + int num_items_before_association) { + num_items_before_association_ = num_items_before_association; +} + +void SyncMergeResult::set_num_items_after_association( + int num_items_after_association) { + num_items_after_association_ = num_items_after_association; +} + +void SyncMergeResult::set_num_items_added(int num_items_added) { + num_items_added_ = num_items_added; +} + +void SyncMergeResult::set_num_items_deleted(int num_items_deleted) { + num_items_deleted_ = num_items_deleted; +} + +void SyncMergeResult::set_num_items_modified(int num_items_modified) { + num_items_modified_ = num_items_modified; +} + +ModelType SyncMergeResult::model_type() const { + return model_type_; +} + +SyncError SyncMergeResult::error() const { + return error_; +} + +int SyncMergeResult::num_items_before_association() const { + return num_items_before_association_; +} + +int SyncMergeResult::num_items_after_association() const { + return num_items_after_association_; +} + +int SyncMergeResult::num_items_added() const { + return num_items_added_; +} + +int SyncMergeResult::num_items_deleted() const { + return num_items_deleted_; +} + +int SyncMergeResult::num_items_modified() const { + return num_items_modified_; +} + +} // namespace syncer diff --git a/sync/api/sync_merge_result.h b/sync/api/sync_merge_result.h new file mode 100644 index 0000000..41accfd --- /dev/null +++ b/sync/api/sync_merge_result.h @@ -0,0 +1,73 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_API_SYNC_MERGE_RESULT_H_ +#define SYNC_API_SYNC_MERGE_RESULT_H_ + +#include "sync/api/sync_error.h" +#include "sync/internal_api/public/base/model_type.h" + +namespace syncer { + +// A model-type-specific view of a sync merge. This class encapsulates the +// state before and after the merge as well as the deltas and any error that +// occurred. +// Note: This class only tracks one side of the merge. In other words, if built +// by the local SyncableService, all values correspond to the local state before +// and after merging, and the delta's applied to that state. Sync's change +// processor will create a separate merge result. +class SyncMergeResult { + public: + // Initialize an empty merge result for model type |type|. + explicit SyncMergeResult(ModelType type); + ~SyncMergeResult(); + + // Default copy and assign welcome. + + // Setters. + // Note: |error.IsSet()| must be true, and |error.type()| must match + // model_type_ + void set_error(SyncError error); + void set_num_items_before_association(int num_items_before_association); + void set_num_items_after_association(int num_items_after_association); + void set_num_items_added(int num_items_added); + void set_num_items_deleted(int num_items_deleted); + void set_num_items_modified(int num_items_modified); + + // Getters. + ModelType model_type() const; + SyncError error() const; + int num_items_before_association() const; + int num_items_after_association() const; + int num_items_added() const; + int num_items_deleted() const; + int num_items_modified() const; + + private: + // Make |this| into a copy of |other|. + void CopyFrom(const SyncMergeResult& other); + + // The datatype that was associated. + ModelType model_type_; + + // The error encountered during association. Unset if no error was + // encountered. + SyncError error_; + + // The state of the world before association. + int num_items_before_association_; + + // The state of the world after association. + int num_items_after_association_; + + // The changes that took place during association. In a correctly working + // system these should be the deltas between before and after. + int num_items_added_; + int num_items_deleted_; + int num_items_modified_; +}; + +} // namespace syncer + +#endif // SYNC_API_SYNC_MERGE_RESULT_H_ diff --git a/sync/api/sync_merge_result_unittest.cc b/sync/api/sync_merge_result_unittest.cc new file mode 100644 index 0000000..dddfa4c --- /dev/null +++ b/sync/api/sync_merge_result_unittest.cc @@ -0,0 +1,77 @@ +// 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 "sync/api/sync_merge_result.h" + +#include "base/location.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +namespace { + +typedef testing::Test SyncMergeResultTest; + +TEST_F(SyncMergeResultTest, Unset) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_FALSE(merge_result.error().IsSet()); + EXPECT_EQ(0, merge_result.num_items_before_association()); + EXPECT_EQ(0, merge_result.num_items_after_association()); + EXPECT_EQ(0, merge_result.num_items_added()); + EXPECT_EQ(0, merge_result.num_items_deleted()); + EXPECT_EQ(0, merge_result.num_items_modified()); +} + +TEST_F(SyncMergeResultTest, SetError) { + SyncError error(FROM_HERE, "message", BOOKMARKS); + SyncMergeResult merge_result(BOOKMARKS); + + merge_result.set_error(error); + EXPECT_TRUE(merge_result.error().IsSet()); + EXPECT_EQ(BOOKMARKS, merge_result.model_type()); +} + +TEST_F(SyncMergeResultTest, SetNumItemsBeforeAssociation) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_EQ(0, merge_result.num_items_before_association()); + + merge_result.set_num_items_before_association(10); + EXPECT_EQ(10, merge_result.num_items_before_association()); +} + +TEST_F(SyncMergeResultTest, SetNumItemsAfterAssociation) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_EQ(0, merge_result.num_items_after_association()); + + merge_result.set_num_items_after_association(10); + EXPECT_EQ(10, merge_result.num_items_after_association()); +} + +TEST_F(SyncMergeResultTest, SetNumItemsAdded) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_EQ(0, merge_result.num_items_added()); + + merge_result.set_num_items_added(10); + EXPECT_EQ(10, merge_result.num_items_added()); +} + +TEST_F(SyncMergeResultTest, SetNumItemsDeleted) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_EQ(0, merge_result.num_items_deleted()); + + merge_result.set_num_items_deleted(10); + EXPECT_EQ(10, merge_result.num_items_deleted()); +} + +TEST_F(SyncMergeResultTest, SetNumItemsModified) { + SyncMergeResult merge_result(BOOKMARKS); + EXPECT_EQ(0, merge_result.num_items_modified()); + + merge_result.set_num_items_modified(10); + EXPECT_EQ(10, merge_result.num_items_modified()); +} + +} // namespace + +} // namespace syncer diff --git a/sync/sync.gyp b/sync/sync.gyp index 6172b84..758ef4c 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -415,6 +415,8 @@ 'api/sync_error.cc', 'api/sync_error_factory.h', 'api/sync_error_factory.cc', + 'api/sync_merge_result.h', + 'api/sync_merge_result.cc', 'api/time.h', ], }, @@ -612,6 +614,7 @@ 'internal_api/public/base/ordinal_unittest.cc', 'internal_api/public/engine/model_safe_worker_unittest.cc', 'internal_api/public/util/immutable_unittest.cc', + 'internal_api/public/util/weak_handle_unittest.cc', 'engine/apply_control_data_updates_unittest.cc', 'engine/apply_updates_and_resolve_conflicts_command_unittest.cc', 'engine/backoff_delay_provider_unittest.cc', @@ -647,7 +650,6 @@ 'util/get_session_name_unittest.cc', 'util/nigori_unittest.cc', 'util/protobuf_unittest.cc', - 'internal_api/public/util/weak_handle_unittest.cc', ], 'conditions': [ ['OS == "ios" and coverage != 0', { @@ -832,6 +834,7 @@ 'sources': [ 'api/sync_change_unittest.cc', 'api/sync_error_unittest.cc', + 'api/sync_merge_result_unittest.cc', ], }, }, |