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..c5d8504fd47ed321762629694fd81b69f922aeae 100644
--- a/chrome/browser/spellchecker/spellcheck_custom_dictionary.h
+++ b/chrome/browser/spellchecker/spellcheck_custom_dictionary.h
@@ -5,97 +5,160 @@
 #ifndef CHROME_BROWSER_SPELLCHECKER_SPELLCHECK_CUSTOM_DICTIONARY_H_
 #define CHROME_BROWSER_SPELLCHECKER_SPELLCHECK_CUSTOM_DICTIONARY_H_
 
-#include <string>
-#include <vector>
-
 #include "base/file_path.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #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.
-// The dictionary contains its own checksum when saved on disk. Example
-// dictionary file contents:
+// must be UTF8, between 1 and 99 bytes long, and without ASCII whitespace. The
+// dictionary contains its own checksum when saved on disk. Example dictionary
+// file contents:
 //
 //   bar
 //   foo
 //   checksum_v1 = ec3df4034567e59e119fcf87f2d9bad4
 //
-class SpellcheckCustomDictionary : public SpellcheckDictionary {
+class SpellcheckCustomDictionary : public SpellcheckDictionary,
+                                   public syncer::SyncableService {
  public:
+  // A change to the dictionary.
+  class Change {
+   public:
+    Change();
+    ~Change();
+
+    // Add |word| in this change.
+    void AddWord(const std::string& word);

[groby-ooo-7-16, 2013/01/15 01:19:20]

Can we either have AddWord/RemoveWord or AddWords/RemoveWords?

Or are both use cases about equally important?

[please use gerrit instead, 2013/01/16 02:06:05]

Let's keep only AddWord/RemoveWord.

+
+    // Add |words| in this change.
+    void AddWords(const chrome::spellcheck_common::WordList& words);
+
+    // Remove |word| in this change.
+    void RemoveWord(const std::string& word);
+
+    // Remove |words| in this change.
+    void RemoveWords(const chrome::spellcheck_common::WordList& words);
+
+    // Returns the words to be added in this change.
+    const chrome::spellcheck_common::WordList& to_add() const;
+    chrome::spellcheck_common::WordList& to_add();
+
+    // Returns the words to be removed in this change.
+    const chrome::spellcheck_common::WordList& to_remove() const;
+    chrome::spellcheck_common::WordList& to_remove();
+
+    // Returns true if there are no changes to be made. Otherwise returns false.
+    bool empty() const;
+
+   private:
+    // The words to be added.
+    chrome::spellcheck_common::WordList to_add_;
+
+    // The words to be removed.
+    chrome::spellcheck_common::WordList to_remove_;
+
+    DISALLOW_COPY_AND_ASSIGN(Change);
+  };
+
+  // Interface to implement for dictionary load and change observers.
   class Observer {
    public:
+    // A callback for when the custom dictionary has loaded the words saved in

[groby-ooo-7-16, 2013/01/15 01:19:20]

Shorten to "called when the custom dictionary file has been loaded".

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

+    // the custom dictionary file.
     virtual void OnCustomDictionaryLoaded() = 0;
-    virtual void OnCustomDictionaryWordAdded(const std::string& word) = 0;
-    virtual void OnCustomDictionaryWordRemoved(const std::string& word) = 0;
+
+    // A callback for when the custom dictionary has been changed.

[groby-ooo-7-16, 2013/01/15 01:19:20]

"Called when.."

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

+    virtual void OnCustomDictionaryChanged(const Change* dictionary_change) = 0;
   };
 
   explicit SpellcheckCustomDictionary(Profile* profile);
   virtual ~SpellcheckCustomDictionary();
 
-  // Overridden from SpellcheckDictionary:
-  virtual void Load() OVERRIDE;
-
+  // Returns the in-memory cache of words in the custom dictionary.
   const chrome::spellcheck_common::WordList& GetWords() const;
 
-  // 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);
-
-  // Moves the words from the |custom_words| argument into its own private
-  // member variable. Does not delete the memory at |custom_words|.
-  void SetCustomWordList(chrome::spellcheck_common::WordList* custom_words);
-
-  // Adds the given word to the custom words list and informs renderers of the
-  // update. Returns false for duplicate and invalid words.
+  // Adds |word| to the dictionary, schedules a write to disk, and notifies
+  // observers of the change. Returns true if |word| is valid and not a
+  // duplicate. Otherwise returns false.
   bool AddWord(const std::string& word);
 
-  // Returns false for duplicate words.
-  bool CustomWordAddedLocally(const std::string& word);
+  // Removes |word| from the dictionary, schedules a write to disk, and notifies
+  // observers of the change. Returns true if |word| was found. Otherwise
+  // returns false.
+  bool RemoveWord(const std::string& word);
 
-  void WriteWordToCustomDictionary(const std::string& word);
+  // Adds |observer| to be notified of dictionary events and changes.
+  void AddObserver(Observer* observer);
 
-  // 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
-  // invalid words.
-  bool RemoveWord(const std::string& word);
+  // Removes |observer| to stop notifications of dictionary events and changes.
+  void RemoveObserver(Observer* observer);
 
-  // Returns false for words that are not in the dictionary.
-  bool CustomWordRemovedLocally(const std::string& word);
+  // Whether the dictionary is loaded.

[groby-ooo-7-16, 2013/01/15 01:19:20]

"Return true if the dictionary is loaded." - I prefer complete sentences for my
docs :)

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

+  bool IsLoaded();
 
-  void EraseWordFromCustomDictionary(const std::string& word);
+  // Whether the dictionary is being synced.
+  bool IsSyncing();

[groby-ooo-7-16, 2013/01/15 01:19:20]

See above.

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  // Overridden from SpellcheckDictionary:
+  virtual void Load() OVERRIDE;
+
+  // Overridden from syncer::SyncableService:
+  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;
 
  private:
-  // Returns a newly allocated list of words read from custom dictionary file.
-  // The caller owns this new list.
-  chrome::spellcheck_common::WordList* LoadDictionary();
-
-  // The reply point for PostTaskAndReplyWithResult. Called when LoadDictionary
-  // is finished reading words from custom dictionary file. Moves the strings
-  // from the |custom_words| argument into the private member variable |words_|
-  // and deletes the memory at |custom_words|.
-  void SetCustomWordListAndDelete(
-      chrome::spellcheck_common::WordList* custom_words);
-
-  // Loads the dictionary file into |custom_words|. If the dictionary checksum
-  // 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);
-
-  // Backs up the original dictionary, saves |custom_words| and its checksum
-  // into the dictionary file.
-  void SaveDictionaryFileReliably(
-      const chrome::spellcheck_common::WordList& custom_words);
+  friend class DictionarySyncIntegrationTestHelper;
+  friend class SpellcheckCustomDictionaryTest;
+
+  // Returns the list of words in the custom spellcheck dictionary at |path|.
+  // Makes sure that the custom dictionary file is sorted, does not have
+  // duplicates, and contains only valid words.
+  static scoped_ptr<chrome::spellcheck_common::WordList> LoadDictionaryFile(

[groby-ooo-7-16, 2013/01/15 01:19:20]

Since this doesn't need any member access, and is private to the implementation,
why not add it to an anonymous namespace in the impl. file?

[please use gerrit instead, 2013/01/16 02:06:05]

Unit tests access this function directly.

+      const FilePath& path);
+
+  // Applies the change in |dictionary_change| to the custom spellcheck
+  // dictionary. Assumes that |dictionary_change| has already been processed to
+  // clean the words that should not be added or removed.
+  static void UpdateDictionaryFile(
+      scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change,
+      const FilePath& path);

[groby-ooo-7-16, 2013/01/15 01:19:20]

See above re: anon. ns.

[please use gerrit instead, 2013/01/16 02:06:05]

Unit tests access this function directly, too.

+
+  // The reply point for PostTaskAndReplyWithResult, called when
+  // LoadDictionaryFile finishes reading the dictionary file.
+  void OnLoaded(scoped_ptr<chrome::spellcheck_common::WordList> custom_words);
+
+  // Applies the |dictionary_change| to the in-memory copy of the dictionary and
+  // cleans up |dictionary_change| to be without duplicates, invalid words, and
+  // missing words. Does not take ownership of |dictionary_change|. Returns a
+  // bitmap of |ChangeResult| values.
+  int Apply(Change* dictionary_change);

[groby-ooo-7-16, 2013/01/15 01:19:20]

I'm still in favor of sanitizing the changes on the Change object - I believe
Apply could then take a const ref.

[please use gerrit instead, 2013/01/16 02:06:05]

Const refs everywhere. Sanitizing the Change object outside of Apply.

+
+  // Schedules a write of |dictionary_change| to disk.
+  void Save(scoped_ptr<Change> dictionary_change);

[groby-ooo-7-16, 2013/01/15 01:19:20]

"... and takes ownership". 

Even better, pass a const-ref for the Change. Yes, the PostTask will create a
copy, but usually, changes are fairly small, and it's a rare operation. It
simplifies the code by getting rid of lots of ownership transfers.

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

+
+  // Notifies the sync service of the |dictionary_change|. Syncs up to the
+  // maximum syncable words on the server. Disables syncing of this dictionary
+  // if the server contains the maximum number of syncable words. Does not take
+  // ownership of |dictionary_change|.
+  syncer::SyncError Sync(const Change* dictionary_change);

[groby-ooo-7-16, 2013/01/15 01:19:20]

I might be wrong, but at first glance, it doesn't look like sync needs a pointer
- why not const-ref?

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

+
+  // Notifies observers of the dictionary change if the dictionary has been
+  // changed. Does not take ownership of |dictionary_change|.
+  void Notify(const Change* dictionary_change);

[groby-ooo-7-16, 2013/01/15 01:19:20]

const-ref?

[please use gerrit instead, 2013/01/16 02:06:05]

Done.

 
   // In-memory cache of the custom words file.
   chrome::spellcheck_common::WordList words_;
@@ -103,10 +166,21 @@ class SpellcheckCustomDictionary : public SpellcheckDictionary {
   // A path for custom dictionary per profile.
   FilePath custom_dictionary_path_;
 
+  // Used to create weak pointers for an instance of this class.
   base::WeakPtrFactory<SpellcheckCustomDictionary> weak_ptr_factory_;
 
+  // Observers for changes in dictionary load status and content changes.
   ObserverList<Observer> observers_;
 
+  // Used to send local changes to the sync infrastructure.
+  scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
+
+  // Used to send sync-related errors to the sync infrastructure.
+  scoped_ptr<syncer::SyncErrorFactory> sync_error_handler_;
+
+  // Whether the dictionary has been loaded.
+  bool is_loaded_;
+
   DISALLOW_COPY_AND_ASSIGN(SpellcheckCustomDictionary);
 };