Pass an EntitySpecifics with delete changes to generalize delete handling (and fix it for typed urls).

BUG=45883
TEST=sync extension/typed url/autofill, verify delete propagation

Review URL: http://codereview.chromium.org/2920002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@51969 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 35 insertions, 85 deletions

diff --git a/chrome/browser/sync/engine/change_reorder_buffer.cc b/chrome/browser/sync/engine/change_reorder_buffer.cc
index 7560e21..2d68e1e2 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.cc
+++ b/chrome/browser/sync/engine/change_reorder_buffer.cc
@@ -131,8 +131,8 @@ void ChangeReorderBuffer::GetAllChangesInTreeOrder(
       ChangeRecord record;
       record.id = i->first;
       record.action = ChangeRecord::ACTION_DELETE;
-      if (extra_data_.find(record.id) != extra_data_.end())
-        record.extra = extra_data_[record.id];
+      if (specifics_.find(record.id) != specifics_.end())
+        record.specifics = specifics_[record.id];
       changelist->push_back(record);
     } else {
       traversal.ExpandToInclude(trans, i->first);
@@ -162,8 +162,8 @@ void ChangeReorderBuffer::GetAllChangesInTreeOrder(
         record.action = ChangeRecord::ACTION_ADD;
       else
         record.action = ChangeRecord::ACTION_UPDATE;
-      if (extra_data_.find(record.id) != extra_data_.end())
-        record.extra = extra_data_[record.id];
+      if (specifics_.find(record.id) != specifics_.end())
+        record.specifics = specifics_[record.id];
       changelist->push_back(record);
     }
 
diff --git a/chrome/browser/sync/engine/change_reorder_buffer.h b/chrome/browser/sync/engine/change_reorder_buffer.h
index 6c199ed..9da05e3 100644
--- a/chrome/browser/sync/engine/change_reorder_buffer.h
+++ b/chrome/browser/sync/engine/change_reorder_buffer.h
@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "chrome/browser/sync/engine/syncapi.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"
 
 namespace sync_api {
 
@@ -36,7 +37,6 @@ namespace sync_api {
 class ChangeReorderBuffer {
  public:
   typedef SyncManager::ChangeRecord ChangeRecord;
-  typedef SyncManager::ExtraChangeRecordData ExtraChangeRecordData;
 
   ChangeReorderBuffer() { }
 
@@ -64,8 +64,8 @@ class ChangeReorderBuffer {
                                          OP_UPDATE_PROPERTIES_ONLY;
   }
 
-  void SetExtraDataForId(int64 id, ExtraChangeRecordData* extra) {
-    extra_data_[id] = extra;
+  void SetSpecificsForId(int64 id, const sync_pb::EntitySpecifics& specifics) {
+    specifics_[id] = specifics;
   }
 
   // Reset the buffer, forgetting any pushed items, so that it can be used
@@ -93,14 +93,14 @@ class ChangeReorderBuffer {
     OP_UPDATE_POSITION_AND_PROPERTIES,  // UpdatedItem with position_changed=1.
   };
   typedef std::map<int64, Operation> OperationMap;
-  typedef std::map<int64, ExtraChangeRecordData*> ExtraDataMap;
+  typedef std::map<int64, sync_pb::EntitySpecifics> SpecificsMap;
 
   // Stores the items that have been pushed into the buffer, and the type of
   // operation that was associated with them.
   OperationMap operations_;
 
-  // Stores extra ChangeRecord data per-ID.
-  ExtraDataMap extra_data_;
+  // Stores entity-specific ChangeRecord data per-ID.
+  SpecificsMap specifics_;
 
   DISALLOW_COPY_AND_ASSIGN(ChangeReorderBuffer);
 };
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index efc5966..e17d652 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -1713,27 +1713,10 @@ void SyncManager::SyncInternal::SetExtraChangeRecordData(int64 id,
     syncable::ModelType type, ChangeReorderBuffer* buffer,
     const syncable::EntryKernel& original, bool existed_before,
     bool exists_now) {
-  // Extra data for autofill deletions.
-  switch (type) {
-    case syncable::AUTOFILL:
-      if (!exists_now && existed_before) {
-        sync_pb::AutofillSpecifics* s = new sync_pb::AutofillSpecifics;
-        s->CopyFrom(original.ref(SPECIFICS).GetExtension(sync_pb::autofill));
-        ExtraChangeRecordData* extra = new ExtraAutofillChangeRecordData(s);
-        buffer->SetExtraDataForId(id, extra);
-      }
-      break;
-    case syncable::EXTENSIONS:
-      if (!exists_now && existed_before) {
-        const std::string& extension_id =
-            original.ref(SPECIFICS).GetExtension(sync_pb::extension).id();
-        ExtraChangeRecordData* extra =
-            new ExtraExtensionChangeRecordData(extension_id);
-        buffer->SetExtraDataForId(id, extra);
-      }
-      break;
-    default:
-      break;
+  // If this is a deletion, attach the entity specifics as extra data
+  // so that the delete can be processed.
+  if (!exists_now && existed_before) {
+    buffer->SetSpecificsForId(id, original.ref(SPECIFICS));
   }
 }
 
@@ -2132,8 +2115,4 @@ UserShare* SyncManager::GetUserShare() const {
   return data_->GetUserShare();
 }
 
-SyncManager::ExtraAutofillChangeRecordData::~ExtraAutofillChangeRecordData() {
-  delete pre_deletion_data;
-}
-
 }  // namespace sync_api
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index c9380e5..7f84abe 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -520,14 +520,6 @@ class SyncManager {
   // internal types from clients of the interface.
   class SyncInternal;
 
-  // Derive from this class and add your own data members to associate extra
-  // information with a ChangeRecord.
-  class ExtraChangeRecordData {
-   public:
-    ExtraChangeRecordData() {}
-    virtual ~ExtraChangeRecordData() {}
-  };
-
   // ChangeRecord indicates a single item that changed as a result of a sync
   // operation.  This gives the sync id of the node that changed, and the type
   // of change.  To get the actual property values after an ADD or UPDATE, the
@@ -538,29 +530,10 @@ class SyncManager {
       ACTION_DELETE,
       ACTION_UPDATE,
     };
-    ChangeRecord() : id(kInvalidId), action(ACTION_ADD), extra(NULL) {}
+    ChangeRecord() : id(kInvalidId), action(ACTION_ADD) {}
     int64 id;
     Action action;
-    ExtraChangeRecordData* extra;
-  };
-
-  // Extra specifics data that certain model types require. This is only
-  // used for autofill DELETE changes.
-  class ExtraAutofillChangeRecordData : public ExtraChangeRecordData {
-   public:
-    explicit ExtraAutofillChangeRecordData(sync_pb::AutofillSpecifics* s)
-        : pre_deletion_data(s) {}
-    virtual ~ExtraAutofillChangeRecordData();
-    const sync_pb::AutofillSpecifics* pre_deletion_data;
-  };
-
-  // Extra data used only for extension DELETE changes.
-  class ExtraExtensionChangeRecordData : public ExtraChangeRecordData {
-   public:
-    explicit ExtraExtensionChangeRecordData(const std::string& extension_id)
-        : extension_id(extension_id) {}
-    virtual ~ExtraExtensionChangeRecordData() {}
-    const std::string extension_id;
+    sync_pb::EntitySpecifics specifics;
   };
 
   // Status encapsulates detailed state about the internals of the SyncManager.
diff --git a/chrome/browser/sync/glue/autofill_change_processor.cc b/chrome/browser/sync/glue/autofill_change_processor.cc
index 06e7e86..dd01c6a 100644
--- a/chrome/browser/sync/glue/autofill_change_processor.cc
+++ b/chrome/browser/sync/glue/autofill_change_processor.cc
@@ -18,9 +18,6 @@
 #include "chrome/browser/webdata/web_database.h"
 #include "chrome/common/notification_service.h"
 
-typedef sync_api::SyncManager::ExtraAutofillChangeRecordData
-    ExtraAutofillChangeRecordData;
-
 namespace browser_sync {
 
 AutofillChangeProcessor::AutofillChangeProcessor(
@@ -302,14 +299,14 @@ void AutofillChangeProcessor::ApplyChangesFromSyncModel(
   for (int i = 0; i < change_count; ++i) {
     sync_api::SyncManager::ChangeRecord::Action action(changes[i].action);
     if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE == action) {
-      scoped_ptr<ExtraAutofillChangeRecordData> data(
-          static_cast<ExtraAutofillChangeRecordData*>(changes[i].extra));
-      DCHECK(data.get()) << "Extra autofill change record data not present!";
-      const sync_pb::AutofillSpecifics* autofill(data->pre_deletion_data);
-      if (autofill->has_value())
-        ApplySyncAutofillEntryDelete(*autofill);
-      else if (autofill->has_profile())
-        ApplySyncAutofillProfileDelete(autofill->profile(), changes[i].id);
+      DCHECK(changes[i].specifics.HasExtension(sync_pb::autofill))
+          << "Autofill specifics data not present on delete!";
+      const sync_pb::AutofillSpecifics& autofill =
+          changes[i].specifics.GetExtension(sync_pb::autofill);
+      if (autofill.has_value())
+        ApplySyncAutofillEntryDelete(autofill);
+      else if (autofill.has_profile())
+        ApplySyncAutofillProfileDelete(autofill.profile(), changes[i].id);
       else
         NOTREACHED() << "Autofill specifics has no data!";
       continue;
diff --git a/chrome/browser/sync/glue/extension_change_processor.cc b/chrome/browser/sync/glue/extension_change_processor.cc
index 3e2d10d..b6f4b42 100644
--- a/chrome/browser/sync/glue/extension_change_processor.cc
+++ b/chrome/browser/sync/glue/extension_change_processor.cc
@@ -16,9 +16,6 @@
 #include "chrome/common/notification_details.h"
 #include "chrome/common/notification_source.h"
 
-typedef sync_api::SyncManager::ExtraExtensionChangeRecordData
-    ExtraExtensionChangeRecordData;
-
 namespace browser_sync {
 
 ExtensionChangeProcessor::ExtensionChangeProcessor(
@@ -112,10 +109,9 @@ void ExtensionChangeProcessor::ApplyChangesFromSyncModel(
       }
       case sync_api::SyncManager::ChangeRecord::ACTION_DELETE: {
         StopObserving();
-        scoped_ptr<ExtraExtensionChangeRecordData>
-            data(static_cast<ExtraExtensionChangeRecordData*>(change.extra));
-        if (data.get()) {
-          extension_model_associator_->OnServerRemove(data->extension_id);
+        if (change.specifics.HasExtension(sync_pb::extension)) {
+          extension_model_associator_->OnServerRemove(
+              change.specifics.GetExtension(sync_pb::extension).id());
         } else {
           std::stringstream error;
           error << "Could not get extension ID for deleted node "
diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc
index 87cfa4f..c80426f 100644
--- a/chrome/browser/sync/glue/typed_url_change_processor.cc
+++ b/chrome/browser/sync/glue/typed_url_change_processor.cc
@@ -202,6 +202,14 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
   TypedUrlModelAssociator::TypedUrlUpdateVector updated_urls;
 
   for (int i = 0; i < change_count; ++i) {
+    if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE ==
+        changes[i].action) {
+      DCHECK(changes[i].specifics.HasExtension(sync_pb::typed_url)) <<
+          "Typed URL delete change does not have necessary specifics.";
+      GURL url(changes[i].specifics.GetExtension(sync_pb::typed_url).url());
+      history_backend_->DeleteURL(url);
+      continue;
+    }
 
     sync_api::ReadNode sync_node(trans);
     if (!sync_node.InitByIdLookup(changes[i].id)) {
@@ -249,9 +257,6 @@ void TypedUrlChangeProcessor::ApplyChangesFromSyncModel(
 
       new_visits.push_back(
           std::pair<GURL, std::vector<base::Time> >(url, added_visits));
-    } else if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE ==
-               changes[i].action) {
-      history_backend_->DeleteURL(url);
     } else {
       history::URLRow old_url;
       if (!history_backend_->GetURL(url, &old_url)) {