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 660 matching lines @@
   for (std::vector<TemplateURLData>::const_iterator i =
            actions.added_engines.begin();
        i < actions.added_engines.end();
        ++i) {
     AddNoNotify(base::MakeUnique<TemplateURL>(*i), true);
   }
 
   base::AutoReset<DefaultSearchChangeOrigin> change_origin(
       &dsp_change_origin_, DSP_CHANGE_PROFILE_RESET);
 
+  DefaultSearchManager::Source old_source = default_search_provider_source_;
   default_search_manager_.ClearUserSelectedDefaultSearchEngine();
 
   if (!default_search_provider_) {
     // If the default search provider came from a user pref we would have been
     // 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);

[vasilii, 2017/02/03 13:13:57]

I can't correlate your changes with
https://bugs.chromium.org/p/chromium/issues/detail?id=680197#c24
You claim that the engine isn't overwritten because MatchesData now returns
true. But you won't move to the "else if" block you added in #695. What was
happening and what should happen?

[Alexander Yashkin, 2017/02/03 14:44:26]

I object :)

My original claim was - 
"As a result default search changes from RepairPrepopulatedSearchEngines call
are not propagated to sync subsystem.
So the previous default search selected by user is returned after next sync
attempt."

I did not claim that engine wasn't overwritten. It was, so
default_search_provider_ was equal to new engine chosen from fallback.

By propagating to sync subsystem I mean that new engine sync_guid
(FROM_FALLBACK) was not written to kSyncedDefaultSearchProviderGUID pref. And it
had old value from previously user selected engine and that means that on next
sync attempt it was restored.

I had added debug log, reverted my fixes and run my added unittest
TemplateURLServiceTest.RepairPrepopulatedEnginesUpdatesSyncGuid 
so you could see the sequence of events, they are all in one callstack from
RepairPrepopulatedSearchEngines.
You could do the same to check, sequence of calls is rather complicated.

[template_url_service.cc(639)] RepairPrepopulatedSearchEngines 
[template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER
[template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER
[template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER
[default_search_manager.cc(188)]
DefaultSearchManager::ClearUserSelectedDefaultSearchEngine 
[template_url_service.cc(1899)] OnDefaultSearchChange 
[template_url_service.cc(1927)] ApplyDefaultSearchChangeNoMetrics 
[template_url_service.cc(1928)]  current source = FROM_USER new source
FROM_FALLBACK
[template_url_service.cc(1944)]  current keyword = NULL DEFAULT ENGINE new
keyword google.com
[template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER
[default_search_manager.cc(159)]
DefaultSearchManager::SetUserSelectedDefaultSearchEngine new keyword =
google.com
[template_url_service.cc(1899)] OnDefaultSearchChange 
[template_url_service.cc(1927)] ApplyDefaultSearchChangeNoMetrics 
[template_url_service.cc(1928)]  current source = FROM_USER new source FROM_USER
[template_url_service.cc(1943)]  current keyword = google.com new keyword
google.com
[template_url.cc(1224)] TemplateURL::MatchesData google.com vs google.com, espv
vs {google:instantExtendedEnabledKey}, returns false
[template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER
[default_search_manager.cc(159)]
DefaultSearchManager::SetUserSelectedDefaultSearchEngine 
[default_search_manager.cc(160)]  new keyword = google.com
[template_url_service.cc(2027)]  write to kSyncedDefaultSearchProviderGUID
google.com
[default_search_manager.cc(159)]
DefaultSearchManager::SetUserSelectedDefaultSearchEngine 
[default_search_manager.cc(160)]  new keyword = google.com

Where is one call to MatchesData in this callstack and it returns false because
its params are default_search_provider_ - TemplateUrl which contains google
engine with already substituted search_terms_replacement_key 
and another is TemplateURLData for google engine

So this MatchesData returns false and statements after it performs write of
google engine sync_guid to kSyncedDefaultSearchProviderGUID

+  } else if (old_source == DefaultSearchManager::FROM_USER &&
+             default_search_provider_source_ ==
+                 DefaultSearchManager::FROM_FALLBACK) {
+    // Set fallback engine as user selected, because repair is considered a user
+    // action and the new engine is expected to sync to other devices.
+    default_search_manager_.SetUserSelectedDefaultSearchEngine(
+        default_search_provider_->data());
   } else {
     NotifyObservers();
   }
 }
 
 void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) {
   model_observers_.AddObserver(observer);
 }
 
 void TemplateURLService::RemoveObserver(TemplateURLServiceObserver* observer) {
@@ skipping 1226 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();
   }
 }