Implement SyncableServices in TemplateURLService. Add related unittests.

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>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/browser/search_engines/search_host_to_urls_map.h"
 #include "chrome/browser/search_engines/template_url_id.h"
+#include "chrome/browser/sync/api/sync_change.h"
+#include "chrome/browser/sync/api/syncable_service.h"
+#include "chrome/browser/sync/protocol/search_engine_specifics.pb.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
 
 class GURL;
 class Extension;
 class PrefService;
 class Profile;
 class PrefSetObserver;
 class SearchHostToURLsMap;
 class SearchTermsData;
+class SyncData;
 class TemplateURLServiceObserver;
 class TemplateURLRef;
 
 namespace history {
 struct URLVisitedDetails;
 }
 
+typedef std::map<std::string, SyncData> SyncDataMap;

[Peter Kasting, 2011/08/09 21:31:04]

Nit: Should this typedef be inside the class?  Seems kinda bad to make it
global.

[SteveT, 2011/08/10 15:15:12]

Done.

+
 // TemplateURLService is the backend for keywords. It's used by
 // KeywordAutocomplete.
 //
 // TemplateURLService stores a vector of TemplateURLs. The TemplateURLs are
 // persisted to the database maintained by WebDataService. *ALL* mutations
 // to the TemplateURLs must funnel through TemplateURLService. This allows
 // TemplateURLService to notify listeners of changes as well as keep the
 // database in sync.
 //
 // There is a TemplateURLService per Profile.
 //
 // TemplateURLService does not load the vector of TemplateURLs in its
 // constructor (except for testing). Use the Load method to trigger a load.
 // When TemplateURLService has completed loading, observers are notified via
 // OnTemplateURLServiceChanged as well as the TEMPLATE_URL_SERVICE_LOADED
 // notification message.
 //
 // TemplateURLService takes ownership of any TemplateURL passed to it. If there
 // is a WebDataService, deletion is handled by WebDataService, otherwise
 // TemplateURLService handles deletion.
 
 class TemplateURLService : public WebDataServiceConsumer,
                            public ProfileKeyedService,
-                           public NotificationObserver {
+                           public NotificationObserver,
+                           public SyncableService {
  public:
   typedef std::map<std::string, std::string> QueryTerms;
   typedef std::vector<const TemplateURL*> TemplateURLVector;
   // Type for a static function pointer that acts as a time source.
   typedef base::Time(TimeProvider)();
 
   // Struct used for initializing the data store with fake data.
   // Each initializer is mapped to a TemplateURL.
   struct Initializer {
     const char* const keyword;
@@ skipping 45 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.
+  // The caller should not try to delete the returned pointer; the data store
+  // 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);
 
   // Removes the keyword from the model. This deletes the supplied TemplateURL.
   // This fails if the supplied template_url is the default search provider.
@@ skipping 31 matching lines @@
   // Called when a URL is loaded that was generated from a keyword.
   void IncrementUsageCount(const TemplateURL* url);
 
   // Resets the title, keyword and search url of the specified TemplateURL.
   // The TemplateURL is marked as not replaceable.
   void ResetTemplateURL(const TemplateURL* url,
                         const string16& title,
                         const string16& keyword,
                         const std::string& search_url);
 
+  // Resets the title, keyword, search url and sync GUID of the specified
+  // TemplateURL. The TemplateURL is marked as not replaceable.
+  void ResetTemplateURL(const TemplateURL* url,

[Peter Kasting, 2011/08/09 21:31:04]

By adding this, we've effectively created a "default argument" for the last arg.
 This is banned by the style guide.  Can we remove the version above and have
everyone only call this one?  Should we keep these both but name them
differently?  Should we have a separate function to reset the GUID?

[SteveT, 2011/08/10 15:15:12]

You're right - the correct thing to do here is to introduced a separate method
that resets just the GUID and persists the change to the DB (rather than add
more crap to this method). Done.

+                        const string16& title,
+                        const string16& keyword,
+                        const std::string& search_url,
+                        const std::string& guid);
+
   // Return true if the given |url| can be made the default.
   bool CanMakeDefault(const TemplateURL* url);
 
   // Set the default search provider.  |url| may be null.
   // This will assert if the default search is managed; the UI should not be
   // invoking this method in that situation.
   void SetDefaultSearchProvider(const TemplateURL* url);
 
   // Returns the default search provider. If the TemplateURLService hasn't been
   // loaded, the default search provider is pulled from preferences.
@@ skipping 38 matching lines @@
   // notification types:
   // . NOTIFY_HISTORY_URL_VISITED: adds keyword search terms if the visit
   //   corresponds to a keyword.
   // . NOTIFY_GOOGLE_URL_UPDATED: updates mapping for any keywords containing
   //   a google base url replacement term.
   // . PREF_CHANGED: checks whether the default search engine has changed.
   virtual void Observe(int type,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
+  // SyncableService implementation.
+
+  // Returns all syncable TemplateURLs from this model as SyncData. This should
+  // include every search engine and no Extension keywords.
+  virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE;
+  // Process new search engine changes from Sync, merging them into our local
+  // data. This may send notifications if local search engines are added,
+  // updated or removed.
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE;
+  // Merge initial search engine data from Sync and push any local changes up
+  // to Sync. This may send notifications if local search engines are added,
+  // updated or removed.
+  virtual SyncError MergeDataAndStartSyncing(
+      syncable::ModelType type,
+      const SyncDataList& initial_sync_data,
+      SyncChangeProcessor* sync_processor) OVERRIDE;
+  virtual void StopSyncing(syncable::ModelType type) OVERRIDE;
+
+  // Processes a local TemplateURL change for Sync. |turl| is the TemplateURL
+  // that has been modified, and |type| is the Sync ChangeType that took place.
+  // This may send a new SyncChange to the cloud. If our model has not yet been
+  // associated with Sync, or if this is triggered by a Sync change, then this
+  // does nothing.
+  void ProcessTemplateURLChange(const TemplateURL* turl,
+                                SyncChange::SyncChangeType type);
+
   Profile* profile() const { return profile_; }
 
   void SetSearchEngineDialogSlot(int slot) {
     search_engine_dialog_chosen_slot_ = slot;
   }
 
   int GetSearchEngineDialogSlot() const {
     return search_engine_dialog_chosen_slot_;
   }
 
   // Registers the preferences used to save a TemplateURL to prefs.
   static void RegisterUserPrefs(PrefService* prefs);
 
+  // Fills |sync_data| with a sync representation of the search engine data from
+  // |turl|. Returns true if successful, false otherwise.

[Peter Kasting, 2011/08/09 21:31:04]

Nit: You can probably just say "Returns whether creation succeeded", but you
might also want to note what can cause failure if that wouldn't be obvious to
all callers.

[SteveT, 2011/08/10 15:15:12]

Changed the wording. Actually this currently always returns true because we're
not doing anything complicated enough to fail. I'm mostly taking the format from
Pref's CreatePrefSyncData method, which can possibly fail. I can just make this
void for now if that makes more sense.

[Peter Kasting, 2011/08/10 17:39:11]

Yeah, I'd do that; it's easy to change the signature later as needed.

+  static bool CreateSearchEngineSyncData(const TemplateURL& turl,
+                                         SyncData* sync_data);
+
+  // Read a TemplateURL object from SyncData containing SearchEngineSpecifics.
+  // This does the opposite of CreateSearchEngineSyncData. The caller owns the
+  // returned TemplateURL*.
+  static TemplateURL* ReadSearchEngineSyncData(const SyncData& sync_data);

[Peter Kasting, 2011/08/09 21:31:04]

Nit: These two functions are paired, but one returns a bool and takes an
outparam, while the other returns a (heap-allocated) pointer directly.  Also
(less importantly), their names aren't strict mirrors, the way they would be
with e.g. "CreateSyncDataFromTURL()" and "CreateTURLFromSyncData()" or
something.  Should these be made more parallel in structure?

[SteveT, 2011/08/10 15:15:12]

Actually, since our version of CreateSyncData (above) can't really fail, I'll
have it return a SyncData so the structure is a bit more similar. I'll also
change the wording. I think copying Pref's structure around these two helpers
wasn't the best idea.

+
+  // 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
+  // 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);
+
+  // 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.
+  void MergeSyncAndLocalURLDuplicates(TemplateURL* sync_url,
+                                      TemplateURL* local_url,
+                                      SyncChangeList& change_list);
+
+  // Returns a map mapping Sync GUIDs to pointers to SyncData.
+  static SyncDataMap CreateGUIDToSyncDataMap(const SyncDataList& sync_data);
+
 #if defined(UNIT_TEST)
   // Set a different time provider function, such as
   // base::MockTimeProvider::StaticNow, when testing calls to base::Time::Now.
   void set_time_provider(TimeProvider* time_provider) {
     time_provider_ = time_provider;
   }
 #endif
 
  protected:
   // Cover method for the method of the same name on the HistoryService.
   // url is the one that was visited with the given search terms.
   //
   // This exists and is virtual for testing.
   virtual void SetKeywordSearchTermsForURL(const TemplateURL* t_url,
                                            const GURL& url,
                                            const string16& term);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, BuildQueryTerms);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, TestManagedDefaultSearch);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
                            UpdateKeywordSearchTermsForURL);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
                            DontUpdateKeywordSearchForNonReplaceable);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, ChangeGoogleBaseValue);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, MergeDeletesUnusedProviders);
   friend class TemplateURLServiceTestUtil;
 
   typedef std::map<string16, const TemplateURL*> KeywordToTemplateMap;
+  typedef std::map<std::string, const TemplateURL*> GUIDToTemplateMap;
 
   // Helper functor for FindMatchingKeywords(), for finding the range of
   // keywords which begin with a prefix.
   class LessWithPrefix;
 
   void Init(const Initializer* initializers, int num_initializers);
 
   void RemoveFromMaps(const TemplateURL* template_url);
 
   // Removes the supplied template_url from the keyword maps. This searches
@@ skipping 105 matching lines @@
   void RemoveProvidersCreatedByPolicy(
       std::vector<TemplateURL*>* template_urls,
       const TemplateURL** default_search_provider,
       const TemplateURL* default_from_prefs);
 
   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_;
 
   ObserverList<TemplateURLServiceObserver> model_observers_;
 
   // Maps from host to set of TemplateURLs whose search url host is host.
   SearchHostToURLsMap provider_map_;
 
   // Used to obtain the WebDataService.
   // When Load is invoked, if we haven't yet loaded, the WebDataService is
   // obtained from the Profile. This allows us to lazily access the database.
@@ skipping 37 matching lines @@
   // ID assigned to next TemplateURL added to this model. This is an ever
   // increasing integer that is initialized from the database.
   TemplateURLID next_id_;
 
   // List of extension IDs waiting for Load to have keywords registered.
   std::vector<std::string> pending_extension_ids_;
 
   // Function returning current time in base::Time units.
   TimeProvider* time_provider_;
 
+  // Do we have an active association between the TemplateURLs and sync models?
+  // Set when start syncing, reset in StopSyncing. While this is not set, we

[Peter Kasting, 2011/08/09 21:31:04]

Nit: "Set when start syncing" isn't grammatical (maybe "Set in
MergeDataAndStartSyncing()"?)

[SteveT, 2011/08/10 15:15:12]

Done.

+  // ignore any local search engine changes (when we start syncing we will look
+  // up the most recent values anyways).
+  bool models_associated_;
+
+  // Whether we're currently processing changes from the syncer. While this is
+  // 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_