Rewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_DELETEs to Sync.

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
@@ skipping 176 matching lines @@
       old_base_url(old_base_url) {
 }
 
 OldBaseURLSearchTermsData::~OldBaseURLSearchTermsData() {
 }
 
 std::string OldBaseURLSearchTermsData::GoogleBaseURLValue() const {
   return old_base_url;
 }
 
+// Returns true if |turl|'s GUID is not found inside |sync_data|. This is to be
+// used in MergeDataAndStartSyncing to differentiate between TemplateURLs from
+// Sync and TemplateURLs that were initially local, assuming |sync_data| is the
+// |initial_sync_data| parameter.
+bool IsFromSync(const TemplateURL* turl, const SyncDataMap& sync_data) {
+  return (sync_data.find(turl->sync_guid()) != sync_data.end());

[Peter Kasting, 2012/08/14 17:47:06]

Nit: You can use !!sync_data.count(turl->sync_guid()) or similar here.

[SteveT, 2012/08/14 19:15:10]

Done.

+}
+
 }  // namespace
 
-
 class TemplateURLService::LessWithPrefix {
  public:
   // We want to find the set of keywords that begin with a prefix.  The STL
   // algorithms will return the set of elements that are "equal to" the
   // prefix, where "equal(x, y)" means "!(cmp(x, y) || cmp(y, x))".  When
   // cmp() is the typical std::less<>, this results in lexicographic equality;
   // we need to extend this to mark a prefix as "not less than" a keyword it
   // begins, which will cause the desired elements to be considered "equal to"
   // the prefix.  Note: this is still a strict weak ordering, as required by
   // equal_range() (though I will not prove that here).
@@ skipping 795 matching lines @@
       Remove(existing_turl);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_ADD) {
       if (existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_ADD");
         LOG(ERROR) << "Trying to add an existing TemplateURL.";
         continue;
       }
-      std::string guid = turl->sync_guid();
-      if (!existing_keyword_turl || ResolveSyncKeywordConflict(turl.get(),
-          existing_keyword_turl, &new_changes)) {
-        // Force the local ID to kInvalidTemplateURLID so we can add it.
-        TemplateURLData data(turl->data());
-        data.id = kInvalidTemplateURLID;
-        Add(new TemplateURL(profile_, data));
+      const std::string guid = turl->sync_guid();
+      if (existing_keyword_turl) {
+        // Resolve any conflicts so we can safely add the new entry.
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
+      }
+      // Force the local ID to kInvalidTemplateURLID so we can add it.
+      TemplateURLData data(turl->data());
+      data.id = kInvalidTemplateURLID;
+      Add(new TemplateURL(profile_, data));
 
-        // Possibly set the newly added |turl| as the default search provider.
-        SetDefaultSearchProviderIfNewlySynced(guid);
-      }
+      // Possibly set the newly added |turl| as the default search provider.
+      SetDefaultSearchProviderIfNewlySynced(guid);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
       if (!existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
         error = sync_error_factory_->CreateAndUploadError(
             FROM_HERE,
             "ProcessSyncChanges failed on ChangeType ACTION_UPDATE");
         LOG(ERROR) << "Trying to update a non-existent TemplateURL.";
         continue;
       }
-      // Possibly resolve a keyword conflict if they have the same keywords but
-      // are not the same entry.
-      if (existing_keyword_turl && (existing_keyword_turl != existing_turl) &&
-          !ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
-                                      &new_changes)) {
-        // Note that because we're processing changes, this Remove() call won't
-        // generate an ACTION_DELETE; but ResolveSyncKeywordConflict() did
-        // already, so we should be OK.
-        Remove(existing_turl);
-        continue;
+      if (existing_keyword_turl && (existing_keyword_turl != existing_turl)) {
+        // Resolve any conflicts with other entries so we can safely update the
+        // keyword.
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
       }
       UIThreadSearchTermsData search_terms_data(existing_turl->profile());
       if (UpdateNoNotify(existing_turl, *turl, search_terms_data))
         NotifyObservers();
     } else {
       // We've unexpectedly received an ACTION_INVALID.
       NOTREACHED() << "Unexpected sync change state.";
       error = sync_error_factory_->CreateAndUploadError(
           FROM_HERE,
           "ProcessSyncChanges received an ACTION_INVALID");
@@ skipping 67 matching lines @@
       // preprocessing in TemplateURLService's loading code). Ignore it and send
       // an ACTION_DELETE up to the server.
       new_changes.push_back(
           syncer::SyncChange(FROM_HERE,
                              syncer::SyncChange::ACTION_DELETE,
                              iter->second));
       continue;
     }
 
     if (local_turl) {
+      DCHECK(IsFromSync(local_turl, sync_data_map));
       // This local search engine is already synced. If the timestamp differs
       // from Sync, we need to update locally or to the cloud. Note that if the
       // timestamps are equal, we touch neither.
       if (sync_turl->last_modified() > local_turl->last_modified()) {
         // We've received an update from Sync. We should replace all synced
         // fields in the local TemplateURL. Note that this includes the
         // TemplateURLID and the TemplateURL may have to be reparsed. This
         // also makes the local data's last_modified timestamp equal to Sync's,
         // avoiding an Update on the next MergeData call.
         UIThreadSearchTermsData search_terms_data(local_turl->profile());
         if (UpdateNoNotify(local_turl, *sync_turl, search_terms_data))
           NotifyObservers();
       } else if (sync_turl->last_modified() < local_turl->last_modified()) {
         // Otherwise, we know we have newer data, so update Sync with our
         // data fields.
         new_changes.push_back(
             syncer::SyncChange(FROM_HERE,
                                syncer::SyncChange::ACTION_UPDATE,
                                local_data_map[local_turl->sync_guid()]));
       }
       local_data_map.erase(iter->first);
     } else {
-      // The search engine from the cloud has not been synced locally, but there
-      // might be a local search engine that is a duplicate that needs to be
-      // merged.
-      TemplateURL* dupe_turl = FindDuplicateOfSyncTemplateURL(*sync_turl);
-      if (dupe_turl) {
-        // Merge duplicates and remove the processed local TURL from the map.
-        std::string old_guid = dupe_turl->sync_guid();
-        MergeSyncAndLocalURLDuplicates(sync_turl.release(), dupe_turl,
-                                       &new_changes);
-        local_data_map.erase(old_guid);
-      } else {
-        std::string guid = sync_turl->sync_guid();
-        // Keyword conflict is possible in this case. Resolve it first before
-        // adding the new TemplateURL. Note that we don't remove the local TURL
-        // from local_data_map in this case as it may still need to be pushed to
-        // the cloud.  We also explicitly don't resolve conflicts against
-        // extension keywords; see comments in ProcessSyncChanges().
-        TemplateURL* existing_keyword_turl =
-            FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
-        if (!existing_keyword_turl || ResolveSyncKeywordConflict(
-            sync_turl.get(), existing_keyword_turl, &new_changes)) {
-          // Force the local ID to kInvalidTemplateURLID so we can add it.
-          TemplateURLData data(sync_turl->data());
-          data.id = kInvalidTemplateURLID;
-          Add(new TemplateURL(profile_, data));
-
-          // Possibly set the newly added |turl| as the default search provider.
-          SetDefaultSearchProviderIfNewlySynced(guid);
-        }
-      }
+      // The search engine from the cloud has not been synced locally. Merge it
+      // into our local model. This will handle any conflicts with local (and
+      // already-synced) TemplateURLs. It will prefer to keep entries from Sync
+      // over not-yet-synced TemplateURLs.
+      MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes,
+                             &local_data_map);
     }
   }
 
   // The remaining SyncData in local_data_map should be everything that needs to
   // be pushed as ADDs to sync.
   for (SyncDataMap::const_iterator iter = local_data_map.begin();
       iter != local_data_map.end(); ++iter) {
     new_changes.push_back(
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_ADD,
@@ skipping 1123 matching lines @@
   // This is a best-effort approach where we try to preserve the original
   // keyword and let the user do what they will after our attempt.
   string16 keyword_candidate(turl.keyword());
   do {
     keyword_candidate.append(ASCIIToUTF16("_"));
   } while (GetTemplateURLForKeyword(keyword_candidate));
 
   return keyword_candidate;
 }
 
-bool TemplateURLService::ResolveSyncKeywordConflict(
-    TemplateURL* sync_turl,
-    TemplateURL* local_turl,
+bool TemplateURLService::IsLocalTemplateURLBetter(
+    const TemplateURL* local_turl,
+    const TemplateURL* sync_turl) {
+  DCHECK(GetTemplateURLForGUID(local_turl->sync_guid()));
+  return local_turl->last_modified() > sync_turl->last_modified() ||
+         local_turl->created_by_policy() ||
+         local_turl== GetDefaultSearchProvider();
+}
+
+void TemplateURLService::ResolveSyncKeywordConflict(
+    TemplateURL* unapplied_sync_turl,
+    TemplateURL* applied_sync_turl,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
-  DCHECK(sync_turl);
-  DCHECK(local_turl);
-  DCHECK(sync_turl->sync_guid() != local_turl->sync_guid());
-  DCHECK(!local_turl->IsExtensionKeyword());
+  DCHECK(unapplied_sync_turl);
+  DCHECK(applied_sync_turl);
   DCHECK(change_list);
+  DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
+  DCHECK(!applied_sync_turl->IsExtensionKeyword());
 
-  const bool local_is_better =
-      (local_turl->last_modified() > sync_turl->last_modified()) ||
-      local_turl->created_by_policy() ||
-      (local_turl == GetDefaultSearchProvider());
-  const bool can_replace_local = CanReplace(local_turl);
-  if (CanReplace(sync_turl) && (local_is_better || !can_replace_local)) {
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    return false;
+  // Both |unapplied_sync_turl| and |applied_sync_turl| are known to Sync, so
+  // don't delete either of them. Instead, determine which is "better" and
+  // uniquify the other one, sending an update to the server for the updated
+  // entry.
+  const bool applied_turl_is_better =
+      IsLocalTemplateURLBetter(applied_sync_turl, unapplied_sync_turl);
+  TemplateURL* loser = applied_turl_is_better ?
+      unapplied_sync_turl : applied_sync_turl;
+  string16 new_keyword = UniquifyKeyword(*loser, false);
+  DCHECK(!GetTemplateURLForKeyword(new_keyword));
+  if (applied_turl_is_better) {
+    // Just set the keyword of |unapplied_sync_turl|. The caller is responsible
+    // for adding or updating unapplied_sync_turl in the local model.
+    unapplied_sync_turl->data_.SetKeyword(new_keyword);
+  } else {
+    // Update |applied_sync_turl| in the local model with the new keyword.
+    TemplateURLData data(applied_sync_turl->data());
+    data.SetKeyword(new_keyword);
+    TemplateURL new_turl(applied_sync_turl->profile(), data);
+    UIThreadSearchTermsData search_terms_data(applied_sync_turl->profile());
+    if (UpdateNoNotify(applied_sync_turl, new_turl, search_terms_data))
+      NotifyObservers();
   }
-  if (can_replace_local) {
-    // Since we're processing sync changes, the upcoming Remove() won't generate
-    // an ACTION_DELETE.  We need to do it manually to keep the server in sync
-    // with us.  Note that if we're being called from
-    // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
-    // than having just been brought down, then this is wrong, because the
-    // server doesn't yet know about this entity; but in this case,
-    // PruneSyncChanges() will prune out the ACTION_DELETE we create here.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    Remove(local_turl);
-  } else if (local_is_better) {
-    string16 new_keyword = UniquifyKeyword(*sync_turl, false);
-    DCHECK(!GetTemplateURLForKeyword(new_keyword));
-    sync_turl->data_.SetKeyword(new_keyword);
-    // If we update the cloud TURL, we need to push an update back to sync
-    // informing it that something has changed.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
-  } else {
-    string16 new_keyword = UniquifyKeyword(*local_turl, false);
-    TemplateURLData data(local_turl->data());
-    data.SetKeyword(new_keyword);
-    TemplateURL new_turl(local_turl->profile(), data);
-    UIThreadSearchTermsData search_terms_data(local_turl->profile());
-    if (UpdateNoNotify(local_turl, new_turl, search_terms_data))
-      NotifyObservers();
-    // Since we're processing sync changes, the UpdateNoNotify() above didn't
-    // generate an ACTION_UPDATE.  We need to do it manually to keep the server
-    // in sync with us.  Note that if we're being called from
-    // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
-    // than having just been brought down, then this is wrong, because the
-    // server won't know about this entity until it processes the ACTION_ADD our
-    // caller will later generate; but in this case, PruneSyncChanges() will
-    // prune out the ACTION_UPDATE we create here.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
-  }
-  return true;
+  // The losing TemplateURL should have their keyword updated. Send a change to
+  // the server to reflect this change.
+  syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*loser);
+  change_list->push_back(syncer::SyncChange(FROM_HERE,
+      syncer::SyncChange::ACTION_UPDATE,
+      sync_data));
 }
 
-TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL(
-    const TemplateURL& sync_turl) {
-  TemplateURL* existing_turl = GetTemplateURLForKeyword(sync_turl.keyword());
-  return existing_turl && (existing_turl->url() == sync_turl.url()) ?
-      existing_turl : NULL;
-}
+void TemplateURLService::MergeInSyncTemplateURL(
+    TemplateURL* sync_turl,
+    const SyncDataMap& sync_data,
+    syncer::SyncChangeList* change_list,
+    SyncDataMap* local_data) {
+  DCHECK(sync_turl);
+  DCHECK(!GetTemplateURLForGUID(sync_turl->sync_guid()));
+  DCHECK(IsFromSync(sync_turl, sync_data));
 
-void TemplateURLService::MergeSyncAndLocalURLDuplicates(
-    TemplateURL* sync_turl,
-    TemplateURL* local_turl,
-    syncer::SyncChangeList* change_list) {
-  DCHECK(loaded_);
-  DCHECK(sync_turl);
-  DCHECK(local_turl);
-  DCHECK(change_list);
-  scoped_ptr<TemplateURL> scoped_sync_turl(sync_turl);
-  if (sync_turl->last_modified() > local_turl->last_modified()) {
-    // Fully replace local_url with Sync's copy. Note that because use Add
-    // rather than ResetTemplateURL, |sync_url| is added with a fresh
-    // TemplateURLID. We don't need to sync the new ID back to the server since
-    // it's only relevant locally.
-    bool delete_default = (local_turl == GetDefaultSearchProvider());
-    DCHECK(!delete_default || !is_default_search_managed_);
-    if (delete_default)
-      default_search_provider_ = NULL;
+  TemplateURL* conflicting_turl =
+      FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
+  bool should_add_sync_turl = true;
 
-    // See comments in ResolveSyncKeywordConflict() regarding generating an
-    // ACTION_DELETE manually since Remove() won't do it.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    Remove(local_turl);
+  // If there was no TemplateURL in the local model that conflicts with
+  // |sync_turl|, skip the following preparation steps and just add |sync_turl|
+  // directly. Otherwise, modify |conflicting_turl| to make room for
+  // |sync_turl|.
+  if (conflicting_turl) {
+    if (IsFromSync(conflicting_turl, sync_data)) {
+      // |conflicting_turl| is already known to Sync, so we're not allowed to
+      // remove it. In this case, we want to uniquify the worse one and send an
+      // update for the changed keyword to sync. We can reuse the logic from
+      // ResolveSyncKeywordConflict for this.
+      ResolveSyncKeywordConflict(sync_turl, conflicting_turl, change_list);
+    } else {
+      // |conflicting_turl| is not yet known to Sync. If it is better, then we
+      // want to transfer its values up to sync. Otherwise, we remove it and
+      // allow the entry from Sync to overtake it in the model.
+      const std::string guid = conflicting_turl->sync_guid();
+      if (IsLocalTemplateURLBetter(conflicting_turl, sync_turl)) {
+        ResetTemplateURLGUID(conflicting_turl, sync_turl->sync_guid());
+        syncer::SyncData sync_data =
+            CreateSyncDataFromTemplateURL(*conflicting_turl);
+        change_list->push_back(syncer::SyncChange(
+            FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data));
+        if (conflicting_turl == GetDefaultSearchProvider() &&
+            !pending_synced_default_search_) {
+          // If we're not waiting for the Synced default to come in, we should
+          // override the pref with our new GUID. If we are waiting for the
+          // arrival of a synced default, setting the pref here would cause us
+          // to lose the GUID we are waiting on.
+          PrefService* prefs = GetPrefs();
+          if (prefs) {
+            prefs->SetString(prefs::kSyncedDefaultSearchProviderGUID,
+                             conflicting_turl->sync_guid());
+          }
+        }
+        // Note that in this case we do not add the Sync TemplateURL to the
+        // local model, since we've effectively "merged" it in by updating the
+        // local conflicting entry with its sync_guid.
+        should_add_sync_turl = false;
+      } else {
+        // We guarantee that this isn't the local search provider. Otherwise,
+        // local would have won.
+        DCHECK(conflicting_turl != GetDefaultSearchProvider());
+        Remove(conflicting_turl);
+      }
+      // This TemplateURL was either removed or overwritten in the local model.
+      // Remove the entry from the local data so it isn't pushed up to Sync.
+      local_data->erase(guid);
+    }
+  }
 
+  if (should_add_sync_turl) {
+    const std::string guid = sync_turl->sync_guid();
     // Force the local ID to kInvalidTemplateURLID so we can add it.
-    sync_turl->data_.id = kInvalidTemplateURLID;
-    Add(scoped_sync_turl.release());
-    if (delete_default)
-      SetDefaultSearchProvider(sync_turl);
-  } else {
-    // Change the local TURL's GUID to the server's GUID and push an update to
-    // Sync. This ensures that the rest of local_url's fields are sync'd up to
-    // the server, and the next time local_url is synced, it is recognized by
-    // having the same GUID.
-    ResetTemplateURLGUID(local_turl, sync_turl->sync_guid());
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
+    TemplateURLData data(sync_turl->data());
+    data.id = kInvalidTemplateURLID;
+    Add(new TemplateURL(profile_, data));
+
+    // Possibly set the newly added |turl| as the default search provider.
+    SetDefaultSearchProviderIfNewlySynced(guid);
   }
 }
 
 void TemplateURLService::SetDefaultSearchProviderIfNewlySynced(
     const std::string& guid) {
   // If we're not syncing or if default search is managed by policy, ignore.
   if (!sync_processor_.get() || is_default_search_managed_)
     return;
 
   PrefService* prefs = GetPrefs();
@@ skipping 32 matching lines @@
     // TODO(mpcomplete): If we allow editing extension keywords, then those
     // should be persisted to disk and synced.
     if (template_url->sync_guid().empty() &&
         !template_url->IsExtensionKeyword()) {
       template_url->data_.sync_guid = base::GenerateGUID();
       if (service_.get())
         service_->UpdateKeyword(template_url->data());
     }
   }
 }