Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

components/search_engines/template_url_service.cc

Index: components/search_engines/template_url_service.cc
diff --git a/components/search_engines/template_url_service.cc b/components/search_engines/template_url_service.cc
index 0e77d27379f265169564fc3bb4cf7eff964c7a4f..0be22bc8a57730c5ae421fa844503681a4f823cf 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -678,6 +678,7 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
   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_) {
@@ -692,7 +693,16 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
     // 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();
+    }
   }
 }
 
@@ -1937,6 +1947,9 @@ bool TemplateURLService::ApplyDefaultSearchChangeNoMetrics(
         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) {
@@ -1987,11 +2000,8 @@ bool TemplateURLService::ApplyDefaultSearchChangeNoMetrics(
       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();