[Sync] Refactor non-frontend DTC to handle new API properly.

chrome/browser/sync/glue/generic_change_processor.cc

 // 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/glue/generic_change_processor.h"
 
 #include "base/location.h"
 #include "base/utf_string_conversions.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/internal_api/base_node.h"
 #include "chrome/browser/sync/internal_api/change_record.h"
 #include "chrome/browser/sync/internal_api/read_node.h"
 #include "chrome/browser/sync/internal_api/read_transaction.h"
 #include "chrome/browser/sync/internal_api/write_node.h"
 #include "chrome/browser/sync/internal_api/write_transaction.h"
 #include "chrome/browser/sync/unrecoverable_error_handler.h"
+#include "content/browser/browser_thread.h"
 
 namespace browser_sync {
 
 GenericChangeProcessor::GenericChangeProcessor(
+    UnrecoverableErrorHandler* error_handler,
     SyncableService* local_service,
-    UnrecoverableErrorHandler* error_handler,
     sync_api::UserShare* user_share)
     : ChangeProcessor(error_handler),
-      local_service_(local_service),
-      user_share_(user_share) {
-  DCHECK(local_service_);
+      share_handle_(user_share) {
+  DCHECK(CalledOnValidThread());
+  if (local_service)

[akalin, 2011/10/11 22:41:08]

hmm, prefer passing weak ptr directly to constructor

[Nicolas Zea, 2011/10/12 04:24:19]

Done.

+    local_service_ = local_service->AsWeakPtr();
 }
 
 GenericChangeProcessor::~GenericChangeProcessor() {
-  // Set to null to ensure it's not used after destruction.
-  local_service_ = NULL;
+  // We can either be deleted when the DTC is destroyed (on UI thread),
+  // or when the SyncableService stop's syncing (datatype thread).
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||

[akalin, 2011/10/11 22:41:08]

prefer removing this, and making the owner ensure that the GCP is destroyed on
correct thread

[Nicolas Zea, 2011/10/12 04:24:19]

Done.

+         CalledOnValidThread());
+  DetachFromThread();
 }
 
 void GenericChangeProcessor::ApplyChangesFromSyncModel(
     const sync_api::BaseTransaction* trans,
     const sync_api::ImmutableChangeRecordList& changes) {
+  DCHECK(CalledOnValidThread());
   DCHECK(running());
   DCHECK(syncer_changes_.empty());
   for (sync_api::ChangeRecordList::const_iterator it =
            changes.Get().begin(); it != changes.Get().end(); ++it) {
     if (it->action == sync_api::ChangeRecord::ACTION_DELETE) {
       syncer_changes_.push_back(
           SyncChange(SyncChange::ACTION_DELETE,
                      SyncData::CreateRemoteData(it->id, it->specifics)));
     } else {
       SyncChange::SyncChangeType action =
           (it->action == sync_api::ChangeRecord::ACTION_ADD) ?
           SyncChange::ACTION_ADD : SyncChange::ACTION_UPDATE;
       // Need to load specifics from node.
       sync_api::ReadNode read_node(trans);
       if (!read_node.InitByIdLookup(it->id)) {
         error_handler()->OnUnrecoverableError(FROM_HERE, "Failed to look up "
             " data for received change with id " + it->id);
         return;
       }
       syncer_changes_.push_back(
           SyncChange(action,
                      SyncData::CreateRemoteData(
                          it->id, read_node.GetEntitySpecifics())));
     }
   }
 }
 
 void GenericChangeProcessor::CommitChangesFromSyncModel() {
+  DCHECK(CalledOnValidThread());
   if (!running())
     return;
   if (syncer_changes_.empty())
     return;
+  if (!local_service_) {
+    syncable::ModelType type = syncer_changes_[0].sync_data().GetDataType();
+    SyncError error(FROM_HERE, "Local service destroyed.", type);
+    error_handler()->OnUnrecoverableError(error.location(), error.message());
+  }
   SyncError error = local_service_->ProcessSyncChanges(FROM_HERE,
-                                                        syncer_changes_);
+                                                       syncer_changes_);
   syncer_changes_.clear();
   if (error.IsSet()) {
     error_handler()->OnUnrecoverableError(error.location(), error.message());
   }
 }
 
 SyncError GenericChangeProcessor::GetSyncDataForType(
     syncable::ModelType type,
     SyncDataList* current_sync_data) {
+  DCHECK(CalledOnValidThread());
   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))) {
     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;
   }
@@ skipping 37 matching lines @@
   }
   node->Remove();
   return true;
 }
 
 }  // namespace
 
 SyncError GenericChangeProcessor::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const SyncChangeList& list_of_changes) {
+  DCHECK(CalledOnValidThread());
   sync_api::WriteTransaction trans(from_here, share_handle());
 
   for (SyncChangeList::const_iterator iter = list_of_changes.begin();
        iter != list_of_changes.end();
        ++iter) {
     const SyncChange& change = *iter;
     DCHECK_NE(change.sync_data().GetDataType(), syncable::UNSPECIFIED);
     syncable::ModelType type = change.sync_data().GetDataType();
     std::string type_str = syncable::ModelTypeToString(type);
     sync_api::WriteNode sync_node(&trans);
@@ skipping 59 matching lines @@
                                             error.message());
       return error;
     }
   }
   return SyncError();
 }
 
 bool GenericChangeProcessor::SyncModelHasUserCreatedNodes(
     syncable::ModelType type,
     bool* has_nodes) {
+  DCHECK(CalledOnValidThread());
   DCHECK(has_nodes);
   DCHECK_NE(type, syncable::UNSPECIFIED);
   std::string type_name = syncable::ModelTypeToString(type);
   std::string err_str = "Server did not create the top-level " + type_name +
       " node. We might be running against an out-of-date server.";
   *has_nodes = false;
   sync_api::ReadTransaction trans(FROM_HERE, share_handle());
   sync_api::ReadNode type_root_node(&trans);
   if (!type_root_node.InitByTagLookup(syncable::ModelTypeToRootTag(type))) {
     LOG(ERROR) << err_str;
     return false;
   }
 
   // The sync model has user created nodes if the type's root node has any
   // children.
   *has_nodes = sync_api::kInvalidId != type_root_node.GetFirstChildId();
   return true;
 }
 
 bool GenericChangeProcessor::CryptoReadyIfNecessary(syncable::ModelType type) {
+  DCHECK(CalledOnValidThread());
   DCHECK_NE(type, syncable::UNSPECIFIED);
   // We only access the cryptographer while holding a transaction.
   sync_api::ReadTransaction trans(FROM_HERE, share_handle());
   const syncable::ModelTypeSet& encrypted_types =
       GetEncryptedTypes(&trans);
   return encrypted_types.count(type) == 0 ||
          trans.GetCryptographer()->is_ready();
 }
 
-void GenericChangeProcessor::StartImpl(Profile* profile) {}
+void GenericChangeProcessor::StartImpl(Profile* profile) {
+  DCHECK(CalledOnValidThread());
+}
 
-void GenericChangeProcessor::StopImpl() {}
+void GenericChangeProcessor::StopImpl() {
+}

[akalin, 2011/10/11 22:41:08]

stopimpl is called on the UI thread or something weird, right?  Add
DCHECK/header comment?

[Nicolas Zea, 2011/10/12 04:24:19]

Done.

 
-sync_api::UserShare* GenericChangeProcessor::share_handle() {
-  return user_share_;
+sync_api::UserShare* GenericChangeProcessor::share_handle() const {
+  DCHECK(CalledOnValidThread());
+  return share_handle_;
 }
 
 }  // namespace browser_sync