Sync user's custom spellcheck dictionary

chrome/browser/spellchecker/spellcheck_custom_dictionary.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/spellchecker/spellcheck_custom_dictionary.h"
 
 #include <functional>
 
 #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"
 
 using content::BrowserThread;
 using chrome::spellcheck_common::WordList;
 
 namespace {
 
+const size_t MAX_SYNC_SIZE = 1300;

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

Please document vars. (Specifically, what does "1300" mean in this context)?

Name might be better of as MAX_SYNCABLE_DICTIONARY_SIZE_IN_WORDS or something.

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

Done. I renamed the variable, commented it, and moved it to spellcheck_common.h
so that unit, integration, and performance tests can use it, too.

 const FilePath::CharType BACKUP_EXTENSION[] = FILE_PATH_LITERAL("backup");
 const char CHECKSUM_PREFIX[] = "checksum_v1 = ";
 
+const std::string DictionaryTooBigToSyncErrorMessage(size_t dictionary_size) {

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

Shouldn't we i18n error messages?

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

This message is for server-side logging. It will be uploaded using
CreateAndUploadError() method on the sync error handler. Other syncable services
do not localize it either. See
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/themes/the...
for example.

+  return "Turning off sync for custom spelling dictionary with " +
+      base::Uint64ToString(dictionary_size) + " words. Sync limit is " +
+      base::Uint64ToString(MAX_SYNC_SIZE) + " words.";
+}
+
 // 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) {
   lines->clear();
   std::string contents;
   file_util::ReadFileToString(file_path, &contents);
   size_t pos = contents.rfind(CHECKSUM_PREFIX);
   if (pos != std::string::npos) {
     std::string checksum = contents.substr(pos + strlen(CHECKSUM_PREFIX));
     contents = contents.substr(0, pos);
     if (checksum != base::MD5String(contents))
       return false;
   }
   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);
-}
-
 bool IsInvalidWord(const std::string& word) {
-  return !IsValidWord(word);
+  return !IsStringUTF8(word) || word.length() > 128 || word.empty() ||

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

128 is a magic number. Is there a constant for this somewhere?

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

Great catch! The length check needs to match
HunspellEngine::AddWordsToHunspell(), which is (word.length() < MAXWORDLEN),
where MAXWORDLEN is defined as 100 in
third_party/hunspell/src/hunspell/atypes.hxx. I changed IsInvalidWord() to check
for word.length() >= MAXWORDLEN.

+      word.find_first_of(kWhitespaceASCII) != std::string::npos;
 }
 
 }  // namespace
 
 SpellcheckCustomDictionary::SpellcheckCustomDictionary(Profile* profile)
     : SpellcheckDictionary(profile),
       custom_dictionary_path_(),
-      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      is_loaded_(false) {
   DCHECK(profile);
   custom_dictionary_path_ =
       profile_->GetPath().Append(chrome::kCustomDictionaryFileName);
 }
 
 SpellcheckCustomDictionary::~SpellcheckCustomDictionary() {
 }
 
 void SpellcheckCustomDictionary::Load() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   BrowserThread::PostTaskAndReplyWithResult<WordList*>(
       BrowserThread::FILE,
       FROM_HERE,
       base::Bind(&SpellcheckCustomDictionary::LoadDictionary,
-                 base::Unretained(this)),
+                 custom_dictionary_path_),
       base::Bind(&SpellcheckCustomDictionary::SetCustomWordListAndDelete,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 const WordList& SpellcheckCustomDictionary::GetWords() const {
   return words_;
 }
 
+// static
 void SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
-    WordList* custom_words) {
+    WordList* custom_words,
+    const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  LoadDictionaryFileReliably(custom_words);
+  LoadDictionaryFileReliably(custom_words, path);
   if (custom_words->empty())
     return;
 
   // Clean up the dictionary file contents by removing duplicates and invalid
   // words.
   std::sort(custom_words->begin(), custom_words->end());
   custom_words->erase(std::unique(custom_words->begin(), custom_words->end()),
                       custom_words->end());
   custom_words->erase(std::remove_if(custom_words->begin(),
                                      custom_words->end(),
                                      IsInvalidWord),
                       custom_words->end());
 
-  SaveDictionaryFileReliably(*custom_words);
+  SaveDictionaryFileReliably(*custom_words, path);
 }
 
 void SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   words_.clear();
   if (custom_words)
     std::swap(words_, *custom_words);
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded());
 }
 
 bool SpellcheckCustomDictionary::AddWord(const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!IsValidWord(word))
-    return false;
-
-  if (!CustomWordAddedLocally(word))
-    return false;
-
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SpellcheckCustomDictionary::WriteWordToCustomDictionary,
-                 base::Unretained(this), word));
-
-  for (content::RenderProcessHost::iterator i(
-          content::RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    i.GetCurrentValue()->Send(new SpellCheckMsg_WordAdded(word));
-  }
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordAdded(word));
-
-  return true;
+  WordList to_add;
+  to_add.push_back(word);
+  WordList to_remove;
+  return UpdateWords(&to_add, &to_remove);
 }
 
 bool SpellcheckCustomDictionary::CustomWordAddedLocally(
     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_.push_back(word);
-    return true;
-  }
-  return false;
-  // TODO(rlp): record metrics on custom word size
-}
-
+  WordList to_add;
+  to_add.push_back(word);
+  WordList to_remove;
+  return CustomWordsUpdatedLocally(&to_add, &to_remove);
+}
+
+// static
 void SpellcheckCustomDictionary::WriteWordToCustomDictionary(
-    const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(IsValidWord(word));
-
-  WordList custom_words;
-  LoadDictionaryFileReliably(&custom_words);
-  custom_words.push_back(word);
-  SaveDictionaryFileReliably(custom_words);
+    const std::string& word,
+    const FilePath& path) {
+  WordList to_write;
+  to_write.push_back(word);
+  WordList to_erase;
+  WriteAndEraseWordsInCustomDictionary(&to_write, &to_erase, path);
 }
 
 bool SpellcheckCustomDictionary::RemoveWord(const std::string& word) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!IsValidWord(word))
-    return false;
-
-  if (!CustomWordRemovedLocally(word))
-    return false;
-
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(&SpellcheckCustomDictionary::EraseWordFromCustomDictionary,
-                 base::Unretained(this), word));
-
-  for (content::RenderProcessHost::iterator i(
-          content::RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    i.GetCurrentValue()->Send(new SpellCheckMsg_WordRemoved(word));
-  }
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryWordRemoved(word));
-
-  return true;
+  WordList to_add;
+  WordList to_remove;
+  to_remove.push_back(word);
+  return UpdateWords(&to_add, &to_remove);
 }
 
 bool SpellcheckCustomDictionary::CustomWordRemovedLocally(
     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;
-}
-
+  WordList to_add;

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

There's a potential refactor here, since we have so many instances of "create
two lists, add/remove one word".

How about having a DictionaryChange class that encapsulates that? (Bonus points
- you can get rid of the weird bool return value, and instead have a
DidDetectInvalidWords() on the DictionaryChange)

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

Done.

+  WordList to_remove;
+  to_remove.push_back(word);
+  return CustomWordsUpdatedLocally(&to_add, &to_remove);
+}
+
+// static
 void SpellcheckCustomDictionary::EraseWordFromCustomDictionary(
-    const std::string& word) {
+    const std::string& word,
+    const FilePath& path) {
+  WordList to_write;
+  WordList to_erase;
+  to_erase.push_back(word);
+  WriteAndEraseWordsInCustomDictionary(&to_write, &to_erase, path);
+}
+
+void SpellcheckCustomDictionary::AddObserver(Observer* observer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  observers_.AddObserver(observer);
+}
+
+void SpellcheckCustomDictionary::RemoveObserver(Observer* observer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  observers_.RemoveObserver(observer);
+}
+
+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();
+
+  WordList to_add_locally;

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

Please comment what you're actually doing here. (Goes for all the sync code)

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

Done.

+  WordList to_add_remotely(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());
+    word = it->GetSpecifics().dictionary().word();
+    WordList::iterator found = std::find(

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

I might be wrong, but I think most of this can be simplified to use
std::symmetric_set_difference

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

Done. Used std::set_difference and std::set_intersection.

+        to_add_remotely.begin(), to_add_remotely.end(), word);
+    if (found == to_add_remotely.end())
+      to_add_locally.push_back(word);
+    else
+      to_add_remotely.erase(found);
+  }
+
+  WordList to_remove;
+  UpdateWords(&to_add_locally, &to_remove, true);
+
+  syncer::SyncChangeList local_changes;
+  size_t i = initial_sync_data.size();
+  for (WordList::iterator it = to_add_remotely.begin();
+       it != to_add_remotely.end() && i < MAX_SYNC_SIZE;
+       ++it, ++i) {
+    word = *it;
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    local_changes.push_back(syncer::SyncChange(
+        FROM_HERE,
+        syncer::SyncChange::ACTION_ADD,
+        syncer::SyncData::CreateLocalData(word, word, specifics)));
+  }
+
+  syncer::SyncMergeResult result(type);
+  result.set_error(sync_processor_->ProcessSyncChanges(FROM_HERE,
+                                                       local_changes));
+  if (result.error().IsSet())
+    return result;
+
+  if (words_.size() > MAX_SYNC_SIZE) {
+    result.set_error(sync_error_handler_->CreateAndUploadError(
+        FROM_HERE,
+        DictionaryTooBigToSyncErrorMessage(words_.size())));
+    StopSyncing(syncer::DICTIONARY);
+  }
+
+  return result;
+}
+
+void SpellcheckCustomDictionary::StopSyncing(syncer::ModelType type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(syncer::DICTIONARY, type);
+
+  sync_processor_.reset();
+  sync_error_handler_.reset();
+}
+
+void SpellcheckCustomDictionary::StopSyncingForTesting() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::IgnoreResult(sync_processor_.release());

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

Why not sync_processor.reset()?

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

Removed StopSyncingForTesting(). Done.

+  sync_error_handler_.reset();
+}
+
+bool SpellcheckCustomDictionary::IsLoaded() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return is_loaded_;
+}
+
+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 < MAX_SYNC_SIZE;
+       ++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));
+
+  WordList to_add;
+  WordList to_remove;
+  for (syncer::SyncChangeList::const_iterator it = change_list.begin();
+       it != change_list.end();
+       ++it) {
+    DCHECK(it->IsValid());
+    std::string word = it->sync_data().GetSpecifics().dictionary().word();
+    switch (it->change_type()) {
+      case syncer::SyncChange::ACTION_ADD:
+        to_add.push_back(word);
+        break;
+      case syncer::SyncChange::ACTION_DELETE:
+        to_remove.push_back(word);
+        break;
+      default:
+        NOTREACHED() << "Unexpected sync change state.";

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

I think we should either do a NOTREACHED _or_ the appropriate error handling.
(Cf. Chromium style guide/DCHECK use)

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

Done. Handling error condition normally.

+        return sync_error_handler_->CreateAndUploadError(
+            FROM_HERE,
+            "Processing sync changes failed on change type " +
+                syncer::SyncChange::ChangeTypeToString(it->change_type()));
+    }
+  }
+
+  UpdateWords(&to_add, &to_remove, true);
+
+  return syncer::SyncError();
+}
+
+// static
+WordList* SpellcheckCustomDictionary::LoadDictionary(const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DCHECK(IsValidWord(word));
-
-  WordList custom_words;
-  LoadDictionaryFileReliably(&custom_words);
-  if (custom_words.empty())
-    return;
-
-  WordList::iterator it = std::find(custom_words.begin(),
-                                    custom_words.end(),
-                                    word);
-  if (it != custom_words.end())
-    custom_words.erase(it);
-
-  SaveDictionaryFileReliably(custom_words);
-}
-
-void SpellcheckCustomDictionary::AddObserver(Observer* observer) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  observers_.AddObserver(observer);
-}
-
-void SpellcheckCustomDictionary::RemoveObserver(Observer* observer) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  observers_.RemoveObserver(observer);
-}
-
-WordList* SpellcheckCustomDictionary::LoadDictionary() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   WordList* custom_words = new WordList;
-  LoadDictionaryIntoCustomWordList(custom_words);
+  LoadDictionaryIntoCustomWordList(custom_words, path);

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

Can we get rid of LoadDictionary and have API consumers call
LoadDictionaryIntoCustomWordList directly?

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

Got rid of LoadDictionaryIntoCustomWordList in favor of LoadDictionary.

   return custom_words;
 }
 
 void SpellcheckCustomDictionary::SetCustomWordListAndDelete(
     WordList* custom_words) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  SetCustomWordList(custom_words);
+  if (sync_processor_.get()) {
+    WordList to_remove;
+    CustomWordsUpdatedLocally(custom_words, &to_remove);
+  } else {
+    SetCustomWordList(custom_words);
+  }
+
   delete custom_words;
+
+  is_loaded_ = true;
+  FOR_EACH_OBSERVER(Observer, observers_, OnCustomDictionaryLoaded());
 }
 
+// static
 void SpellcheckCustomDictionary::LoadDictionaryFileReliably(
-    WordList* custom_words) {
+    WordList* custom_words,
+    const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Load the contents and verify the checksum.
-  if (LoadFile(custom_dictionary_path_, custom_words))
+  if (LoadFile(path, custom_words))
     return;
 
   // Checksum is not valid. See if there's a backup.
-  FilePath backup = custom_dictionary_path_.AddExtension(BACKUP_EXTENSION);
+  FilePath backup = path.AddExtension(BACKUP_EXTENSION);
   if (!file_util::PathExists(backup))
     return;
 
   // Load the backup and verify its checksum.
   if (!LoadFile(backup, custom_words))
     return;
 
   // Backup checksum is valid. Restore the backup.
-  file_util::CopyFile(backup, custom_dictionary_path_);
+  file_util::CopyFile(backup, path);
 }
 
+// static
 void SpellcheckCustomDictionary::SaveDictionaryFileReliably(
-    const WordList& custom_words) {
+    const WordList& custom_words,
+    const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   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(custom_dictionary_path_,
-                      custom_dictionary_path_.AddExtension(BACKUP_EXTENSION));
-  base::ImportantFileWriter::WriteFileAtomically(custom_dictionary_path_,
-                                                 content.str());
+  file_util::CopyFile(path,path.AddExtension(BACKUP_EXTENSION));
+  base::ImportantFileWriter::WriteFileAtomically(path, content.str());
 }
+
+bool SpellcheckCustomDictionary::UpdateWords(
+    WordList* to_add,
+    WordList* to_remove) {
+  return UpdateWords(to_add, to_remove, false);
+}
+
+bool SpellcheckCustomDictionary::UpdateWords(
+    WordList* to_add,
+    WordList* to_remove,
+    bool is_sync_update) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(to_add);
+  DCHECK(to_remove);
+
+  bool success = CustomWordsUpdatedLocally(to_add, to_remove, is_sync_update);
+
+  WordList* to_write = new WordList(*to_add);
+  WordList* to_erase = new WordList(*to_remove);
+  BrowserThread::PostTaskAndReply(

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

Since the reply task does nothing but delete to_write/to_erase, why not simply
PostTask with base::Owned?

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

Posting a task with base::Passed(scoped_ptr).

+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(
+          &SpellcheckCustomDictionary::WriteAndEraseWordsInCustomDictionary,
+          to_write,
+          to_erase,
+          custom_dictionary_path_),
+      base::Bind(
+          &SpellcheckCustomDictionary::OnWriteAndEraseFinished,
+          base::Unretained(this),
+          to_write,
+          to_erase));
+
+  return success;
+}
+
+bool SpellcheckCustomDictionary::CustomWordsUpdatedLocally(
+    WordList* to_add,
+    WordList* to_remove) {
+  return CustomWordsUpdatedLocally(to_add, to_remove, false);
+}
+
+// TODO(rlp): record metrics on custom word size
+bool SpellcheckCustomDictionary::CustomWordsUpdatedLocally(
+    WordList* to_add,
+    WordList* to_remove,
+    bool is_sync_update) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(to_add);
+  DCHECK(to_remove);
+
+  std::string word;
+  WordList cannot_add;

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

Why not use std::remove_if?

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

Done.

+  for (WordList::iterator it = to_add->begin(); it != to_add->end(); ++it) {
+    word = *it;
+    if (IsInvalidWord(word) ||
+        std::find(words_.begin(), words_.end(), word) != words_.end()) {
+      cannot_add.push_back(word);
+    }
+  }
+
+  for (WordList::iterator it = cannot_add.begin(); it != cannot_add.end(); ++it)
+    to_add->erase(std::find(to_add->begin(), to_add->end(), *it));
+
+  syncer::SyncChangeList local_changes;
+  for (WordList::iterator it = to_add->begin(); it != to_add->end(); ++it) {
+    word = *it;
+    words_.push_back(word);
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    local_changes.push_back(syncer::SyncChange(
+        FROM_HERE,
+        syncer::SyncChange::ACTION_ADD,
+        syncer::SyncData::CreateLocalData(word, word, specifics)));
+  }
+
+  WordList cannot_remove;

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

remove_if

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

Done.

+  for (WordList::iterator it = to_remove->begin();
+       it != to_remove->end();
+       ++it) {
+    word = *it;
+    if (std::find(words_.begin(), words_.end(), word) == words_.end())
+      cannot_remove.push_back(word);
+  }
+
+  for (WordList::iterator it = cannot_remove.begin();
+       it != cannot_remove.end();
+       ++it) {
+    to_remove->erase(std::find(to_remove->begin(), to_remove->end(), *it));
+  }
+
+  for (WordList::iterator it = to_remove->begin();
+       it != to_remove->end();
+       ++it) {
+    word = *it;
+    words_.erase(std::find(words_.begin(), words_.end(), word));
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_dictionary()->set_word(word);
+    local_changes.push_back(syncer::SyncChange(
+        FROM_HERE,
+        syncer::SyncChange::ACTION_DELETE,
+        syncer::SyncData::CreateLocalData(word, word, specifics)));
+  }
+
+  FOR_EACH_OBSERVER(Observer,

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

If you go with the DictionaryChange object, just one notification
(OnCustomDictionaryChanged) needed.

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

Done.

+                    observers_,
+                    OnCustomDictionaryWordsAdded(*to_add));
+  FOR_EACH_OBSERVER(Observer,
+                    observers_,
+                    OnCustomDictionaryWordsRemoved(*to_remove));
+
+  bool success = cannot_add.empty() && cannot_remove.empty();
+  if (is_sync_update || !sync_processor_.get())
+    return success;
+
+  if (words_.size() <= MAX_SYNC_SIZE) {
+    sync_processor_->ProcessSyncChanges(FROM_HERE, local_changes);
+  } else {
+    sync_error_handler_->CreateAndUploadError(
+        FROM_HERE,
+        DictionaryTooBigToSyncErrorMessage(words_.size()));
+    StopSyncing(syncer::DICTIONARY);
+  }
+
+  return success;
+}
+
+// static
+void SpellcheckCustomDictionary::WriteAndEraseWordsInCustomDictionary(
+    const WordList* to_write,
+    const WordList* to_erase,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  WordList custom_words;
+  LoadDictionaryFileReliably(&custom_words, path);
+
+  for (WordList::const_iterator it = to_write->begin();

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

custom_words.insert(custom_words.end(), to_write.begin(), to_write.end())

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

Done.

+       it != to_write->end();
+       ++it) {
+    custom_words.push_back(*it);
+  }
+
+  for (WordList::const_iterator it = to_erase->begin();

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

If custom_words is sorted, set_difference does the trick. If to_erase is ever
large, we should do that, since otherwise O(mxn)

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

Done.

+       it != to_erase->end();
+       ++it) {
+    custom_words.erase(std::find(custom_words.begin(),
+                                 custom_words.end(),
+                                 *it));
+  }
+
+  SaveDictionaryFileReliably(custom_words, path);
+}
+
+void SpellcheckCustomDictionary::OnWriteAndEraseFinished(

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

Can be killed - see above.

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

Done.

+    const WordList* written,
+    const WordList* erased) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  delete written;
+  delete erased;
+}