SpellCheck: the custom dictionary should be per profile.

The current user custom spell check dictionary is shared among profiles. This should be per profile.

BUG=None
TEST=SpellCheckProfileTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107062

[shinyak (Google), 2011-10-19 04:40:56]

Hi, reviewers,

I wanted to land the previous patch
http://codereview.chromium.org/8233040/
before this patch, but it was stuck because it was a bit difficult for now to
avoid make SpellCheckProfile ref-counted.

So for now, I just try to make user dictionary per profile instead of per user.
This change will enable us to sync user custom dictionary later.

[gmorrita, 2011-10-19 05:27:36]

Looks reasonable. We can do the refactoring later...

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:24: custom_dictionary_path_ =
Why not use initializer?

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile_provider.h (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_provider.h:12: class FilePath;
Do we need this?

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile_unittest.cc (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:232:
EXPECT_EQ(*loaded_custom_words, expected);
Coud you add some more lines,
to verify these newly-saved words are available on the next load?

[shinyak (Google), 2011-10-19 05:55:03]

Thank you for reviewing this!
I've fixed the issues you mentioned. So I would like you to review this again.

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:24: custom_dictionary_path_ =
On 2011/10/19 05:27:37, morrita wrote:
> Why not use initializer?

Done.

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile_provider.h (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_provider.h:12: class FilePath;
On 2011/10/19 05:27:37, morrita wrote:
> Do we need this?

Done.

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
File chrome/browser/spellchecker/spellcheck_profile_unittest.cc (right):

http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:232:
EXPECT_EQ(*loaded_custom_words, expected);
On 2011/10/19 05:27:37, morrita wrote:
> Coud you add some more lines,
> to verify these newly-saved words are available on the next load?

Done.

[Miranda Callahan, 2011-10-19 19:44:30]

Profile code LGTM.

On 2011/10/19 05:55:03, shinyak wrote:
> Thank you for reviewing this!
> I've fixed the issues you mentioned. So I would like you to review this again.
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> File chrome/browser/spellchecker/spellcheck_profile.cc (right):
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> chrome/browser/spellchecker/spellcheck_profile.cc:24: custom_dictionary_path_
=
> On 2011/10/19 05:27:37, morrita wrote:
> > Why not use initializer?
> 
> Done.
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> File chrome/browser/spellchecker/spellcheck_profile_provider.h (right):
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> chrome/browser/spellchecker/spellcheck_profile_provider.h:12: class FilePath;
> On 2011/10/19 05:27:37, morrita wrote:
> > Do we need this?
> 
> Done.
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> File chrome/browser/spellchecker/spellcheck_profile_unittest.cc (right):
> 
>
http://codereview.chromium.org/8345034/diff/1/chrome/browser/spellchecker/spe...
> chrome/browser/spellchecker/spellcheck_profile_unittest.cc:232:
> EXPECT_EQ(*loaded_custom_words, expected);
> On 2011/10/19 05:27:37, morrita wrote:
> > Coud you add some more lines,
> > to verify these newly-saved words are available on the next load?
> 
> Done.

[Hironori Bono, 2011-10-20 02:20:36]

Greetings Kawanaka-san,

Thank you for your change.
Even though it looks pretty good, is it possible to fix some minor issues of
your change?

Regards,

Hironori Bono

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:25:
profile_dir.Append(chrome::kCustomDictionaryFileName)) {
Even though file_util::Append() does not usually take so long time, it is better
to append it for the first time when we call LoadCustomDictionar() or
WriteWordToCustomDictionary() so we can start our spellchecker as fast as
possible.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:127: // Stored in UTF-8.
nit: it would be good to add 'DCHECK(IsStringUTF8(word));' here.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:132: file_util::CloseFile(f);
nit: it may be better to call file_util::CloseFile() only when f is not NULL as
listed in the following snippet.
  if (f) {
     fputs(word_to_add.c_str(), f);
     file_util::CloseFile(f);
  }

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile_provider.h (right):

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_provider.h:28: // Load custom
dictionary from the disk.
nit: 'Load custom dictionary' -> 'Loads the custom dictionary associated with
this profile'. (A function comment should be descriptive, i.e. the subject of a
function comment is the function itself.)

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_provider.h:31: // Wrirt a word to
custom dictionary on the disk.
nit: 'Wrirt' -> 'Writes', 'to custom dictionary' -> 'to the custom dictionary
associated with this profile'.

[shinyak (Google), 2011-10-20 04:08:41]

Thank you for reviewing!

I've fix several issues you mentioned.
I would like you to review this again.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:25:
profile_dir.Append(chrome::kCustomDictionaryFileName)) {
On 2011/10/20 02:20:36, hbono wrote:
> Even though file_util::Append() does not usually take so long time, it is
better
> to append it for the first time when we call LoadCustomDictionar() or
> WriteWordToCustomDictionary() so we can start our spellchecker as fast as
> possible.

Done.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:127: // Stored in UTF-8.
On 2011/10/20 02:20:36, hbono wrote:
> nit: it would be good to add 'DCHECK(IsStringUTF8(word));' here.

Done.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:132: file_util::CloseFile(f);
On 2011/10/20 02:20:36, hbono wrote:
> nit: it may be better to call file_util::CloseFile() only when f is not NULL
as
> listed in the following snippet.
>   if (f) {
>      fputs(word_to_add.c_str(), f);
>      file_util::CloseFile(f);
>   }

Done.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile_provider.h (right):

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_provider.h:28: // Load custom
dictionary from the disk.
On 2011/10/20 02:20:36, hbono wrote:
> nit: 'Load custom dictionary' -> 'Loads the custom dictionary associated with
> this profile'. (A function comment should be descriptive, i.e. the subject of
a
> function comment is the function itself.)

Done.

http://codereview.chromium.org/8345034/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_provider.h:31: // Wrirt a word to
custom dictionary on the disk.
On 2011/10/20 02:20:36, hbono wrote:
> nit: 'Wrirt' -> 'Writes', 'to custom dictionary' -> 'to the custom dictionary
> associated with this profile'.

Done.

[Hironori Bono, 2011-10-20 04:48:50]

Greetings Kawanaka-san,

I have added some more style nits.

Regards,

Hironori Bono

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_host_impl.cc (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:309: std::string contents;
nit: move this declaration above Line 317 "file_util::ReadFileToString(...);'
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...>.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:310: CustomWordList
list_of_words;
nit: ditto.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:318:
base::SplitString(contents, '\n', &list_of_words);
nit: need 'if (!contents.empty()) {' to avoid calling base::SplitString() for
empty strings?

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:113: std::string contents;
nit: move this declaration above Line 116.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:117:
base::SplitString(contents, '\n', &list_of_words);
nit: need 'if (!contents.empty()) {' to avoid calling this function and the loop
below for empty strings.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:141:
chrome::kCustomDictionaryFileName);
nit: I prefer wrapping this line before 'profile_dir_.Append(' as listed in the
following snippet:
  custom_dictionary_path_ =
      profile_dir_.Append(...);

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.h (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:90: // Methods about custom
dictionary.
nit: we do not need this comment and the blank line above because the following
functions are override ones.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:91: virtual void
LoadCustomDictionary(CustomWordList* custom_words);
nit: is it possible to add 'OVERRIDE' keywords to all override functions in this
class? (We also need to include "base/compiler_specific.h".)

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:108: const FilePath&
GetCustomDictionaryPath();
This function should be above of |host_| to follow our coding rule:
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...>

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:120: FilePath
custom_dictionary_path_;
nit: can we use scoped_ptr<FilePath> to avoid allocating memory for this
variable when we do not use custom dictionaries?

[shinyak (Google), 2011-10-20 12:04:52]

Thanks for reviewing.
I've fixed what you mentioned.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_host_impl.cc (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:309: std::string contents;
On 2011/10/20 04:48:50, hbono wrote:
> nit: move this declaration above Line 317 "file_util::ReadFileToString(...);'
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...>.

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:310: CustomWordList
list_of_words;
On 2011/10/20 04:48:50, hbono wrote:
> nit: ditto.

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:318:
base::SplitString(contents, '\n', &list_of_words);
On 2011/10/20 04:48:50, hbono wrote:
> nit: need 'if (!contents.empty()) {' to avoid calling base::SplitString() for
> empty strings?

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:113: std::string contents;
On 2011/10/20 04:48:50, hbono wrote:
> nit: move this declaration above Line 116.

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:117:
base::SplitString(contents, '\n', &list_of_words);
On 2011/10/20 04:48:50, hbono wrote:
> nit: need 'if (!contents.empty()) {' to avoid calling this function and the
loop
> below for empty strings.

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:141:
chrome::kCustomDictionaryFileName);
On 2011/10/20 04:48:50, hbono wrote:
> nit: I prefer wrapping this line before 'profile_dir_.Append(' as listed in
the
> following snippet:
>   custom_dictionary_path_ =
>       profile_dir_.Append(...);

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
File chrome/browser/spellchecker/spellcheck_profile.h (right):

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:90: // Methods about custom
dictionary.
On 2011/10/20 04:48:50, hbono wrote:
> nit: we do not need this comment and the blank line above because the
following
> functions are override ones.

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:91: virtual void
LoadCustomDictionary(CustomWordList* custom_words);
On 2011/10/20 04:48:50, hbono wrote:
> nit: is it possible to add 'OVERRIDE' keywords to all override functions in
this
> class? (We also need to include "base/compiler_specific.h".)

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:108: const FilePath&
GetCustomDictionaryPath();
On 2011/10/20 04:48:50, hbono wrote:
> This function should be above of |host_| to follow our coding rule:
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...>

Done.

http://codereview.chromium.org/8345034/diff/9001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:120: FilePath
custom_dictionary_path_;
On 2011/10/20 04:48:50, hbono wrote:
> nit: can we use scoped_ptr<FilePath> to avoid allocating memory for this
> variable when we do not use custom dictionaries?

Done.

[shinyak (Google), 2011-10-20 12:59:34]

Fixed typo.

[Hironori Bono, 2011-10-21 05:08:26]

LGTM.
Thank you so much for your change.

Regards,

Hironori Bono

[commit-bot: I haz the power, 2011-10-21 05:08:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shinyak@google.com/8345034/12003

[commit-bot: I haz the power, 2011-10-21 08:32:55]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 106684, job name
was 8345034-12003 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-24 05:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shinyak@google.com/8345034/12003

[commit-bot: I haz the power, 2011-10-24 06:43:14]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 106890, job name
was 8345034-12003 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[shinyak (Google), 2011-10-25 02:13:59]

Hi, 

I've recreated a patch from the latest source. I hope this patch can be used by
trybots.

[commit-bot: I haz the power, 2011-10-25 02:19:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shinyak@google.com/8345034/18001

[commit-bot: I haz the power, 2011-10-25 04:50:29]

Change committed as 107062