Implement SyncableServices in TemplateURLService. Add related unittests.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
===================================================================
--- chrome/browser/search_engines/template_url_service.cc	(revision 97053)
+++ chrome/browser/search_engines/template_url_service.cc	(working copy)
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
+#include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/i18n/case_conversion.h"
@@ -28,18 +29,21 @@
 #include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/util.h"
+#include "chrome/browser/sync/api/sync_change.h"
+#include "chrome/browser/sync/protocol/search_engine_specifics.pb.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/guid.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/common/notification_service.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using base::Time;
 typedef SearchHostToURLsMap::TemplateURLSet TemplateURLSet;
+typedef TemplateURLService::SyncDataMap SyncDataMap;
 
 namespace {
 
@@ -104,7 +108,10 @@
       default_search_provider_(NULL),
       is_default_search_managed_(false),
       next_id_(1),
-      time_provider_(&base::Time::Now) {
+      time_provider_(&base::Time::Now),
+      models_associated_(false),
+      processing_syncer_changes_(false),
+      sync_processor_(NULL) {
   DCHECK(profile_);
   Init(NULL, 0);
 }
@@ -119,7 +126,10 @@
       default_search_provider_(NULL),
       is_default_search_managed_(false),
       next_id_(1),
-      time_provider_(&base::Time::Now) {
+      time_provider_(&base::Time::Now),
+      models_associated_(false),
+      processing_syncer_changes_(false),
+      sync_processor_(NULL) {
   Init(initializers, count);
 }
 
@@ -286,6 +296,13 @@
   return (elem == keyword_to_template_map_.end()) ? NULL : elem->second;
 }
 
+const TemplateURL* TemplateURLService::GetTemplateURLForGUID(
+    const std::string& sync_guid) const {
+  GUIDToTemplateMap::const_iterator elem(
+      guid_to_template_map_.find(sync_guid));
+  return (elem == guid_to_template_map_.end()) ? NULL : elem->second;
+}
+
 const TemplateURL* TemplateURLService::GetTemplateURLForHost(
     const std::string& host) const {
   return provider_map_.GetTemplateURLForHost(host);
@@ -301,8 +318,8 @@
   NotifyObservers();
 }
 
-void TemplateURLService::RemoveAutoGeneratedBetween(Time created_after,
-                                                    Time created_before) {
+void TemplateURLService::RemoveAutoGeneratedBetween(base::Time created_after,
+                                                    base::Time created_before) {
   bool should_notify = false;
   for (size_t i = 0; i < template_urls_.size();) {
     if (template_urls_[i]->date_created() >= created_after &&
@@ -319,8 +336,8 @@
     NotifyObservers();
 }
 
-void TemplateURLService::RemoveAutoGeneratedSince(Time created_after) {
-  RemoveAutoGeneratedBetween(created_after, Time());
+void TemplateURLService::RemoveAutoGeneratedSince(base::Time created_after) {
+  RemoveAutoGeneratedBetween(created_after, base::Time());
 }
 
 void TemplateURLService::RegisterExtensionKeyword(const Extension* extension) {
@@ -604,6 +621,199 @@
   }
 }
 
+SyncDataList TemplateURLService::GetAllSyncData(
+    syncable::ModelType type) const {
+  DCHECK_EQ(syncable::SEARCH_ENGINES, type);
+
+  SyncDataList current_data;
+  for (TemplateURLVector::const_iterator iter = template_urls_.begin();
+      iter != template_urls_.end(); ++iter) {
+    // We don't sync extension keywords.
+    if ((*iter)->IsExtensionKeyword())
+      continue;
+    current_data.push_back(CreateSyncDataFromTemplateURL(**iter));
+  }
+
+  return current_data;
+}
+
+SyncError TemplateURLService::ProcessSyncChanges(
+    const tracked_objects::Location& from_here,
+    const SyncChangeList& change_list) {
+  if (!models_associated_) {
+    SyncError error(FROM_HERE, "Models not yet associated.",
+                    syncable::SEARCH_ENGINES);
+    return error;
+  }
+
+  AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
+
+  SyncChangeList new_changes;
+  for (SyncChangeList::const_iterator iter = change_list.begin();
+      iter != change_list.end(); ++iter) {
+    DCHECK_EQ(syncable::SEARCH_ENGINES, iter->sync_data().GetDataType());
+
+    scoped_ptr<TemplateURL> turl(
+        CreateTemplateURLFromSyncData(iter->sync_data()));
+    if (!turl.get()) {
+      NOTREACHED() << "Failed to read search engine.";
+      continue;
+    }
+
+    const TemplateURL* existing_turl = GetTemplateURLForGUID(turl->sync_guid());
+    const TemplateURL* existing_keyword_turl =
+        GetTemplateURLForKeyword(turl->keyword());
+
+    if (iter->change_type() == SyncChange::ACTION_DELETE && existing_turl) {
+      Remove(existing_turl);
+    } else if (iter->change_type() == SyncChange::ACTION_ADD &&
+               !existing_turl) {
+      if (existing_keyword_turl)
+        ResolveSyncKeywordConflict(turl.get(), new_changes);
+      // Force the local ID to 0 so we can add it.
+      turl->set_id(0);
+      Add(turl.release());
+    } else if (iter->change_type() == SyncChange::ACTION_UPDATE &&
+               existing_turl) {
+      if (existing_keyword_turl)
+        ResolveSyncKeywordConflict(turl.get(), new_changes);
+      ResetTemplateURL(existing_turl, turl->short_name(), turl->keyword(),
+          turl->url() ? turl->url()->url() : std::string());
+    } else {
+      // Something really unexpected happened. Either we received an
+      // ACTION_INVALID, or Sync is in a crazy state:
+      //  . Trying to DELETE or UPDATE a non-existent search engine.
+      //  . Trying to ADD a search engine that already exists.
+      NOTREACHED() << "Unexpected sync change state.";
+      SyncError error(FROM_HERE, "ProcessSyncChanges failed on ChangeType " +
+          SyncChange::ChangeTypeToString(iter->change_type()),
+          syncable::SEARCH_ENGINES);
+      return error;

[sky, 2011/08/17 23:22:29]

Are you sure we want to return here and not continue on? Say you have two
machines, A and B. A is connected, B is offline. If you delete the same TURL on
both machines, then B becomes connected, might you hit this branch? Maybe
MergeData happens first so this isn't an issue?

+    }
+  }
+
+  SyncError sync_error =
+      sync_processor_->ProcessSyncChanges(from_here, new_changes);
+
+  return sync_error;
+}
+
+SyncError TemplateURLService::MergeDataAndStartSyncing(

[sky, 2011/08/17 23:22:29]

Is it guaranteed these methods are only invoked after loading has completed?
Could you add a DCHECK to that effect?

+    syncable::ModelType type,
+    const SyncDataList& initial_sync_data,
+    SyncChangeProcessor* sync_processor) {
+  DCHECK_EQ(type, syncable::SEARCH_ENGINES);
+  DCHECK(!sync_processor_);
+  sync_processor_ = sync_processor;
+
+  // We do a lot of calls to Add/Remove/ResetTemplateURL here, so ensure we
+  // don't step on our own toes.
+  AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
+
+  SyncChangeList new_changes;
+
+  // Build maps of our sync GUIDs to SyncData.
+  SyncDataMap local_data_map = CreateGUIDToSyncDataMap(
+      GetAllSyncData(syncable::SEARCH_ENGINES));
+  SyncDataMap sync_data_map = CreateGUIDToSyncDataMap(initial_sync_data);
+
+  for (SyncDataMap::const_iterator iter = sync_data_map.begin();
+      iter != sync_data_map.end(); ++iter) {
+    scoped_ptr<TemplateURL> sync_turl(
+        CreateTemplateURLFromSyncData(iter->second));
+    DCHECK(sync_turl.get());
+    const TemplateURL* local_turl = GetTemplateURLForGUID(iter->first);
+
+    if (local_turl) {
+      // 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()) {
+        // TODO(stevet): For now we just reset the local TemplateURL with the
+        // more important Sync data fields. We may want to transfer over
+        // additional fields.
+        ResetTemplateURL(
+            local_turl,
+            sync_turl->short_name(),
+            sync_turl->keyword(),
+            sync_turl->url() ? sync_turl->url()->url() : std::string());

[sky, 2011/08/17 23:22:29]

I believe you'll also want safe_for_autoreplace copied over too, otherwise any
synced TURL will end up with safe_for_autoreplace false.

+      } else if (sync_turl->last_modified() < local_turl->last_modified()) {
+        new_changes.push_back(SyncChange(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.
+      const TemplateURL* dupe_turl = FindDuplicateOfSyncTemplateURL(*sync_turl);
+      if (dupe_turl) {
+        // Merge duplicates and remove the processed local TURL from the map.
+        TemplateURL* modifiable_dupe_turl =
+            const_cast<TemplateURL*>(dupe_turl);
+        std::string old_guid = dupe_turl->sync_guid();
+        MergeSyncAndLocalURLDuplicates(sync_turl.release(),
+                                       modifiable_dupe_turl,
+                                       new_changes);
+        local_data_map.erase(old_guid);
+      } else {
+        // 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.
+        ResolveSyncKeywordConflict(sync_turl.get(), new_changes);
+        // Force the local ID to 0 so we can add it.
+        sync_turl->set_id(0);
+        Add(sync_turl.release());
+      }
+    }
+  }  // for
+
+  // 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(SyncChange(SyncChange::ACTION_ADD, iter->second));
+  }
+
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
+  if (error.IsSet())
+    return error;
+
+  models_associated_ = true;
+  return SyncError();
+}
+
+void TemplateURLService::StopSyncing(syncable::ModelType type) {
+  DCHECK_EQ(type, syncable::SEARCH_ENGINES);
+  models_associated_ = false;
+  sync_processor_ = NULL;
+}
+
+void TemplateURLService::ProcessTemplateURLChange(
+    const TemplateURL* turl,
+    SyncChange::SyncChangeType type) {
+  DCHECK_NE(type, SyncChange::ACTION_INVALID);
+  DCHECK(turl);
+
+  if (!models_associated_)
+    return;  // Not syncing.
+
+  if (processing_syncer_changes_)
+    return;  // These are changes originating from us. Ignore.
+
+  // Avoid syncing Extension keywords.
+  if (turl->IsExtensionKeyword())
+    return;
+
+  SyncChangeList changes;
+
+  SyncData sync_data = CreateSyncDataFromTemplateURL(*turl);
+  changes.push_back(SyncChange(type, sync_data));
+
+  sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+}
+
 // static
 void TemplateURLService::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kDefaultSearchProviderEnabled,
@@ -638,6 +848,80 @@
                             PrefService::UNSYNCABLE_PREF);
 }
 
+// static
+SyncData TemplateURLService::CreateSyncDataFromTemplateURL(
+    const TemplateURL& turl) {
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::SearchEngineSpecifics* se_specifics = specifics.MutableExtension(
+      sync_pb::search_engine);
+  se_specifics->set_short_name(UTF16ToUTF8(turl.short_name()));
+  se_specifics->set_keyword(UTF16ToUTF8(turl.keyword()));
+  se_specifics->set_favicon_url(turl.GetFaviconURL().spec());
+  se_specifics->set_url(turl.url() ? turl.url()->url() : std::string());
+  se_specifics->set_safe_for_autoreplace(turl.safe_for_autoreplace());
+  se_specifics->set_originating_url(turl.originating_url().spec());
+  se_specifics->set_date_created(turl.date_created().ToInternalValue());
+  se_specifics->set_input_encodings(JoinString(turl.input_encodings(), ';'));
+  se_specifics->set_show_in_default_list(turl.show_in_default_list());
+  se_specifics->set_suggestions_url(turl.suggestions_url() ?
+      turl.suggestions_url()->url() : std::string());
+  se_specifics->set_prepopulate_id(turl.prepopulate_id());
+  se_specifics->set_autogenerate_keyword(turl.autogenerate_keyword());
+  se_specifics->set_logo_id(turl.logo_id());
+  se_specifics->set_created_by_policy(turl.created_by_policy());
+  se_specifics->set_instant_url(turl.instant_url() ?
+      turl.instant_url()->url() : std::string());
+  se_specifics->set_id(turl.id());

[sky, 2011/08/17 23:22:29]

Is there any protection to make sure we don't end up with duplicate ids? In fact
do we even need to sync the id?

+  se_specifics->set_last_modified(turl.last_modified().ToInternalValue());
+  se_specifics->set_sync_guid(turl.sync_guid());
+  return SyncData::CreateLocalData(se_specifics->sync_guid(),
+                                   se_specifics->keyword(),
+                                   specifics);
+}
+
+// static
+TemplateURL* TemplateURLService::CreateTemplateURLFromSyncData(
+    const SyncData& sync_data) {
+  sync_pb::SearchEngineSpecifics specifics =
+      sync_data.GetSpecifics().GetExtension(sync_pb::search_engine);
+  TemplateURL* turl = new TemplateURL();
+  turl->set_short_name(UTF8ToUTF16(specifics.short_name()));
+  turl->set_keyword(UTF8ToUTF16(specifics.keyword()));
+  turl->SetFaviconURL(GURL(specifics.favicon_url()));
+  turl->SetURL(specifics.url(), 0, 0);
+  turl->set_safe_for_autoreplace(specifics.safe_for_autoreplace());
+  turl->set_originating_url(GURL(specifics.originating_url()));
+  turl->set_date_created(
+      base::Time::FromInternalValue(specifics.date_created()));
+  std::vector<std::string> input_encodings;
+  base::SplitString(specifics.input_encodings(), ';', &input_encodings);
+  turl->set_input_encodings(input_encodings);
+  turl->set_show_in_default_list(specifics.show_in_default_list());
+  turl->SetSuggestionsURL(specifics.suggestions_url(), 0, 0);
+  turl->SetPrepopulateId(specifics.prepopulate_id());
+  turl->set_autogenerate_keyword(specifics.autogenerate_keyword());
+  turl->set_logo_id(specifics.logo_id());
+  turl->set_created_by_policy(specifics.created_by_policy());
+  turl->SetInstantURL(specifics.instant_url(), 0, 0);
+  turl->set_id(specifics.id());
+  turl->set_last_modified(
+      base::Time::FromInternalValue(specifics.last_modified()));
+  turl->set_sync_guid(specifics.sync_guid());
+  return turl;
+}
+
+// static
+SyncDataMap TemplateURLService::CreateGUIDToSyncDataMap(
+    const SyncDataList& sync_data) {
+  SyncDataMap data_map;
+  SyncDataList::const_iterator iter;
+  for (iter = sync_data.begin(); iter != sync_data.end(); ++iter) {
+    data_map[iter->GetSpecifics().GetExtension(
+        sync_pb::search_engine).sync_guid()] = *iter;
+  }
+  return data_map;
+}
+
 void TemplateURLService::SetKeywordSearchTermsForURL(const TemplateURL* t_url,
                                                      const GURL& url,
                                                      const string16& term) {
@@ -713,6 +997,8 @@
 void TemplateURLService::RemoveFromMaps(const TemplateURL* template_url) {
   if (!template_url->keyword().empty())
     keyword_to_template_map_.erase(template_url->keyword());
+  if (!template_url->sync_guid().empty())
+    guid_to_template_map_.erase(template_url->sync_guid());
   if (loaded_)
     provider_map_.Remove(template_url);
 }
@@ -734,6 +1020,8 @@
 void TemplateURLService::AddToMaps(const TemplateURL* template_url) {
   if (!template_url->keyword().empty())
     keyword_to_template_map_[template_url->keyword()] = template_url;
+  if (!template_url->sync_guid().empty())
+    guid_to_template_map_[template_url->sync_guid()] = template_url;
   if (loaded_) {
     UIThreadSearchTermsData search_terms_data;
     provider_map_.Add(template_url, search_terms_data);
@@ -933,6 +1221,8 @@
 
   if (!existing_turl->keyword().empty())
     keyword_to_template_map_.erase(existing_turl->keyword());
+  if (!existing_turl->sync_guid().empty())
+    guid_to_template_map_.erase(existing_turl->sync_guid());
 
   // This call handles copying over the values (while retaining the id).
   UIThreadSearchTermsData search_terms_data;
@@ -940,10 +1230,15 @@
 
   if (!existing_turl->keyword().empty())
     keyword_to_template_map_[existing_turl->keyword()] = existing_turl;
+  if (!existing_turl->sync_guid().empty())
+    guid_to_template_map_[existing_turl->sync_guid()] = existing_turl;
 
   if (service_.get())
     service_->UpdateKeyword(*existing_turl);
 
+  // Inform sync of the update.
+  ProcessTemplateURLChange(existing_turl, SyncChange::ACTION_UPDATE);
+
   if (default_search_provider_ == existing_turl)
     SetDefaultSearchProviderNoNotify(existing_turl);
 }
@@ -1240,6 +1535,10 @@
 
   if (service_.get())
     service_->AddKeyword(*template_url);
+
+  // Inform sync of the addition. Note that this will assign a GUID to
+  // template_url and add it to the guid_to_template_map_.
+  ProcessTemplateURLChange(template_url, SyncChange::ACTION_ADD);
 }
 
 void TemplateURLService::RemoveNoNotify(const TemplateURL* template_url) {
@@ -1263,6 +1562,9 @@
   if (service_.get())
     service_->RemoveKeyword(*template_url);
 
+  // Inform sync of the deletion.
+  ProcessTemplateURLChange(template_url, SyncChange::ACTION_DELETE);
+
   if (profile_) {
     Source<Profile> source(profile_);
     TemplateURLID id = template_url->id();
@@ -1330,3 +1632,106 @@
     }
   }
 }
+
+void TemplateURLService::ResetTemplateURLGUID(const TemplateURL* url,
+                                              const std::string& guid) {
+  DCHECK(!guid.empty());
+
+  TemplateURL new_url(*url);
+  new_url.set_sync_guid(guid);
+  UpdateNoNotify(url, new_url);
+}
+
+string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl) const {
+  // Already unique.
+  if (!GetTemplateURLForKeyword(turl.keyword()))
+    return turl.keyword();
+
+  // First, try to return the generated keyword for the TemplateURL.
+  string16 keyword_candidate = GenerateKeyword(
+      turl.url() ? GURL(turl.url()->url()) : GURL(), true);
+  if (!GetTemplateURLForKeyword(keyword_candidate) &&
+      !keyword_candidate.empty()) {
+    return keyword_candidate;
+  }
+
+  // We try to uniquify the keyword by appending a special character to the end.
+  // 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.
+  keyword_candidate = turl.keyword();
+  do {
+    keyword_candidate.append(UTF8ToUTF16("_"));
+  } while (GetTemplateURLForKeyword(keyword_candidate));
+
+  return keyword_candidate;
+}
+
+bool TemplateURLService::ResolveSyncKeywordConflict(
+    TemplateURL* sync_turl,
+    SyncChangeList& change_list) {
+  DCHECK(sync_turl);
+
+  const TemplateURL* existing_turl =
+      GetTemplateURLForKeyword(sync_turl->keyword());
+  if (!existing_turl)
+    return false;
+
+  string16 new_keyword;

[sky, 2011/08/17 23:22:29]

nit: pull into if as you don't use it outside the if.

+  if (existing_turl->last_modified() > sync_turl->last_modified())  {
+    new_keyword = UniquifyKeyword(*sync_turl);
+    DCHECK(!GetTemplateURLForKeyword(new_keyword));
+    sync_turl->set_keyword(new_keyword);
+    // If we update the cloud TURL, we need to push an update back to sync
+    // informing it that something has changed.
+    SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
+    change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+  } else {
+    new_keyword = UniquifyKeyword(*existing_turl);
+    ResetTemplateURL(existing_turl, existing_turl->short_name(), new_keyword,
+        existing_turl->url() ? existing_turl->url()->url() : std::string());
+  }
+  return true;
+}
+
+const TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL(
+    const TemplateURL& sync_turl) {
+  const TemplateURL* existing_turl =
+      GetTemplateURLForKeyword(sync_turl.keyword());
+  if (!existing_turl)
+    return NULL;
+
+  if (existing_turl->url() && sync_turl.url() &&
+      existing_turl->url()->url() == sync_turl.url()->url()) {
+    return existing_turl;
+  }
+  return NULL;
+}
+
+void TemplateURLService::MergeSyncAndLocalURLDuplicates(
+    TemplateURL* sync_turl,
+    TemplateURL* local_turl,
+    SyncChangeList& change_list) {
+  DCHECK(sync_turl);
+  DCHECK(local_turl);
+
+  scoped_ptr<TemplateURL> scoped_sync_turl(sync_turl);
+
+  if (scoped_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.
+    Remove(local_turl);
+    // Force the local ID to 0 so we can add it.
+    scoped_sync_turl->set_id(0);
+    Add(scoped_sync_turl.release());
+  } 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());
+    SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
+    change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+  }
+}