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 95804)
+++ 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,10 +29,12 @@
 #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/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"
@@ -39,6 +42,8 @@
 #include "ui/base/l10n/l10n_util.h"
 
 using base::Time;
+using syncable::SEARCH_ENGINES;

[Peter Kasting, 2011/08/09 21:31:04]

Nit: We normally try to avoid using declarations (I'd personally kill base::Time
here as well), would putting the extra qualifier in the code below really make
things much worse?

[SteveT, 2011/08/10 15:15:12]

Agreed. Cleaned up both using declarations.

+
 typedef SearchHostToURLsMap::TemplateURLSet TemplateURLSet;
 
 namespace {
@@ -104,7 +109,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 +127,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 +297,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);
@@ -391,6 +409,14 @@
                                           const string16& title,
                                           const string16& keyword,
                                           const std::string& search_url) {
+  ResetTemplateURL(url, title, keyword, search_url, "");

[Peter Kasting, 2011/08/09 21:31:04]

Nit: Use std::string() instead of ""

[SteveT, 2011/08/10 15:15:12]

Done.

+}
+
+void TemplateURLService::ResetTemplateURL(const TemplateURL* url,
+                                          const string16& title,
+                                          const string16& keyword,
+                                          const std::string& search_url,
+                                          const std::string& guid) {
   TemplateURL new_url(*url);
   new_url.set_short_name(title);
   new_url.set_keyword(keyword);
@@ -401,6 +427,8 @@
     new_url.SetFaviconURL(GURL());
     new_url.SetURL(search_url, 0, 0);
   }
+  if (!guid.empty())
+    new_url.set_sync_guid(guid);
   new_url.set_safe_for_autoreplace(false);
   new_url.set_last_modified(time_provider_());
   UpdateNoNotify(url, new_url);
@@ -604,6 +632,206 @@
   }
 }
 
+SyncDataList TemplateURLService::GetAllSyncData(syncable::ModelType type)
+    const {

[Peter Kasting, 2011/08/09 21:31:04]

Nit: I think our pattern has been to wrap after "(" when "const" doesn't fit on
the same line as the rest of the declaration.

[SteveT, 2011/08/10 15:15:12]

Done.

+  DCHECK_EQ(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;
+    SyncData data;
+    if (!CreateSearchEngineSyncData(**iter, &data))
+      continue;
+    current_data.push_back(data);
+  }
+
+  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.", 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(SEARCH_ENGINES, iter->sync_data().GetDataType());
+
+    scoped_ptr<TemplateURL> turl(ReadSearchEngineSyncData(iter->sync_data()));
+    if (!turl.get()) {
+      LOG(ERROR) << "Failed to read search engine.";
+      NOTREACHED();
+      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.
+      LOG(ERROR) << "Unexpected sync change state.";
+      NOTREACHED();
+    }
+  }
+
+  SyncError sync_error =
+      sync_processor_->ProcessSyncChanges(from_here, new_changes);
+  if (sync_error.IsSet())
+    return sync_error;
+
+  return SyncError();
+}
+
+SyncError TemplateURLService::MergeDataAndStartSyncing(
+    syncable::ModelType type,
+    const SyncDataList& initial_sync_data,
+    SyncChangeProcessor* sync_processor) {
+  DCHECK_EQ(type, 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(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) {
+    const TemplateURL* sync_turl = ReadSearchEngineSyncData(iter->second);
+    DCHECK(sync_turl);
+    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());
+      } 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.
+      local_turl = FindDuplicateOfSyncTemplateURL(*sync_turl);
+      TemplateURL* modifiable_sync_turl = const_cast<TemplateURL*>(sync_turl);
+      if (local_turl) {
+        // Merge duplicates and remove the processed local TURL from the map.
+        TemplateURL* modifiable_local_turl =
+            const_cast<TemplateURL*>(local_turl);
+        std::string old_guid = local_turl->sync_guid();
+        MergeSyncAndLocalURLDuplicates(modifiable_sync_turl,
+                                       modifiable_local_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(modifiable_sync_turl, new_changes);
+        // Force the local ID to 0 so we can add it.
+        modifiable_sync_turl->set_id(0);
+        Add(modifiable_sync_turl);
+      }
+    }
+  }  // 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, 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;
+
+  AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
+
+  SyncData sync_data;
+  if (!CreateSearchEngineSyncData(*turl, &sync_data)) {
+    LOG(ERROR) << "Failed to update preference.";
+    return;
+  }
+  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 +866,192 @@
                             PrefService::UNSYNCABLE_PREF);
 }
 
+// static
+bool TemplateURLService::CreateSearchEngineSyncData(const TemplateURL& turl,
+                                                    SyncData* sync_data) {
+  DCHECK(sync_data);
+
+  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());
+  se_specifics->set_last_modified(turl.last_modified().ToInternalValue());
+  se_specifics->set_sync_guid(turl.sync_guid());
+  *sync_data = SyncData::CreateLocalData(se_specifics->sync_guid(),
+                                         se_specifics->keyword(),
+                                         specifics);
+  return true;
+}
+
+// static
+TemplateURL* TemplateURLService::ReadSearchEngineSyncData(
+    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;
+}
+
+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;
+  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;
+    if (!CreateSearchEngineSyncData(*sync_turl, &sync_data)) {
+      LOG(ERROR) << "Failed to update search engine.";
+      return false;
+    }
+    change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+  } else {
+    new_keyword = UniquifyKeyword(*existing_turl);
+    ResetTemplateURL(
+        existing_turl,

[Peter Kasting, 2011/08/09 21:31:04]

Nit: No need for one-arg-per-line or putting even the first arg on a new line
like we do for function declarations; you're free to collapse args into fewer
lines when making calls.

[SteveT, 2011/08/10 15:15:12]

Cool. Hope this works for you. It was mostly the last big argument that forced
the line breaks.

+        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);
+
+  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.
+    Remove(local_turl);
+    // Force the local ID to 0 so we can add it.
+    sync_turl->set_id(0);
+    Add(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. We ensure that the local_turl's new GUID is
+    // persisted to the database by calling ResetTemplateURL.
+    ResetTemplateURL(
+        local_turl,
+        local_turl->short_name(),
+        local_turl->keyword(),
+        local_turl->url() ? local_turl->url()->url() : std::string(),
+        sync_turl->sync_guid());
+    SyncData sync_data;
+    if (!CreateSearchEngineSyncData(*local_turl, &sync_data)) {
+      LOG(ERROR) << "Failed to update search engine.";
+      return;
+    }
+    change_list.push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
+  }
+}
+
+// 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 +1127,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 +1150,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 +1351,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 +1360,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 +1665,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 +1692,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();