diff options
59 files changed, 786 insertions, 273 deletions
diff --git a/chrome/browser/prefs/pref_model_associator.cc b/chrome/browser/prefs/pref_model_associator.cc index 5c004a7b..88c9640 100644 --- a/chrome/browser/prefs/pref_model_associator.cc +++ b/chrome/browser/prefs/pref_model_associator.cc @@ -116,7 +116,7 @@ void PrefModelAssociator::InitPrefAndAssociate( return; } -bool PrefModelAssociator::MergeDataAndStartSyncing( +SyncError PrefModelAssociator::MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, SyncChangeProcessor* sync_processor) { @@ -158,9 +158,14 @@ bool PrefModelAssociator::MergeDataAndStartSyncing( } // Push updates to sync. - sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes); + SyncError error = + sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes); + if (error.IsSet()) { + return error; + } + models_associated_ = true; - return true; + return SyncError(); } void PrefModelAssociator::StopSyncing(syncable::ModelType type) { @@ -306,11 +311,15 @@ SyncDataList PrefModelAssociator::GetAllSyncData(syncable::ModelType type) return current_data; } -void PrefModelAssociator::ProcessSyncChanges( +SyncError PrefModelAssociator::ProcessSyncChanges( const tracked_objects::Location& from_here, const SyncChangeList& change_list) { - if (!models_associated_) - return; + if (!models_associated_) { + SyncError error(FROM_HERE, + "Models not yet associated.", + PREFERENCES); + return error; + } AutoReset<bool> processing_changes(&processing_syncer_changes_, true); SyncChangeList::const_iterator iter; for (iter = change_list.begin(); iter != change_list.end(); ++iter) { @@ -359,6 +368,7 @@ void PrefModelAssociator::ProcessSyncChanges( SendUpdateNotificationsIfNecessary(name); } + return SyncError(); } Value* PrefModelAssociator::ReadPreferenceSpecifics( @@ -432,5 +442,9 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) { } changes.push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data)); } - sync_processor_->ProcessSyncChanges(FROM_HERE, changes); + + SyncError error = + sync_processor_->ProcessSyncChanges(FROM_HERE, changes); + if (error.IsSet()) + StopSyncing(PREFERENCES); } diff --git a/chrome/browser/prefs/pref_model_associator.h b/chrome/browser/prefs/pref_model_associator.h index 1503153..a185a78 100644 --- a/chrome/browser/prefs/pref_model_associator.h +++ b/chrome/browser/prefs/pref_model_associator.h @@ -37,9 +37,10 @@ class PrefModelAssociator // SyncableService implementation. virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE; - virtual void ProcessSyncChanges(const tracked_objects::Location& from_here, - const SyncChangeList& change_list) OVERRIDE; - virtual bool MergeDataAndStartSyncing( + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE; + virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, SyncChangeProcessor* sync_processor) OVERRIDE; diff --git a/chrome/browser/sync/api/sync_change_processor.h b/chrome/browser/sync/api/sync_change_processor.h index 80be610..edc7c0f 100644 --- a/chrome/browser/sync/api/sync_change_processor.h +++ b/chrome/browser/sync/api/sync_change_processor.h @@ -8,6 +8,8 @@ #include <vector> +#include "chrome/browser/sync/api/sync_error.h" + class SyncChange; namespace tracked_objects { @@ -20,8 +22,15 @@ typedef std::vector<SyncChange> SyncChangeList; class SyncChangeProcessor { public: // Process a list of SyncChanges. - virtual void ProcessSyncChanges(const tracked_objects::Location& from_here, - const SyncChangeList& change_list) = 0; + // Returns: A default SyncError (IsSet() == false) if no errors were + // encountered, and a filled SyncError (IsSet() == true) + // otherwise. + // Inputs: + // |from_here|: allows tracking of where sync changes originate. + // |change_list|: is the list of sync changes in need of processing. + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) = 0; protected: virtual ~SyncChangeProcessor(); }; diff --git a/chrome/browser/sync/api/sync_error.cc b/chrome/browser/sync/api/sync_error.cc new file mode 100644 index 0000000..6ffa495 --- /dev/null +++ b/chrome/browser/sync/api/sync_error.cc @@ -0,0 +1,92 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/api/sync_error.h" + +#include "base/logging.h" +#include "base/tracked.h" + +SyncError::SyncError() { + Clear(); +} + +SyncError::SyncError(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type) { + Init(location, message, type); + PrintLogError(); +} + +SyncError::SyncError(const SyncError& other) { + Copy(other); +} + +SyncError::~SyncError() { +} + +SyncError& SyncError::operator=(const SyncError& other) { + if (this == &other) { + return *this; + } + Copy(other); + return *this; +} + +void SyncError::Copy(const SyncError& other) { + if (other.IsSet()) { + Init(other.location(), + other.message(), + other.type()); + } else { + Clear(); + } +} + +void SyncError::Clear() { + location_.reset(); + message_ = std::string(); + type_ = syncable::UNSPECIFIED; +} + +void SyncError::Reset(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type) { + Init(location, message, type); + PrintLogError(); +} + +void SyncError::Init(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type) { + location_.reset(new tracked_objects::Location(location)); + message_ = message; + type_ = type; +} + +bool SyncError::IsSet() const { + return location_.get() != NULL; +} + +const tracked_objects::Location& SyncError::location() const { + CHECK(IsSet()); + return *location_; +} + +const std::string& SyncError::message() const { + CHECK(IsSet()); + return message_; +} + +syncable::ModelType SyncError::type() const { + CHECK(IsSet()); + return type_; +} + +void SyncError::PrintLogError() const { + LAZY_STREAM(logging::LogMessage(location_->file_name(), + location_->line_number(), + logging::LOG_ERROR).stream(), + LOG_IS_ON(ERROR)) + << syncable::ModelTypeToString(type_) << " Sync Error: " << message_; +} diff --git a/chrome/browser/sync/api/sync_error.h b/chrome/browser/sync/api/sync_error.h new file mode 100644 index 0000000..c4bc7c3 --- /dev/null +++ b/chrome/browser/sync/api/sync_error.h @@ -0,0 +1,79 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_SYNC_API_SYNC_ERROR_H_ +#define CHROME_BROWSER_SYNC_API_SYNC_ERROR_H_ +#pragma once + +#include <string> + +#include "base/memory/scoped_ptr.h" +#include "chrome/browser/sync/syncable/model_type.h" + +namespace tracked_objects { +class Location; +} // namespace tracked_objects + +// Sync errors are used for debug purposes and handled internally and/or +// exposed through Chrome's "about:sync" internal page. They are considered +// unrecoverable for the datatype creating them, and should only be used as +// such. +// This class is copy-friendly and thread-safe. +class SyncError { + public: + // Default constructor refers to "no error", and IsSet() will return false. + SyncError(); + + // Create a new Sync error triggered by datatype |type| with debug message + // |message| from the specified location. IsSet() will return true. + // Will print the new error to LOG(ERROR). + SyncError(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type); + + // Copy and assign via deep copy. + SyncError(const SyncError& other); + SyncError& operator=(const SyncError& other); + + ~SyncError(); + + // Reset the current error to a new error. May be called irrespective of + // whether IsSet() is true. After this is called, IsSet() will return true. + // Will print the new error to LOG(ERROR). + void Reset(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type); + + // Whether this is a valid error or not. + bool IsSet() const; + + // These must only be called if IsSet() is true. + const tracked_objects::Location& location() const; + const std::string& message() const; + syncable::ModelType type() const; + + private: + // Print error information to log. + void PrintLogError() const; + + // Make a copy of a SyncError. If other.IsSet() == false, this->IsSet() will + // now return false. + void Copy(const SyncError& other); + + // Initialize the local error data with the specified error data. After this + // is called, IsSet() will return true. + void Init(const tracked_objects::Location& location, + const std::string& message, + syncable::ModelType type); + + // Reset the error to it's default (unset) values. + void Clear(); + + // scoped_ptr is necessary because Location objects aren't assignable. + scoped_ptr<tracked_objects::Location> location_; + std::string message_; + syncable::ModelType type_; +}; + +#endif // CHROME_BROWSER_SYNC_API_SYNC_ERROR_H_ diff --git a/chrome/browser/sync/api/sync_error_unittest.cc b/chrome/browser/sync/api/sync_error_unittest.cc new file mode 100644 index 0000000..6e2d9a8 --- /dev/null +++ b/chrome/browser/sync/api/sync_error_unittest.cc @@ -0,0 +1,114 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/api/sync_error.h" + +#include <string> + +#include "base/tracked.h" +#include "testing/gtest/include/gtest/gtest.h" + +using std::string; +using syncable::ModelType; + +namespace { + +typedef testing::Test SyncErrorTest; + +TEST_F(SyncErrorTest, Unset) { + SyncError error; + EXPECT_FALSE(error.IsSet()); +} + +TEST_F(SyncErrorTest, Default) { + tracked_objects::Location location = FROM_HERE; + std::string msg = "test"; + ModelType type = syncable::PREFERENCES; + SyncError error(location, msg, type); + EXPECT_TRUE(error.IsSet()); + EXPECT_EQ(location.line_number(), error.location().line_number()); + EXPECT_EQ(msg, error.message()); + EXPECT_EQ(type, error.type()); +} + +TEST_F(SyncErrorTest, Reset) { + tracked_objects::Location location = FROM_HERE; + std::string msg = "test"; + ModelType type = syncable::PREFERENCES; + + SyncError error; + EXPECT_FALSE(error.IsSet()); + + error.Reset(location, msg, type); + EXPECT_TRUE(error.IsSet()); + EXPECT_EQ(location.line_number(), error.location().line_number()); + EXPECT_EQ(msg, error.message()); + EXPECT_EQ(type, error.type()); + + tracked_objects::Location location2 = FROM_HERE; + std::string msg2 = "test"; + ModelType type2 = syncable::PREFERENCES; + error.Reset(location2, msg2, type2); + EXPECT_TRUE(error.IsSet()); + EXPECT_EQ(location2.line_number(), error.location().line_number()); + EXPECT_EQ(msg2, error.message()); + EXPECT_EQ(type2, error.type()); +} + +TEST_F(SyncErrorTest, Copy) { + tracked_objects::Location location = FROM_HERE; + std::string msg = "test"; + ModelType type = syncable::PREFERENCES; + + SyncError error1; + EXPECT_FALSE(error1.IsSet()); + SyncError error2(error1); + EXPECT_FALSE(error2.IsSet()); + + error1.Reset(location, msg, type); + EXPECT_TRUE(error1.IsSet()); + EXPECT_EQ(location.line_number(), error1.location().line_number()); + EXPECT_EQ(msg, error1.message()); + EXPECT_EQ(type, error1.type()); + + SyncError error3(error1); + EXPECT_TRUE(error3.IsSet()); + EXPECT_EQ(error1.location().line_number(), error3.location().line_number()); + EXPECT_EQ(error1.message(), error3.message()); + EXPECT_EQ(error1.type(), error3.type()); + + SyncError error4; + EXPECT_FALSE(error4.IsSet()); + SyncError error5(error4); + EXPECT_FALSE(error5.IsSet()); +} + +TEST_F(SyncErrorTest, Assign) { + tracked_objects::Location location = FROM_HERE; + std::string msg = "test"; + ModelType type = syncable::PREFERENCES; + + SyncError error1; + EXPECT_FALSE(error1.IsSet()); + SyncError error2; + error2 = error1; + EXPECT_FALSE(error2.IsSet()); + + error1.Reset(location, msg, type); + EXPECT_TRUE(error1.IsSet()); + EXPECT_EQ(location.line_number(), error1.location().line_number()); + EXPECT_EQ(msg, error1.message()); + EXPECT_EQ(type, error1.type()); + + error2 = error1; + EXPECT_TRUE(error2.IsSet()); + EXPECT_EQ(error1.location().line_number(), error2.location().line_number()); + EXPECT_EQ(error1.message(), error2.message()); + EXPECT_EQ(error1.type(), error2.type()); + + error2 = SyncError(); + EXPECT_FALSE(error2.IsSet()); +} + +} // namespace diff --git a/chrome/browser/sync/api/syncable_service.h b/chrome/browser/sync/api/syncable_service.h index 3c40975..9d7ae62 100644 --- a/chrome/browser/sync/api/syncable_service.h +++ b/chrome/browser/sync/api/syncable_service.h @@ -12,6 +12,7 @@ #include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/api/sync_change_processor.h" #include "chrome/browser/sync/api/sync_data.h" +#include "chrome/browser/sync/api/sync_error.h" class SyncData; @@ -25,7 +26,10 @@ class SyncableService : public SyncChangeProcessor { // two. After this, the SyncableService's local data should match the server // data, and the service should be ready to receive and process any further // SyncChange's as they occur. - virtual bool MergeDataAndStartSyncing( + // Returns: A default SyncError (IsSet() == false) if no errors were + // encountered, and a filled SyncError (IsSet() == true) + // otherwise. + virtual SyncError MergeDataAndStartSyncing( syncable::ModelType type, const SyncDataList& initial_sync_data, SyncChangeProcessor* sync_processor) = 0; @@ -40,7 +44,10 @@ class SyncableService : public SyncChangeProcessor { // SyncChangeProcessor interface. // Process a list of new SyncChanges and update the local data as necessary. - virtual void ProcessSyncChanges( + // Returns: A default SyncError (IsSet() == false) if no errors were + // encountered, and a filled SyncError (IsSet() == true) + // otherwise. + virtual SyncError ProcessSyncChanges( const tracked_objects::Location& from_here, const SyncChangeList& change_list) OVERRIDE = 0; diff --git a/chrome/browser/sync/api/syncable_service_mock.h b/chrome/browser/sync/api/syncable_service_mock.h index 29c5e9b..e1e6bf7 100644 --- a/chrome/browser/sync/api/syncable_service_mock.h +++ b/chrome/browser/sync/api/syncable_service_mock.h @@ -16,13 +16,15 @@ class SyncableServiceMock : public SyncableService { SyncableServiceMock(); virtual ~SyncableServiceMock(); - MOCK_METHOD3(MergeDataAndStartSyncing, bool( - syncable::ModelType, const SyncDataList&, - SyncChangeProcessor* sync_processor)); + MOCK_METHOD3(MergeDataAndStartSyncing, + SyncError(syncable::ModelType, + const SyncDataList&, + SyncChangeProcessor* sync_processor)); MOCK_METHOD1(StopSyncing, void(syncable::ModelType)); - MOCK_CONST_METHOD1(GetAllSyncData, SyncDataList(syncable::ModelType)); + MOCK_CONST_METHOD1(GetAllSyncData, SyncDataList(syncable::ModelType type)); MOCK_METHOD2(ProcessSyncChanges, - void(const tracked_objects::Location&, const SyncChangeList&)); + SyncError(const tracked_objects::Location&, + const SyncChangeList&)); }; #endif // CHROME_BROWSER_SYNC_API_SYNCABLE_SERVICE_MOCK_H_ diff --git a/chrome/browser/sync/backend_migrator.cc b/chrome/browser/sync/backend_migrator.cc index 4d8550f..cc0b613 100644 --- a/chrome/browser/sync/backend_migrator.cc +++ b/chrome/browser/sync/backend_migrator.cc @@ -121,9 +121,8 @@ void BackendMigrator::Observe(int type, if (state_ == IDLE) return; - DataTypeManager::ConfigureResultWithErrorLocation* result = - Details<DataTypeManager::ConfigureResultWithErrorLocation>( - details).ptr(); + const DataTypeManager::ConfigureResult* result = + Details<DataTypeManager::ConfigureResult>(details).ptr(); ModelTypeSet intersection; std::set_intersection(result->requested_types.begin(), @@ -151,7 +150,7 @@ void BackendMigrator::Observe(int type, return; } - if (result->result != DataTypeManager::OK) { + if (result->status != DataTypeManager::OK) { // If this fails, and we're disabling types, a type may or may not be // disabled until the user restarts the browser. If this wasn't an abort, // any failure will be reported as an unrecoverable error to the UI. If it diff --git a/chrome/browser/sync/backend_migrator_unittest.cc b/chrome/browser/sync/backend_migrator_unittest.cc index cdc2643..bd9677b 100644 --- a/chrome/browser/sync/backend_migrator_unittest.cc +++ b/chrome/browser/sync/backend_migrator_unittest.cc @@ -59,15 +59,25 @@ class BackendMigratorTest : public testing::Test { .WillOnce(Return(snap_.get())); } - void SendConfigureDone(DataTypeManager::ConfigureResult result, + void SendConfigureDone(DataTypeManager::ConfigureStatus status, const syncable::ModelTypeSet& types) { - DataTypeManager::ConfigureResultWithErrorLocation result_with_location( - result, FROM_HERE, types); - NotificationService::current()->Notify( - chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, - Source<DataTypeManager>(&manager_), - Details<DataTypeManager::ConfigureResultWithErrorLocation>( - &result_with_location)); + if (status == DataTypeManager::OK) { + DataTypeManager::ConfigureResult result(status, types); + NotificationService::current()->Notify( + chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, + Source<DataTypeManager>(&manager_), + Details<const DataTypeManager::ConfigureResult>(&result)); + } else { + DataTypeManager::ConfigureResult result( + status, + types, + syncable::ModelTypeSet(), + FROM_HERE); + NotificationService::current()->Notify( + chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, + Source<DataTypeManager>(&manager_), + Details<const DataTypeManager::ConfigureResult>(&result)); + } } ProfileSyncService* service() { return &service_; } diff --git a/chrome/browser/sync/glue/app_model_associator.cc b/chrome/browser/sync/glue/app_model_associator.cc index fd6dd24..a0a566d 100644 --- a/chrome/browser/sync/glue/app_model_associator.cc +++ b/chrome/browser/sync/glue/app_model_associator.cc @@ -7,6 +7,7 @@ #include "base/logging.h" #include "base/tracked.h" #include "chrome/browser/extensions/extension_sync_data.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/extension_sync_traits.h" #include "chrome/browser/sync/glue/extension_sync.h" @@ -31,22 +32,24 @@ AppModelAssociator::~AppModelAssociator() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } -bool AppModelAssociator::AssociateModels() { +bool AppModelAssociator::AssociateModels(SyncError* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ExtensionDataMap extension_data_map; if (!SlurpExtensionData( traits_, *extension_service_, user_share_, &extension_data_map)) { + error->Reset(FROM_HERE, "Failed to get app data.", model_type()); return false; } if (!FlushExtensionData( traits_, extension_data_map, extension_service_, user_share_)) { + error->Reset(FROM_HERE, "Failed to flush app data.", model_type()); return false; } return true; } -bool AppModelAssociator::DisassociateModels() { +bool AppModelAssociator::DisassociateModels(SyncError* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // Nothing to do. return true; diff --git a/chrome/browser/sync/glue/app_model_associator.h b/chrome/browser/sync/glue/app_model_associator.h index 65c5d55..0d975cd 100644 --- a/chrome/browser/sync/glue/app_model_associator.h +++ b/chrome/browser/sync/glue/app_model_associator.h @@ -32,8 +32,8 @@ class AppModelAssociator : public AssociatorInterface { static syncable::ModelType model_type() { return syncable::APPS; } // AssociatorInterface implementation. - virtual bool AssociateModels(); - virtual bool DisassociateModels(); + virtual bool AssociateModels(SyncError* error); + virtual bool DisassociateModels(SyncError* error); virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes); virtual void AbortAssociation(); virtual bool CryptoReadyIfNecessary(); 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 51446bb..c340190 100644 --- a/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc @@ -124,7 +124,7 @@ class AutofillDataTypeControllerTest : public testing::Test { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); EXPECT_CALL(service_, ActivateDataType(_, _)); EXPECT_CALL(*change_processor_, IsRunning()).WillRepeatedly(Return(true)); @@ -132,7 +132,7 @@ class AutofillDataTypeControllerTest : public testing::Test { void SetStopExpectations() { EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } void WaitForEmptyDBMessageLoop() { @@ -259,7 +259,7 @@ TEST_F(AutofillDataTypeControllerTest, AbortWhileAssociatingActivated) { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); EXPECT_CALL(*change_processor_, IsRunning()).WillRepeatedly(Return(true)); diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc index 435cb1c..73cff83 100644 --- a/chrome/browser/sync/glue/autofill_model_associator.cc +++ b/chrome/browser/sync/glue/autofill_model_associator.cc @@ -14,6 +14,7 @@ #include "base/utf_string_conversions.h" #include "chrome/browser/autofill/autofill_profile.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/autofill_change_processor.h" #include "chrome/browser/sync/glue/autofill_profile_model_associator.h" @@ -136,7 +137,7 @@ bool AutofillModelAssociator::LoadAutofillData( return true; } -bool AutofillModelAssociator::AssociateModels() { +bool AutofillModelAssociator::AssociateModels(SyncError* error) { VLOG(1) << "Associating Autofill Models"; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); { @@ -149,7 +150,9 @@ bool AutofillModelAssociator::AssociateModels() { ScopedVector<AutofillProfile> profiles; if (!LoadAutofillData(&entries, &profiles.get())) { - LOG(ERROR) << "Could not get the autofill data from WebDatabase."; + error->Reset(FROM_HERE, + "Could not get the autofill data from WebDatabase.", + model_type()); return false; } @@ -159,13 +162,16 @@ bool AutofillModelAssociator::AssociateModels() { sync_api::ReadNode autofill_root(&trans); if (!autofill_root.InitByTagLookup(kAutofillTag)) { - LOG(ERROR) << "Server did not create the top-level autofill node. We " - << "might be running against an out-of-date server."; + error->Reset(FROM_HERE, + "Server did not create the top-level autofill node. We " + "might be running against an out-of-date server.", + model_type()); return false; } if (!TraverseAndAssociateChromeAutofillEntries(&trans, autofill_root, entries, &bundle.current_entries, &bundle.new_entries)) { + error->Reset(FROM_HERE, "Failed to associate entries.", model_type()); return false; } @@ -174,6 +180,7 @@ bool AutofillModelAssociator::AssociateModels() { autofill_root, &bundle, profiles.get())) { + error->Reset(FROM_HERE, "Failed to associate sync nodes.", model_type()); return false; } } @@ -184,7 +191,7 @@ bool AutofillModelAssociator::AssociateModels() { // to worry about the sync model getting out of sync, because changes are // propagated to the ChangeProcessor on this thread. if (!SaveChangesToWebData(bundle)) { - LOG(ERROR) << "Failed to update autofill entries."; + error->Reset(FROM_HERE, "Failed to update webdata.", model_type()); return false; } @@ -345,7 +352,7 @@ void AutofillModelAssociator::AddNativeProfileIfNeeded( } } -bool AutofillModelAssociator::DisassociateModels() { +bool AutofillModelAssociator::DisassociateModels(SyncError* error) { id_map_.clear(); id_map_inverse_.clear(); return true; diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h index 3eb9186..baffaa8 100644 --- a/chrome/browser/sync/glue/autofill_model_associator.h +++ b/chrome/browser/sync/glue/autofill_model_associator.h @@ -54,10 +54,10 @@ class AutofillModelAssociator // PerDataTypeAssociatorInterface implementation. // // Iterates through the sync model looking for matched pairs of items. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); // Clears all associations. - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // The has_nodes out param is true if the sync model has nodes other // than the permanent tagged nodes. diff --git a/chrome/browser/sync/glue/autofill_profile_model_associator.cc b/chrome/browser/sync/glue/autofill_profile_model_associator.cc index ca01b08..9c2692b 100644 --- a/chrome/browser/sync/glue/autofill_profile_model_associator.cc +++ b/chrome/browser/sync/glue/autofill_profile_model_associator.cc @@ -6,6 +6,7 @@ #include "base/tracked.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/autofill_profile_change_processor.h" #include "chrome/browser/sync/glue/do_optimistic_refresh_task.h" #include "chrome/browser/sync/profile_sync_service.h" @@ -136,7 +137,7 @@ bool AutofillProfileModelAssociator::LoadAutofillData( return true; } -bool AutofillProfileModelAssociator::AssociateModels() { +bool AutofillProfileModelAssociator::AssociateModels(SyncError* error) { VLOG(1) << "Associating Autofill Models"; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); { @@ -147,7 +148,9 @@ bool AutofillProfileModelAssociator::AssociateModels() { ScopedVector<AutofillProfile> profiles; if (!LoadAutofillData(&profiles.get())) { - LOG(ERROR) << "Could not get the autofill data from WebDatabase."; + error->Reset(FROM_HERE, + "Could not get the autofill data from WebDatabase.", + model_type()); return false; } @@ -162,8 +165,10 @@ bool AutofillProfileModelAssociator::AssociateModels() { sync_api::ReadNode autofill_root(&trans); if (!autofill_root.InitByTagLookup(kAutofillProfileTag)) { - LOG(ERROR) << "Server did not create the top-level autofill node. We " - << "might be running against an out-of-date server."; + error->Reset(FROM_HERE, + "Server did not create the top-level autofill node. We " + "might be running against an out-of-date server.", + model_type()); return false; } @@ -173,12 +178,15 @@ bool AutofillProfileModelAssociator::AssociateModels() { &bundle.new_profiles, &bundle.profiles_to_delete) || !TraverseAndAssociateAllSyncNodes(&trans, autofill_root, &bundle)) { + error->Reset(FROM_HERE, + "Failed to associate all sync nodes.", + model_type()); return false; } } if (!SaveChangesToWebData(bundle)) { - LOG(ERROR) << "Failed to update autofill entries."; + error->Reset(FROM_HERE, "Failed to update webdata.", model_type()); return false; } @@ -187,7 +195,7 @@ bool AutofillProfileModelAssociator::AssociateModels() { return true; } -bool AutofillProfileModelAssociator::DisassociateModels() { +bool AutofillProfileModelAssociator::DisassociateModels(SyncError* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); id_map_.clear(); id_map_inverse_.clear(); diff --git a/chrome/browser/sync/glue/autofill_profile_model_associator.h b/chrome/browser/sync/glue/autofill_profile_model_associator.h index c8b99ac..4b0b1a2 100644 --- a/chrome/browser/sync/glue/autofill_profile_model_associator.h +++ b/chrome/browser/sync/glue/autofill_profile_model_associator.h @@ -59,10 +59,10 @@ class AutofillProfileModelAssociator // PerDataTypeAssociatorInterface implementation. // // Iterates through the sync model looking for matched pairs of items. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); // Clears all associations. - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // TODO(lipalani) Bug 64111. // The has_nodes out param is true if the sync model has nodes other 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 d385e76c7..0c4bce0 100644 --- a/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc @@ -74,14 +74,14 @@ class BookmarkDataTypeControllerTest : public testing::Test { EXPECT_CALL(*profile_sync_factory_, CreateBookmarkSyncComponents(_, _)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); EXPECT_CALL(service_, ActivateDataType(_, _)); } void SetStopExpectations() { EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } MessageLoopForUI message_loop_; @@ -160,8 +160,9 @@ TEST_F(BookmarkDataTypeControllerTest, StartAssociationFailed) { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillRepeatedly(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillRepeatedly(DoAll(browser_sync::SetSyncError(syncable::BOOKMARKS), + Return(false))); EXPECT_CALL(start_callback_, Run(DataTypeController::ASSOCIATION_FAILED, _)); bookmark_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run)); diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc index ec57895..6e0ba5c 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.cc +++ b/chrome/browser/sync/glue/bookmark_model_associator.cc @@ -14,6 +14,7 @@ #include "base/utf_string_conversions.h" #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/syncable/nigori_util.h" @@ -44,6 +45,9 @@ namespace browser_sync { static const char kBookmarkBarTag[] = "bookmark_bar"; static const char kSyncedBookmarksTag[] = "synced_bookmarks"; static const char kOtherBookmarksTag[] = "other_bookmarks"; +static const char kServerError[] = + "Server did not create top-level nodes. Possibly we are running against " + "an out-of-date server?"; // Bookmark comparer for map of bookmark nodes. class BookmarkComparer { @@ -178,7 +182,7 @@ BookmarkModelAssociator::~BookmarkModelAssociator() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } -bool BookmarkModelAssociator::DisassociateModels() { +bool BookmarkModelAssociator::DisassociateModels(SyncError* error) { id_map_.clear(); id_map_inverse_.clear(); dirty_associations_sync_ids_.clear(); @@ -315,20 +319,20 @@ bool BookmarkModelAssociator::GetSyncIdForTaggedNode(const std::string& tag, return true; } -bool BookmarkModelAssociator::AssociateModels() { +bool BookmarkModelAssociator::AssociateModels(SyncError* error) { // Try to load model associations from persisted associations first. If that // succeeds, we don't need to run the complex model matching algorithm. if (LoadAssociations()) return true; - DisassociateModels(); + DisassociateModels(error); // We couldn't load model associations from persisted associations. So build // them. - return BuildAssociations(); + return BuildAssociations(error); } -bool BookmarkModelAssociator::BuildAssociations() { +bool BookmarkModelAssociator::BuildAssociations(SyncError* error) { // Algorithm description: // Match up the roots and recursively do the following: // * For each sync node for the current sync parent node, find the best @@ -352,14 +356,12 @@ bool BookmarkModelAssociator::BuildAssociations() { // and Other Bookmarks. if (!AssociateTaggedPermanentNode(bookmark_model_->other_node(), kOtherBookmarksTag)) { - LOG(ERROR) << "Server did not create top-level nodes. Possibly we " - << "are running against an out-of-date server?"; + error->Reset(FROM_HERE, kServerError, model_type()); return false; } if (!AssociateTaggedPermanentNode(bookmark_model_->bookmark_bar_node(), kBookmarkBarTag)) { - LOG(ERROR) << "Server did not create top-level nodes. Possibly we " - << "are running against an out-of-date server?"; + error->Reset(FROM_HERE, kServerError, model_type()); return false; } if (!AssociateTaggedPermanentNode(bookmark_model_->synced_node(), @@ -368,8 +370,7 @@ bool BookmarkModelAssociator::BuildAssociations() { // server if the command line flag is set. CommandLine::ForCurrentProcess()->HasSwitch( switches::kEnableSyncedBookmarksFolder)) { - LOG(ERROR) << "Server did not create top-level synced nodes. Possibly " - << "we are running against an out-of-date server?"; + error->Reset(FROM_HERE, kServerError, model_type()); return false; } int64 bookmark_bar_sync_id = GetSyncIdFromChromeId( @@ -399,6 +400,7 @@ bool BookmarkModelAssociator::BuildAssociations() { sync_api::ReadNode sync_parent(&trans); if (!sync_parent.InitByIdLookup(sync_parent_id)) { + error->Reset(FROM_HERE, "Failed to lookup node.", model_type()); return false; } // Only folder nodes are pushed on to the stack. @@ -414,6 +416,7 @@ bool BookmarkModelAssociator::BuildAssociations() { while (sync_child_id != sync_api::kInvalidId) { sync_api::WriteNode sync_child_node(&trans); if (!sync_child_node.InitByIdLookup(sync_child_id)) { + error->Reset(FROM_HERE, "Failed to lookup node.", model_type()); return false; } diff --git a/chrome/browser/sync/glue/bookmark_model_associator.h b/chrome/browser/sync/glue/bookmark_model_associator.h index ac0da1c..27471c2 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.h +++ b/chrome/browser/sync/glue/bookmark_model_associator.h @@ -52,9 +52,9 @@ class BookmarkModelAssociator // node. After successful completion, the models should be identical and // corresponding. Returns true on success. On failure of this step, we // should abort the sync operation and report an error to the user. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // The has_nodes out param is true if the sync model has nodes other // than the permanent tagged nodes. @@ -111,7 +111,7 @@ class BookmarkModelAssociator // Matches up the bookmark model and the sync model to build model // associations. - bool BuildAssociations(); + bool BuildAssociations(SyncError* error); // Associate a top-level node of the bookmark model with a permanent node in // the sync domain. Such permanent nodes are identified by a tag that is diff --git a/chrome/browser/sync/glue/data_type_manager.cc b/chrome/browser/sync/glue/data_type_manager.cc index 45b836e..d71e0a2 100644 --- a/chrome/browser/sync/glue/data_type_manager.cc +++ b/chrome/browser/sync/glue/data_type_manager.cc @@ -6,12 +6,45 @@ namespace browser_sync { -DataTypeManager::ConfigureResultWithErrorLocation:: - ~ConfigureResultWithErrorLocation() {} +DataTypeManager::ConfigureResult::ConfigureResult() {} -DataTypeManager::ConfigureResultWithErrorLocation:: - ConfigureResultWithErrorLocation() - : result(OK) { +DataTypeManager::ConfigureResult::ConfigureResult(ConfigureStatus status, + TypeSet requested_types) + : status(status), + requested_types(requested_types) { + DCHECK_EQ(OK, status); +} + +DataTypeManager::ConfigureResult::ConfigureResult( + ConfigureStatus status, + TypeSet requested_types, + TypeSet failed_types, + const tracked_objects::Location& location) + : status(status), + requested_types(requested_types), + failed_types(failed_types), + location(location) { + DCHECK_NE(OK, status); +} + +DataTypeManager::ConfigureResult::~ConfigureResult() { +} + +// Static. +std::string DataTypeManager::ConfigureStatusToString(ConfigureStatus status) { + switch (status) { + case OK: + return "Ok"; + case ASSOCIATION_FAILED: + return "Association Failed"; + case ABORTED: + return "Aborted"; + case UNRECOVERABLE_ERROR: + return "Unrecoverable Error"; + default: + NOTREACHED(); + return std::string(); + } } } // namespace browser_sync diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h index eef71a1..b346eb8 100644 --- a/chrome/browser/sync/glue/data_type_manager.h +++ b/chrome/browser/sync/glue/data_type_manager.h @@ -7,6 +7,7 @@ #pragma once #include <set> +#include <string> #include "base/memory/scoped_ptr.h" #include "base/task.h" @@ -36,7 +37,7 @@ class DataTypeManager { // Update NotifyDone() in data_type_manager_impl.cc if you update // this. - enum ConfigureResult { + enum ConfigureStatus { OK, // Configuration finished without error. ASSOCIATION_FAILED, // An error occurred during model association. ABORTED, // Start was aborted by calling Stop() before @@ -47,35 +48,27 @@ class DataTypeManager { typedef std::set<syncable::ModelType> TypeSet; - // In case of an error the location is filled with the location the - // error originated from. In case of a success the error location value - // is to be not used. - // TODO(tim): We should rename this / ConfigureResult to something more - // flexible like SyncConfigureDoneDetails. - struct ConfigureResultWithErrorLocation { - ConfigureResult result; + // Note: location and failed_types are only filled when status is not OK. + struct ConfigureResult { + ConfigureResult(); + ConfigureResult(ConfigureStatus status, + TypeSet requested_types); + ConfigureResult(ConfigureStatus status, + TypeSet requested_types, + TypeSet failed_types, + const tracked_objects::Location& location); + ~ConfigureResult(); + ConfigureStatus status; TypeSet requested_types; - scoped_ptr<tracked_objects::Location> location; - - ConfigureResultWithErrorLocation(); - ConfigureResultWithErrorLocation(const ConfigureResult& result, - const tracked_objects::Location& location, - const TypeSet& requested_types) - : result(result), - requested_types(requested_types) { - this->location.reset(new tracked_objects::Location( - location.function_name(), - location.file_name(), - location.line_number(), - location.program_counter())); - } - - ~ConfigureResultWithErrorLocation(); + TypeSet failed_types; + tracked_objects::Location location; }; - virtual ~DataTypeManager() {} + // Convert a ConfigureStatus to string for debug purposes. + static std::string ConfigureStatusToString(ConfigureStatus status); + // Begins asynchronous configuration of data types. Any currently // running data types that are not in the desired_types set will be // stopped. Any stopped data types that are in the desired_types diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc index ebafd82..5a7cf23a 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -130,7 +130,8 @@ void DataTypeManagerImpl::ConfigureImpl(const TypeSet& desired_types, // If we're already configured and the types haven't changed, we can exit // out early. NotifyStart(); - NotifyDone(OK, FROM_HERE); + ConfigureResult result(OK, last_requested_types_); + NotifyDone(result); return; } @@ -221,7 +222,7 @@ void DataTypeManagerImpl::DownloadReady(bool success) { DCHECK(state_ == DOWNLOAD_PENDING); if (!success) { - NotifyDone(UNRECOVERABLE_ERROR, FROM_HERE); + Abort(UNRECOVERABLE_ERROR, FROM_HERE, needs_start_[0]->type()); return; } @@ -276,7 +277,8 @@ void DataTypeManagerImpl::StartNextType() { // If no more data types need starting, we're done. state_ = CONFIGURED; - NotifyDone(OK, FROM_HERE); + ConfigureResult result(OK, last_requested_types_); + NotifyDone(result); } void DataTypeManagerImpl::TypeStartCallback( @@ -291,7 +293,7 @@ void DataTypeManagerImpl::TypeStartCallback( // DataTypeManager::Stop() was called while the current data type // was starting. Now that it has finished starting, we can finish // stopping the DataTypeManager. This is considered an ABORT. - FinishStopAndNotify(ABORTED, FROM_HERE); + Abort(ABORTED, location, needs_start_[0]->type()); return; } else if (state_ == STOPPED) { // If our state_ is STOPPED, we have already stopped all of the data @@ -306,8 +308,6 @@ void DataTypeManagerImpl::TypeStartCallback( DCHECK_EQ(needs_start_[0], started_dtc); needs_start_.erase(needs_start_.begin()); - if (result == DataTypeController::NEEDS_CRYPTO) { - } // 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. @@ -322,22 +322,29 @@ void DataTypeManagerImpl::TypeStartCallback( // managed to start up to this point and pass the result to the // callback. VLOG(0) << "Failed " << started_dtc->name(); - ConfigureResult configure_result = DataTypeManager::ABORTED; + ConfigureStatus configure_status = DataTypeManager::ABORTED; switch (result) { case DataTypeController::ABORTED: - configure_result = DataTypeManager::ABORTED; + configure_status = DataTypeManager::ABORTED; break; case DataTypeController::ASSOCIATION_FAILED: - configure_result = DataTypeManager::ASSOCIATION_FAILED; + configure_status = DataTypeManager::ASSOCIATION_FAILED; break; case DataTypeController::UNRECOVERABLE_ERROR: - configure_result = DataTypeManager::UNRECOVERABLE_ERROR; + configure_status = DataTypeManager::UNRECOVERABLE_ERROR; break; default: NOTREACHED(); break; } - FinishStopAndNotify(configure_result, location); + + // TODO(sync): We currently only specify the last attempted type as the failed + // type. At some point we should allow a datatype to fail without preventing + // other datatypes from continuing. In that case we'll have to support + // multiple types failing. + Abort(configure_status, + location, + started_dtc->type()); } void DataTypeManagerImpl::Stop() { @@ -368,7 +375,10 @@ void DataTypeManagerImpl::Stop() { // If Stop() is called while waiting for download, cancel all // outstanding tasks. weak_ptr_factory_.InvalidateWeakPtrs(); - FinishStopAndNotify(ABORTED, FROM_HERE); + syncable::ModelType type = syncable::UNSPECIFIED; + if (needs_start_.size() > 0) + type = needs_start_[0]->type(); + Abort(ABORTED, FROM_HERE, type); return; } @@ -390,10 +400,20 @@ void DataTypeManagerImpl::FinishStop() { state_ = STOPPED; } -void DataTypeManagerImpl::FinishStopAndNotify(ConfigureResult result, - const tracked_objects::Location& location) { +void DataTypeManagerImpl::Abort( + ConfigureStatus status, + const tracked_objects::Location& location, + syncable::ModelType last_attempted_type) { + DCHECK_NE(OK, status); FinishStop(); - NotifyDone(result, location); + TypeSet attempted_types; + if (syncable::IsRealDataType(last_attempted_type)) + attempted_types.insert(last_attempted_type); + ConfigureResult result(status, + last_requested_types_, + attempted_types, + location); + NotifyDone(result); } void DataTypeManagerImpl::NotifyStart() { @@ -403,14 +423,12 @@ void DataTypeManagerImpl::NotifyStart() { NotificationService::NoDetails()); } -void DataTypeManagerImpl::NotifyDone(ConfigureResult result, - const tracked_objects::Location& location) { - ConfigureResultWithErrorLocation result_with_location(result, location, - last_requested_types_); +void DataTypeManagerImpl::NotifyDone(const ConfigureResult& result) { AddToConfigureTime(); + VLOG(1) << "Total time spent configuring: " << configure_time_delta_.InSecondsF() << "s"; - switch (result) { + switch (result.status) { case DataTypeManager::OK: VLOG(1) << "NotifyDone called with result: OK"; UMA_HISTOGRAM_TIMES("Sync.ConfigureTime.OK", @@ -438,7 +456,7 @@ void DataTypeManagerImpl::NotifyDone(ConfigureResult result, NotificationService::current()->Notify( chrome::NOTIFICATION_SYNC_CONFIGURE_DONE, Source<DataTypeManager>(this), - Details<ConfigureResultWithErrorLocation>(&result_with_location)); + Details<const ConfigureResult>(&result)); } const DataTypeController::TypeMap& DataTypeManagerImpl::controllers() { diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h index 8e8076f..65db005 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.h +++ b/chrome/browser/sync/glue/data_type_manager_impl.h @@ -50,8 +50,9 @@ class DataTypeManagerImpl : public DataTypeManager { // Stops all data types. void FinishStop(); - void FinishStopAndNotify(ConfigureResult result, - const tracked_objects::Location& location); + void Abort(ConfigureStatus status, + const tracked_objects::Location& location, + syncable::ModelType last_attempted_type); // Returns true if any last_requested_types_ currently needs to start model // association. If non-null, fills |needs_start| with all such controllers. @@ -61,8 +62,7 @@ class DataTypeManagerImpl : public DataTypeManager { void Restart(sync_api::ConfigureReason reason, bool enable_nigori); void DownloadReady(bool success); void NotifyStart(); - void NotifyDone(ConfigureResult result, - const tracked_objects::Location& location); + void NotifyDone(const ConfigureResult& result); void SetBlockedAndNotify(); // Add to |configure_time_delta_| the time since we last called 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 24dbe9c..e21e680 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -53,12 +53,12 @@ ACTION_P2(InvokeCallbackPointer, callback, argument) { delete callback; } -DataTypeManager::ConfigureResult GetResult( +DataTypeManager::ConfigureStatus GetStatus( const NotificationDetails& details) { - DataTypeManager::ConfigureResultWithErrorLocation* result_with_location = - Details<DataTypeManager::ConfigureResultWithErrorLocation>( + const DataTypeManager::ConfigureResult* result = + Details<const DataTypeManager::ConfigureResult>( details).ptr(); - return result_with_location->result; + return result->status; } class DataTypeManagerImplTest : public testing::Test { @@ -149,11 +149,11 @@ class DataTypeManagerImplTest : public testing::Test { } - void SetConfigureDoneExpectation(DataTypeManager::ConfigureResult result) { + void SetConfigureDoneExpectation(DataTypeManager::ConfigureStatus status) { EXPECT_CALL( observer_, Observe(int(chrome::NOTIFICATION_SYNC_CONFIGURE_DONE), _, - ::testing::ResultOf(&GetResult, result))); + ::testing::ResultOf(&GetStatus, status))); } MessageLoopForUI message_loop_; diff --git a/chrome/browser/sync/glue/data_type_manager_mock.cc b/chrome/browser/sync/glue/data_type_manager_mock.cc index a530d51..fa088ff 100644 --- a/chrome/browser/sync/glue/data_type_manager_mock.cc +++ b/chrome/browser/sync/glue/data_type_manager_mock.cc @@ -9,7 +9,7 @@ namespace browser_sync { DataTypeManagerMock::DataTypeManagerMock() - : result_(OK, FROM_HERE, syncable::ModelTypeSet()) { + : result_(OK, syncable::ModelTypeSet()) { // By default, calling Configure will send a SYNC_CONFIGURE_START // and SYNC_CONFIGURE_DONE notification with a DataTypeManager::OK diff --git a/chrome/browser/sync/glue/data_type_manager_mock.h b/chrome/browser/sync/glue/data_type_manager_mock.h index be45d9f..4e110ed 100644 --- a/chrome/browser/sync/glue/data_type_manager_mock.h +++ b/chrome/browser/sync/glue/data_type_manager_mock.h @@ -17,7 +17,7 @@ ACTION_P3(NotifyFromDataTypeManagerWithResult, dtm, type, result) { NotificationService::current()->Notify( type, Source<browser_sync::DataTypeManager>(dtm), - Details<browser_sync::DataTypeManager::ConfigureResultWithErrorLocation>( + Details<const browser_sync::DataTypeManager::ConfigureResult>( result)); } @@ -42,7 +42,7 @@ class DataTypeManagerMock : public DataTypeManager { MOCK_METHOD0(state, State()); private: - browser_sync::DataTypeManager::ConfigureResultWithErrorLocation result_; + browser_sync::DataTypeManager::ConfigureResult result_; }; } // namespace browser_sync diff --git a/chrome/browser/sync/glue/extension_data_type_controller_unittest.cc b/chrome/browser/sync/glue/extension_data_type_controller_unittest.cc index 4a6953c..5c4fb18 100644 --- a/chrome/browser/sync/glue/extension_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/extension_data_type_controller_unittest.cc @@ -59,7 +59,7 @@ class ExtensionDataTypeControllerTest : public testing::Test { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); } @@ -69,7 +69,7 @@ class ExtensionDataTypeControllerTest : public testing::Test { void SetStopExpectations() { EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } MessageLoopForUI message_loop_; @@ -117,8 +117,9 @@ TEST_F(ExtensionDataTypeControllerTest, StartOk) { TEST_F(ExtensionDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); SetAssociateExpectations(); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillRepeatedly(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillRepeatedly(DoAll(browser_sync::SetSyncError(syncable::EXTENSIONS), + Return(false))); EXPECT_CALL(start_callback_, Run(DataTypeController::ASSOCIATION_FAILED, _)); extension_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run)); diff --git a/chrome/browser/sync/glue/extension_model_associator.cc b/chrome/browser/sync/glue/extension_model_associator.cc index 46594b6..fbe0e0d 100644 --- a/chrome/browser/sync/glue/extension_model_associator.cc +++ b/chrome/browser/sync/glue/extension_model_associator.cc @@ -32,22 +32,24 @@ ExtensionModelAssociator::~ExtensionModelAssociator() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } -bool ExtensionModelAssociator::AssociateModels() { +bool ExtensionModelAssociator::AssociateModels(SyncError* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ExtensionDataMap extension_data_map; if (!SlurpExtensionData( traits_, *extension_service_, user_share_, &extension_data_map)) { + error->Reset(FROM_HERE, "Failed to get extension data.", model_type()); return false; } if (!FlushExtensionData( traits_, extension_data_map, extension_service_, user_share_)) { + error->Reset(FROM_HERE, "Failed to flush extension data.", model_type()); return false; } return true; } -bool ExtensionModelAssociator::DisassociateModels() { +bool ExtensionModelAssociator::DisassociateModels(SyncError* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // Nothing to do. return true; diff --git a/chrome/browser/sync/glue/extension_model_associator.h b/chrome/browser/sync/glue/extension_model_associator.h index b71a281..48f51b2 100644 --- a/chrome/browser/sync/glue/extension_model_associator.h +++ b/chrome/browser/sync/glue/extension_model_associator.h @@ -7,6 +7,7 @@ #pragma once #include "base/basictypes.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/extension_sync_traits.h" #include "chrome/browser/sync/glue/model_associator.h" #include "chrome/browser/sync/syncable/model_type.h" @@ -32,8 +33,8 @@ class ExtensionModelAssociator : public AssociatorInterface { static syncable::ModelType model_type() { return syncable::EXTENSIONS; } // AssociatorInterface implementation. - virtual bool AssociateModels(); - virtual bool DisassociateModels(); + virtual bool AssociateModels(SyncError* error); + virtual bool DisassociateModels(SyncError* error); virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes); virtual void AbortAssociation(); virtual bool CryptoReadyIfNecessary(); diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.cc b/chrome/browser/sync/glue/frontend_data_type_controller.cc index 9a22cb8..6d99787 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/frontend_data_type_controller.cc @@ -6,6 +6,7 @@ #include "base/logging.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/model_associator.h" #include "chrome/browser/sync/profile_sync_factory.h" @@ -92,10 +93,11 @@ bool FrontendDataTypeController::Associate() { } base::TimeTicks start_time = base::TimeTicks::Now(); - bool merge_success = model_associator()->AssociateModels(); + SyncError error; + bool merge_success = model_associator()->AssociateModels(&error); RecordAssociationTime(base::TimeTicks::Now() - start_time); if (!merge_success) { - StartFailed(ASSOCIATION_FAILED, FROM_HERE); + StartFailed(ASSOCIATION_FAILED, error.location()); return false; } @@ -105,7 +107,8 @@ bool FrontendDataTypeController::Associate() { return true; } -void FrontendDataTypeController::StartFailed(StartResult result, +void FrontendDataTypeController::StartFailed( + StartResult result, const tracked_objects::Location& location) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); CleanUpState(); @@ -118,10 +121,12 @@ void FrontendDataTypeController::StartFailed(StartResult result, // invoking the callback will trigger a call to STOP(), which will get // confused by the non-NULL start_callback_. scoped_ptr<StartCallback> callback(start_callback_.release()); + // TODO(zea): Send the full SyncError on failure and handle it higher up. callback->Run(result, location); } -void FrontendDataTypeController::FinishStart(StartResult result, +void FrontendDataTypeController::FinishStart( + StartResult result, const tracked_objects::Location& location) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -149,8 +154,10 @@ void FrontendDataTypeController::Stop() { if (change_processor_.get()) sync_service_->DeactivateDataType(this, change_processor_.get()); - if (model_associator()) - model_associator()->DisassociateModels(); + if (model_associator()) { + SyncError error; + model_associator()->DisassociateModels(&error); + } set_model_associator(NULL); change_processor_.reset(); diff --git a/chrome/browser/sync/glue/frontend_data_type_controller.h b/chrome/browser/sync/glue/frontend_data_type_controller.h index 86c3e60..a1a7b01 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller.h +++ b/chrome/browser/sync/glue/frontend_data_type_controller.h @@ -15,6 +15,7 @@ class Profile; class ProfileSyncService; class ProfileSyncFactory; +class SyncError; namespace base { class TimeDelta; } namespace browser_sync { @@ -84,11 +85,11 @@ class FrontendDataTypeController : public DataTypeController { // Cleans up state and calls callback when start fails. virtual void StartFailed(StartResult result, - const tracked_objects::Location& from_here); + const tracked_objects::Location& location); // Helper method to run the stashed start callback with a given result. virtual void FinishStart(StartResult result, - const tracked_objects::Location& from_here); + const tracked_objects::Location& location); // DataType specific histogram methods. Because histograms use static's, the // specific datatype controllers must implement this themselves. 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 ee3f42f..2f90f9f 100644 --- a/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc @@ -111,7 +111,7 @@ class FrontendDataTypeControllerTest : public testing::Test { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); } @@ -124,7 +124,7 @@ class FrontendDataTypeControllerTest : public testing::Test { void SetStopExpectations() { EXPECT_CALL(*dtc_mock_, CleanUpState()); EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } void SetStartFailExpectations(DataTypeController::StartResult result) { @@ -160,7 +160,7 @@ TEST_F(FrontendDataTypeControllerTest, StartFirstRun) { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(false), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetActivateExpectations(DataTypeController::OK_FIRST_RUN); @@ -185,8 +185,9 @@ TEST_F(FrontendDataTypeControllerTest, StartAssociationFailed) { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillOnce(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillOnce(DoAll(browser_sync::SetSyncError(syncable::PREFERENCES), + Return(false))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED); // Set up association to fail with an association failed error. diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc index 6c85de9..d24d477 100644 --- a/chrome/browser/sync/glue/generic_change_processor.cc +++ b/chrome/browser/sync/glue/generic_change_processor.cc @@ -7,6 +7,7 @@ #include "base/tracked.h" #include "chrome/browser/sync/api/syncable_service.h" #include "chrome/browser/sync/api/sync_change.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/syncable/nigori_util.h" @@ -69,37 +70,45 @@ void GenericChangeProcessor::CommitChangesFromSyncModel() { return; if (syncer_changes_.empty()) return; - local_service_->ProcessSyncChanges(FROM_HERE, syncer_changes_); + SyncError error = local_service_->ProcessSyncChanges(FROM_HERE, + syncer_changes_); syncer_changes_.clear(); + if (error.IsSet()) { + error_handler()->OnUnrecoverableError(error.location(), error.message()); + } } -bool GenericChangeProcessor::GetSyncDataForType( +SyncError GenericChangeProcessor::GetSyncDataForType( syncable::ModelType type, SyncDataList* current_sync_data) { std::string type_name = syncable::ModelTypeToString(type); sync_api::ReadTransaction trans(FROM_HERE, share_handle()); sync_api::ReadNode root(&trans); if (!root.InitByTagLookup(syncable::ModelTypeToRootTag(type))) { - LOG(ERROR) << "Server did not create the top-level " + type_name + " node." - << " We might be running against an out-of-date server."; - return false; + SyncError error(FROM_HERE, + "Server did not create the top-level " + type_name + + " node. We might be running against an out-of-date server.", + type); + return error; } int64 sync_child_id = root.GetFirstChildId(); while (sync_child_id != sync_api::kInvalidId) { sync_api::ReadNode sync_child_node(&trans); if (!sync_child_node.InitByIdLookup(sync_child_id)) { - LOG(ERROR) << "Failed to fetch child node for type " + type_name + "."; - return false; + SyncError error(FROM_HERE, + "Failed to fetch child node for type " + type_name + ".", + type); + return error; } current_sync_data->push_back(SyncData::CreateRemoteData( sync_child_node.GetEntitySpecifics())); sync_child_id = sync_child_node.GetSuccessorId(); } - return true; + return SyncError(); } -void GenericChangeProcessor::ProcessSyncChanges( +SyncError GenericChangeProcessor::ProcessSyncChanges( const tracked_objects::Location& from_here, const SyncChangeList& list_of_changes) { sync_api::WriteTransaction trans(from_here, share_handle()); @@ -109,17 +118,20 @@ void GenericChangeProcessor::ProcessSyncChanges( ++iter) { const SyncChange& change = *iter; DCHECK_NE(change.sync_data().GetDataType(), syncable::UNSPECIFIED); - std::string type_str = syncable::ModelTypeToString( - change.sync_data().GetDataType()); + syncable::ModelType type = change.sync_data().GetDataType(); + std::string type_str = syncable::ModelTypeToString(type); sync_api::WriteNode sync_node(&trans); if (change.change_type() == SyncChange::ACTION_DELETE) { if (change.sync_data().GetTag() == "" || !sync_node.InitByClientTagLookup(change.sync_data().GetDataType(), change.sync_data().GetTag())) { NOTREACHED(); - error_handler()->OnUnrecoverableError(FROM_HERE, - "Failed to delete " + type_str + " node."); - return; + SyncError error(FROM_HERE, + "Failed to delete " + type_str + " node.", + type); + error_handler()->OnUnrecoverableError(error.location(), + error.message()); + return error; } sync_node.Remove(); } else if (change.change_type() == SyncChange::ACTION_ADD) { @@ -129,16 +141,23 @@ void GenericChangeProcessor::ProcessSyncChanges( if (!root_node.InitByTagLookup( syncable::ModelTypeToRootTag(change.sync_data().GetDataType()))) { NOTREACHED(); - error_handler()->OnUnrecoverableError(FROM_HERE, - "Failed to look up root node for type " + type_str); - return; + SyncError error(FROM_HERE, + "Failed to look up root node for type " + type_str, + type); + error_handler()->OnUnrecoverableError(error.location(), + error.message()); + return error; } if (!sync_node.InitUniqueByCreation(change.sync_data().GetDataType(), root_node, change.sync_data().GetTag())) { - error_handler()->OnUnrecoverableError(FROM_HERE, - "Failed to create " + type_str + " node."); - return; + NOTREACHED(); + SyncError error(FROM_HERE, + "Failed to create " + type_str + " node.", + type); + error_handler()->OnUnrecoverableError(error.location(), + error.message()); + return error; } sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle())); sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics()); @@ -147,9 +166,12 @@ void GenericChangeProcessor::ProcessSyncChanges( !sync_node.InitByClientTagLookup(change.sync_data().GetDataType(), change.sync_data().GetTag())) { NOTREACHED(); - error_handler()->OnUnrecoverableError(FROM_HERE, - "Failed to update " + type_str + " node"); - return; + SyncError error(FROM_HERE, + "Failed to update " + type_str + " node.", + type); + error_handler()->OnUnrecoverableError(error.location(), + error.message()); + return error; } sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle())); sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics()); @@ -157,11 +179,15 @@ void GenericChangeProcessor::ProcessSyncChanges( // successor, parent, etc.). } else { NOTREACHED(); - error_handler()->OnUnrecoverableError(FROM_HERE, - "Received unset SyncChange in the change processor."); - return; + SyncError error(FROM_HERE, + "Received unset SyncChange in the change processor.", + type); + error_handler()->OnUnrecoverableError(error.location(), + error.message()); + return error; } } + return SyncError(); } bool GenericChangeProcessor::SyncModelHasUserCreatedNodes( diff --git a/chrome/browser/sync/glue/generic_change_processor.h b/chrome/browser/sync/glue/generic_change_processor.h index d3176e5..4864360 100644 --- a/chrome/browser/sync/glue/generic_change_processor.h +++ b/chrome/browser/sync/glue/generic_change_processor.h @@ -45,12 +45,13 @@ class GenericChangeProcessor : public ChangeProcessor, virtual void CommitChangesFromSyncModel() OVERRIDE; // SyncChangeProcessor implementation. - virtual void ProcessSyncChanges(const tracked_objects::Location& from_here, - const SyncChangeList& change_list) OVERRIDE; + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE; // Fills |current_sync_data| with all the syncer data for the specified type. - virtual bool GetSyncDataForType(syncable::ModelType type, - SyncDataList* current_sync_data); + virtual SyncError GetSyncDataForType(syncable::ModelType type, + SyncDataList* current_sync_data); // Generic versions of AssociatorInterface methods. Called by // SyncableServiceAdapter. diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h index eaa2a45..eb65aab 100644 --- a/chrome/browser/sync/glue/model_associator.h +++ b/chrome/browser/sync/glue/model_associator.h @@ -9,6 +9,8 @@ #include "base/basictypes.h" #include "chrome/browser/sync/syncable/model_type.h" +class SyncError; + namespace sync_api { class BaseNode; } @@ -27,10 +29,12 @@ class AssociatorInterface { // should be identical and corresponding. Returns true on // success. On failure of this step, we should abort the sync // operation and report an error to the user. - virtual bool AssociateModels() = 0; + // TODO(zea): return a SyncError instead of passing one in. + virtual bool AssociateModels(SyncError* error) = 0; // Clears all the associations between the chrome and sync models. - virtual bool DisassociateModels() = 0; + // TODO(zea): return a SyncError instead of passing one in. + virtual bool DisassociateModels(SyncError* error) = 0; // The has_nodes out parameter is set to true if the sync model has // nodes other than the permanent tagged nodes. The method may diff --git a/chrome/browser/sync/glue/model_associator_mock.h b/chrome/browser/sync/glue/model_associator_mock.h index 51cb7c4..3474eff 100644 --- a/chrome/browser/sync/glue/model_associator_mock.h +++ b/chrome/browser/sync/glue/model_associator_mock.h @@ -6,18 +6,24 @@ #define CHROME_BROWSER_SYNC_GLUE_MODEL_ASSOCIATOR_MOCK_H__ #pragma once +#include "base/tracked.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/model_associator.h" #include "testing/gmock/include/gmock/gmock.h" namespace browser_sync { +ACTION_P(SetSyncError, type) { + arg0->Reset(FROM_HERE, "test", type); +} + class ModelAssociatorMock : public AssociatorInterface { public: ModelAssociatorMock(); virtual ~ModelAssociatorMock(); - MOCK_METHOD0(AssociateModels, bool()); - MOCK_METHOD0(DisassociateModels, bool()); + MOCK_METHOD1(AssociateModels, bool(SyncError* error)); + MOCK_METHOD1(DisassociateModels, bool(SyncError* error)); MOCK_METHOD1(SyncModelHasUserCreatedNodes, bool(bool* has_nodes)); MOCK_METHOD0(AbortAssociation, void()); MOCK_METHOD0(CryptoReadyIfNecessary, bool()); 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 5200f5d..96d6843 100644 --- a/chrome/browser/sync/glue/non_frontend_data_type_controller.cc +++ b/chrome/browser/sync/glue/non_frontend_data_type_controller.cc @@ -6,6 +6,7 @@ #include "base/logging.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/model_associator.h" #include "chrome/browser/sync/profile_sync_factory.h" @@ -104,10 +105,11 @@ void NonFrontendDataTypeController::StartAssociation() { } base::TimeTicks start_time = base::TimeTicks::Now(); - bool merge_success = model_associator_->AssociateModels(); + SyncError error; + bool merge_success = model_associator_->AssociateModels(&error); RecordAssociationTime(base::TimeTicks::Now() - start_time); if (!merge_success) { - StartFailed(ASSOCIATION_FAILED, FROM_HERE); + StartFailed(ASSOCIATION_FAILED, error.location()); return; } @@ -115,11 +117,13 @@ void NonFrontendDataTypeController::StartAssociation() { StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK, RUNNING, FROM_HERE); } -void NonFrontendDataTypeController::StartFailed(StartResult result, +void NonFrontendDataTypeController::StartFailed( + StartResult result, const tracked_objects::Location& location) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); model_associator_.reset(); change_processor_.reset(); + // TODO(zea): Send the full SyncError on failure and handle it higher up. StartDone(result, NOT_RUNNING, location); } @@ -216,8 +220,10 @@ void NonFrontendDataTypeController::StopModels() { void NonFrontendDataTypeController::StopAssociation() { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (model_associator_.get()) - model_associator_->DisassociateModels(); + if (model_associator_.get()) { + SyncError error; + model_associator_->DisassociateModels(&error); + } model_associator_.reset(); change_processor_.reset(); datatype_stopped_.Signal(); @@ -237,7 +243,7 @@ void NonFrontendDataTypeController::OnUnrecoverableError( const std::string& message) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI)); RecordUnrecoverableError(from_here, message); - BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, NewRunnableMethod(this, + BrowserThread::PostTask(BrowserThread::UI, from_here, NewRunnableMethod(this, &NonFrontendDataTypeController::OnUnrecoverableErrorImpl, from_here, message)); } 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 eeda4c4..e1fc3dd 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 @@ -152,7 +152,7 @@ class NonFrontendDataTypeControllerTest : public testing::Test { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); } @@ -166,7 +166,7 @@ class NonFrontendDataTypeControllerTest : public testing::Test { EXPECT_CALL(*dtc_mock_, StopAssociationAsync()); EXPECT_CALL(*dtc_mock_, StopModels()); EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } void SetStartFailExpectations(DataTypeController::StartResult result) { @@ -222,7 +222,7 @@ TEST_F(NonFrontendDataTypeControllerTest, StartFirstRun) { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(false), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetActivateExpectations(DataTypeController::OK_FIRST_RUN); @@ -250,8 +250,9 @@ TEST_F(NonFrontendDataTypeControllerTest, StartAssociationFailed) { WillOnce(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillOnce(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillOnce(DoAll(browser_sync::SetSyncError(syncable::AUTOFILL), + Return(false))); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); SetStartFailExpectations(DataTypeController::ASSOCIATION_FAILED); // Set up association to fail with an association failed error. @@ -308,7 +309,7 @@ TEST_F(NonFrontendDataTypeControllerTest, AbortDuringAssociationInactive) { Return(true))); EXPECT_CALL(*model_associator_, AbortAssociation()).WillOnce( SignalEvent(&pause_db_thread)); - EXPECT_CALL(*model_associator_, AssociateModels()).WillOnce(Return(true)); + EXPECT_CALL(*model_associator_, AssociateModels(_)).WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); EXPECT_CALL(service_, ActivateDataType(_, _)); EXPECT_CALL(start_callback_, Run(DataTypeController::ABORTED,_)); @@ -337,7 +338,7 @@ TEST_F(NonFrontendDataTypeControllerTest, AbortDuringAssociationActivated) { Return(true))); EXPECT_CALL(*model_associator_, AbortAssociation()).WillOnce( SignalEvent(&pause_db_thread)); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillOnce(Return(true)); EXPECT_CALL(*dtc_mock_, RecordAssociationTime(_)); EXPECT_CALL(service_, ActivateDataType(_, _)).WillOnce(DoAll( diff --git a/chrome/browser/sync/glue/password_model_associator.cc b/chrome/browser/sync/glue/password_model_associator.cc index 1a1ca01..6d96186 100644 --- a/chrome/browser/sync/glue/password_model_associator.cc +++ b/chrome/browser/sync/glue/password_model_associator.cc @@ -10,6 +10,7 @@ #include "base/tracked.h" #include "base/utf_string_conversions.h" #include "chrome/browser/password_manager/password_store.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/protocol/password_specifics.pb.h" @@ -39,7 +40,7 @@ PasswordModelAssociator::PasswordModelAssociator( PasswordModelAssociator::~PasswordModelAssociator() {} -bool PasswordModelAssociator::AssociateModels() { +bool PasswordModelAssociator::AssociateModels(SyncError* error) { DCHECK(expected_loop_ == MessageLoop::current()); { base::AutoLock lock(abort_association_pending_lock_); @@ -53,7 +54,9 @@ bool PasswordModelAssociator::AssociateModels() { if (!password_store_->FillAutofillableLogins(&passwords) || !password_store_->FillBlacklistLogins(&passwords)) { STLDeleteElements(&passwords); - LOG(ERROR) << "Could not get the password entries."; + error->Reset(FROM_HERE, + "Could not get the password entries.", + model_type()); return false; } @@ -64,16 +67,20 @@ bool PasswordModelAssociator::AssociateModels() { sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); sync_api::ReadNode password_root(&trans); if (!password_root.InitByTagLookup(kPasswordTag)) { - LOG(ERROR) << "Server did not create the top-level password node. We " - << "might be running against an out-of-date server."; + error->Reset(FROM_HERE, + "Server did not create the top-level password node. We " + "might be running against an out-of-date server.", + model_type()); return false; } for (std::vector<webkit_glue::PasswordForm*>::iterator ix = passwords.begin(); ix != passwords.end(); ++ix) { - if (IsAbortPending()) + if (IsAbortPending()) { + error->Reset(FROM_HERE, "Abort pending", model_type()); return false; + } std::string tag = MakeTag(**ix); sync_api::ReadNode node(&trans); @@ -88,7 +95,9 @@ bool PasswordModelAssociator::AssociateModels() { sync_api::WriteNode write_node(&trans); if (!write_node.InitByClientTagLookup(syncable::PASSWORDS, tag)) { STLDeleteElements(&passwords); - LOG(ERROR) << "Failed to edit password sync node."; + error->Reset(FROM_HERE, + "Failed to edit password sync node.", + model_type()); return false; } WriteToSyncNode(new_password, &write_node); @@ -101,7 +110,9 @@ bool PasswordModelAssociator::AssociateModels() { if (!node.InitUniqueByCreation(syncable::PASSWORDS, password_root, tag)) { STLDeleteElements(&passwords); - LOG(ERROR) << "Failed to create password sync node."; + error->Reset(FROM_HERE, + "Failed to create password sync node.", + model_type()); return false; } @@ -119,7 +130,7 @@ bool PasswordModelAssociator::AssociateModels() { while (sync_child_id != sync_api::kInvalidId) { sync_api::ReadNode sync_child_node(&trans); if (!sync_child_node.InitByIdLookup(sync_child_id)) { - LOG(ERROR) << "Failed to fetch child node."; + error->Reset(FROM_HERE, "Failed to fetch child node.", model_type()); return false; } const sync_pb::PasswordSpecificsData& password = @@ -144,7 +155,7 @@ bool PasswordModelAssociator::AssociateModels() { // store, as it can post tasks to the UI thread which can itself be blocked // on our transaction, resulting in deadlock. (http://crbug.com/70658) if (!WriteToPasswordStore(&new_passwords, &updated_passwords, NULL)) { - LOG(ERROR) << "Failed to write passwords."; + error->Reset(FROM_HERE, "Failed to write passwords.", model_type()); return false; } @@ -169,7 +180,7 @@ bool PasswordModelAssociator::DeleteAllNodes( return true; } -bool PasswordModelAssociator::DisassociateModels() { +bool PasswordModelAssociator::DisassociateModels(SyncError* error) { id_map_.clear(); id_map_inverse_.clear(); return true; diff --git a/chrome/browser/sync/glue/password_model_associator.h b/chrome/browser/sync/glue/password_model_associator.h index 0b3bac5..d612c03 100644 --- a/chrome/browser/sync/glue/password_model_associator.h +++ b/chrome/browser/sync/glue/password_model_associator.h @@ -56,13 +56,13 @@ class PasswordModelAssociator // PerDataTypeAssociatorInterface implementation. // // Iterates through the sync model looking for matched pairs of items. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); // Delete all password nodes. bool DeleteAllNodes(sync_api::WriteTransaction* trans); // Clears all associations. - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // The has_nodes out param is true if the sync model has nodes other // than the permanent tagged nodes. diff --git a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc index ba436d9..3bca18b 100644 --- a/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/preference_data_type_controller_unittest.cc @@ -61,7 +61,7 @@ class PreferenceDataTypeControllerTest : public testing::Test { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); } @@ -71,7 +71,7 @@ class PreferenceDataTypeControllerTest : public testing::Test { void SetStopExpectations() { EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } MessageLoopForUI message_loop_; @@ -119,8 +119,9 @@ TEST_F(PreferenceDataTypeControllerTest, StartOk) { TEST_F(PreferenceDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); SetAssociateExpectations(); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillRepeatedly(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillRepeatedly(DoAll(browser_sync::SetSyncError(syncable::PREFERENCES), + Return(false))); EXPECT_CALL(start_callback_, Run(DataTypeController::ASSOCIATION_FAILED, _)); preference_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run)); diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc index a923081..5811049 100644 --- a/chrome/browser/sync/glue/session_model_associator.cc +++ b/chrome/browser/sync/glue/session_model_associator.cc @@ -14,6 +14,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sessions/restore_tab_helper.h" #include "chrome/browser/sessions/session_service_factory.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/glue/synced_window_delegate.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/syncable/syncable.h" @@ -407,7 +408,7 @@ void SessionModelAssociator::Disassociate(int64 sync_id) { // TODO(zea): we will need this once we support deleting foreign sessions. } -bool SessionModelAssociator::AssociateModels() { +bool SessionModelAssociator::AssociateModels(SyncError* error) { DCHECK(CalledOnValidThread()); // Ensure that we disassociated properly, otherwise memory might leak. @@ -423,7 +424,7 @@ bool SessionModelAssociator::AssociateModels() { sync_api::ReadNode root(&trans); if (!root.InitByTagLookup(kSessionsTag)) { - LOG(ERROR) << kNoSessionsFolderError; + error->Reset(FROM_HERE, kNoSessionsFolderError, model_type()); return false; } @@ -438,7 +439,9 @@ bool SessionModelAssociator::AssociateModels() { sync_api::WriteNode write_node(&trans); if (!write_node.InitUniqueByCreation(syncable::SESSIONS, root, current_machine_tag_)) { - LOG(ERROR) << "Failed to create sessions header sync node."; + error->Reset(FROM_HERE, + "Failed to create sessions header sync node.", + model_type()); return false; } write_node.SetTitle(UTF8ToWide(current_machine_tag_)); @@ -454,7 +457,7 @@ bool SessionModelAssociator::AssociateModels() { return true; } -bool SessionModelAssociator::DisassociateModels() { +bool SessionModelAssociator::DisassociateModels(SyncError* error) { DCHECK(CalledOnValidThread()); synced_session_tracker_.clear(); tab_map_.clear(); diff --git a/chrome/browser/sync/glue/session_model_associator.h b/chrome/browser/sync/glue/session_model_associator.h index 72fa1e4..486b49f 100644 --- a/chrome/browser/sync/glue/session_model_associator.h +++ b/chrome/browser/sync/glue/session_model_associator.h @@ -116,7 +116,7 @@ class SessionModelAssociator // with local client data. Processes/reuses any sync nodes owned by this // client and creates any further sync nodes needed to store local header and // tab info. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); // Initializes the given sync node from the given chrome node id. // Returns false if no sync node was found for the given chrome node id or @@ -126,7 +126,7 @@ class SessionModelAssociator // Clear local sync data buffers. Does not delete sync nodes to avoid // tombstones. TODO(zea): way to eventually delete orphaned nodes. - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // Returns the tag used to uniquely identify this machine's session in the // sync model. diff --git a/chrome/browser/sync/glue/syncable_service_adapter.cc b/chrome/browser/sync/glue/syncable_service_adapter.cc index 68dcfae..88b6ab1 100644 --- a/chrome/browser/sync/glue/syncable_service_adapter.cc +++ b/chrome/browser/sync/glue/syncable_service_adapter.cc @@ -30,18 +30,26 @@ SyncableServiceAdapter::~SyncableServiceAdapter() { } } -bool SyncableServiceAdapter::AssociateModels() { +bool SyncableServiceAdapter::AssociateModels(SyncError* error) { syncing_ = true; SyncDataList initial_sync_data; - if (!sync_processor_->GetSyncDataForType(type_, &initial_sync_data)) { + SyncError temp_error = + sync_processor_->GetSyncDataForType(type_, &initial_sync_data); + if (temp_error.IsSet()) { + *error = temp_error; return false; } - return service_->MergeDataAndStartSyncing(type_, - initial_sync_data, - sync_processor_); + temp_error = service_->MergeDataAndStartSyncing(type_, + initial_sync_data, + sync_processor_); + if (temp_error.IsSet()) { + *error = temp_error; + return false; + } + return true; } -bool SyncableServiceAdapter::DisassociateModels() { +bool SyncableServiceAdapter::DisassociateModels(SyncError* error) { service_->StopSyncing(type_); syncing_ = false; return true; diff --git a/chrome/browser/sync/glue/syncable_service_adapter.h b/chrome/browser/sync/glue/syncable_service_adapter.h index b19a867..7390f1c 100644 --- a/chrome/browser/sync/glue/syncable_service_adapter.h +++ b/chrome/browser/sync/glue/syncable_service_adapter.h @@ -29,8 +29,8 @@ class SyncableServiceAdapter : public AssociatorInterface { virtual ~SyncableServiceAdapter(); // AssociatorInterface implementation. - virtual bool AssociateModels() OVERRIDE; - virtual bool DisassociateModels() OVERRIDE; + virtual bool AssociateModels(SyncError* error) OVERRIDE; + virtual bool DisassociateModels(SyncError* error) OVERRIDE; virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes) OVERRIDE; virtual void AbortAssociation() OVERRIDE; virtual bool CryptoReadyIfNecessary() OVERRIDE; diff --git a/chrome/browser/sync/glue/theme_data_type_controller_unittest.cc b/chrome/browser/sync/glue/theme_data_type_controller_unittest.cc index 4d2a1ad..1c89cbb 100644 --- a/chrome/browser/sync/glue/theme_data_type_controller_unittest.cc +++ b/chrome/browser/sync/glue/theme_data_type_controller_unittest.cc @@ -58,7 +58,7 @@ class ThemeDataTypeControllerTest : public testing::Test { WillRepeatedly(Return(true)); EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)). WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true))); - EXPECT_CALL(*model_associator_, AssociateModels()). + EXPECT_CALL(*model_associator_, AssociateModels(_)). WillRepeatedly(Return(true)); } @@ -68,7 +68,7 @@ class ThemeDataTypeControllerTest : public testing::Test { void SetStopExpectations() { EXPECT_CALL(service_, DeactivateDataType(_, _)); - EXPECT_CALL(*model_associator_, DisassociateModels()); + EXPECT_CALL(*model_associator_, DisassociateModels(_)); } MessageLoopForUI message_loop_; @@ -116,8 +116,9 @@ TEST_F(ThemeDataTypeControllerTest, StartOk) { TEST_F(ThemeDataTypeControllerTest, StartAssociationFailed) { SetStartExpectations(); SetAssociateExpectations(); - EXPECT_CALL(*model_associator_, AssociateModels()). - WillRepeatedly(Return(false)); + EXPECT_CALL(*model_associator_, AssociateModels(_)). + WillRepeatedly(DoAll(browser_sync::SetSyncError(syncable::THEMES), + Return(false))); EXPECT_CALL(start_callback_, Run(DataTypeController::ASSOCIATION_FAILED, _)); theme_dtc_->Start(NewCallback(&start_callback_, &StartCallback::Run)); diff --git a/chrome/browser/sync/glue/theme_model_associator.cc b/chrome/browser/sync/glue/theme_model_associator.cc index e53b021..5638bad 100644 --- a/chrome/browser/sync/glue/theme_model_associator.cc +++ b/chrome/browser/sync/glue/theme_model_associator.cc @@ -8,6 +8,7 @@ #include "base/logging.h" #include "base/tracked.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/glue/theme_util.h" @@ -35,11 +36,11 @@ ThemeModelAssociator::ThemeModelAssociator( ThemeModelAssociator::~ThemeModelAssociator() {} -bool ThemeModelAssociator::AssociateModels() { +bool ThemeModelAssociator::AssociateModels(SyncError* error) { sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); sync_api::ReadNode root(&trans); if (!root.InitByTagLookup(kThemesTag)) { - LOG(ERROR) << kNoThemesFolderError; + error->Reset(FROM_HERE, kNoThemesFolderError, model_type()); return false; } @@ -62,7 +63,9 @@ bool ThemeModelAssociator::AssociateModels() { sync_api::WriteNode node(&trans); if (!node.InitUniqueByCreation(syncable::THEMES, root, kCurrentThemeClientTag)) { - LOG(ERROR) << "Could not create current theme node."; + error->Reset(FROM_HERE, + "Could not create current theme node.", + model_type()); return false; } node.SetIsFolder(false); @@ -74,7 +77,7 @@ bool ThemeModelAssociator::AssociateModels() { return true; } -bool ThemeModelAssociator::DisassociateModels() { +bool ThemeModelAssociator::DisassociateModels(SyncError* error) { // We don't maintain any association state, so nothing to do. return true; } diff --git a/chrome/browser/sync/glue/theme_model_associator.h b/chrome/browser/sync/glue/theme_model_associator.h index e76b5bd..357c7b6 100644 --- a/chrome/browser/sync/glue/theme_model_associator.h +++ b/chrome/browser/sync/glue/theme_model_associator.h @@ -27,8 +27,8 @@ class ThemeModelAssociator : public AssociatorInterface { static syncable::ModelType model_type() { return syncable::THEMES; } // AssociatorInterface implementation. - virtual bool AssociateModels(); - virtual bool DisassociateModels(); + virtual bool AssociateModels(SyncError* error); + virtual bool DisassociateModels(SyncError* error); virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes); virtual void AbortAssociation() { // No implementation needed, this associator runs on the main diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc index 4aad3e5..961f2b9 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.cc +++ b/chrome/browser/sync/glue/typed_url_model_associator.cc @@ -11,6 +11,7 @@ #include "base/tracked.h" #include "base/utf_string_conversions.h" #include "chrome/browser/history/history_backend.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/protocol/typed_url_specifics.pb.h" @@ -46,13 +47,15 @@ TypedUrlModelAssociator::TypedUrlModelAssociator( TypedUrlModelAssociator::~TypedUrlModelAssociator() {} -bool TypedUrlModelAssociator::AssociateModels() { +bool TypedUrlModelAssociator::AssociateModels(SyncError* error) { VLOG(1) << "Associating TypedUrl Models"; DCHECK(expected_loop_ == MessageLoop::current()); std::vector<history::URLRow> typed_urls; if (!history_backend_->GetAllTypedURLs(&typed_urls)) { - LOG(ERROR) << "Could not get the typed_url entries."; + error->Reset(FROM_HERE, + "Could not get the typed_url entries.", + model_type()); return false; } @@ -62,7 +65,7 @@ bool TypedUrlModelAssociator::AssociateModels() { ix != typed_urls.end(); ++ix) { if (!history_backend_->GetVisitsForURL(ix->id(), &(visit_vectors[ix->id()]))) { - LOG(ERROR) << "Could not get the url's visits."; + error->Reset(FROM_HERE, "Could not get the url's visits.", model_type()); return false; } // Sometimes (due to a bug elsewhere in the history or sync code, or due to @@ -87,8 +90,10 @@ bool TypedUrlModelAssociator::AssociateModels() { sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare()); sync_api::ReadNode typed_url_root(&trans); if (!typed_url_root.InitByTagLookup(kTypedUrlTag)) { - LOG(ERROR) << "Server did not create the top-level typed_url node. We " - << "might be running against an out-of-date server."; + error->Reset(FROM_HERE, + "Server did not create the top-level typed_url node. We " + "might be running against an out-of-date server.", + model_type()); return false; } @@ -120,7 +125,9 @@ bool TypedUrlModelAssociator::AssociateModels() { if (difference & DIFF_UPDATE_NODE) { sync_api::WriteNode write_node(&trans); if (!write_node.InitByClientTagLookup(syncable::TYPED_URLS, tag)) { - LOG(ERROR) << "Failed to edit typed_url sync node."; + error->Reset(FROM_HERE, + "Failed to edit typed_url sync node.", + model_type()); return false; } // We don't want to resurrect old visits that have been aged out by @@ -159,7 +166,9 @@ bool TypedUrlModelAssociator::AssociateModels() { sync_api::WriteNode node(&trans); if (!node.InitUniqueByCreation(syncable::TYPED_URLS, typed_url_root, tag)) { - LOG(ERROR) << "Failed to create typed_url sync node."; + error->Reset(FROM_HERE, + "Failed to create typed_url sync node.", + model_type()); return false; } @@ -179,7 +188,7 @@ bool TypedUrlModelAssociator::AssociateModels() { while (sync_child_id != sync_api::kInvalidId) { sync_api::ReadNode sync_child_node(&trans); if (!sync_child_node.InitByIdLookup(sync_child_id)) { - LOG(ERROR) << "Failed to fetch child node."; + error->Reset(FROM_HERE, "Failed to fetch child node.", model_type()); return false; } const sync_pb::TypedUrlSpecifics& typed_url( @@ -228,7 +237,9 @@ bool TypedUrlModelAssociator::AssociateModels() { ++it) { sync_api::WriteNode sync_node(&trans); if (!sync_node.InitByIdLookup(*it)) { - LOG(ERROR) << "Failed to fetch obsolete node."; + error->Reset(FROM_HERE, + "Failed to fetch obsolete node.", + model_type()); return false; } sync_node.Remove(); @@ -266,7 +277,7 @@ bool TypedUrlModelAssociator::DeleteAllNodes( return true; } -bool TypedUrlModelAssociator::DisassociateModels() { +bool TypedUrlModelAssociator::DisassociateModels(SyncError* error) { id_map_.clear(); id_map_inverse_.clear(); return true; diff --git a/chrome/browser/sync/glue/typed_url_model_associator.h b/chrome/browser/sync/glue/typed_url_model_associator.h index fb75550..ae4c5c1 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.h +++ b/chrome/browser/sync/glue/typed_url_model_associator.h @@ -61,13 +61,13 @@ class TypedUrlModelAssociator // PerDataTypeAssociatorInterface implementation. // // Iterates through the sync model looking for matched pairs of items. - virtual bool AssociateModels(); + virtual bool AssociateModels(SyncError* error); // Delete all typed url nodes. bool DeleteAllNodes(sync_api::WriteTransaction* trans); // Clears all associations. - virtual bool DisassociateModels(); + virtual bool DisassociateModels(SyncError* error); // The has_nodes out param is true if the sync model has nodes other // than the permanent tagged nodes. diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index e9625e6..f0b3a64 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -1263,13 +1263,12 @@ void ProfileSyncService::Observe(int type, break; } case chrome::NOTIFICATION_SYNC_CONFIGURE_DONE: { - DataTypeManager::ConfigureResultWithErrorLocation* result_with_location = - Details<DataTypeManager::ConfigureResultWithErrorLocation>( - details).ptr(); + DataTypeManager::ConfigureResult* result = + Details<DataTypeManager::ConfigureResult>(details).ptr(); - DataTypeManager::ConfigureResult result = result_with_location->result; - VLOG(1) << "PSS SYNC_CONFIGURE_DONE called with result: " << result; - if (result == DataTypeManager::ABORTED && + DataTypeManager::ConfigureStatus status = result->status; + VLOG(1) << "PSS SYNC_CONFIGURE_DONE called with status: " << status; + if (status == DataTypeManager::ABORTED && expect_sync_configuration_aborted_) { VLOG(0) << "ProfileSyncService::Observe Sync Configure aborted"; expect_sync_configuration_aborted_ = false; @@ -1277,11 +1276,13 @@ void ProfileSyncService::Observe(int type, } // Clear out the gaia password if it is already there. gaia_password_ = std::string(); - if (result != DataTypeManager::OK) { + if (status != DataTypeManager::OK) { VLOG(0) << "ProfileSyncService::Observe: Unrecoverable error detected"; - std::string message = StringPrintf("Sync Configuration failed with %d", - result); - OnUnrecoverableError(*(result_with_location->location), message); + std::string message = + "Sync Configuration failed while configuring " + + syncable::ModelTypeSetToString(result->failed_types) + + ": " + DataTypeManager::ConfigureStatusToString(status); + OnUnrecoverableError(result->location, message); cached_passphrase_ = CachedPassphrase(); return; } diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc index 1d05bc6..7f36c97 100644 --- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc @@ -21,6 +21,7 @@ #include "base/utf_string_conversions.h" #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/sync/abstract_profile_sync_service_test.h" +#include "chrome/browser/sync/api/sync_error.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/bookmark_change_processor.h" #include "chrome/browser/sync/glue/bookmark_model_associator.h" @@ -325,7 +326,8 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { profile_.GetBookmarkModel(), test_user_share_.user_share(), &mock_unrecoverable_error_handler_)); - EXPECT_TRUE(model_associator_->AssociateModels()); + SyncError error; + EXPECT_TRUE(model_associator_->AssociateModels(&error)); MessageLoop::current()->RunAllPending(); // Set up change processor. @@ -338,8 +340,8 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { void StopSync() { change_processor_->Stop(); change_processor_.reset(); - - EXPECT_TRUE(model_associator_->DisassociateModels()); + SyncError error; + EXPECT_TRUE(model_associator_->DisassociateModels(&error)); model_associator_.reset(); message_loop_.RunAllPending(); diff --git a/chrome/browser/sync/profile_sync_service_startup_unittest.cc b/chrome/browser/sync/profile_sync_service_startup_unittest.cc index 322f33c..652230e 100644 --- a/chrome/browser/sync/profile_sync_service_startup_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_startup_unittest.cc @@ -275,10 +275,13 @@ TEST_F(ProfileSyncServiceStartupTest, ClearServerData) { TEST_F(ProfileSyncServiceStartupTest, StartFailure) { DataTypeManagerMock* data_type_manager = SetUpDataTypeManager(); - DataTypeManager::ConfigureResult configure_result = + DataTypeManager::ConfigureStatus status = DataTypeManager::ASSOCIATION_FAILED; - browser_sync::DataTypeManager::ConfigureResultWithErrorLocation result( - configure_result, FROM_HERE, syncable::ModelTypeSet()); + browser_sync::DataTypeManager::ConfigureResult result( + status, + syncable::ModelTypeSet(), + syncable::ModelTypeSet(), + FROM_HERE); EXPECT_CALL(*data_type_manager, Configure(_, _)). WillRepeatedly( DoAll( diff --git a/chrome/browser/sync/syncable/model_type.cc b/chrome/browser/sync/syncable/model_type.cc index 868b43c..28bafef 100644 --- a/chrome/browser/sync/syncable/model_type.cc +++ b/chrome/browser/sync/syncable/model_type.cc @@ -543,4 +543,8 @@ bool intToRealModelType(const std::string& notification_type, return false; } +bool IsRealDataType(ModelType model_type) { + return model_type >= FIRST_REAL_MODEL_TYPE && model_type < MODEL_TYPE_COUNT; +} + } // namespace syncable diff --git a/chrome/browser/sync/syncable/model_type.h b/chrome/browser/sync/syncable/model_type.h index d8582a3..e15da9e 100644 --- a/chrome/browser/sync/syncable/model_type.h +++ b/chrome/browser/sync/syncable/model_type.h @@ -162,6 +162,9 @@ bool RealModelTypeToint(ModelType model_type, bool intToRealModelType(const std::string& notification_type, ModelType* model_type); +// Returns true if |model_type| is a real datatype +bool IsRealDataType(ModelType model_type); + } // namespace syncable #endif // CHROME_BROWSER_SYNC_SYNCABLE_MODEL_TYPE_H_ diff --git a/chrome/browser/sync/syncable/model_type_unittest.cc b/chrome/browser/sync/syncable/model_type_unittest.cc index f946f95..5461f76 100644 --- a/chrome/browser/sync/syncable/model_type_unittest.cc +++ b/chrome/browser/sync/syncable/model_type_unittest.cc @@ -78,5 +78,14 @@ TEST_F(ModelTypeTest, ModelTypeBitSetFromString) { EXPECT_FALSE(ModelTypeBitSetFromString(input_string, &output)); } +TEST_F(ModelTypeTest, IsRealDataType) { + EXPECT_FALSE(IsRealDataType(UNSPECIFIED)); + EXPECT_FALSE(IsRealDataType(MODEL_TYPE_COUNT)); + EXPECT_FALSE(IsRealDataType(TOP_LEVEL_FOLDER)); + EXPECT_TRUE(IsRealDataType(FIRST_REAL_MODEL_TYPE)); + EXPECT_TRUE(IsRealDataType(BOOKMARKS)); + EXPECT_TRUE(IsRealDataType(APPS)); +} + } // namespace } // namespace syncable diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index 647ad32..8b28b7d 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -581,6 +581,8 @@ 'browser/sync/api/sync_change.cc', 'browser/sync/api/sync_change_processor.h', 'browser/sync/api/sync_change_processor.cc', + 'browser/sync/api/sync_error.h', + 'browser/sync/api/sync_error.cc', ], 'include_dirs': [ '..', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 414e187..904afe7 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -3304,6 +3304,8 @@ 'sources': [ '<(protoc_out_dir)/chrome/browser/sync/protocol/test.pb.cc', 'app/breakpad_mac_stubs.mm', + 'browser/sync/api/sync_change_unittest.cc', + 'browser/sync/api/sync_error_unittest.cc', 'browser/sync/engine/apply_updates_command_unittest.cc', 'browser/sync/engine/build_commit_command_unittest.cc', 'browser/sync/engine/clear_data_command_unittest.cc', @@ -3346,7 +3348,6 @@ 'browser/sync/syncable/syncable_enum_conversions_unittest.cc', 'browser/sync/syncable/syncable_id_unittest.cc', 'browser/sync/syncable/syncable_unittest.cc', - 'browser/sync/api/sync_change_unittest.cc', 'browser/sync/util/data_encryption_unittest.cc', 'browser/sync/util/extensions_activity_monitor_unittest.cc', 'browser/sync/util/protobuf_unittest.cc', |