Skip to content

Commit

Permalink
SyncableService::DeactivateDataType isn't needed.
Browse files Browse the repository at this point in the history
SyncableService::DeactivateDataType isn't needed and should be
removed for 3 reasons:
1) It is called only from Stop() method of
old implementations of DataTypeController:
- FrontendDataTypeController
- NonFrontendDataTypeController
UI/NonUI DataType controllers don't call this method from
Stop() and work fine without it.
2) Datatype deactivation already done from
DataTypeManagerImpl::OnSingleDataTypeWillStop() which is called
just before calling the DTC::Stop() method.
3) As a result of ongoing refactoring of USS code there will
be two separate deactivation methods - one for directory
datatypes, another for USS datatypes. It would be difficult for
SyncableService::DeactivateDataType to figure out which of
the two specific deactivation codepaths to take.

Note: SyncDataTypeManagerImplTest tests still pass with
this change. There is a test that verifies deactivation of
Bookmarks data type which is based on FrontendDataTypeController.
A few tests that bypass DataTypeManager had to be tweaked
to not expect SyncableService::DeactivateDataType() to be
called when DTC::Stop() is called.

BUG=515962

Review URL: https://codereview.chromium.org/1364043002

Cr-Commit-Position: refs/heads/master@{#350634}
  • Loading branch information
stanisc authored and Commit bot committed Sep 24, 2015
1 parent 1c45553 commit 6446739
Show file tree
Hide file tree
Showing 11 changed files with 0 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ class SyncBookmarkDataTypeControllerTest : public testing::Test,
}

void SetStopExpectations() {
EXPECT_CALL(service_, DeactivateDataType(_));
EXPECT_CALL(*model_associator_, DisassociateModels()).
WillOnce(Return(syncer::SyncError()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ class SyncFrontendDataTypeControllerTest : public testing::Test,

void SetStopExpectations() {
EXPECT_CALL(*dtc_mock_.get(), CleanUpState());
EXPECT_CALL(service_, DeactivateDataType(_));
EXPECT_CALL(*model_associator_, DisassociateModels()).
WillOnce(Return(syncer::SyncError()));
}
Expand Down
7 changes: 0 additions & 7 deletions chrome/browser/sync/glue/non_frontend_data_type_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,6 @@ void NonFrontendDataTypeController::Stop() {
if (state_ == NOT_RUNNING)
return;

// Deactivate the date type on the UI thread first to stop processing
// sync server changes. This needs to happen before posting task to destroy
// processor and associator on backend. Otherwise it could crash if syncer
// post work to backend after destruction task and that work is run before
// deactivation.
sync_client_->GetSyncService()->DeactivateDataType(type());

// Ignore association callback.
weak_ptr_factory_.InvalidateWeakPtrs();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ class SyncNonFrontendDataTypeControllerTest

void SetStopExpectations() {
EXPECT_CALL(*dtc_mock_.get(), DisconnectProcessor(_));
EXPECT_CALL(service_, DeactivateDataType(_));
EXPECT_CALL(*model_associator_, DisassociateModels()).
WillOnce(Return(syncer::SyncError()));
}
Expand Down Expand Up @@ -368,9 +367,6 @@ TEST_F(SyncNonFrontendDataTypeControllerTest,
SetAssociateExpectations();
SetActivateExpectations(DataTypeController::OK);
EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, "Test"));
EXPECT_CALL(service_, DisableDatatype(_))
.WillOnce(InvokeWithoutArgs(non_frontend_dtc_.get(),
&NonFrontendDataTypeController::Stop));
SetStopExpectations();
EXPECT_EQ(DataTypeController::NOT_RUNNING, non_frontend_dtc_->state());
Start();
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/sync/profile_sync_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2073,12 +2073,6 @@ base::Value* ProfileSyncService::GetTypeStatusMap() const {
return result.release();
}

void ProfileSyncService::DeactivateDataType(syncer::ModelType type) {
if (!backend_)
return;
backend_->DeactivateDataType(type);
}

void ProfileSyncService::ConsumeCachedPassphraseIfPossible() {
// If no cached passphrase, or sync backend hasn't started up yet, just exit.
// If the backend isn't running yet, OnBackendInitialized() will call this
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/sync/profile_sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ class ProfileSyncService : public sync_driver::SyncService,
void RegisterDataTypeController(
sync_driver::DataTypeController* data_type_controller) override;
void ReenableDatatype(syncer::ModelType type) override;
void DeactivateDataType(syncer::ModelType type) override;
SyncTokenStatus GetSyncTokenStatus() const override;
std::string QuerySyncStatusSummaryString() override;
bool QueryDetailedSyncStatus(syncer::SyncStatus* result) override;
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/sync/profile_sync_service_mock.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class ProfileSyncServiceMock : public ProfileSyncService {
const std::string& message));
MOCK_METHOD1(DisableDatatype, void(const syncer::SyncError&));
MOCK_CONST_METHOD0(GetUserShare, syncer::UserShare*());
MOCK_METHOD1(DeactivateDataType, void(syncer::ModelType));
MOCK_METHOD0(RequestStart, void());
MOCK_METHOD1(RequestStop, void(ProfileSyncService::SyncStopDataFate));

Expand Down
2 changes: 0 additions & 2 deletions components/sync_driver/fake_sync_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,6 @@ void FakeSyncService::RegisterDataTypeController(

void FakeSyncService::ReenableDatatype(syncer::ModelType type) {}

void FakeSyncService::DeactivateDataType(syncer::ModelType type) {}

bool FakeSyncService::IsPassphraseRequired() const {
return false;
}
Expand Down
1 change: 0 additions & 1 deletion components/sync_driver/fake_sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class FakeSyncService : public sync_driver::SyncService {
void RegisterDataTypeController(
sync_driver::DataTypeController* data_type_controller) override;
void ReenableDatatype(syncer::ModelType type) override;
void DeactivateDataType(syncer::ModelType type) override;
SyncTokenStatus GetSyncTokenStatus() const override;
std::string QuerySyncStatusSummaryString() override;
bool QueryDetailedSyncStatus(syncer::SyncStatus* result) override;
Expand Down
2 changes: 0 additions & 2 deletions components/sync_driver/frontend_data_type_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ void FrontendDataTypeController::Stop() {

CleanUpState();

sync_client_->GetSyncService()->DeactivateDataType(type());

if (model_associator()) {
syncer::SyncError error; // Not used.
error = model_associator()->DisassociateModels();
Expand Down
3 changes: 0 additions & 3 deletions components/sync_driver/sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,6 @@ class SyncService : public DataTypeEncryptionHandler {
// restarts.
virtual void ReenableDatatype(syncer::ModelType type) = 0;

// TODO(zea): Remove these and have the dtc's call directly into the SBH.
virtual void DeactivateDataType(syncer::ModelType type) = 0;

// Return sync token status.
virtual SyncTokenStatus GetSyncTokenStatus() const = 0;

Expand Down

0 comments on commit 6446739

Please sign in to comment.