Don't persist and sync omnibox extension keywords.

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 8d4d72a9fb5da2ae1a2cbf443ee3654c90a42a21..aafe5d98778b32c0d39ccfbdf8e7e5140d407bd8 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -664,7 +664,7 @@ TemplateURLService::TemplateURLVector TemplateURLService::GetTemplateURLs() {
 void TemplateURLService::IncrementUsageCount(TemplateURL* url) {
   DCHECK(url);
   // Extension-controlled search engines are not persisted.
-  if (url->GetType() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)
+  if (url->GetType() != TemplateURL::NORMAL)
     return;
   if (std::find(template_urls_.begin(), template_urls_.end(), url) ==
       template_urls_.end())
@@ -876,11 +876,19 @@ void TemplateURLService::OnWebDataServiceRequestDone(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "422460 TemplateURLService::OnWebDataServiceRequestDone 3"));
 
+    // crbug/411197. We don't store omnibox extensions anymore.

[Peter Kasting, 2014/10/29 18:55:51]

Nit: Don't refer to bug numbers in comments; just give the reader all the
context he needs right here.

Is this code going to become unnecessary at some point?  Should we be doing this
as a database version migration pass instead?

[vasilii, 2014/11/03 15:14:30]

It can be removed in the next version. Database migration happens when somebody
changes prepopulated_engines.json only.

[Peter Kasting, 2014/11/03 18:42:03]

Really?  What if someone doesn't run Chrome for a couple months or something?

[vasilii, 2014/11/04 18:22:30]

Then probably in 2 versions. The code is no longer needed if the user restarted
Chrome at least once.

[Peter Kasting, 2014/11/04 19:11:56]

Here's the situation I'm worried about:

* User has some persisted extension keywords.
* User doesn't use Chrome for a long time.
* User starts Chrome on a version where we've removed this code.

I worry that in this case, we try to load the old extension keywords, and other
code in this file breaks in weird ways because it assumes that these aren't
persisted.

We can deal with this worry in a couple ways:
(1) Leave this code in long enough that the scenario above is very unlikely. 
There won't be a specific time when we know it's impossible, however.
(2) Ensure that if such keywords do get loaded here, nothing bad actually
happens.  Then on shutdown the keywords hopefully won't be persisted back to
disk and we'll be OK.
(3) Instead of doing this here, do this as a database version change in the web
data code (this is different than a prepopulate data change).  This allows us to
introduce a migration pass that will be run once for anyone whose data files are
old, no matter what the source and target Chrome versions are.  It also allows
us to remove the pass someday and do so in a way that triggers an error message,
or recreating the file, if someone manages to run a new Chrome version with an
outdated file.

Above, you're basically suggesting (1).  I think (2) and (3) are safer, and (3)
would make me most comfortable because it's more future-proof, more performant
(we don't scan the URLs if we're not running the migration pass), and keeps code
related to behavioral changes about what data we store in a dedicated place in
the web data code.

Doing (3) would mostly involve moving this code to a new
MigrateToVersionXAndBlah() function in keyword_table.h alongside the migrations
already there.

WDYT?

[vasilii, 2014/11/05 18:04:50]

Implemented #3.

+    TemplateURLVector template_urls_no_extensions;
     // Restore extension info of loaded TemplateURLs.
     for (size_t i = 0; i < template_urls.size(); ++i) {
       DCHECK(!template_urls[i]->extension_info_);
       client_->RestoreExtensionInfoIfNecessary(template_urls[i]);
+      if (template_urls[i]->GetType() == TemplateURL::NORMAL) {

[Peter Kasting, 2014/10/29 18:55:51]

Nit: No {}

[vasilii, 2014/11/03 15:14:30]

I changed the code.

+        template_urls_no_extensions.push_back(template_urls[i]);
+      } else {
+        delete template_urls[i];
+      }
     }
+    template_urls.swap(template_urls_no_extensions);
   }
 
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
@@ -999,7 +1007,7 @@ syncer::SyncDataList TemplateURLService::GetAllSyncData(
     if ((*iter)->created_by_policy())
       continue;
     // We don't sync extension-controlled search engines.
-    if ((*iter)->GetType() == TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)
+    if ((*iter)->GetType() != TemplateURL::NORMAL)
       continue;
     current_data.push_back(CreateSyncDataFromTemplateURL(**iter));
   }
@@ -1032,15 +1040,15 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
   syncer::SyncChangeList new_changes;
   syncer::SyncError error;
   for (syncer::SyncChangeList::const_iterator iter = change_list.begin();
-      iter != change_list.end(); ++iter) {
+       iter != change_list.end(); ++iter) {
     DCHECK_EQ(syncer::SEARCH_ENGINES, iter->sync_data().GetDataType());
 
     std::string guid =
         iter->sync_data().GetSpecifics().search_engine().sync_guid();
     TemplateURL* existing_turl = GetTemplateURLForGUID(guid);
     scoped_ptr<TemplateURL> turl(CreateTemplateURLFromTemplateURLAndSyncData(
-        prefs_, search_terms_data(), existing_turl, iter->sync_data(),
-        &new_changes));
+        client_.get(), prefs_, search_terms_data(), existing_turl,
+        iter->sync_data(), &new_changes));
     if (!turl.get())
       continue;
 
@@ -1194,8 +1202,8 @@ syncer::SyncMergeResult TemplateURLService::MergeDataAndStartSyncing(
     TemplateURL* local_turl = GetTemplateURLForGUID(iter->first);
     scoped_ptr<TemplateURL> sync_turl(
         CreateTemplateURLFromTemplateURLAndSyncData(
-            prefs_, search_terms_data(), local_turl, iter->second,
-            &new_changes));
+            client_.get(), prefs_, search_terms_data(), local_turl,
+            iter->second, &new_changes));
     if (!sync_turl.get())
       continue;
 
@@ -1359,7 +1367,9 @@ syncer::SyncData TemplateURLService::CreateSyncDataFromTemplateURL(
 }
 
 // static
-TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
+scoped_ptr<TemplateURL>
+TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
+    TemplateURLServiceClient* client,
     PrefService* prefs,
     const SearchTermsData& search_terms_data,
     TemplateURL* existing_turl,
@@ -1424,12 +1434,24 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
     data.alternate_urls.push_back(specifics.alternate_urls(i));
   data.search_terms_replacement_key = specifics.search_terms_replacement_key();
 
-  TemplateURL* turl = new TemplateURL(data);
+  scoped_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, prefs);
-  DCHECK_NE(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION, turl->GetType());
+  UpdateTemplateURLIfPrepopulated(turl.get(), prefs);
+
+  // crbug/411197. The omnibox keywords aren't synced anymore.

[Peter Kasting, 2014/10/29 18:55:51]

Nit: See comments above

[vasilii, 2014/11/03 15:14:30]

Done.

+  DCHECK(client);
+  client->RestoreExtensionInfoIfNecessary(turl.get());
+  if (turl->GetType() == TemplateURL::OMNIBOX_API_EXTENSION) {
+    change_list->push_back(
+        syncer::SyncChange(FROM_HERE,
+                           syncer::SyncChange::ACTION_DELETE,

[Peter Kasting, 2014/10/29 18:55:51]

Will this mean if a user syncs a newer Chrome version, his older Chrome versions
will suddenly have their omnibox keywords stop working?

[vasilii, 2014/11/03 15:14:30]

Yeah, there is a race condition here. Don't we believe that Chrome updates
itself very fast?

[Peter Kasting, 2014/11/03 18:42:03]

People frequently have long-lived Chrome processes.  For example, my work
machine tends to have Chrome instances that run for weeks at a time without me
restarting them.

Assume someone has sync running between an old and a new version of Chrome. 
What will the consequences be?  Are the two going to continually fight each
other?  Will the older copy of Chrome just have these keywords break until it's
updated to a newer version, with no other effects?

I don't know if we need to be smarter here.  Maybe the sync guys would have
ideas.

[vasilii, 2014/11/04 18:22:30]

Let's leave them in the Sync model then but ignore them on the client side. We
can remove the keywords from Sync when we believe there are no old versions
running.

+                           sync_data));
+    return NULL;
+  }
+
+  DCHECK_EQ(TemplateURL::NORMAL, turl->GetType());
   if (reset_keyword || deduped) {
     if (reset_keyword)
       turl->ResetKeywordIfNecessary(search_terms_data, true);
@@ -1453,7 +1475,7 @@ TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
     }
   }
 
-  return turl;
+  return turl.Pass();
 }
 
 // static
@@ -1554,6 +1576,8 @@ void TemplateURLService::RemoveFromMaps(TemplateURL* template_url) {
       keyword_to_template_map_.erase(keyword);
   }
 
+  if (template_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION)
+    return;

[Peter Kasting, 2014/10/29 18:55:51]

Nit: Should maybe have a blank line below this

[vasilii, 2014/11/03 15:14:30]

Done.

   if (!template_url->sync_guid().empty())
     guid_to_template_map_.erase(template_url->sync_guid());
   // |provider_map_| is only initialized after loading has completed.
@@ -1584,6 +1608,8 @@ void TemplateURLService::AddToMaps(TemplateURL* template_url) {
       keyword_to_template_map_[keyword] = template_url;
   }
 
+  if (template_url_is_omnibox_api)
+    return;

[Peter Kasting, 2014/10/29 18:55:51]

Nit: Should maybe have a blank line below this

[vasilii, 2014/11/03 15:14:30]

Done.

   if (!template_url->sync_guid().empty())
     guid_to_template_map_[template_url->sync_guid()] = template_url;
   // |provider_map_| is only initialized after loading has completed.
@@ -1684,6 +1710,8 @@ bool TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl,
       template_urls_.end())
     return false;
 
+  DCHECK(existing_turl->GetType() != TemplateURL::OMNIBOX_API_EXTENSION);

[Peter Kasting, 2014/10/29 18:55:51]

Nit: DCHECK_NE

[vasilii, 2014/11/03 15:14:30]

Done.

+
   base::string16 old_keyword(existing_turl->keyword());
   keyword_to_template_map_.erase(old_keyword);
   if (!existing_turl->sync_guid().empty())
@@ -2024,7 +2052,9 @@ bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
   // that any "pre-existing" entries we find are actually coming from
   // |template_urls_|, lest we detect a "conflict" between the
   // |initial_default_search_provider_| and the web data version of itself.
-  if (existing_keyword_turl &&
+  if (template_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION &&
+      existing_keyword_turl &&
+      existing_keyword_turl->GetType() != TemplateURL::OMNIBOX_API_EXTENSION &&
       (std::find(template_urls_.begin(), template_urls_.end(),
                  existing_keyword_turl) != template_urls_.end())) {
     DCHECK_NE(existing_keyword_turl, template_url);
@@ -2049,8 +2079,7 @@ bool TemplateURLService::AddNoNotify(TemplateURL* template_url,
   AddToMaps(template_url);
 
   if (newly_adding &&
-      (template_url->GetType() !=
-          TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)) {
+      (template_url->GetType() == TemplateURL::NORMAL)) {
     if (web_data_service_.get())
       web_data_service_->AddKeyword(template_url->data());
 
@@ -2077,7 +2106,7 @@ void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
   // Remove it from the vector containing all TemplateURLs.
   template_urls_.erase(i);
 
-  if (template_url->GetType() != TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION) {
+  if (template_url->GetType() == TemplateURL::NORMAL) {
     if (web_data_service_.get())
       web_data_service_->RemoveKeyword(template_url->id());
 
@@ -2235,8 +2264,7 @@ void TemplateURLService::ResolveSyncKeywordConflict(
   DCHECK(applied_sync_turl);
   DCHECK(change_list);
   DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
-  DCHECK_NE(TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION,
-            applied_sync_turl->GetType());
+  DCHECK_EQ(TemplateURL::NORMAL, applied_sync_turl->GetType());
 
   // Both |unapplied_sync_turl| and |applied_sync_turl| are known to Sync, so
   // don't delete either of them. Instead, determine which is "better" and
@@ -2347,8 +2375,7 @@ void TemplateURLService::PatchMissingSyncGUIDs(
     TemplateURL* template_url = *i;
     DCHECK(template_url);
     if (template_url->sync_guid().empty() &&
-        (template_url->GetType() !=
-            TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION)) {
+        (template_url->GetType() == TemplateURL::NORMAL)) {
       template_url->data_.sync_guid = base::GenerateGUID();
       if (web_data_service_.get())
         web_data_service_->UpdateKeyword(template_url->data());