Sync user's custom spellcheck dictionary

chrome/browser/spellchecker/spellcheck_custom_dictionary.h

Index: chrome/browser/spellchecker/spellcheck_custom_dictionary.h
diff --git a/chrome/browser/spellchecker/spellcheck_custom_dictionary.h b/chrome/browser/spellchecker/spellcheck_custom_dictionary.h
index cedbb734c2c762df57ad3b030f80f276ea0b8278..2283d2f9279c2ba6978c0214da0f6e844767d75b 100644
--- a/chrome/browser/spellchecker/spellcheck_custom_dictionary.h
+++ b/chrome/browser/spellchecker/spellcheck_custom_dictionary.h
@@ -14,6 +14,7 @@
 #include "base/observer_list.h"
 #include "chrome/browser/spellchecker/spellcheck_dictionary.h"
 #include "chrome/common/spellcheck_common.h"
+#include "sync/api/syncable_service.h"
 
 // Defines a custom dictionary where users can add their own words. All words
 // must be UTF8, between 1 and 128 bytes long, and without ASCII whitespace.
@@ -24,13 +25,16 @@
 //   foo
 //   checksum_v1 = ec3df4034567e59e119fcf87f2d9bad4
 //
-class SpellcheckCustomDictionary : public SpellcheckDictionary {
+class SpellcheckCustomDictionary : public SpellcheckDictionary,
+                                   public syncer::SyncableService {
  public:
   class Observer {
    public:
     virtual void OnCustomDictionaryLoaded() = 0;
-    virtual void OnCustomDictionaryWordAdded(const std::string& word) = 0;
-    virtual void OnCustomDictionaryWordRemoved(const std::string& word) = 0;
+    virtual void OnCustomDictionaryWordsAdded(
+        const chrome::spellcheck_common::WordList& words) = 0;
+    virtual void OnCustomDictionaryWordsRemoved(
+        const chrome::spellcheck_common::WordList& words) = 0;
   };
 
   explicit SpellcheckCustomDictionary(Profile* profile);
@@ -44,8 +48,9 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   // Populates the |custom_words| with the list of words in the custom
   // dictionary file. Makes sure that the custom dictionary file is sorted, does
   // not have duplicates, and contains only valid words.
-  void LoadDictionaryIntoCustomWordList(
-      chrome::spellcheck_common::WordList* custom_words);
+  static void LoadDictionaryIntoCustomWordList(

[groby-ooo-7-16, 2012/12/19 22:16:00]

Personal preference: I like to make functions stand-alone if they require truly
no access to members. (I.e. outside of the class, instead of static). It
prevents accidental entanglement later on.

[please use gerrit instead, 2012/12/22 03:20:19]

Done.

+      chrome::spellcheck_common::WordList* custom_words,
+      const FilePath& path);
 
   // Moves the words from the |custom_words| argument into its own private
   // member variable. Does not delete the memory at |custom_words|.
@@ -58,7 +63,9 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   // Returns false for duplicate words.
   bool CustomWordAddedLocally(const std::string& word);
 
-  void WriteWordToCustomDictionary(const std::string& word);
+  static void WriteWordToCustomDictionary(
+      const std::string& word,
+      const FilePath& path);
 
   // Removes the given word from the custom words list and informs renderers of
   // the update. Returns false for words that are not in the dictionary and
@@ -68,15 +75,38 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   // Returns false for words that are not in the dictionary.
   bool CustomWordRemovedLocally(const std::string& word);
 
-  void EraseWordFromCustomDictionary(const std::string& word);
+  static void EraseWordFromCustomDictionary(
+      const std::string& word,
+      const FilePath& path);
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Overridden from syncer::SyncableService:

[groby-ooo-7-16, 2012/12/19 22:16:00]

Personal preference: I like overridden interfaces together, and either at the
top or bottom of the public section.

[please use gerrit instead, 2012/12/22 03:20:19]

Done.

+  virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
+      syncer::ModelType type,
+      const syncer::SyncDataList& initial_sync_data,
+      scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+      scoped_ptr<syncer::SyncErrorFactory> sync_error_handler) OVERRIDE;
+  virtual void StopSyncing(syncer::ModelType type) OVERRIDE;
+  virtual syncer::SyncDataList GetAllSyncData(
+      syncer::ModelType type) const OVERRIDE;
+  virtual syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const syncer::SyncChangeList& change_list) OVERRIDE;
+
+  // Relinquishes the management of |sync_processor_|, because it is managed by
+  // |SpellcheckService| in testing.
+  void StopSyncingForTesting();
+
+  // Whether the dictionary is loaded.
+  bool IsLoaded();
+
  private:
   // Returns a newly allocated list of words read from custom dictionary file.
   // The caller owns this new list.
-  chrome::spellcheck_common::WordList* LoadDictionary();
+  static chrome::spellcheck_common::WordList* LoadDictionary(
+      const FilePath& path);
 
   // The reply point for PostTaskAndReplyWithResult. Called when LoadDictionary
   // is finished reading words from custom dictionary file. Moves the strings
@@ -89,13 +119,56 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   // is not valid, but backup checksum is valid, then restores the backup and
   // loads that into |custom_words| instead. If the backup is invalid too, then
   // clears |custom_words|.
-  void LoadDictionaryFileReliably(
-      chrome::spellcheck_common::WordList* custom_words);
+  static void LoadDictionaryFileReliably(
+      chrome::spellcheck_common::WordList* custom_words,
+      const FilePath& path);
 
   // Backs up the original dictionary, saves |custom_words| and its checksum
   // into the dictionary file.
-  void SaveDictionaryFileReliably(
-      const chrome::spellcheck_common::WordList& custom_words);
+  static void SaveDictionaryFileReliably(
+      const chrome::spellcheck_common::WordList& custom_words,
+      const FilePath& path);
+
+  // Adds words |to_add| and removes words |to_remove| in the custom words list
+  // and informs renderers of the updates. Returns true on success, false on
+  // duplicate words that were not added, missing words that could not be

[groby-ooo-7-16, 2012/12/19 22:16:00]

That's a confusing API - usually boolean returns indicate a failure condition.

Also, two overloaded versions just for an additional bool parameter seems
overkill. Can we just have the one with |is_sync_update|?

And finally, consider making the bool an enum instead, so it's either to
decipher at the invocation side what's going on.

[please use gerrit instead, 2012/12/22 03:20:19]

Done.

+  // removed, and invalid words. Removes duplicates and invalid words from
+  // |to_add| and missing words from |to_remove|. The optional parameter
+  // |is_sync_update| indicates that this change comes from a sync update, which
+  // means that we should not notify the sync process of these changes to avoid
+  // an infinite loop. |is_sync_update| is false by default.
+  bool UpdateWords(chrome::spellcheck_common::WordList* to_add,
+                   chrome::spellcheck_common::WordList* to_remove);
+  bool UpdateWords(chrome::spellcheck_common::WordList* to_add,
+                   chrome::spellcheck_common::WordList* to_remove,
+                   bool is_sync_update);
+
+  // Adds words |to_add| and removes words |to_remove| in the custom words list.
+  // Returns true on success, false on duplicate words that were not added,
+  // missing words that could not be removed, and invalid words. Removes
+  // duplicates and invalid words from |to_add| and missing words from
+  // |to_remove|. The optional parameter |is_sync_update| indicates that this
+  // change comes from a sync update, which means that we should not notify the
+  // sync process of these changes to avoid an infinite loop. |is_sync_update|
+  // is false by default.
+  bool CustomWordsUpdatedLocally(
+      chrome::spellcheck_common::WordList* to_add,
+      chrome::spellcheck_common::WordList* to_remove);
+  bool CustomWordsUpdatedLocally(
+      chrome::spellcheck_common::WordList* to_add,
+      chrome::spellcheck_common::WordList* to_remove,
+      bool is_sync_update);
+
+  // Writes words |to_write| and erases words |to_erase| in the dictionary file.
+  static void WriteAndEraseWordsInCustomDictionary(
+      const chrome::spellcheck_common::WordList* to_write,
+      const chrome::spellcheck_common::WordList* to_erase,
+      const FilePath& path);
+
+  // Deletes the memory at |written| and |erased|.
+  void OnWriteAndEraseFinished(
+      const chrome::spellcheck_common::WordList* written,
+      const chrome::spellcheck_common::WordList* erased);
 
   // In-memory cache of the custom words file.
   chrome::spellcheck_common::WordList words_;
@@ -106,6 +179,9 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   base::WeakPtrFactory<SpellcheckCustomDictionary> weak_ptr_factory_;
 
   ObserverList<Observer> observers_;
+  scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;

[groby-ooo-7-16, 2012/12/19 22:16:00]

Please document member vars.

[please use gerrit instead, 2012/12/22 03:20:19]

Done.

+  scoped_ptr<syncer::SyncErrorFactory> sync_error_handler_;
+  bool is_loaded_;
 
   DISALLOW_COPY_AND_ASSIGN(SpellcheckCustomDictionary);
 };