Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

components/search_engines/template_url_service.cc

 // Copyright 2014 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 "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 674 matching lines @@
     // notified of the new (fallback-provided) value in
     // ClearUserSelectedDefaultSearchEngine() above. Since we are here, the
     // value was presumably originally a fallback value (which may have been
     // repaired).
     DefaultSearchManager::Source source;
     const TemplateURLData* new_dse =
         default_search_manager_.GetDefaultSearchEngine(&source);
     // ApplyDefaultSearchChange will notify observers once it is done.
     ApplyDefaultSearchChange(new_dse, source);
   } else {
-    NotifyObservers();
+    // Set fallback engine as user selected, because repair is considered a user
+    // action and we are expected to sync new fallback engine to other devices.
+    TemplateURLData* fallback_engine_data =
+        default_search_manager_.GetFallbackSearchEngine();
+    if (fallback_engine_data) {
+      TemplateURL* fallback_engine =
+          FindPrepopulatedTemplateURL(fallback_engine_data->prepopulate_id);
+      if (fallback_engine) {
+        // Write fallback engine guid to sync pref. This will also set fallback
+        // engine as new user selected in
+        // OnSyncedDefaultSearchProviderGUIDChanged.
+        prefs_->SetString(prefs::kSyncedDefaultSearchProviderGUID,
+                          fallback_engine->sync_guid());

[vasilii, 2017/02/13 14:37:54]

Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine?
Currently you overwrite the GUID but will other DSE properties be synced too?

[Peter Kasting, 2017/02/13 22:24:57]

As the comment notes, that will happen automatically via
OnSyncedDefaultSearchProviderGUIDChanged().

However, I was going to ask the opposite -- why can't we just always write the
GUID here, instead of only doing it for prepopulated engines?  Non-prepopulated
engines should have a sync GUID too.

[Alexander Yashkin, 2017/02/14 12:04:43]

Yes, that was my first solution, yet luckily one of my added test started to
fail and I discovered, that sync_guid in
fallback_default_search_(TemplateURLData structure) stored in
DefaultSearchManager does not correspond to any TemplateURL stored in
TemplateURLService.

sync_guid field is automatically generated in TemplateURLData constructor. 
DefaultSearchManager creates its fallback_default_search_ structure calling
GetPrepopulatedDefaultSearch. It creates TemplateURLData with new sync_guid on
every browser start.

TemplateURLs stored in TemplateURLService are loaded from keywords DB and have
different sync_guids that are initialized once at keywords DB creation somewhere
in profile creation.

I think that looking for prepopulated_id will work in any scenario, especially
after engines are repaired. Yet I have added write to
SetUserSelectedDefaultSearchEngine if I am wrong and search for prepopulated_id
will sometimes fail.

[Peter Kasting, 2017/02/15 02:09:57]

This seems worrisome.  Does this mean that if I reset, then I'll sync up an
engine which is identical to some existing one in every way except it has a
different GUID?  Will this lead to the issue we've seen in the past, with
duplicate search engine entries (which then get deduped by appending "_" to the
keyword) proliferating?  Or will the sync system figure this out and delete one,
telling all clients to update to the other?
I think so too.  Presumably the repair will have installed the
prepopulated(/override) engines if they were missing, and overrides are required
to set an ID, so this should always succeed.  I think we should DCHECK that the
fallback engine is non-null and eliminate the second conditional arm.

[Alexander Yashkin, 2017/02/15 12:46:10]

I think this issue with sync is taken care of - see
UpdateTemplateURLIfPrepopulated.
If new engine will come from sync that has same prepopulated_id as local engine,
local engine sync_guid will be updated.
Done.

+      } else {
+        // Fallback engine can not be found by prepopulate_id. Write its data
+        // to user pref.
+        default_search_manager_.SetUserSelectedDefaultSearchEngine(
+            *fallback_engine_data);
+      }
+      NotifyObservers();
+    }
   }
 }
 
 void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) {
   model_observers_.AddObserver(observer);
 }
 
 void TemplateURLService::RemoveObserver(TemplateURLServiceObserver* observer) {
   model_observers_.RemoveObserver(observer);
 }
@@ skipping 1224 matching lines @@
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
   if (default_search_provider_source_ == DefaultSearchManager::FROM_POLICY ||
       source == DefaultSearchManager::FROM_POLICY) {
     // We do this both to remove any no-longer-applicable policy-defined DSE as
     // well as to add the new one, if appropriate.
     UpdateProvidersCreatedByPolicy(
         &template_urls_,
         source == DefaultSearchManager::FROM_POLICY ? data : nullptr);
   }
 
+  // |default_search_provider_source_| must be set before calling
+  // UpdateNoNotify(), since that function needs to know the source of the
+  // update in question.
+  default_search_provider_source_ = source;
+
   if (!data) {
     default_search_provider_ = nullptr;
   } else if (source == DefaultSearchManager::FROM_EXTENSION) {
     default_search_provider_ = FindMatchingExtensionTemplateURL(
         *data, TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION);
   } else if (source == DefaultSearchManager::FROM_FALLBACK) {
     default_search_provider_ =
         FindPrepopulatedTemplateURL(data->prepopulate_id);
     if (default_search_provider_) {
       TemplateURLData update_data(*data);
@@ skipping 30 matching lines @@
       std::unique_ptr<TemplateURL> new_dse_ptr =
           base::MakeUnique<TemplateURL>(new_data);
       TemplateURL* new_dse = new_dse_ptr.get();
       if (AddNoNotify(std::move(new_dse_ptr), true))
         default_search_provider_ = new_dse;
     }
     if (default_search_provider_ && prefs_) {
       prefs_->SetString(prefs::kSyncedDefaultSearchProviderGUID,
                         default_search_provider_->sync_guid());
     }
-
   }
 
-  default_search_provider_source_ = source;
-
   bool changed = default_search_provider_ != previous_default_search_engine;
   if (changed)
     RequestGoogleURLTrackerServerCheckIfNecessary();
 
   NotifyObservers();
 
   return changed;
 }
 
 TemplateURL* TemplateURLService::AddNoNotify(
@@ skipping 472 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }