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

[vasilii, 2017/02/07 14:45:37]

If the reproduction steps are:
- set the DSE to Bling
- reset the settings
then why don't we land in this block? How is it possible to end up here?

I'm asking because the current behavior looks to me like this:
- default_search_manager_.ClearUserSelectedDefaultSearchEngine does a hell of
callbacks and something isn't done right there.
- we are trying to patch the flaw by calling SetUserSelectedDefaultSearchEngine
again.

What is happening if there is an extension overriding the DSE (on top of the
reproduction steps)? The control won't reach the new block in #698. However, the
syncing DSE should still be Google and not Bling.

[Alexander Yashkin, 2017/02/07 16:26:23]

We can land here only if previous DSE was fallback and new DSE is fallback also.
DefaultSearchManager does not notify its observers on change from fallback to
fallback, because this means nothing changed. 
In this case we reset default_search_provider_ at function start and it remains
nullptr because DefaultSearchManager did not notify TemplateURLService.
You can see details in  ClearUserSelectedDefaultSearchEngine and
OnDefaultSearchPrefChanged of DefaultSearchManager.

This seems a little bit too much knowing of DefaultSearchManager internals here,
yet here we are.
I see it in different way - in old behaviour,
default_search_manager_.ClearUserSelectedDefaultSearchEngine call from
RepairPrepopulatedSearchEngines called too many callbacks of TemplateURLService
only as consequence of two bugs in TemplateURL and in
ApplyDefaultSearchChangeNoMetrics. 

Only by accident it set user selected engine pref and sync guid pref and only
for google engine and only when changing from user DSE to fallback.
ClearUserSelectedDefaultSearchEngine should do what it was made for - reset user
engine and choose DSE according to its rules - fallback engine if no
user/extension/policy engine is specified.
I would say that RepairPrepopulatedEngines is special case of user engine reset
- its called by user expicitly and is expected to propagate to sync.
See comments in Reset bug: "e.g. if we set 'Show Home button' in preferences,
that preference syncs. If we then click 'Reset settings', the 'Show home button'
preference is reset to false and the new value is synced to other browsers /
devices."

So I have added an explicit call to SetUserSelectedDefaultSearchEngine in Repair
and only when changing from user to fallback to keep previously implemented
behaviour.
I tried to minimize impact of my changes and kept previous logic here.
I 'll try to check tomorrow how it worked.

     // 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 if (old_source == DefaultSearchManager::FROM_USER) {

[vasilii, 2017/02/07 17:12:56]

Your reasoning sounds good. It seems that we should call
SetUserSelectedDefaultSearchEngine unconditionally. I don't understand why
|old_source| matters here.

+    // We expect that if old default source was FROM_USER we could only get to
+    // fallback engine source after ClearUserSelectedDefaultSearchEngine.
+    DCHECK(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();
   }
 }