Remove the "autogenerate keyword" bit on TemplateURL.

chrome/browser/search_engines/template_url_service.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 #define CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_
 #pragma once
 
 #include <list>
 #include <map>
@@ skipping 65 matching lines @@
     const char* const keyword;
     const char* const url;
     const char* const content;
   };
 
   explicit TemplateURLService(Profile* profile);
   // The following is for testing.
   TemplateURLService(const Initializer* initializers, const int count);
   virtual ~TemplateURLService();
 
-  // Generates a suitable keyword for the specified url.  Returns an empty
-  // string if a keyword couldn't be generated.  If |autodetected| is true, we
-  // don't generate keywords for a variety of situations where we would probably
-  // not want to auto-add keywords, such as keywords for searches on pages that
-  // themselves come from form submissions.
-  static string16 GenerateKeyword(const GURL& url, bool autodetected);
+  // Generates a suitable keyword for the specified url, which must be valid.
+  // This is guaranteed not to return an empty string, since TemplateURLs should
+  // never have an empty keyword.
+  static string16 GenerateKeyword(const GURL& url);

[sky, 2012/05/04 23:20:25]

Can you add a comment to this and GenerateSearchURLUsingTermsData that they are
invoked from non-ui threads.

[Peter Kasting, 2012/05/07 17:50:24]

Is there a reason to call that out for this function?  It's so simple now that
it doesn't matter whether it's restricted to the UI thread or not.

I added a comment to GenerateSearchURLUsingTermsData().

 
   // Removes any unnecessary characters from a user input keyword.
   // This removes the leading scheme, "www." and any trailing slash.
   static string16 CleanUserInputKeyword(const string16& keyword);
 
   // Returns the search url for t_url.  Returns an empty GURL if t_url has no
   // url().
   // NOTE: |t_url| is non-const in this version because of the need to access
   // t_url->profile().
   static GURL GenerateSearchURL(TemplateURL* t_url);
@@ skipping 31 matching lines @@
 
   // Returns that TemplateURL with the specified GUID, or NULL if not found.
   // The caller should not try to delete the returned pointer; the data store
   // retains ownership of it.
   TemplateURL* GetTemplateURLForGUID(const std::string& sync_guid);
 
   // Returns the first TemplateURL found with a URL using the specified |host|,
   // or NULL if there are no such TemplateURLs
   TemplateURL* GetTemplateURLForHost(const std::string& host);
 
-  // Takes ownership of |template_url| and adds it to this model.
+  // Takes ownership of |template_url| and adds it to this model.  For obvious
+  // reasons, it is illegal to Add() the same |template_url| pointer twice.
   void Add(TemplateURL* template_url);
 
   // Like Add(), but overwrites the |template_url|'s values with the provided
   // ones.
   void AddAndSetProfile(TemplateURL* template_url, Profile* profile);
   void AddWithOverrides(TemplateURL* template_url,
                         const string16& short_name,
                         const string16& keyword,
                         const std::string& url);
 
@@ skipping 252 matching lines @@
   // be replaced.
   bool CanReplaceKeywordForHost(const std::string& host,
                                 TemplateURL** to_replace);
 
   // Returns true if the TemplateURL is replaceable. This doesn't look at the
   // uniqueness of the keyword or host and is intended to be called after those
   // checks have been done. This returns true if the TemplateURL doesn't appear
   // in the default list and is marked as safe_for_autoreplace.
   bool CanReplace(const TemplateURL* t_url);
 
+  // It's possible for an extension TemplateURL to mask a replaceable
+  // non-extension TemplateURL with the same keyword (see comments in
+  // AddToMaps()).  Find the masked TemplateURL, if any.

[SteveT, 2012/05/07 18:17:47]

This returns NULL if no such masked TemplateURL exists?

[Peter Kasting, 2012/05/07 19:39:14]

I rewrote this comment entirely in more recent patches.

+  TemplateURL* FindNonExtensionTemplateURLForKeyword(const string16& keyword);
+
   // Updates the information in |existing_turl| using the information from
   // |new_values|, but the ID for |existing_turl| is retained.
   // Notifying observers is the responsibility of the caller.
   // NOTE: This should not be called with an extension keyword as there are no
   // updates needed in that case.
   void UpdateNoNotify(TemplateURL* existing_turl,
                       const TemplateURL& new_values);
 
   // Returns the preferences we use.
   PrefService* GetPrefs();
@@ skipping 26 matching lines @@
 
   // Set the default search provider even if it is managed. |url| may be null.
   // Caller is responsible for notifying observers.
   void SetDefaultSearchProviderNoNotify(TemplateURL* url);
 
   // Adds a new TemplateURL to this model. TemplateURLService will own the
   // reference, and delete it when the TemplateURL is removed.
   // If |newly_adding| is false, we assume that this TemplateURL was already
   // part of the model in the past, and therefore we don't need to do things
   // like assign it an ID or notify sync.
-  // Caller is responsible for notifying observers.
-  void AddNoNotify(TemplateURL* template_url, bool newly_adding);
+  // This function guarantees that on return the model will not have two
+  // non-extension TemplateURLs with the same keyword.  If that means that it
+  // cannot add the provided argument, it will delete it and return false.
+  // Caller is responsible for notifying observers if this function returns
+  // true.
+  bool AddNoNotify(TemplateURL* template_url, bool newly_adding);
 
   // Removes the keyword from the model. This deletes the supplied TemplateURL.
   // This fails if the supplied template_url is the default search provider.
   // Caller is responsible for notifying observers.
   void RemoveNoNotify(TemplateURL* template_url);
 
   // Notify the observers that the model has changed.  This is done only if the
   // model is loaded.
   void NotifyObservers();
 
@@ skipping 10 matching lines @@
   // to the database. This does not notify observers.
   void ResetTemplateURLGUID(TemplateURL* url, const std::string& guid);
 
   // Attempts to generate a unique keyword for |turl| based on its original
   // keyword. If its keyword is already unique, that is returned. Otherwise, it
   // tries to return the autogenerated keyword if that is unique to the Service,
   // and finally it repeatedly appends special characters to the keyword until
   // it is unique to the Service.
   string16 UniquifyKeyword(const TemplateURL& turl);
 
-  // Given a TemplateURL from Sync (cloud), resolves any keyword conflicts by
-  // checking the local keywords and uniquifying either the cloud keyword or a
-  // conflicting local keyword (whichever is older). If the cloud TURL is
-  // changed, then an appropriate SyncChange is appended to |change_list|. If
-  // a local TURL is changed, the service is updated with the new keyword. If
-  // there was no conflict to begin with, this does nothing. In the case of tied
-  // last_modified dates, |sync_turl| wins. Returns true iff there was a
-  // conflict.
-  bool ResolveSyncKeywordConflict(TemplateURL* sync_turl,
+  // Given a TemplateURL from Sync (cloud) and a local TemplateURL with the same
+  // keyword, resolves the conflict by uniquifying either the cloud keyword or
+  // the local keyword (whichever is older). If the cloud TURL is changed, then
+  // an appropriate SyncChange is appended to |change_list|. If a local TURL is
+  // changed, the service is updated with the new keyword. In the case of tied
+  // last_modified dates, |sync_turl| wins.
+  void ResolveSyncKeywordConflict(TemplateURL* sync_turl,
+                                  TemplateURL* local_turl,
                                   SyncChangeList* change_list);
 
   // Returns a TemplateURL from the service that has the same keyword and search
   // URL as |sync_turl|, if it exists.
   TemplateURL* FindDuplicateOfSyncTemplateURL(const TemplateURL& sync_turl);
 
   // Given a TemplateURL from the cloud and a local matching duplicate found by
   // FindDuplcateOfSyncTemplateURL, merges the two. If |sync_url| is newer, this
   // replaces |local_url| with |sync_url| using the service's Remove and Add.
   // If |local_url| is newer, this copies the GUID from |sync_url| over to
@@ skipping 97 matching lines @@
   // from Sync. This is to facilitate the fact that changes to the value of
   // prefs::kSyncedDefaultSearchProviderGUID do not always come before the
   // TemplateURL entry it refers to, and to handle the case when we want to use
   // the Synced default when the default search provider becomes unmanaged.
   bool pending_synced_default_search_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_