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);
   } else {
-    NotifyObservers();
+    if (old_source == DefaultSearchManager::FROM_USER &&

[Peter Kasting, 2017/02/02 00:04:26]

Nit: Hoist this up to the parent conditional:

  if (...) {
  } else if (...) {
  } else {

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

Done

+        default_search_provider_source_ ==
+            DefaultSearchManager::FROM_FALLBACK) {

[Peter Kasting, 2017/02/02 00:04:26]

If the old source was FROM_USER, will the new source always be FROM_FALLBACK? 
Or can it fall back to extension/policy sometimes?

Separately, it'd be great if the Source enum actually documented exactly what
each of these FROM_XXX values means :P

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

I have kept the old logic here - fallback engine was written to user selected
pref only if transferring FROM_USER -> FROM_FALLBACK
At my current level of understanding, I think from FROM_USER we could only get
to FROM_FALLBACK state.

Yet, after encountering previous reset engine problem, I am a little bit gunshy
of bold changes :)
Done

[Peter Kasting, 2017/02/05 05:09:58]

I guess the question is whether a user choice can override the default engine
request of an extension or policy to begin with.  If someone installs an
extension that sets the DSE, can they override that without uninstalling the
extension?  If yes, can you test whether, in this case, we reach here with
default_search_provider_source_ == FROM_EXTENSION?  If so, we need to keep your
conditional as you wrote it.

(I'm asking because I'd rather not have the code have conditionals that are
really always true, as it results in a lot of head-scratching later trying to
figure out what's possible.)

[Alexander Yashkin, 2017/02/05 13:57:04]

I don't think that we can get from FROM_USER to any other choice than
FROM_FALLBACK. I could not think of such scenario.
If extension overriding DSE is installed user could not override it manually. 
DefaultSearchManager returns current default engine by choosing first policy if
exists, than extension, than user, and fallback.

So I don't see how we can get from FROM_USER to extension/policy.
I will delete the check for FROM_FALLBACK.

+      // Set fallback engine as user selected, because repair is considered user

[Peter Kasting, 2017/02/02 00:04:26]

Nit: user -> a user

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

Done

+      // action and new engine is expected to sync to other user devices.

[Peter Kasting, 2017/02/02 00:04:26]

Nit: new -> the new; eliminate "user"

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

arghh, this articles, done.

+      default_search_manager_.SetUserSelectedDefaultSearchEngine(
+          default_search_provider_->data());
+    } else {
+      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);
   }
 
+  // Set new DSE source as current before calling UpdateNoNotify below which
+  // is checking it to update user selected engine.

[Peter Kasting, 2017/02/02 00:04:26]

Nit: Maybe "|default_search_provider_source_| must be set before calling
UpdateNoNotify(), since that function needs to know the source of the update in
question."

I'd also stick a blank line after this assignment.

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

Done

+  default_search_provider_source_ = source;

[Alexander Yashkin, 2017/01/30 11:48:32]

Problem in https://bugs.chromium.org/p/chromium/issues/detail?id=680197
arise due to default_search_provider_source_ field being set after
default_search_provider_ and all other engine manipulation.

So for example when switching from USER to FALLBACK state (called from
RepairPrepopulatedEngines)
1. ApplyDefaultSearchChangeNoMetrics is called with FALLBACK but current value
of default_search_provider_source_ was still USER.

2. At the processing code for new FALLBACK state we call UpdateNoNotify which
have check at the end:
  if (default_search_provider_ == existing_turl &&
      default_search_provider_source_ == DefaultSearchManager::FROM_USER) {
    default_search_manager_.SetUserSelectedDefaultSearchEngine(
        default_search_provider_->data());
  }
And this check was succesfull because default_search_provider_source_ was still
FROM_USER.
And this code called SetUserSelectedDefaultSearchEngine which changed default
search to user again and wrote fallback engine to user pref.

3. At the finish of RepairPrepopulatedEngines we had FALLBACK engine set as
current source in TemplateURLService but fallback engine was also written to
user pref and SyncGuid and on next browser start TemplateURLService is loaded as
with user selected engine.


As a solution I fixed current default_search_provider_source_ value assignment
and added explicit set of user selected engine in RepairPrepopulatedEngines.

   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();
   }
 }