Ensure post-sync TemplateURL of prepopulated engines use built-in version.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index bd754ec4a885599c28d50b5fdd76ac2e102f1a66..fedde5c444ae1f152d38d4e9308f1e36d01555d0 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -10,6 +10,7 @@
 #include "base/environment.h"
 #include "base/guid.h"
 #include "base/i18n/case_conversion.h"
+#include "base/memory/scoped_vector.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
@@ -628,7 +629,6 @@ void TemplateURLService::SetDefaultSearchProvider(TemplateURL* url) {
 TemplateURL* TemplateURLService::GetDefaultSearchProvider() {
   if (loaded_ && !load_failed_)
     return default_search_provider_;
-
   // We're not loaded, rely on the default search provider stored in prefs.
   return initial_default_search_provider_.get();
 }
@@ -1316,6 +1316,10 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
   data.search_terms_replacement_key = specifics.search_terms_replacement_key();
 
   TemplateURL* turl = new TemplateURL(profile, data);
+  // If this TemplateURL matches a built-in prepopulated template URL, it's
+  // possible that sync is trying to modify fields that should not be touched.
+  // Revert these fields to the built-in values.
+  UpdateTemplateURLIfPrepopulated(turl, profile);

[Peter Kasting, 2013/02/04 20:35:32]

Could this result in telling sync to go update these values to something else
for this entry on other clients?

If so, we could have endless fighting between two clients with different
prepopulate versions.

[SteveT, 2013/02/04 21:05:44]

At first glance, I don't believe this would be the case. What happens is that
the local TURL gets its URL updated/repaired, but it doesn't actually sync back
up to the server.

From the client's point of view, it looks like the server sent down a TURL which
already has the repaired URL.

However, this does mean that the client is momentarily out of sync with the
server (the client has an updated URL and the server doesn't know about it..
yet). It seems to me like the correct thing to do here is that, in the case of
upgrading a client's TURL, we want to send that new state back up to the server
via an ACTION_UPDATE.

However, the one issue here is that older clients synced to this account will
receive the updated URL, which it may not know how to handle. However, it sounds
like the TemplateURLService code knows what to do in this case (drop the
parameter entirely).

What's your take on this problem, here? I believe we do support such cases (not
necessarily the case of syncing between say Dev and Stable, but syncing between
one client that has updated recently, and one that has not).

[Peter Kasting, 2013/02/04 21:40:08]

No, this is the very case I'm worried about.  If you were to send this back to
the server, it would then send that to the other clients, including the one who
triggered the original update here.  _That_ client must have differing
prepopulate data (in order for it to have sent said original update at all), and
the new update would run through this code, be reset to the existing local URL,
get bounced back to the server as an ACTION_UPDATE, repeat forever (well, until
both clients update to the same prepopulate version).

I think any changes we cause due to this new function must not get mirrored to
the server.  If they are later sent to the server because e.g. the user
subsequently edits the entry's keyword, that's fine; as long as other clients
obey this "don't auto-mirror updates due to enforcing the prepopulate data"
invariant, we won't chain-reaction.

   DCHECK(!turl->IsExtensionKeyword());
   if (reset_keyword || deduped) {
     if (reset_keyword)
@@ -1756,6 +1760,7 @@ bool TemplateURLService::UpdateNoNotify(
   TemplateURLID previous_id = existing_turl->id();
   existing_turl->CopyFrom(new_values);
   existing_turl->data_.id = previous_id;
+

[Peter Kasting, 2013/02/04 20:35:32]

Why add this?

[beaudoin, 2013/02/04 22:33:40]

Unfortunate back-and-forth editing mistake.

Done.

   UIThreadSearchTermsData new_search_terms_data(profile_);
   provider_map_->Add(existing_turl, new_search_terms_data);
 
@@ -1805,6 +1810,28 @@ bool TemplateURLService::UpdateNoNotify(
   return true;
 }
 
+// static
+void TemplateURLService::UpdateTemplateURLIfPrepopulated(
+    TemplateURL* template_url,
+    Profile* profile) {
+  int prepopulate_id = template_url->prepopulate_id();
+  if (template_url->prepopulate_id() == 0)
+    return;
+
+  ScopedVector<TemplateURL> prepopulated_urls;
+  size_t default_search_index;
+  TemplateURLPrepopulateData::GetPrepopulatedEngines(profile,
+      &prepopulated_urls.get(), &default_search_index);
+
+  for (size_t i = 0; i < prepopulated_urls.size(); ++i) {
+    if (prepopulated_urls[i]->prepopulate_id() == prepopulate_id) {
+      MergeIntoPrepopulatedEngineData(&prepopulated_urls[i]->data_,
+                                      template_url);
+      template_url->CopyFrom(*prepopulated_urls[i]);
+    }
+  }
+}
+
 PrefService* TemplateURLService::GetPrefs() {
   return profile_ ? profile_->GetPrefs() : NULL;
 }
@@ -2135,7 +2162,6 @@ bool TemplateURLService::SetDefaultSearchProviderNoNotify(TemplateURL* url) {
 bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
                                      bool newly_adding) {
   DCHECK(template_url);
-
   if (newly_adding) {
     DCHECK_EQ(kInvalidTemplateURLID, template_url->id());
     DCHECK(std::find(template_urls_.begin(), template_urls_.end(),