Sync user's custom spellcheck dictionary

Sync user's custom spellcheck dictionary

This CL is initial work for for syncing user dictionary across multiple
computers. The sync is hidden behind --enable-sync-dictionary flag for now. Do
not flip this flag unless your are connected to a sync server that supports
dictionary sync. If you don't know whether your sync server supports dictionary
sync, then most likely it does not.

BUG=51636
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178178

[groby-ooo-7-16, 2012-12-05 01:12:13]

Go Rouslan!

[please use gerrit instead, 2012-12-19 04:06:36]

PTAL. This change list adds syncing for custom spellcheck dictionary. This
feature is hidden behind a flag for now. The sync works over the local python
server that you can run on your machine. The sync team needs to implement the
server-side changes before we can dogfood this feature with a wider audience.

Jhawkins - chrome/browser/about_flags.cc

    Letting users flip the --enable-sync-dictionary flag from chrome://flags.

Csilv - chrome/browser/resources/*
        chrome/browser/ui/webui/*

    Changes to UI for showing the dictionary as one of the syncable data types
    in chrome://settings/setupSync. Also showing sync changes in
    chrome://settings/editDictionary as they come in.

Groby - chrome/browser/spellchecker/*
        chrome/renderer/spellchecker/*

    The changes necessary in the custom spellchecker dictionary to hook into the
    sync infrastructure. Also unit tests for the new sync methods.

Akalin - chrome/browser/sync/*
         net/tools/testserver/chromiumsync.py
         sync/*

    The changes necessary in the sync infrastructure to allow syncing of the
    dictionary. Also integration and performance tests for syncing the
    dictionary.

Tsepez - chrome/common/spellcheck_messages.h

    Changed SpellCheckMsg_WordAdded/Removed(string) to
    SpellCheckMsg_WordsAdded/Removed(vector).

[Tom Sepez, 2012-12-19 18:08:36]

spellcheck_messages.h LGTM.  Thanks for fixing the spelling errors in
spellcheck_messages.h, too.

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

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:28: const size_t
MAX_SYNC_SIZE = 1300;
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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:32: const
std::string DictionaryTooBigToSyncErrorMessage(size_t dictionary_size) {
Shouldn't we i18n error messages?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:58: return
!IsStringUTF8(word) || word.length() > 128 || word.empty() ||
128 is a magic number. Is there a constant for this somewhere?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:158: WordList
to_add;
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)

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:201: WordList
to_add_locally;
Please comment what you're actually doing here. (Goes for all the sync code)

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:209:
WordList::iterator found = std::find(
I might be wrong, but I think most of this can be simplified to use
std::symmetric_set_difference

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:261:
base::IgnoreResult(sync_processor_.release());
Why not sync_processor.reset()?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:309: NOTREACHED() <<
"Unexpected sync change state.";
I think we should either do a NOTREACHED _or_ the appropriate error handling.
(Cf. Chromium style guide/DCHECK use)

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:327:
LoadDictionaryIntoCustomWordList(custom_words, path);
Can we get rid of LoadDictionary and have API consumers call
LoadDictionaryIntoCustomWordList directly?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:408:
BrowserThread::PostTaskAndReply(
Since the reply task does nothing but delete to_write/to_erase, why not simply
PostTask with base::Owned?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:441: WordList
cannot_add;
Why not use std::remove_if?

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:465: WordList
cannot_remove;
remove_if

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:493:
FOR_EACH_OBSERVER(Observer,
If you go with the DictionaryChange object, just one notification
(OnCustomDictionaryChanged) needed.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:526: for
(WordList::const_iterator it = to_write->begin();
custom_words.insert(custom_words.end(), to_write.begin(), to_write.end())

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:532: for
(WordList::const_iterator it = to_erase->begin();
If custom_words is sorted, set_difference does the trick. If to_erase is ever
large, we should do that, since otherwise O(mxn)

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:543: void
SpellcheckCustomDictionary::OnWriteAndEraseFinished(
Can be killed - see above.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:51: static void
LoadDictionaryIntoCustomWordList(
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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:85: // Overridden
from syncer::SyncableService:
Personal preference: I like overridden interfaces together, and either at the
top or bottom of the public section.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:134: // duplicate
words that were not added, missing words that could not be
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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:182:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
Please document member vars.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:28: const
size_t LIMIT = 1300;
Please document. Or use a shared constant.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:288:
MessageLoop::current()->RunUntilIdle();
Why not run them during TearDown()? It seems _every_ test needs to do this at
the end, so TearDown seems the logical place.

[please use gerrit instead, 2012-12-22 03:20:18]

Groby: I've addressed your comments. PTAL.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:28: const size_t
MAX_SYNC_SIZE = 1300;
On 2012/12/19 22:16:00, groby wrote:
> 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.

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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:32: const
std::string DictionaryTooBigToSyncErrorMessage(size_t dictionary_size) {
On 2012/12/19 22:16:00, groby wrote:
> Shouldn't we i18n error messages?

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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:58: return
!IsStringUTF8(word) || word.length() > 128 || word.empty() ||
On 2012/12/19 22:16:00, groby wrote:
> 128 is a magic number. Is there a constant for this somewhere?

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.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:158: WordList
to_add;
On 2012/12/19 22:16:00, groby wrote:
> 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)

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:201: WordList
to_add_locally;
On 2012/12/19 22:16:00, groby wrote:
> Please comment what you're actually doing here. (Goes for all the sync code)

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:209:
WordList::iterator found = std::find(
On 2012/12/19 22:16:00, groby wrote:
> I might be wrong, but I think most of this can be simplified to use
> std::symmetric_set_difference

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

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:261:
base::IgnoreResult(sync_processor_.release());
On 2012/12/19 22:16:00, groby wrote:
> Why not sync_processor.reset()?

Removed StopSyncingForTesting(). Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:309: NOTREACHED() <<
"Unexpected sync change state.";
On 2012/12/19 22:16:00, groby wrote:
> I think we should either do a NOTREACHED _or_ the appropriate error handling.
> (Cf. Chromium style guide/DCHECK use)

Done. Handling error condition normally.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:327:
LoadDictionaryIntoCustomWordList(custom_words, path);
On 2012/12/19 22:16:00, groby wrote:
> Can we get rid of LoadDictionary and have API consumers call
> LoadDictionaryIntoCustomWordList directly?

Got rid of LoadDictionaryIntoCustomWordList in favor of LoadDictionary.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:408:
BrowserThread::PostTaskAndReply(
On 2012/12/19 22:16:00, groby wrote:
> Since the reply task does nothing but delete to_write/to_erase, why not simply
> PostTask with base::Owned?

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

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:441: WordList
cannot_add;
On 2012/12/19 22:16:00, groby wrote:
> Why not use std::remove_if?

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:465: WordList
cannot_remove;
On 2012/12/19 22:16:00, groby wrote:
> remove_if

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:493:
FOR_EACH_OBSERVER(Observer,
On 2012/12/19 22:16:00, groby wrote:
> If you go with the DictionaryChange object, just one notification
> (OnCustomDictionaryChanged) needed.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:526: for
(WordList::const_iterator it = to_write->begin();
On 2012/12/19 22:16:00, groby wrote:
> custom_words.insert(custom_words.end(), to_write.begin(), to_write.end())

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:532: for
(WordList::const_iterator it = to_erase->begin();
On 2012/12/19 22:16:00, groby wrote:
> If custom_words is sorted, set_difference does the trick. If to_erase is ever
> large, we should do that, since otherwise O(mxn)

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:543: void
SpellcheckCustomDictionary::OnWriteAndEraseFinished(
On 2012/12/19 22:16:00, groby wrote:
> Can be killed - see above.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:51: static void
LoadDictionaryIntoCustomWordList(
On 2012/12/19 22:16:00, groby wrote:
> 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.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:85: // Overridden
from syncer::SyncableService:
On 2012/12/19 22:16:00, groby wrote:
> Personal preference: I like overridden interfaces together, and either at the
> top or bottom of the public section.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:134: // duplicate
words that were not added, missing words that could not be
On 2012/12/19 22:16:00, groby wrote:
> 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.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:182:
scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
On 2012/12/19 22:16:00, groby wrote:
> Please document member vars.

Done.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:28: const
size_t LIMIT = 1300;
On 2012/12/19 22:16:00, groby wrote:
> Please document. Or use a shared constant.

Done. Using constant in chrome::spellcheck_common.

https://codereview.chromium.org/11445002/diff/100003/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:288:
MessageLoop::current()->RunUntilIdle();
On 2012/12/19 22:16:00, groby wrote:
> Why not run them during TearDown()? It seems _every_ test needs to do this at
> the end, so TearDown seems the logical place.

We need to run RunUntilIdle here before the spellcheck goes out of scope and
gets destroyed. SpellcheckHunspellDictionary posts tasks to the file thread that
use member variables. If the object is destroyed before the file-thread task
finishes, unit tests crash.

[akalin, 2012-12-29 09:58:39]

I'll let zea@ handle this review.

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

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:21: #include
"sync/protocol/sync.pb.h"
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)

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:422:
to_change_locally = Apply(to_change_locally.Pass());
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?

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:481:
dictionary_change->AddWord(word);
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(..).

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:508: // Do not add
duplicate words.
consider skipping all this if to_add is empty (and similarly skipping the remove
section if to_remove is empty)

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:586: if
(!IsSyncing())
return early if no changes?

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:609: // Turn off
sync if total number of words is more than maximum syncable words.
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.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/model_association_manager.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/glue...
chrome/browser/sync/glue/model_association_manager.cc:46: syncer::DICTIONARY,
move this up into the "UI thread datatypes" section

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1288: syncer::DICTIONARY,
Do we want Dictionary to be a separately selectable datatype? I forget if we
have discussed this already or not, but it seems to me it makes sense to tie
this to the PREFERENCES datatype, in the same way as SEARCH_ENGINES is tied to
it.

https://codereview.chromium.org/11445002/diff/123001/sync/protocol/dictionary...
File sync/protocol/dictionary_specifics.proto (right):

https://codereview.chromium.org/11445002/diff/123001/sync/protocol/dictionary...
sync/protocol/dictionary_specifics.proto:16: optional string word = 1;
Add description for field. Maybe something like "A spelling which when seen is
treated as a valid word."

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

Zea: I've addressed your comments, PTAL.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:21: #include
"sync/protocol/sync.pb.h"
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> 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)

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

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:422:
to_change_locally = Apply(to_change_locally.Pass());
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> 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?

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.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:481:
dictionary_change->AddWord(word);
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> 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(..).

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.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:508: // Do not add
duplicate words.
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> consider skipping all this if to_add is empty (and similarly skipping the
remove
> section if to_remove is empty)

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

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:586: if
(!IsSyncing())
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> return early if no changes?

Good idea. Returning early if there are no changes.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:609: // Turn off
sync if total number of words is more than maximum syncable words.
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> 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.

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.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/model_association_manager.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/glue...
chrome/browser/sync/glue/model_association_manager.cc:46: syncer::DICTIONARY,
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> move this up into the "UI thread datatypes" section

Moved.

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1288: syncer::DICTIONARY,
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> Do we want Dictionary to be a separately selectable datatype? I forget if we
> have discussed this already or not, but it seems to me it makes sense to tie
> this to the PREFERENCES datatype, in the same way as SEARCH_ENGINES is tied to
> it.

I do not recall this discussion, but it's a good idea. I've moved DICTIONARY
under PREFERENCES.

https://codereview.chromium.org/11445002/diff/123001/sync/protocol/dictionary...
File sync/protocol/dictionary_specifics.proto (right):

https://codereview.chromium.org/11445002/diff/123001/sync/protocol/dictionary...
sync/protocol/dictionary_specifics.proto:16: optional string word = 1;
On 2013/01/02 23:12:15, Nicolas Zea wrote:
> Add description for field. Maybe something like "A spelling which when seen is
> treated as a valid word."

Added this description: "A spelling which when typed is treated as a correctly
spelled word."

[groby-ooo-7-16, 2013-01-07 19:02:14]

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1288: syncer::DICTIONARY,
Can't speak to the pros/cons, but I'd like to point out that dictionary data is
independent of system preferences. It's user created data, not a preference.

(I'm not sure if that should influence the discussion, since I'm not familiar
enough with syncing)

On 2013/01/04 23:30:50, Rouslan Solomakhin wrote:
> On 2013/01/02 23:12:15, Nicolas Zea wrote:
> > Do we want Dictionary to be a separately selectable datatype? I forget if we
> > have discussed this already or not, but it seems to me it makes sense to tie
> > this to the PREFERENCES datatype, in the same way as SEARCH_ENGINES is tied
to
> > it.
> 
> I do not recall this discussion, but it's a good idea. I've moved DICTIONARY
> under PREFERENCES.

[Nicolas Zea, 2013-01-07 19:07:41]

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/11445002/diff/123001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1288: syncer::DICTIONARY,
On 2013/01/07 19:02:14, groby wrote:
> Can't speak to the pros/cons, but I'd like to point out that dictionary data
is
> independent of system preferences. It's user created data, not a preference.
> 
> (I'm not sure if that should influence the discussion, since I'm not familiar
> enough with syncing)
> 
> On 2013/01/04 23:30:50, Rouslan Solomakhin wrote:
> > On 2013/01/02 23:12:15, Nicolas Zea wrote:
> > > Do we want Dictionary to be a separately selectable datatype? I forget if
we
> > > have discussed this already or not, but it seems to me it makes sense to
tie
> > > this to the PREFERENCES datatype, in the same way as SEARCH_ENGINES is
tied
> to
> > > it.
> > 
> > I do not recall this discussion, but it's a good idea. I've moved DICTIONARY
> > under PREFERENCES.
> 

The user selectable option is actually called "Settings", and it enables
PREFERENCES, and by virtue of that SEARCH ENGINES. My point is that this seems
to me like it should fall under "Settings", and therefore be auto enabled if
PREFERENCES is enabled.

This isn't meant to imply that the dictionary is implemented via preferences.

[groby-ooo-7-16, 2013-01-10 19:53:02]

Quick feedback, just based on API surface change. More to come :)

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:302:
SpellcheckCustomDictionary::Change::Result
Oy. I apologize for the confusion - in my previous comment, I intended for the
_public_ API to remain bool. (That means Change can be a private class, nobody
needs to see it)

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:21:
scoped_ptr<chrome::spellcheck_common::WordList> LoadDictionary(
Why are these APIs exposed? Isn't CustomDictionary the only consumer? (And if
not, it should be :)

https://codereview.chromium.org/11445002/diff/166001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.h (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.h:33: virtual void
OnWordsAddedRemoved(
"OnCustomDictionaryChanged" instead?

[groby-ooo-7-16, 2013-01-10 22:04:44]

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:22: #include
"third_party/hunspell/src/hunspell/hunspell.hxx"
We probably shouldn't include hunspell here - technically, this is
cross-platform code.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:216:
SpellcheckCustomDictionary::Change::Change(
Why pointer magic, and not just a copy ctor? (In fact, why do we need to copy
Change at all?)

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:249: 
Kill a newline, save some space :)

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:295: Change::Result
result(dictionary_change->result());
Why doesn't this just call RemoveWordLocally?

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:345:
Apply(dictionary_change.get());
Hm. It seems every Apply causes a Sync. Why not call Sync from Apply and be
done?

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:481:
std::set_difference(dictionary_change->to_add().begin(),
That's similar to the sanitizing code at load time. Should we just unify the
two? (Maybe a "SanitizeData()" on Change?

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:60: void
set_detected_invalid_words() { invalid_ = true; }
As discussed - let's just make this a struct, without setter/getter overhead.
Or, even better, an enum :)

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:79: void
AddWords(const chrome::spellcheck_common::WordList* words);
Since we copy those words over to internal vectors anyways, why not a const-ref?

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:114: Change::Result
CustomWordAddedLocally(const std::string& word);
Do we need the xxxLocally functions to be part of the public API?

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:137: void
OnLoaded(scoped_ptr<chrome::spellcheck_common::WordList> custom_words);
Does this need to be public?

[please use gerrit instead, 2013-01-12 02:50:46]

Groby: I've addressed your comments. PTAL.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:22: #include
"third_party/hunspell/src/hunspell/hunspell.hxx"
On 2013/01/10 22:04:44, groby wrote:
> We probably shouldn't include hunspell here - technically, this is
> cross-platform code.

Defined a chrome::spellcheck_common::MAX_CUSTOM_DICTIONARY_WORD_BYTES variable
in chrome/common/spellcheck_common.h that can be shared between here and
chrome/renderer/spellchecker/hunspell_engine.cc.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:216:
SpellcheckCustomDictionary::Change::Change(
On 2013/01/10 22:04:44, groby wrote:
> Why pointer magic, and not just a copy ctor? (In fact, why do we need to copy
> Change at all?)

No longer used, removed.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:249: 
On 2013/01/10 22:04:44, groby wrote:
> Kill a newline, save some space :)

Killed newlines after all dchecks.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:295: Change::Result
result(dictionary_change->result());
On 2013/01/10 22:04:44, groby wrote:
> Why doesn't this just call RemoveWordLocally?

I deleted RemoveWordLocally and AddWordLocally completely.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:302:
SpellcheckCustomDictionary::Change::Result
On 2013/01/10 19:53:03, groby wrote:
> Oy. I apologize for the confusion - in my previous comment, I intended for the
> _public_ API to remain bool. (That means Change can be a private class, nobody
> needs to see it)

I moved the public API back to bool. SpellcheckCustomDictionary::Change is used
to notify observers of changes in the dictionary, so let's keep it public, but
make Result into a private struct.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:345:
Apply(dictionary_change.get());
On 2013/01/10 22:04:44, groby wrote:
> Hm. It seems every Apply causes a Sync. Why not call Sync from Apply and be
> done?

We cannot call Sync from Apply because ProcessSyncChanges calls Apply without
Sync and MergeDataAndStartSyncing calls Apply and Sync with different data.

We cannot call Notify and Save from Apply because OnLoaded calls Apply without
Notify and Save.

I think the current approach is the easiest to understand.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:481:
std::set_difference(dictionary_change->to_add().begin(),
On 2013/01/10 22:04:44, groby wrote:
> That's similar to the sanitizing code at load time. Should we just unify the
> two? (Maybe a "SanitizeData()" on Change?

Good idea! Done.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:21:
scoped_ptr<chrome::spellcheck_common::WordList> LoadDictionary(
On 2013/01/10 19:53:03, groby wrote:
> Why are these APIs exposed? Isn't CustomDictionary the only consumer? (And if
> not, it should be :)

These APIs were used for unit-tests. I have friended the unit-test classes
instead of exposing these APIs.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:60: void
set_detected_invalid_words() { invalid_ = true; }
On 2013/01/10 22:04:44, groby wrote:
> As discussed - let's just make this a struct, without setter/getter overhead.
> Or, even better, an enum :)

Converted into an enum.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:79: void
AddWords(const chrome::spellcheck_common::WordList* words);
On 2013/01/10 22:04:44, groby wrote:
> Since we copy those words over to internal vectors anyways, why not a
const-ref?

Done.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:114: Change::Result
CustomWordAddedLocally(const std::string& word);
On 2013/01/10 22:04:44, groby wrote:
> Do we need the xxxLocally functions to be part of the public API?

*Locally() methods were used only by tests to avoid disk writes. I removed them
in favor of friended tests using the private Apply() method directly.

https://codereview.chromium.org/11445002/diff/166001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:137: void
OnLoaded(scoped_ptr<chrome::spellcheck_common::WordList> custom_words);
On 2013/01/10 22:04:44, groby wrote:
> Does this need to be public?

Was used by unit-tests. Made private and friended the unit-tests.

https://codereview.chromium.org/11445002/diff/166001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.h (right):

https://codereview.chromium.org/11445002/diff/166001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.h:33: virtual void
OnWordsAddedRemoved(
On 2013/01/10 19:53:03, groby wrote:
> "OnCustomDictionaryChanged" instead?

Good point. I had AddedRemoved everywhere. Changed them all to "Changed".

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

Modulo a few nits, spellcheck LGTM

You might want to ping the other reviewers. :)

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:60: ChecksumStatus
LoadFile(FilePath file_path, std::vector<std::string>* lines) {
const FilePath&

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:78: // Returns true
for invalid words and false for valid words. Useful for
Kill "Useful for", save a line :)

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:380:
scoped_ptr<WordList> custom_words(new WordList);
Since we always return custom_words, no need for scoped_ptr. Just return a raw
pointer.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:382: if
(custom_words->empty())
I'd pick if(!custom_words.empty()) { Sanitize(); Save(); }

That way, there's only one return path.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:36: void
AddWord(const std::string& word);
Can we either have AddWord/RemoveWord or AddWords/RemoveWords?

Or are both use cases about equally important?

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:71: // A callback for
when the custom dictionary has loaded the words saved in
Shorten to "called when the custom dictionary file has been loaded".

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:75: // A callback for
when the custom dictionary has been changed.
"Called when.."

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:101: // Whether the
dictionary is loaded.
"Return true if the dictionary is loaded." - I prefer complete sentences for my
docs :)

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:105: bool
IsSyncing();
See above.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:130: static
scoped_ptr<chrome::spellcheck_common::WordList> LoadDictionaryFile(
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?

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:138: const FilePath&
path);
See above re: anon. ns.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:148: int
Apply(Change* dictionary_change);
I'm still in favor of sanitizing the changes on the Change object - I believe
Apply could then take a const ref.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:151: void
Save(scoped_ptr<Change> dictionary_change);
"... 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.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:157:
syncer::SyncError Sync(const Change* dictionary_change);
I might be wrong, but at first glance, it doesn't look like sync needs a pointer
- why not const-ref?

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:161: void
Notify(const Change* dictionary_change);
const-ref?

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_service.h (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_service.h:95: const
SpellcheckCustomDictionary::Change* dictionary_change) OVERRIDE;
I'd prefer this to be a const ref. (For one, this should never be called with a
NULL ptr.)

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

One reviewer less, more chance of landing. (s/csilv/jhawkins)

jhawkins: 
    chrome/browser/about_flags.cc
        Letting users flip the --enable-sync-dictionary flag from
chrome://flags.

    chrome/browser/resources/*
    chrome/browser/ui/webui/*
        Changes to UI for showing the dictionary as one of the syncable data
types in chrome://settings/setupSync. 
        Also showing sync changes in chrome://settings/editDictionary as they
come in.

[please use gerrit instead, 2013-01-15 01:25:47]

On 2013/01/15 01:22:20, groby wrote:
>     chrome/browser/resources/*
>     chrome/browser/ui/webui/*
>         Changes to UI for showing the dictionary as one of the syncable data
> types in chrome://settings/setupSync. 

Zea recommended that we lump dictionary sync under "Settings". "Dictionary" will
not show up under chrome://settings/setupSync.

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

Groby: I've addressed your comments, with the following couple of things to
note:

Since we are using const-ref for "void Save(const Change& change)", which
creates a copy of the change, why not use "WordList
SpellcheckCustomDictionary::LoadDictionaryFile(const
FilePath& path)" instead of "scoped_ptr<WordList>" or "WordList*"?

I am keeping LoadDictionaryFile() and UpdateDictionaryFile() as private static
functions in SpellcheckCustomDictionary because they are accessed in unit tests.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:60: ChecksumStatus
LoadFile(FilePath file_path, std::vector<std::string>* lines) {
On 2013/01/15 01:19:20, groby wrote:
> const FilePath&

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:78: // Returns true
for invalid words and false for valid words. Useful for
On 2013/01/15 01:19:20, groby wrote:
> Kill "Useful for", save a line :)

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:380:
scoped_ptr<WordList> custom_words(new WordList);
On 2013/01/15 01:19:20, groby wrote:
> Since we always return custom_words, no need for scoped_ptr. Just return a raw
> pointer.

How about a "WordList SpellcheckCustomDictionary::LoadDictionaryFile(const
FilePath& path)" instead?

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:382: if
(custom_words->empty())
On 2013/01/15 01:19:20, groby wrote:
> I'd pick if(!custom_words.empty()) { Sanitize(); Save(); }
> 
> That way, there's only one return path. 

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:36: void
AddWord(const std::string& word);
On 2013/01/15 01:19:20, groby wrote:
> Can we either have AddWord/RemoveWord or AddWords/RemoveWords?
> 
> Or are both use cases about equally important?

Let's keep only AddWord/RemoveWord.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:71: // A callback for
when the custom dictionary has loaded the words saved in
On 2013/01/15 01:19:20, groby wrote:
> Shorten to "called when the custom dictionary file has been loaded".

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:75: // A callback for
when the custom dictionary has been changed.
On 2013/01/15 01:19:20, groby wrote:
> "Called when.."

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:101: // Whether the
dictionary is loaded.
On 2013/01/15 01:19:20, groby wrote:
> "Return true if the dictionary is loaded." - I prefer complete sentences for
my
> docs :)

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:105: bool
IsSyncing();
On 2013/01/15 01:19:20, groby wrote:
> See above.

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:130: static
scoped_ptr<chrome::spellcheck_common::WordList> LoadDictionaryFile(
On 2013/01/15 01:19:20, groby wrote:
> 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?

Unit tests access this function directly.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:138: const FilePath&
path);
On 2013/01/15 01:19:20, groby wrote:
> See above re: anon. ns.

Unit tests access this function directly, too.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:148: int
Apply(Change* dictionary_change);
On 2013/01/15 01:19:20, groby wrote:
> I'm still in favor of sanitizing the changes on the Change object - I believe
> Apply could then take a const ref.

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

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:151: void
Save(scoped_ptr<Change> dictionary_change);
On 2013/01/15 01:19:20, groby wrote:
> "... 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.

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:157:
syncer::SyncError Sync(const Change* dictionary_change);
On 2013/01/15 01:19:20, groby wrote:
> I might be wrong, but at first glance, it doesn't look like sync needs a
pointer
> - why not const-ref?

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:161: void
Notify(const Change* dictionary_change);
On 2013/01/15 01:19:20, groby wrote:
> const-ref?

Done.

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
File chrome/browser/spellchecker/spellcheck_service.h (right):

https://codereview.chromium.org/11445002/diff/192001/chrome/browser/spellchec...
chrome/browser/spellchecker/spellcheck_service.h:95: const
SpellcheckCustomDictionary::Change* dictionary_change) OVERRIDE;
On 2013/01/15 01:19:20, groby wrote:
> I'd prefer this to be a const ref. (For one, this should never be called with
a
> NULL ptr.)

Done.

[please use gerrit instead, 2013-01-16 02:06:46]

Jhawkins + Zea: Ping.

[James Hawkins, 2013-01-17 19:37:18]

On 2013/01/16 02:06:46, Rouslan Solomakhin wrote:
> Jhawkins + Zea: Ping.

This CL needs to be broken up.  I suggest starting with the flag.

[James Hawkins, 2013-01-17 19:37:30]

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/resources...
File chrome/browser/resources/options/language_dictionary_overlay_word_list.js
(right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/resources...
chrome/browser/resources/options/language_dictionary_overlay_word_list.js:148: *
Add non-duplicate dictionary words.
Adds

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/resources...
chrome/browser/resources/options/language_dictionary_overlay_word_list.js:153:
for (var i in entries) {
Don't use 'for in' for arrays.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/resources...
chrome/browser/resources/options/language_dictionary_overlay_word_list.js:161:
for (var i in to_add)
Same here.

[Nicolas Zea, 2013-01-17 20:14:51]

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/dictionary_helper.cc (right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.cc:22: static bool
ApplyChange(
Does this need the class around it? Can you just have this as a method in the
anon namespace below?

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.cc:103: LOG(ERROR) <<
"Loading dictionary for profile " << i;
These aren't actually errors right?

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/dictionary_helper.h (right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:19: // Loads the
dictionaries across all profiles. Also loads the dictionary for the
Synchronously loads

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:25: // Uses to check
the size of the dictionary within a particular sync profile.
Uses -> Used

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:28: // Uses to check
the size of the dictionary within the verifier sync profile.
here too

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:39: // Adds the |word|
to the dictionary for profile with index |index|. Also adds
Adds |word| (and below)

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc
(right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc:33:
MessageLoop::current()->RunUntilIdle();
Is the run until idle necessary for correctness on all these tests?

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc:70:
// Add 1300i completely different words to all i clients before sync.
1300i?

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
(right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc:105:
ASSERT_TRUE(dictionary_helper::AddWord(1, word));
Without waiting for sync between these events, I suspect this will be a racy
test. What if B sync before A? You'll wind up with no items in the dictionary
(A's addword will be redundant)

[please use gerrit instead, 2013-01-17 22:25:28]

Zea: I've addressed your comments. PTAL.

Jhawkins: I've addressed your comments in two separate CLs
https://codereview.chromium.org/12007004 and
https://codereview.chromium.org/11968050, which must be checked in before this
CL.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/dictionary_helper.cc (right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.cc:22: static bool
ApplyChange(
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Does this need the class around it? Can you just have this as a method in the
> anon namespace below?

This function is accessing private methods in SpellcheckCustomDictionary to
bypass writing the dictionary file on disk during tests. I've put the function
into DictionarySyncIntegrationTestHelper class so that I can add "friend class
DictionarySyncIntegrationTestHelper;" line to SpellcheckCustomDictionary. Is
this practice okay?

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.cc:103: LOG(ERROR) <<
"Loading dictionary for profile " << i;
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> These aren't actually errors right?

These are debug statements. I've removed them now. Thank you for pointing them
out.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/dictionary_helper.h (right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:19: // Loads the
dictionaries across all profiles. Also loads the dictionary for the
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Synchronously loads

Done.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:25: // Uses to check
the size of the dictionary within a particular sync profile.
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Uses -> Used

Done.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:28: // Uses to check
the size of the dictionary within the verifier sync profile.
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> here too

Done.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.h:39: // Adds the |word|
to the dictionary for profile with index |index|. Also adds
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Adds |word| (and below)

Done.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc
(right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc:33:
MessageLoop::current()->RunUntilIdle();
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Is the run until idle necessary for correctness on all these tests?

It's necessary to prevent crashing in SpellcheckHunspellDictionary. We do it in
our own tests, too.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/multiple_client_dictionary_sync_test.cc:70:
// Add 1300i completely different words to all i clients before sync.
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> 1300i?

Fixed the comment to be "Add 1300 different words to each of the clients before
sync".

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
(right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc:105:
ASSERT_TRUE(dictionary_helper::AddWord(1, word));
On 2013/01/17 20:14:52, Nicolas Zea wrote:
> Without waiting for sync between these events, I suspect this will be a racy
> test. What if B sync before A? You'll wind up with no items in the dictionary
> (A's addword will be redundant)

Added AwaitQuiescence() between RemoveWord(0, word) and AddWord(1, word).

[Nicolas Zea, 2013-01-17 22:40:15]

LGTM! Nice work.

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/dictionary_helper.cc (right):

https://codereview.chromium.org/11445002/diff/208002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/dictionary_helper.cc:22: static bool
ApplyChange(
On 2013/01/17 22:25:29, Rouslan Solomakhin wrote:
> On 2013/01/17 20:14:52, Nicolas Zea wrote:
> > Does this need the class around it? Can you just have this as a method in
the
> > anon namespace below?
> 
> This function is accessing private methods in SpellcheckCustomDictionary to
> bypass writing the dictionary file on disk during tests. I've put the function
> into DictionarySyncIntegrationTestHelper class so that I can add "friend class
> DictionarySyncIntegrationTestHelper;" line to SpellcheckCustomDictionary. Is
> this practice okay?

I see. You could just declare the function as a friend, but this might be more
scalable if you wind up having to add other friended functions. SG.

[please use gerrit instead, 2013-01-18 01:54:04]

Jhawkins: Even though the meaningful changes for your review have been
separated, could you PTAL at the method signature change in
chrome/browser/ui/webui/options/language_dictionary_overlay_handler.h/cc?

[James Hawkins, 2013-01-22 22:12:53]

webui lgtm. Thanks for splitting this CL up.

[commit-bot: I haz the power, 2013-01-22 22:14:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11445002/250001

[commit-bot: I haz the power, 2013-01-23 00:37:11]

Change committed as 178178