Don't persist and sync omnibox extension keywords.

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 646 matching lines @@
   AddExtensionControlledTURL(url, info.Pass());
 }
 
 TemplateURLService::TemplateURLVector TemplateURLService::GetTemplateURLs() {
   return template_urls_;
 }
 
 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())
     return;
   ++url->data_.usage_count;
 
   if (web_data_service_.get())
     web_data_service_->UpdateKeyword(url->data());
 }
 
@@ skipping 191 matching lines @@
         &new_resource_keyword_version,
         &pre_sync_deletes_);
   }
 
   if (client_) {
     // TODO(vadimt): Remove ScopedProfile below once crbug.com/422460 is fixed.
     tracked_objects::ScopedProfile tracking_profile3(
         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());
 
   {
     // TODO(vadimt): Remove ScopedProfile below once crbug.com/422460 is fixed.
     tracked_objects::ScopedProfile tracking_profile4(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "422460 TemplateURLService::OnWebDataServiceRequestDone 4"));
 
@@ skipping 98 matching lines @@
     syncer::ModelType type) const {
   DCHECK_EQ(syncer::SEARCH_ENGINES, type);
 
   syncer::SyncDataList current_data;
   for (TemplateURLVector::const_iterator iter = template_urls_.begin();
       iter != template_urls_.end(); ++iter) {
     // We don't sync keywords managed by policy.
     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));
   }
 
   return current_data;
 }
 
 syncer::SyncError TemplateURLService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
@@ skipping 12 matching lines @@
   // As we move through Sync Code, we may set this to increasingly specific
   // origins so we can tell what exactly caused a DSP change.
   base::AutoReset<DefaultSearchChangeOrigin> change_origin(&dsp_change_origin_,
       DSP_CHANGE_SYNC_UNINTENTIONAL);
 
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
 
   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;
 
     // Explicitly don't check for conflicts against extension keywords; in this
     // case the functions which modify the keyword map know how to handle the
     // conflicts.
     // TODO(mpcomplete): If we allow editing extension keywords, then those will
     // need to undergo conflict resolution.
     TemplateURL* existing_keyword_turl =
         FindNonExtensionTemplateURLForKeyword(turl->keyword());
@@ skipping 133 matching lines @@
   SyncDataMap sync_data_map = CreateGUIDToSyncDataMap(initial_sync_data);
 
   KeywordWebDataService::BatchModeScoper scoper(web_data_service_.get());
 
   merge_result.set_num_items_before_association(local_data_map.size());
   for (SyncDataMap::const_iterator iter = sync_data_map.begin();
       iter != sync_data_map.end(); ++iter) {
     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;
 
     if (pre_sync_deletes_.find(sync_turl->sync_guid()) !=
         pre_sync_deletes_.end()) {
       // This entry was deleted before the initial sync began (possibly through
       // preprocessing in TemplateURLService's loading code). Ignore it and send
       // an ACTION_DELETE up to the server.
       new_changes.push_back(
           syncer::SyncChange(FROM_HERE,
@@ skipping 143 matching lines @@
     se_specifics->add_alternate_urls(turl.alternate_urls()[i]);
   se_specifics->set_search_terms_replacement_key(
       turl.search_terms_replacement_key());
 
   return syncer::SyncData::CreateLocalData(se_specifics->sync_guid(),
                                            se_specifics->keyword(),
                                            specifics);
 }
 
 // static
-TemplateURL* TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
+scoped_ptr<TemplateURL>
+TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
+    TemplateURLServiceClient* client,
     PrefService* prefs,
     const SearchTermsData& search_terms_data,
     TemplateURL* existing_turl,
     const syncer::SyncData& sync_data,
     syncer::SyncChangeList* change_list) {
   DCHECK(change_list);
 
   sync_pb::SearchEngineSpecifics specifics =
       sync_data.GetSpecifics().search_engine();
 
@@ skipping 44 matching lines @@
   bool deduped = DeDupeEncodings(&data.input_encodings);
   data.date_created = base::Time::FromInternalValue(specifics.date_created());
   data.last_modified = base::Time::FromInternalValue(specifics.last_modified());
   data.prepopulate_id = specifics.prepopulate_id();
   data.sync_guid = specifics.sync_guid();
   data.alternate_urls.clear();
   for (int i = 0; i < specifics.alternate_urls_size(); ++i)
     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);
     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)) {
     if (!existing_turl) {
       // We're adding a new TemplateURL that uses the Google base URL, so set
       // its keyword appropriately for the local environment.
       turl->ResetKeywordIfNecessary(search_terms_data, false);
     } else if (existing_turl->IsGoogleSearchURLWithReplaceableKeyword(
         search_terms_data)) {
       // Ignore keyword changes triggered by the Google base URL changing on
       // another client.  If the base URL changes in this client as well, we'll
       // pick that up separately at the appropriate time.  Otherwise, changing
       // the keyword here could result in having the wrong keyword for the local
       // environment.
       turl->data_.SetKeyword(existing_turl->keyword());
     }
   }
 
-  return turl;
+  return turl.Pass();
 }
 
 // static
 SyncDataMap TemplateURLService::CreateGUIDToSyncDataMap(
     const syncer::SyncDataList& sync_data) {
   SyncDataMap data_map;
   for (syncer::SyncDataList::const_iterator i(sync_data.begin());
        i != sync_data.end();
        ++i)
     data_map[i->GetSpecifics().search_engine().sync_guid()] = *i;
@@ skipping 80 matching lines @@
            ((turl->GetType() == TemplateURL::OMNIBOX_API_EXTENSION) &&
             (turl->id() > best_fallback->id()))))
         best_fallback = turl;
     }
     if (best_fallback)
       keyword_to_template_map_[keyword] = best_fallback;
     else
       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.
   if (loaded_) {
     provider_map_->Remove(template_url);
   }
 }
 
 void TemplateURLService::AddToMaps(TemplateURL* template_url) {
   bool template_url_is_omnibox_api =
@@ skipping 10 matching lines @@
     //   Manually-modified keywords > extension keywords > replaceable keywords
     // When there are multiple extensions, the last-added wins.
     bool existing_url_is_omnibox_api =
         existing_url->GetType() == TemplateURL::OMNIBOX_API_EXTENSION;
     DCHECK(existing_url_is_omnibox_api || template_url_is_omnibox_api);
     if (existing_url_is_omnibox_api ?
         !CanReplace(template_url) : CanReplace(existing_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.
   if (loaded_)
     provider_map_->Add(template_url, search_terms_data());
 }
 
 // Helper for partition() call in next function.
 bool HasValidID(TemplateURL* t_url) {
   return t_url->id() != kInvalidTemplateURLID;
@@ skipping 80 matching lines @@
   return NULL;
 }
 
 bool TemplateURLService::UpdateNoNotify(TemplateURL* existing_turl,
                                         const TemplateURL& new_values) {
   DCHECK(existing_turl);
   if (std::find(template_urls_.begin(), template_urls_.end(), 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())
     guid_to_template_map_.erase(existing_turl->sync_guid());
 
   // |provider_map_| is only initialized after loading has completed.
   if (loaded_)
     provider_map_->Remove(existing_turl);
 
   TemplateURLID previous_id = existing_turl->id();
@@ skipping 320 matching lines @@
 
   // Check whether |template_url|'s keyword conflicts with any already in the
   // model.  Note that we can reach here during the loading phase while
   // processing the template URLs from the web data service.  In this case,
   // GetTemplateURLForKeyword() will look not only at what's already in the
   // model, but at the |initial_default_search_provider_|.  Since this engine
   // will presumably also be present in the web data, we need to double-check
   // 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);
     // Only replace one of the TemplateURLs if they are either both extensions,
     // or both not extensions.
     bool are_same_type = existing_keyword_turl->GetType() ==
         template_url->GetType();
     if (CanReplace(existing_keyword_turl) && are_same_type) {
       RemoveNoNotify(existing_keyword_turl);
     } else if (CanReplace(template_url) && are_same_type) {
       delete template_url;
       return false;
     } else {
       base::string16 new_keyword =
           UniquifyKeyword(*existing_keyword_turl, false);
       ResetTemplateURLNoNotify(existing_keyword_turl,
                                existing_keyword_turl->short_name(), new_keyword,
                                existing_keyword_turl->url());
     }
   }
   template_urls_.push_back(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());
 
     // Inform sync of the addition. Note that this will assign a GUID to
     // template_url and add it to the guid_to_template_map_.
     ProcessTemplateURLChange(FROM_HERE,
                              template_url,
                              syncer::SyncChange::ACTION_ADD);
   }
 
   return true;
 }
 
 void TemplateURLService::RemoveNoNotify(TemplateURL* template_url) {
   DCHECK(template_url != default_search_provider_);
 
   TemplateURLVector::iterator i =
       std::find(template_urls_.begin(), template_urls_.end(), template_url);
   if (i == template_urls_.end())
     return;
 
   RemoveFromMaps(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());
 
     // Inform sync of the deletion.
     ProcessTemplateURLChange(FROM_HERE,
                              template_url,
                              syncer::SyncChange::ACTION_DELETE);
 
     UMA_HISTOGRAM_ENUMERATION(kDeleteSyncedEngineHistogramName,
                               DELETE_ENGINE_USER_ACTION, DELETE_ENGINE_MAX);
@@ skipping 137 matching lines @@
 
 void TemplateURLService::ResolveSyncKeywordConflict(
     TemplateURL* unapplied_sync_turl,
     TemplateURL* applied_sync_turl,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
   DCHECK(unapplied_sync_turl);
   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
   // uniquify the other one, sending an update to the server for the updated
   // entry.
   const bool applied_turl_is_better =
       IsLocalTemplateURLBetter(applied_sync_turl, unapplied_sync_turl);
   TemplateURL* loser = applied_turl_is_better ?
       unapplied_sync_turl : applied_sync_turl;
   base::string16 new_keyword = UniquifyKeyword(*loser, false);
@@ skipping 90 matching lines @@
 }
 
 void TemplateURLService::PatchMissingSyncGUIDs(
     TemplateURLVector* template_urls) {
   DCHECK(template_urls);
   for (TemplateURLVector::iterator i = template_urls->begin();
        i != template_urls->end(); ++i) {
     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());
     }
   }
 }
 
 void TemplateURLService::OnSyncedDefaultSearchProviderGUIDChanged() {
   base::AutoReset<DefaultSearchChangeOrigin> change_origin(
       &dsp_change_origin_, DSP_CHANGE_SYNC_PREF);
@@ skipping 61 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();
   }
 }