Sync user's custom spellcheck dictionary

chrome/browser/spellchecker/spellcheck_custom_dictionary.cc

Index: chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
diff --git a/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc b/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
index 588e3675c2fa3d8e22620f578cd497abef322e16..51f4befade939c681cfa6d1b7e016712329bbdae 100644
--- a/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
+++ b/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
@@ -9,25 +9,42 @@
 #include "base/file_util.h"
 #include "base/files/important_file_writer.h"
 #include "base/md5.h"
+#include "base/string_number_conversions.h"
 #include "base/string_split.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/spellcheck_messages.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/render_process_host.h"
+#include "sync/api/sync_change.h"
+#include "sync/api/sync_data.h"
+#include "sync/api/sync_error_factory.h"
+#include "sync/protocol/sync.pb.h"

[Nicolas Zea, 2013/01/02 23:12:15]

Do you need this, or can you get away with just including the proto for your
type? (i.e. sync/protocol/dictionary_specifics.pb.h)

[please use gerrit instead, 2013/01/04 23:30:50]

We need sync.pb.h for sync_pb::EntitySpecifics.

+#include "third_party/hunspell/src/hunspell/hunspell.hxx"
 
 using content::BrowserThread;
 using chrome::spellcheck_common::WordList;
 
+namespace custom_dictionary {
 namespace {
 
+// Filename extension for backup dictionary file.
 const FilePath::CharType BACKUP_EXTENSION[] = FILE_PATH_LITERAL("backup");
+
+// Prefix for the checksum in the dictionary file.
 const char CHECKSUM_PREFIX[] = "checksum_v1 = ";
 
+// The status of the checksum in a custom spellcheck dictionary.
+enum ChecksumStatus {
+  VALID_CHECKSUM,
+  INVALID_CHECKSUM,
+};
+
 // Loads the lines from the file at |file_path| into the |lines| container. If
-// the file has a valid checksum, then returns |true|. If the file has an
-// invalid checksum, then returns |false| and clears |lines|.
-bool LoadFile(FilePath file_path, std::vector<std::string>* lines) {
+// the file has a valid checksum, then returns ChecksumStatus::VALID. If the
+// file has an invalid checksum, then returns ChecksumStatus::INVALID and clears
+// |lines|. Must be called on the file thread.
+ChecksumStatus LoadFile(FilePath file_path, std::vector<std::string>* lines) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   lines->clear();
   std::string contents;
   file_util::ReadFileToString(file_path, &contents);
@@ -36,59 +53,141 @@ bool LoadFile(FilePath file_path, std::vector<std::string>* lines) {
     std::string checksum = contents.substr(pos + strlen(CHECKSUM_PREFIX));
     contents = contents.substr(0, pos);
     if (checksum != base::MD5String(contents))
-      return false;
+      return INVALID_CHECKSUM;
   }
   TrimWhitespaceASCII(contents, TRIM_ALL, &contents);
   base::SplitString(contents, '\n', lines);
-  return true;
-}
-
-bool IsValidWord(const std::string& word) {
-  return IsStringUTF8(word) && word.length() <= 128 && word.length() > 0 &&
-      std::string::npos == word.find_first_of(kWhitespaceASCII);
+  return VALID_CHECKSUM;
 }
 
+// Returns true for invalid words and false for valid words. Useful for
+// std::remove_if() calls.
 bool IsInvalidWord(const std::string& word) {
-  return !IsValidWord(word);
+  return !IsStringUTF8(word) || word.length() >= MAXWORDLEN || word.empty() ||
+      word.find_first_of(kWhitespaceASCII) != std::string::npos;
 }
 
-}  // namespace
+// Loads the custom spellcheck dictionary from |path| 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|. Must be called on the file
+// thread.
+void LoadDictionaryFileReliably(WordList* custom_words, const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-SpellcheckCustomDictionary::SpellcheckCustomDictionary(Profile* profile)
-    : SpellcheckDictionary(profile),
-      custom_dictionary_path_(),
-      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
-  DCHECK(profile);
-  custom_dictionary_path_ =
-      profile_->GetPath().Append(chrome::kCustomDictionaryFileName);
+  // Load the contents and verify the checksum.
+  if (LoadFile(path, custom_words) == VALID_CHECKSUM)
+    return;
+
+  // Checksum is not valid. See if there's a backup.
+  FilePath backup = path.AddExtension(BACKUP_EXTENSION);
+  if (!file_util::PathExists(backup))
+    return;
+
+  // Load the backup and verify its checksum.
+  if (LoadFile(backup, custom_words) != VALID_CHECKSUM)
+    return;
+
+  // Backup checksum is valid. Restore the backup.
+  file_util::CopyFile(backup, path);
 }
 
-SpellcheckCustomDictionary::~SpellcheckCustomDictionary() {
+// Backs up the original dictionary, saves |custom_words| and its checksum into
+// the custom spellcheck dictionary at |path|. Does not take ownership of
+// |custom_words|. Must be called on the file thread.
+void SaveDictionaryFileReliably(
+    const WordList* custom_words,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(custom_words);
+
+  std::stringstream content;
+  for (WordList::const_iterator it = custom_words->begin();
+       it != custom_words->end();
+       ++it) {
+    content << *it << '\n';
+  }
+  std::string checksum = base::MD5String(content.str());
+  content << CHECKSUM_PREFIX << checksum;
+
+  file_util::CopyFile(path, path.AddExtension(BACKUP_EXTENSION));
+  base::ImportantFileWriter::WriteFileAtomically(path, content.str());
 }
 
-void SpellcheckCustomDictionary::Load() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+// Applies the change in |dictionary_change| to the custom spellcheck dictionary
+// at |path|. Assumes that |dictionary_change| has already been processed to
+// clean the words that cannot be added or removed. Deletes |dictionary_change|
+// when done. Must be called on the file thread.
+void WriteAndEraseWordsInCustomDictionary(
+    scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(dictionary_change.get());
 
-  BrowserThread::PostTaskAndReplyWithResult<WordList*>(
-      BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&SpellcheckCustomDictionary::LoadDictionary,
-                 base::Unretained(this)),
-      base::Bind(&SpellcheckCustomDictionary::SetCustomWordListAndDelete,
-                 weak_ptr_factory_.GetWeakPtr()));
+  WordList custom_words;
+  LoadDictionaryFileReliably(&custom_words, path);
+
+  // Add words.
+  custom_words.insert(custom_words.end(),
+                      dictionary_change->to_add().begin(),
+                      dictionary_change->to_add().end());
+
+  // Remove words.
+  std::sort(custom_words.begin(), custom_words.end());
+  std::sort(dictionary_change->to_remove().begin(),
+            dictionary_change->to_remove().end());
+  WordList remaining;
+  std::set_difference(custom_words.begin(),
+                      custom_words.end(),
+                      dictionary_change->to_remove().begin(),
+                      dictionary_change->to_remove().end(),
+                      std::back_inserter(remaining));
+  std::swap(custom_words, remaining);
+
+  SaveDictionaryFileReliably(&custom_words, path);
 }
 
-const WordList& SpellcheckCustomDictionary::GetWords() const {
-  return words_;
+// Converts |dictionary_change| into a sync change list. Does not take ownership
+// of the |dictionary_change|.
+syncer::SyncChangeList GetSyncChangeList(
+    const SpellcheckCustomDictionary::Change* dictionary_change) {
+  DCHECK(dictionary_change);
+  syncer::SyncChangeList sync_change_list;
+  std::string word;
+  for (WordList::const_iterator it = dictionary_change->to_add().begin();
+       it != dictionary_change->to_add().end();
+       ++it) {
+    word = *it;
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    sync_change_list.push_back(syncer::SyncChange(
+        FROM_HERE,
+        syncer::SyncChange::ACTION_ADD,
+        syncer::SyncData::CreateLocalData(word, word, specifics)));
+  }
+  for (WordList::const_iterator it = dictionary_change->to_remove().begin();
+       it != dictionary_change->to_remove().end();
+       ++it) {
+    word = *it;
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    sync_change_list.push_back(syncer::SyncChange(
+        FROM_HERE,
+        syncer::SyncChange::ACTION_DELETE,
+        syncer::SyncData::CreateLocalData(word, word, specifics)));
+  }
+  return sync_change_list;
 }
 
-void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
-    WordList* custom_words) {
+}  // namespace
+
+scoped_ptr<WordList> LoadDictionary(const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  LoadDictionaryFileReliably(custom_words);
+  scoped_ptr<WordList> custom_words(new WordList);
+  LoadDictionaryFileReliably(custom_words.get(), path);
   if (custom_words->empty())
-    return;
+    return custom_words.Pass();
 
   // Clean up the dictionary file contents by removing duplicates and invalid
   // words.
@@ -97,123 +196,152 @@ void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
                       custom_words->end());
   custom_words->erase(std::remove_if(custom_words->begin(),
                                      custom_words->end(),
-                                     IsInvalidWord),
+                                     custom_dictionary::IsInvalidWord),
                       custom_words->end());
 
-  SaveDictionaryFileReliably(*custom_words);
-}
+  SaveDictionaryFileReliably(custom_words.get(), path);
 
-void SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return custom_words.Pass();
+}
 
-  words_.clear();
-  if (custom_words)
-    std::swap(words_, *custom_words);
+void WriteWordToCustomDictionary(
+    const std::string& word,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change(
+      new SpellcheckCustomDictionary::Change);
+  dictionary_change->AddWord(word);
+  WriteAndEraseWordsInCustomDictionary(dictionary_change.Pass(), path);
+}
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded());
+void EraseWordFromCustomDictionary(
+    const std::string& word,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change(
+      new SpellcheckCustomDictionary::Change);
+  dictionary_change->RemoveWord(word);
+  WriteAndEraseWordsInCustomDictionary(dictionary_change.Pass(), path);
 }
 
-bool SpellcheckCustomDictionary::AddWord(const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!IsValidWord(word))
-    return false;
+}  // namespace custom_dictionary
 
-  if (!CustomWordAddedLocally(word))
-    return false;
+SpellcheckCustomDictionary::Change::Result::Result()
+    : invalid_(false),
+      duplicate_(false),
+      missing_(false) {
+}
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SpellcheckCustomDictionary::WriteWordToCustomDictionary,
-                 base::Unretained(this), word));
+SpellcheckCustomDictionary::Change::Result::Result(
+        const SpellcheckCustomDictionary::Change::Result& other)
+    : invalid_(other.detected_invalid_words()),
+      duplicate_(other.detected_duplicate_words()),
+      missing_(other.detected_missing_words()) {
+}
 
-  for (content::RenderProcessHost::iterator i(
-          content::RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    i.GetCurrentValue()->Send(new SpellCheckMsg_WordAdded(word));
-  }
+SpellcheckCustomDictionary::Change::Result::~Result() {
+}
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordAdded(word));
+SpellcheckCustomDictionary::Change::Change() {
+}
 
-  return true;
+SpellcheckCustomDictionary::Change::Change(
+    const SpellcheckCustomDictionary::Change* other) {
+  DCHECK(other);
+
+  AddWords(&other->to_add());
+  RemoveWords(&other->to_remove());
 }
 
-bool SpellcheckCustomDictionary::CustomWordAddedLocally(
-    const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(IsValidWord(word));
+SpellcheckCustomDictionary::Change::~Change() {
+}
 
-  WordList::iterator it = std::find(words_.begin(), words_.end(), word);
-  if (it == words_.end()) {
-    words_.push_back(word);
-    return true;
-  }
-  return false;
-  // TODO(rlp): record metrics on custom word size
+void SpellcheckCustomDictionary::Change::AddWord(const std::string& word) {
+  to_add_.push_back(word);
 }
 
-void SpellcheckCustomDictionary::WriteWordToCustomDictionary(
-    const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(IsValidWord(word));
+void SpellcheckCustomDictionary::Change::RemoveWord(const std::string& word) {
+  to_remove_.push_back(word);
+}
 
-  WordList custom_words;
-  LoadDictionaryFileReliably(&custom_words);
-  custom_words.push_back(word);
-  SaveDictionaryFileReliably(custom_words);
+void SpellcheckCustomDictionary::Change::AddWords(const WordList* words) {
+  to_add_.insert(to_add_.end(), words->begin(), words->end());
 }
 
-bool SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!IsValidWord(word))
-    return false;
+void SpellcheckCustomDictionary::Change::RemoveWords(const WordList* words) {
+  to_remove_.insert(to_remove_.end(), words->begin(), words->end());
+}
 
-  if (!CustomWordRemovedLocally(word))
-    return false;
+SpellcheckCustomDictionary::SpellcheckCustomDictionary(Profile* profile)
+    : SpellcheckDictionary(profile),
+      custom_dictionary_path_(),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      is_loaded_(false) {
+  DCHECK(profile);
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SpellcheckCustomDictionary::EraseWordFromCustomDictionary,
-                 base::Unretained(this), word));
+  custom_dictionary_path_ =
+      profile_->GetPath().Append(chrome::kCustomDictionaryFileName);
+}
 
-  for (content::RenderProcessHost::iterator i(
-          content::RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    i.GetCurrentValue()->Send(new SpellCheckMsg_WordRemoved(word));
-  }
+SpellcheckCustomDictionary::~SpellcheckCustomDictionary() {
+}
 
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordRemoved(word));
+const WordList& SpellcheckCustomDictionary::GetWords() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  return true;
+  return words_;
 }
 
-bool SpellcheckCustomDictionary::CustomWordRemovedLocally(
+SpellcheckCustomDictionary::Change::Result SpellcheckCustomDictionary::AddWord(
     const std::string& word) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(IsValidWord(word));
 
-  WordList::iterator it = std::find(words_.begin(), words_.end(), word);
-  if (it != words_.end()) {
-    words_.erase(it);
-    return true;
-  }
-  return false;
+  scoped_ptr<Change> dictionary_change(new Change);
+  dictionary_change->AddWord(word);
+  dictionary_change = Apply(dictionary_change.Pass());
+  Change::Result result(dictionary_change->result());
+  Notify(dictionary_change.get());
+  Sync(scoped_ptr<Change>(new Change(dictionary_change.get())));
+  Save(dictionary_change.Pass());
+  return result;
 }
 
-void SpellcheckCustomDictionary::EraseWordFromCustomDictionary(
-    const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(IsValidWord(word));
+SpellcheckCustomDictionary::Change::Result
+    SpellcheckCustomDictionary::CustomWordAddedLocally(
+        const std::string& word) {
+  scoped_ptr<Change> dictionary_change(new Change);
+  dictionary_change->AddWord(word);
+  dictionary_change = Apply(dictionary_change.Pass());
+  Notify(dictionary_change.get());
+  Sync(scoped_ptr<Change>(new Change(dictionary_change.get())));
+  return dictionary_change->result();
+}
 
-  WordList custom_words;
-  LoadDictionaryFileReliably(&custom_words);
-  if (custom_words.empty())
-    return;
+SpellcheckCustomDictionary::Change::Result
+    SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  WordList::iterator it = std::find(custom_words.begin(),
-                                    custom_words.end(),
-                                    word);
-  if (it != custom_words.end())
-    custom_words.erase(it);
+  scoped_ptr<Change> dictionary_change(new Change);
+  dictionary_change->RemoveWord(word);
+  dictionary_change = Apply(dictionary_change.Pass());
+  Change::Result result(dictionary_change->result());
+  Notify(dictionary_change.get());
+  Sync(scoped_ptr<Change>(new Change(dictionary_change.get())));
+  Save(dictionary_change.Pass());
+  return result;
+}
+
+SpellcheckCustomDictionary::Change::Result
+    SpellcheckCustomDictionary::CustomWordRemovedLocally(
+        const std::string& word) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  SaveDictionaryFileReliably(custom_words);
+  scoped_ptr<Change> dictionary_change(new Change);
+  dictionary_change->RemoveWord(word);
+  dictionary_change = Apply(dictionary_change.Pass());
+  Notify(dictionary_change.get());
+  Sync(scoped_ptr<Change>(new Change(dictionary_change.get())));
+  return dictionary_change->result();
 }
 
 void SpellcheckCustomDictionary::AddObserver(Observer* observer) {
@@ -228,58 +356,281 @@ void SpellcheckCustomDictionary::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-WordList* SpellcheckCustomDictionary::LoadDictionary() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+bool SpellcheckCustomDictionary::IsLoaded() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  WordList* custom_words = new WordList;
-  LoadDictionaryIntoCustomWordList(custom_words);
-  return custom_words;
+  return is_loaded_;
 }
 
-void SpellcheckCustomDictionary::SetCustomWordListAndDelete(
-    WordList* custom_words) {
+bool SpellcheckCustomDictionary::IsSyncing() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  SetCustomWordList(custom_words);
-  delete custom_words;
+  return !!sync_processor_.get();
 }
 
-void SpellcheckCustomDictionary::LoadDictionaryFileReliably(
-    WordList* custom_words) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+void SpellcheckCustomDictionary::OnLoaded(scoped_ptr<WordList> custom_words) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(custom_words.get());
 
-  // Load the contents and verify the checksum.
-  if (LoadFile(custom_dictionary_path_, custom_words))
-    return;
+  scoped_ptr<Change> dictionary_change(new Change);
+  dictionary_change->AddWords(custom_words.get());
+  dictionary_change = Apply(dictionary_change.Pass());
+  Sync(dictionary_change.Pass());
 
-  // Checksum is not valid. See if there's a backup.
-  FilePath backup = custom_dictionary_path_.AddExtension(BACKUP_EXTENSION);
-  if (!file_util::PathExists(backup))
-    return;
+  is_loaded_ = true;
+  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded());
+}
 
-  // Load the backup and verify its checksum.
-  if (!LoadFile(backup, custom_words))
-    return;
+void SpellcheckCustomDictionary::Load() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // Backup checksum is valid. Restore the backup.
-  file_util::CopyFile(backup, custom_dictionary_path_);
+  BrowserThread::PostTaskAndReplyWithResult<scoped_ptr<WordList> >(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&custom_dictionary::LoadDictionary,
+                 custom_dictionary_path_),
+      base::Bind(&SpellcheckCustomDictionary::OnLoaded,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void SpellcheckCustomDictionary::SaveDictionaryFileReliably(
-    const WordList& custom_words) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+syncer::SyncMergeResult SpellcheckCustomDictionary::MergeDataAndStartSyncing(
+    syncer::ModelType type,
+    const syncer::SyncDataList& initial_sync_data,
+    scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+    scoped_ptr<syncer::SyncErrorFactory> sync_error_handler) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!sync_processor_.get());
+  DCHECK(!sync_error_handler_.get());
+  DCHECK(sync_processor.get());
+  DCHECK(sync_error_handler.get());
+  DCHECK_EQ(syncer::DICTIONARY, type);
+
+  sync_processor_ = sync_processor.Pass();
+  sync_error_handler_ = sync_error_handler.Pass();
+
+  // Add remote words locally.
+  WordList sync_words;
+  std::string word;
+  for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
+     it != initial_sync_data.end();
+     ++it) {
+    DCHECK_EQ(syncer::DICTIONARY, it->GetDataType());
+    sync_words.push_back(it->GetSpecifics().dictionary().word());
+  }
+  scoped_ptr<Change> to_change_locally(new Change);
+  to_change_locally->AddWords(&sync_words);
+  to_change_locally = Apply(to_change_locally.Pass());

[Nicolas Zea, 2013/01/02 23:12:15]

It seems odd that you pass ownership, then use the pointer, then pass ownership
again. Can't this result in use after free and/or double free?

[please use gerrit instead, 2013/01/04 23:30:50]

I don't think use after free or double free could happen here, but I agree that
this code is strange. I have reworked it to pass a const pointer instead of
scoped_ptr.

+  Notify(to_change_locally.get());
+  Save(to_change_locally.Pass());
+
+  // Add as many as possible local words remotely.
+  std::sort(words_.begin(), words_.end());
+  std::sort(sync_words.begin(), sync_words.end());
+  scoped_ptr<Change> to_change_remotely(new Change);
+  std::set_difference(words_.begin(),
+                      words_.end(),
+                      sync_words.begin(),
+                      sync_words.end(),
+                      std::back_inserter(to_change_remotely->to_add()));
+  syncer::SyncMergeResult result(type);
+  result.set_error(Sync(to_change_remotely.Pass()));
+  return result;
+}
 
-  std::stringstream content;
-  for (WordList::const_iterator it = custom_words.begin();
-       it != custom_words.end();
+void SpellcheckCustomDictionary::StopSyncing(syncer::ModelType type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(syncer::DICTIONARY, type);
+
+  sync_processor_.reset();
+  sync_error_handler_.reset();
+}
+
+syncer::SyncDataList SpellcheckCustomDictionary::GetAllSyncData(
+    syncer::ModelType type) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(syncer::DICTIONARY, type);
+
+  syncer::SyncDataList data;
+  std::string word;
+  size_t i = 0;
+  for (WordList::const_iterator it = words_.begin();
+       it != words_.end() &&
+           i < chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS;
+       ++it, ++i) {
+    word = *it;
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    data.push_back(syncer::SyncData::CreateLocalData(word, word, specifics));
+  }
+  return data;
+}
+
+syncer::SyncError SpellcheckCustomDictionary::ProcessSyncChanges(
+    const tracked_objects::Location& from_here,
+    const syncer::SyncChangeList& change_list) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  scoped_ptr<Change> dictionary_change(new Change);
+  for (syncer::SyncChangeList::const_iterator it = change_list.begin();
+       it != change_list.end();
        ++it) {
-    content << *it << '\n';
+    DCHECK(it->IsValid());
+    std::string word = it->sync_data().GetSpecifics().dictionary().word();
+    switch (it->change_type()) {
+      case syncer::SyncChange::ACTION_ADD:
+        dictionary_change->AddWord(word);

[Nicolas Zea, 2013/01/02 23:12:15]

I think this (and the RemoveWord below) is going to itself trigger a call to
Sync(..) with word, which although Sync should be resistant too, seems wasteful.
What we've done in the past is use a processing_sync_changes_ within a
base::AutoReset<bool>, that when set means you don't call Sync(..).

[please use gerrit instead, 2013/01/04 23:30:50]

Apologies for the confusing code :-). This will not trigger a sync, because
"dictionary_change->AddWord(word)" is "Change::AddWord()" which does only
"to_add_.push_back(word)". This is different from
"SpellcheckCustomDictionary::AddWord()", which will trigger a sync.

Below you see the calls to Apply(), Notify(), and Save(). We accomplish not
syncing these changes by not calling Sync() below. That is, all methods that
send data to the sync server explicitly call Sync() in the method body.

+        break;
+      case syncer::SyncChange::ACTION_DELETE:
+        dictionary_change->RemoveWord(word);
+        break;
+      default:
+        return sync_error_handler_->CreateAndUploadError(
+            FROM_HERE,
+            "Processing sync changes failed on change type " +
+                syncer::SyncChange::ChangeTypeToString(it->change_type()));
+    }
   }
-  std::string checksum = base::MD5String(content.str());
-  content << CHECKSUM_PREFIX << checksum;
 
-  file_util::CopyFile(custom_dictionary_path_,
-                      custom_dictionary_path_.AddExtension(BACKUP_EXTENSION));
-  base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_,
-                                                 content.str());
+  dictionary_change = Apply(dictionary_change.Pass());
+  Notify(dictionary_change.get());
+  Save(dictionary_change.Pass());
+
+  return syncer::SyncError();
+}
+
+// TODO(rlp): record metrics on custom word size
+scoped_ptr<SpellcheckCustomDictionary::Change>
+    SpellcheckCustomDictionary::Apply(
+        scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(dictionary_change.get());
+
+  // Do not add duplicate words.

[Nicolas Zea, 2013/01/02 23:12:15]

consider skipping all this if to_add is empty (and similarly skipping the remove
section if to_remove is empty)

[please use gerrit instead, 2013/01/04 23:30:50]

Great idea! Skipping sections on empty as you suggested...

+  std::sort(dictionary_change->to_add().begin(),
+            dictionary_change->to_add().end());
+  std::sort(words_.begin(), words_.end());
+  WordList to_add;
+  std::set_difference(dictionary_change->to_add().begin(),
+                      dictionary_change->to_add().end(),
+                      words_.begin(),
+                      words_.end(),
+                      std::back_inserter(to_add));
+  to_add.erase(std::unique(to_add.begin(), to_add.end()),
+               to_add.end());
+  if (dictionary_change->to_add().size() != to_add.size())
+    dictionary_change->result().set_detected_duplicate_words();
+
+  // Do not add invalid words.
+  size_t size = to_add.size();
+  to_add.erase(std::remove_if(to_add.begin(),
+                              to_add.end(),
+                              custom_dictionary::IsInvalidWord),
+               to_add.end());
+  if (size != to_add.size())
+    dictionary_change->result().set_detected_invalid_words();
+
+  // Add the words to the dictionary.
+  words_.insert(words_.end(), to_add.begin(), to_add.end());
+
+  // Clean up the dictionary change by removing duplicates and invalid words.
+  std::swap(dictionary_change->to_add(), to_add);
+
+  // Do not remove words that are missing from the dictionary.
+  std::sort(dictionary_change->to_remove().begin(),
+            dictionary_change->to_remove().end());
+  std::sort(words_.begin(), words_.end());
+  WordList to_remove;
+  std::set_intersection(words_.begin(),
+                        words_.end(),
+                        dictionary_change->to_remove().begin(),
+                        dictionary_change->to_remove().end(),
+                        std::back_inserter(to_remove));
+  if (dictionary_change->to_remove().size() > to_remove.size())
+    dictionary_change->result().set_detected_missing_words();
+
+  // Remove the words from the dictionary.
+  WordList updated_words;
+  std::set_difference(words_.begin(),
+                      words_.end(),
+                      to_remove.begin(),
+                      to_remove.end(),
+                      std::back_inserter(updated_words));
+  std::swap(words_, updated_words);
+
+  // Clean up the dictionary change by removing missing words.
+  std::swap(dictionary_change->to_remove(), to_remove);
+
+  return dictionary_change.Pass();
+}
+
+void SpellcheckCustomDictionary::Save(
+    scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(dictionary_change.get());
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(
+          &custom_dictionary::WriteAndEraseWordsInCustomDictionary,
+          base::Passed(dictionary_change.Pass()),
+          custom_dictionary_path_));
+}
+
+syncer::SyncError SpellcheckCustomDictionary::Sync(
+    scoped_ptr<SpellcheckCustomDictionary::Change> dictionary_change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(dictionary_change.get());
+
+  syncer::SyncError error;
+  if (!IsSyncing())

[Nicolas Zea, 2013/01/02 23:12:15]

return early if no changes?

[please use gerrit instead, 2013/01/04 23:30:50]

Good idea. Returning early if there are no changes.

+    return error;
+
+  // Truncate remote dictionary change to avoid syncing more than maximum
+  // syncable words.
+  if (words_.size() >
+          chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS) {
+    size_t trim_size = dictionary_change->to_add().size() +
+        chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS -
+        words_.size();
+    dictionary_change->to_add().erase(
+        dictionary_change->to_add().begin() + trim_size,
+        dictionary_change->to_add().end());
+  }
+
+  // Send the changes to the sync processor.
+  error = sync_processor_->ProcessSyncChanges(
+      FROM_HERE,
+      custom_dictionary::GetSyncChangeList(dictionary_change.get()));
+
+  if (error.IsSet())
+    return error;
+
+  // Turn off sync if total number of words is more than maximum syncable words.

[Nicolas Zea, 2013/01/02 23:12:15]

Shouldn't this happen before pushing the change to sync? Also, I didn't see this
check in MergeDataAndStartSyncing. Should it be there?

Note that this is only disabling sync from the local service's perspective. Sync
itself is not away you've called StopSyncing, and might still push changes to
the service via ProcessSyncChanges. Not sure if that makes a difference though.

[please use gerrit instead, 2013/01/04 23:30:50]

The reason why I prevent further syncing at the end of the method is I want to
sync as much as possible first. For example, If the user has computer #1 with
1000 words and computer #2 with 400 different words, then I want both computers
to have 1300 words after sync and then stop syncing.

+  if (words_.size() >
+          chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS) {
+    std::stringstream ss;
+    ss << "Turning off sync for custom spelling dictionary with "
+       << words_.size()
+       << " words. Sync limit is "
+       << chrome::spellcheck_common::MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS
+       << " words.";
+    error = sync_error_handler_->CreateAndUploadError(FROM_HERE, ss.str());
+    StopSyncing(syncer::DICTIONARY);
+  }
+
+  return error;
+}
+
+void  SpellcheckCustomDictionary::Notify(
+    const SpellcheckCustomDictionary::Change* dictionary_change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(dictionary_change);
+
+  if (!IsLoaded() || dictionary_change->empty())
+    return;
+
+  FOR_EACH_OBSERVER(Observer,
+                    observers_,
+                    OnCustomDictionaryChanged(dictionary_change));
 }