SpellCheck refactoring: Moved user custom dictionary for spell check to SpellCheckProfile.

A user custom dictionary for spell check was located on SpellCheckHostImpl. This prevents us from user's having spell check dictionary per profile.

This patch moves a user custom dictionary code from SpellCheckHostImpl to SpellCheckProfile.


BUG=None
TEST=SpellCheckProfileTest.* should cover it.

[shinyak (Google), 2011-10-12 01:59:43]

Bono-san,

could you review this?

[Hironori Bono, 2011-10-12 02:56:56]

Greetings Kawanaka-san,

Thank you for your refactoring. I have added some drive-by comments.

Regards,

Hironori Bono

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:25:
PathService::Get(chrome::DIR_USER_DATA, &personal_file_directory);
This call may create a directory, i.e. this function should be called in a FILE
thread. (If I recall correctly, ProfileImpl creates this object in a UI thread.)

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:116:
scoped_ptr<CustomWordList> word_list(new CustomWordList());
nit: CustomWordList() -> CustomWordList.

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:47: : public
SpellCheckProfileProvider,
nit: need four spaces before ':'.

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:49: {
nit: this '{' should be placed at the end of the previous line.

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:155:
TEST_F(SpellCheckProfileTest, SpellCheckHostInitializedWithCustomWords) {
I'm wondering why this change deletes this test. (If this change is just a
refactoring one, it should have an equivalent test.)

[gmorrita, 2011-10-12 03:41:11]

Hi Shinya, Thanks you for taking this!

Could you add a test that verify the SpellCheckProfile instance can 
save/add custom word successfully?
We dont' have such a test and this is a great opportunity to add it.

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:18: #include
"content/browser/browser_thread.h"
Isn't it possible to have this in .cc file?

[shinyak (Google), 2011-10-12 10:46:54]

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:25:
PathService::Get(chrome::DIR_USER_DATA, &personal_file_directory);
Done.

I made it to use lazy initialization.

On 2011/10/12 02:56:56, hbono wrote:
> This call may create a directory, i.e. this function should be called in a
FILE
> thread. (If I recall correctly, ProfileImpl creates this object in a UI
thread.)

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.cc:116:
scoped_ptr<CustomWordList> word_list(new CustomWordList());
On 2011/10/12 02:56:56, hbono wrote:
> nit: CustomWordList() -> CustomWordList.

Done.

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:18: #include
"content/browser/browser_thread.h"
On 2011/10/12 03:41:11, morrita wrote:
> Isn't it possible to have this in .cc file?

Done.

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:47: : public
SpellCheckProfileProvider,
On 2011/10/12 02:56:56, hbono wrote:
> nit: need four spaces before ':'.

Done.

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile.h:49: {
On 2011/10/12 02:56:56, hbono wrote:
> nit: this '{' should be placed at the end of the previous line.

Done.

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

http://codereview.chromium.org/8233040/diff/1/chrome/browser/spellchecker/spe...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:155:
TEST_F(SpellCheckProfileTest, SpellCheckHostInitializedWithCustomWords) {
On 2011/10/12 02:56:56, hbono wrote:
> I'm wondering why this change deletes this test. (If this change is just a
> refactoring one, it should have an equivalent test.)

Done.

[gmorrita, 2011-10-12 11:03:36]

Thanks for update the patch. It basically looks good.
I added a few small comments.

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

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:98: // Called when
LoadCustomDirectory finished.
Could you mention about a lifetime/ownership of word_list?
(I know original code didn't have that but...)

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

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:79:
file_thread_(BrowserThread::FILE, &message_loop_) {
I guess we need one MessageLoop instance per thread for safe.

[shinyak (Google), 2011-10-12 13:03:24]

>
http://codereview.chromium.org/8233040/diff/5001/chrome/browser/spellchecker/...
> chrome/browser/spellchecker/spellcheck_profile_unittest.cc:79:
> file_thread_(BrowserThread::FILE, &message_loop_) {
> I guess we need one MessageLoop instance per thread for safe.

I tried having one MessageLoop per thread like the following code:

...
file_thread_(BrowserThread::FILE, &message_loop_file_),
ui_thread_(BrowserThread::UI, &message_loop_ui_)
<snip />
 private:
  MessageLoop message_loop_file_;
  MessageLoop message_loop_ui_;

In that case I got an error that each thread can have only one MessageLoop.
Do you know how to avoid this error? I have searched the code, but I couldn't
find a solution. I could find an example that it did the same thing as my code
though...

[Miranda Callahan, 2011-10-12 14:33:04]

Thank you for this change! The profile_impl code LGTM, except for one comment.

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1762: spellcheck_profile_ = new
SpellCheckProfile();
scoped_ptr still uses the .reset function.

[shinyak (Google), 2011-10-13 07:51:22]

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1762: spellcheck_profile_ = new
SpellCheckProfile();
On 2011/10/12 14:33:05, Miranda Callahan wrote:
> scoped_ptr still uses the .reset function.

I've changed SpellCheckProfile ref-counted object (in order to keep it alive
until receiving save task or something like that). So I made spellcheck_profile_
scoped_refptr instead of scoped_ptr. scoped_refptr does not have reset() method.

[gmorrita, 2011-10-13 08:53:49]

> In that case I got an error that each thread can have only one MessageLoop.
> Do you know how to avoid this error? I have searched the code, but I couldn't
> find a solution. I could find an example that it did the same thing as my code
> though...
It looks a thread has at most one EventLoop. So the original code LGTM I guess.

[Hironori Bono, 2011-10-13 09:29:59]

Kawanaka-san,

Thank you for your update.
I have added a couple of comments to clarify this change.

Regards,

Hironori Bono

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1762: spellcheck_profile_ = new
SpellCheckProfile();
On 2011/10/13 07:51:22, shinyak wrote:
> I've changed SpellCheckProfile ref-counted object (in order to keep it alive
> until receiving save task or something like that). So I made
spellcheck_profile_
> scoped_refptr instead of scoped_ptr. scoped_refptr does not have reset()
method.

Is it possible to describe more details about why you need scoped_refptr<>? (As
written in Issue 79886, we are trying to remove scoped_refptr<> from
ProfileImpl.)

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

http://codereview.chromium.org/8233040/diff/12001/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_host_impl.cc:125:
profile_->LoadCustomDictionary();
It seems this code start loading a custom-dictionary file in a FILE thread.
Also, the following PostTask() initializes a dictionary file in a FILE thread,
and sends IPC messages (consisting of its handle and the list of custom words)
to renderer processes in a UI thread. It may be safer to have a method that
verifies we finish loading the custom-dictionary file to prevent sending IPC
messages before we finish loading the custom-dictionary file?

[Miranda Callahan, 2011-10-13 14:33:12]

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8233040/diff/5001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1762: spellcheck_profile_ = new
SpellCheckProfile();
On 2011/10/13 07:51:22, shinyak wrote:
> On 2011/10/12 14:33:05, Miranda Callahan wrote:
> > scoped_ptr still uses the .reset function.
> 
> I've changed SpellCheckProfile ref-counted object (in order to keep it alive
> until receiving save task or something like that). So I made
spellcheck_profile_
> scoped_refptr instead of scoped_ptr. scoped_refptr does not have reset()
method.

My apologies, you're right, of course -- I don't know what happened to my brain
-- I looked at the .h file several times, too. :-/

Having said that -- I agree with hbono about trying to remove scoped_refptr from
Profile -- it would be great if it were possible to avoid this change!