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

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 18f5d5585c522b8d9e45da6c5214531c89da3dc4..06b055ae9ef32bfb5929c4bf70e3681e4934a2fc 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -1344,15 +1344,18 @@ TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
   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) {