Retroactively addressed sky@'s comments from CL http://codereview.chromium.org/7566036/

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2011 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/environment.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 479 matching lines @@
   }
 
   // initial_default_search_provider_ is only needed before we've finished
   // loading. Now that we've loaded we can nuke it.
   initial_default_search_provider_.reset();
   is_default_search_managed_ = false;
 
   std::vector<TemplateURL*> template_urls;
   const TemplateURL* default_search_provider = NULL;
   int new_resource_keyword_version = 0;
   GetSearchProvidersUsingKeywordResult(*result,

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
This may end up modifying the set of TemplateURLs (at least the prepopulate
ones). You'll need to deal with that in some way. In fact it's worth a test case
to make sure migration gets handled correctly.

stevet: Got it. I won't address this here, but I'll make sure we have a test
case that covers it when we get to integration testing.

                                        service_.get(),
                                        GetPrefs(),
                                        &template_urls,
                                        &default_search_provider,
                                        &new_resource_keyword_version);
 
   bool database_specified_a_default = NULL != default_search_provider;
 
   // Check if default search provider is now managed.
   scoped_ptr<TemplateURL> default_from_prefs;
@@ skipping 131 matching lines @@
     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;
+  SyncError error;
   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);
+        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);
+        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 " +
+      error = SyncError(FROM_HERE, "ProcessSyncChanges failed on ChangeType " +
           SyncChange::ChangeTypeToString(iter->change_type()),
           syncable::SEARCH_ENGINES);
-      return error;

[SteveT, 2011/09/01 14:21:54]

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?

stevet: Fixed. We'll return the last error we see from this loop.

     }
   }
 
-  SyncError sync_error =
-      sync_processor_->ProcessSyncChanges(from_here, new_changes);
+  // If something went wrong, we want to prematurely exit to avoid pushing
+  // inconsistent data to Sync. We return the last error we received.
+  if (error.IsSet())
+    return error;
 
-  return sync_error;
+  error = sync_processor_->ProcessSyncChanges(from_here, new_changes);
+
+  return error;
 }
 
 SyncError TemplateURLService::MergeDataAndStartSyncing(

[SteveT, 2011/09/01 14:21:54]

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?

stevet: DCHECK added.

     syncable::ModelType type,
     const SyncDataList& initial_sync_data,
     SyncChangeProcessor* sync_processor) {
+  DCHECK(loaded());
   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());

[SteveT, 2011/09/01 14:21:54]

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.

stevet: We now copy every cloud value over except the local ID (which we don't
sync anymore). This should preserve |safe_for_autoreplace|.

+        // We've received an update from Sync. We should replace all existing
+        // values with the ones from sync except the local TemplateURLID. Note
+        // that this means that 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.
+        sync_turl->set_id(local_turl->id());
+        UpdateNoNotify(local_turl, *sync_turl);
+        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(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);
+                                       &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);
+        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();
@@ skipping 92 matching lines @@
       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_created_by_policy(turl.created_by_policy());
   se_specifics->set_instant_url(turl.instant_url() ?
       turl.instant_url()->url() : std::string());
   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);

[SteveT, 2011/09/01 14:21:54]

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?

stevet: Removed |id| in patch http://codereview.chromium.org/7669052/ (and
|logo_id| as well)

 }
 
 // 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()));
@@ skipping 773 matching lines @@
   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) {
+    SyncChangeList* change_list) {
   DCHECK(sync_turl);
+  DCHECK(change_list);
 
   const TemplateURL* existing_turl =
       GetTemplateURLForKeyword(sync_turl->keyword());
   if (!existing_turl)
     return false;
 
-  string16 new_keyword;

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
nit: pull into if as you don't use it outside the if.

stevet: Done.

   if (existing_turl->last_modified() > sync_turl->last_modified())  {
-    new_keyword = UniquifyKeyword(*sync_turl);
+    string16 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));
+    change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
   } else {
-    new_keyword = UniquifyKeyword(*existing_turl);
+    string16 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) {
+    SyncChangeList* change_list) {
   DCHECK(sync_turl);
   DCHECK(local_turl);
+  DCHECK(change_list);
 
   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));
+    change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
   }
 }