Revert 151391 - Rewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_…

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());
-}
-
 }  // 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;
       }
-      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);
+      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));
+
+        // Possibly set the newly added |turl| as the default search provider.
+        SetDefaultSearchProviderIfNewlySynced(guid);
       }
-      // 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);
     } 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;
       }
-      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);
+      // 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;
       }
       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. 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 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 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::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();
+bool TemplateURLService::ResolveSyncKeywordConflict(
+    TemplateURL* sync_turl,
+    TemplateURL* local_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(change_list);
+
+  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;
+  }
+  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;
 }
 
-void TemplateURLService::ResolveSyncKeywordConflict(
-    TemplateURL* unapplied_sync_turl,
-    TemplateURL* applied_sync_turl,
+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::MergeSyncAndLocalURLDuplicates(
+    TemplateURL* sync_turl,
+    TemplateURL* local_turl,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
-  DCHECK(unapplied_sync_turl);
-  DCHECK(applied_sync_turl);
+  DCHECK(sync_turl);
+  DCHECK(local_turl);
   DCHECK(change_list);
-  DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
-  DCHECK(!applied_sync_turl->IsExtensionKeyword());
+  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;
 
-  // 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);
+    // 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);
+
+    // 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 {
-    // 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();
-  }
-  // 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));
-}
-
-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));
-
-  TemplateURL* conflicting_turl =
-      FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
-  bool should_add_sync_turl = true;
-
-  // 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) {
-    DCHECK(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.
-    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);
+    // 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));
   }
 }
 
 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());
     }
   }
 }