Delete from Sync the artificial search engines created by the omnibox extensions.

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 1326 matching lines @@
     data.alternate_urls.push_back(specifics.alternate_urls(i));
   data.search_terms_replacement_key = specifics.search_terms_replacement_key();
 
   std::unique_ptr<TemplateURL> turl(new TemplateURL(data));
   // If this TemplateURL matches a built-in prepopulated template URL, it's
   // possible that sync is trying to modify fields that should not be touched.
   // Revert these fields to the built-in values.
   UpdateTemplateURLIfPrepopulated(turl.get(), prefs);
 
   // We used to sync keywords associated with omnibox extensions, but no longer
-  // want to.  However, if we delete these keywords from sync, we'll break any
-  // synced old versions of Chrome which were relying on them.  Instead, for now
-  // we simply ignore these.
-  // TODO(vasilii): After a few Chrome versions, change this to go ahead and
-  // delete these from sync.
+  // want to.  Delete them from the server.
+  // TODO(vasilii): After a few Chrome versions, delete this code together with
+  // RestoreExtensionInfoIfNecessary().

[Peter Kasting, 2016/09/22 19:01:44]

Just to make sure I understand: is RestoreExtensionInfoIfNecessary() still
needed today because, since these are still on the server, they are still
getting pulled down and the type/associated extension info incorrectly set (to
not OMNIBOX_API_EXTENSION), so if we were to just delete that function,
everybody's local data would get screwed up?

Does template_url_service_sync_unittest.cc still need a full implementation of
this method?  I'm wondering if that or any associated test code can/should go
away.

[vasilii, 2016/09/28 10:00:11]

Until few days ago there was no |type_| in the TemplateURL. It was deduced from
the format of the URL. If we drop RestoreExtensionInfoIfNecessary() then those
entries will become just normal search engines.
I cleaned the test.

[Peter Kasting, 2016/09/28 19:34:34]

What I'm thinking is, we don't actually need RestoreExtensionInfoIfNecessary();
we just need to detect the things it would detect.  I.e. we can change the code
here to something like:

  if (GURL(data.url()).SchemeIs(extensions::kExtensionScheme)) {
    ... delete entry and return null

This would avoid calling set_type() on the underlying TemplateURL, which is the
thing we're trying to get rid of.

I was trying to falsify this by looking for a way in which doing this leaves the
local data store corrupted in a way the current code does not, but since |turl|
is a local object, I think we're safe there.

[vasilii, 2016/09/29 11:48:06]

We could but it's a layering violation. The component shouldn't know about the
extensions.

   DCHECK(client);
   client->RestoreExtensionInfoIfNecessary(turl.get());
-  if (turl->GetType() == TemplateURL::OMNIBOX_API_EXTENSION)
+  if (turl->GetType() == TemplateURL::OMNIBOX_API_EXTENSION) {
+    change_list->push_back(
+        syncer::SyncChange(FROM_HERE,
+                           syncer::SyncChange::ACTION_DELETE,
+                           sync_data));
     return nullptr;
+  }
 
   DCHECK_EQ(TemplateURL::NORMAL, turl->GetType());
   if (reset_keyword || deduped) {
     if (reset_keyword)
       turl->ResetKeywordIfNecessary(search_terms_data, true);
     syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*turl);
     change_list->push_back(syncer::SyncChange(FROM_HERE,
                                               syncer::SyncChange::ACTION_UPDATE,
                                               sync_data));
   } else if (turl->IsGoogleSearchURLWithReplaceableKeyword(search_terms_data)) {
@@ skipping 1099 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();
   }
 }