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

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"
 #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"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/google/google_url_tracker.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/history/history_service.h"
@@ skipping 598 matching lines @@
 
   // Always persist the setting in the database, that way if the backup
   // signature has changed out from under us it gets reset correctly.
   if (SetDefaultSearchProviderNoNotify(url))
     NotifyObservers();
 }
 
 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();
 }
 
 TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() {
   // See if the prepopulated default still exists.
   scoped_ptr<TemplateURL> prepopulated_default(
       TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(profile_));
   for (TemplateURLVector::iterator i = template_urls_.begin();
        i != template_urls_.end(); ++i) {
@@ skipping 667 matching lines @@
   data.date_created = base::Time::FromInternalValue(specifics.date_created());
   data.last_modified = base::Time::FromInternalValue(specifics.last_modified());
   data.prepopulate_id = specifics.prepopulate_id();
   data.sync_guid = specifics.sync_guid();
   data.alternate_urls.clear();
   for (int i = 0; i < specifics.alternate_urls_size(); ++i)
     data.alternate_urls.push_back(specifics.alternate_urls(i));
   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)
       turl->ResetKeywordIfNecessary(true);
     syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*turl);
     change_list->push_back(
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_UPDATE,
                            sync_data));
   } else if (turl->IsGoogleSearchURLWithReplaceableKeyword()) {
@@ skipping 420 matching lines @@
 
   string16 old_keyword(existing_turl->keyword());
   keyword_to_template_map_.erase(old_keyword);
   if (!existing_turl->sync_guid().empty())
     guid_to_template_map_.erase(existing_turl->sync_guid());
 
   provider_map_->Remove(existing_turl, old_search_terms_data);
   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);
 
   const string16& keyword = existing_turl->keyword();
   KeywordToTemplateMap::const_iterator i =
       keyword_to_template_map_.find(keyword);
   if (i == keyword_to_template_map_.end()) {
     keyword_to_template_map_[keyword] = existing_turl;
   } else {
     // We can theoretically reach here in two cases:
@@ skipping 29 matching lines @@
                            existing_turl,
                            syncer::SyncChange::ACTION_UPDATE);
 
   if (default_search_provider_ == existing_turl) {
     bool success = SetDefaultSearchProviderNoNotify(existing_turl);
     DCHECK(success);
   }
   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;
 }
 
 void TemplateURLService::UpdateKeywordSearchTermsForURL(
     const history::URLVisitedDetails& details) {
   const history::URLRow& row = details.row;
   if (!row.url().is_valid() ||
       !row.url().parsed_for_possibly_invalid_spec().query.is_nonempty()) {
     return;
@@ skipping 310 matching lines @@
   if (url)
     ProcessTemplateURLChange(FROM_HERE,
                              url,
                              syncer::SyncChange::ACTION_UPDATE);
   return true;
 }
 
 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(),
                      template_url) == template_urls_.end());
     template_url->data_.id = ++next_id_;
   }
 
   template_url->ResetKeywordIfNecessary(false);
   if (!template_url->IsExtensionKeyword()) {
     // Check whether |template_url|'s keyword conflicts with any already in the
@@ skipping 350 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());
     }
   }
 }