Retroactively addressed sky@'s comments from CL http://codereview.chromium.org/7566036/

chrome/browser/search_engines/template_url_service.h

 // Copyright (c) 2011 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 <map>
 #include <set>
@@ skipping 111 matching lines @@
   void FindMatchingKeywords(const string16& prefix,
                             bool support_replacement_only,
                             std::vector<string16>* matches) const;
 
   // Looks up |keyword| and returns the element it maps to.  Returns NULL if
   // the keyword was not found.
   // The caller should not try to delete the returned pointer; the data store
   // retains ownership of it.
   const TemplateURL* GetTemplateURLForKeyword(const string16& keyword) const;
 
-  // Looks up |sync_guid| and returns the element it maps to.  Returns NULL if
-  // the guid was not found.
+  // 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

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
nit: Returns the TemplateURL with the specified guid, or NULL if not found.

stevet: Done.

   // retains ownership of it.
   const TemplateURL* GetTemplateURLForGUID(const std::string& sync_guid) const;
 
   // Returns the first TemplateURL found with a URL using the specified |host|,
   // or NULL if there are no such TemplateURLs
   const TemplateURL* GetTemplateURLForHost(const std::string& host) const;
 
   // Adds a new TemplateURL to this model. TemplateURLService will own the
   // reference, and delete it when the TemplateURL is removed.
   void Add(TemplateURL* template_url);
@@ skipping 320 matching lines @@
   // to the database. This does not notify observers.
   void ResetTemplateURLGUID(const 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) const;
 
-  // Given a TemplateURL from Sync, resolves any keyword conflicts by checking

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
nit: add (cloud) after Sync.

stevet: Done.

-  // the local keywords and uniquifying either the cloud keyword or a
+  // 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,
-                                  SyncChangeList& change_list);

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
Because this may modify change_list, it should be a pointer.

stevet: Done.

+                                  SyncChangeList* change_list);
 
   // Returns a TemplateURL from the service that has the same keyword and search
   // URL as |sync_turl|, if it exists.
   const 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
   // |local_url| and adds an update to change_list to notify the server of the
   // change. This method takes ownership of |sync_url|, and adds it to the model
   // if it is newer, so the caller must release it if need be.
   void MergeSyncAndLocalURLDuplicates(TemplateURL* sync_url,
                                       TemplateURL* local_url,
-                                      SyncChangeList& change_list);

[SteveT, 2011/09/01 14:21:54]

sky 2011/08/17 23:22:29
change_list should be a pointer here too.

stevet: Done.

+                                      SyncChangeList* change_list);
 
   NotificationRegistrar registrar_;
 
   // Mapping from keyword to the TemplateURL.
   KeywordToTemplateMap keyword_to_template_map_;
 
   // Mapping from Sync GUIDs to the TemplateURL.
   GUIDToTemplateMap guid_to_template_map_;
 
   TemplateURLVector template_urls_;
@@ skipping 63 matching lines @@
   // true, we ignore any local search engine changes, since we triggered them.
   bool processing_syncer_changes_;
 
   // Sync's SyncChange handler. We push all our changes through this.
   SyncChangeProcessor* sync_processor_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_TEMPLATE_URL_SERVICE_H_