[Refactoring] Extracted SpellCheckProfile from ProfileImpl.

[Refactoring] Extracted SpellCheckProfile from ProfileImpl.

Extracted
- ProfileImpl::spellcheck_host_ready
- ProfileImpl::spellcheck_host_
- ProfileImpl::spellcheck_host_metrics_
into newly created SpellCheckProfile.
This is a preparation for implementing SyncableService.

TEST=SpellCheckProfileTest.*
BUG=none


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

[gmorrita, 2011-08-05 08:02:03]

Bono-san, could you take a look at this?
I'm planning to implement SyncableService on this SpellCheckProfile class.

Hi Miranda, could you check a profile related change?
This just extracted a set of spellcheck-related object into single class.
And the behavior shouldn't change.

[Hironori Bono, 2011-08-08 02:07:12]

Morita-san,

Thank you for your refactoring. Unfortunately, this change seems to have some
style issues. Have your run cpplint before uploading this change?

Regards,

Hironori Bono

[gmorrita, 2011-08-08 02:41:29]

> Thank you for your refactoring. Unfortunately, this change seems to have some
> style issues. Have your run cpplint before uploading this change?
Oops. I only did presubmit check but should have run cpplint.py. 
I updated the patch to fix style errors.
Thanks for taking a look and the feedback, Bono-san.

[Hironori Bono, 2011-08-08 03:28:58]

Morita-san,

Thank you for your update.
I have added some comments.

Regards,

Hironori Bono

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:306: spellcheck_profile_(new
SpellCheckProfile()),
Do we really needs to create a SpellCheckProfile instance in this constructor?
It may be better to use LasyInstance so we can create it when we use it for the
first time?

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:14: : host_ready_(false) {
nit: need four spaces before this initializer list.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...>

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:26:
SpellCheckProfile::ReinitializeResult SpellCheckProfile::ReinitializeHost(
it seems ProfileImpl::ReinitializeSpellCheckHost() checks only whether the
result value is REINITIALIZE_REMOVED_HOST. I'm a little wondering if this
function needs to return an enum variable.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:31: // If we are already
loading the spellchecker, and this is just a hint to
nit: it may be a good idea to add
'DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));' to tell this function
should be called on a UI thread here?

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:69: host_ready_ = host_ &&
host_->IsReady();
I'm wondering if we can copy a scoped_refptr |host_| to a boolean variable
|host_ready_| here. Does this mean 'host_ready_ = !!host.get()'?

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:59: ~SpellCheckProfile();
I think this destructor should be virtual because the one of the
SpellCheckHostObserver is virtual.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:62: // This can return null
when the spell-checking is disabled
nit: null -> NULL.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:86: SpellCheckHostObserver*
observer,
nit: need four spaces before this first parameter.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:53: }
nit: '}' -> '}  // namespace'.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...>.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:65: 
nit: I'm wondering if we need two empty lines here.

[gmorrita, 2011-08-08 05:17:03]

Bono-san, thank for the feedback, Could you please another look?
By the way, try bots are in flight.

Thanks in advance.

[gmorrita, 2011-08-08 05:23:48]

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:306: spellcheck_profile_(new
SpellCheckProfile()),
On 2011/08/08 03:29:03, hbono wrote:
> Do we really needs to create a SpellCheckProfile instance in this constructor?
> It may be better to use LasyInstance so we can create it when we use it for
the
> first time?

Done.

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:14: : host_ready_(false) {
On 2011/08/08 03:29:03, hbono wrote:
> nit: need four spaces before this initializer list.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...>

Done.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:26:
SpellCheckProfile::ReinitializeResult SpellCheckProfile::ReinitializeHost(
On 2011/08/08 03:29:03, hbono wrote:
> it seems ProfileImpl::ReinitializeSpellCheckHost() checks only whether the
> result value is REINITIALIZE_REMOVED_HOST. I'm a little wondering if this
> function needs to return an enum variable.
I thought bool type doesn't explain what happened there and
it is not clear what "true" means because there is no clear success/failure
here. But I agree that the enum might be redundant.
What do you think?

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.cc:69: host_ready_ = host_ &&
host_->IsReady();
On 2011/08/08 03:29:03, hbono wrote:
> I'm wondering if we can copy a scoped_refptr |host_| to a boolean variable
> |host_ready_| here. Does this mean 'host_ready_ = !!host.get()'?

Done.

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:59: ~SpellCheckProfile();
On 2011/08/08 03:29:03, hbono wrote:
> I think this destructor should be virtual because the one of the
> SpellCheckHostObserver is virtual.

Done.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:62: // This can return null
when the spell-checking is disabled
On 2011/08/08 03:29:03, hbono wrote:
> nit: null -> NULL.

Done.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile.h:86: SpellCheckHostObserver*
observer,
On 2011/08/08 03:29:03, hbono wrote:
> nit: need four spaces before this first parameter.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>

Done.

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

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:53: }
On 2011/08/08 03:29:03, hbono wrote:
> nit: '}' -> '}  // namespace'.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...>.

Done.

http://codereview.chromium.org/7583003/diff/2002/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_profile_unittest.cc:65: 
On 2011/08/08 03:29:03, hbono wrote:
> nit: I'm wondering if we need two empty lines here.

Done.

[Hironori Bono, 2011-08-08 09:15:35]

Morita-san,

LGTM (with a style nit) for spellchecker changes.
Thank you for your updates.

Regards,

Hironori Bono

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

http://codereview.chromium.org/7583003/diff/7001/chrome/browser/spellchecker/...
chrome/browser/spellchecker/spellcheck_host_impl.cc:397:
IsUsingPlatformChecker();
nit: need four spaces.

[Miranda Callahan, 2011-08-08 09:26:34]

LGTM with naming nit.

http://codereview.chromium.org/7583003/diff/7001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7583003/diff/7001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:178: SpellCheckProfile*
spellcheck_profile();
This should be named GetSpellcheckProfile(), to match the other lazy-creation
accessor names in Profile, as it's not a strict accessor, and could have the
side effect of creation.

[gmorrita, 2011-08-09 01:40:10]

Bono-san, Miranda, thanks for taking a look!
I'll commit this once I take the feedback.

[commit-bot: I haz the power, 2011-08-09 12:57:31]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 95980, job name
was 7583003-8001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-09 13:31:38]

Try job failure for 7583003-8001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[gmorrita, 2011-08-10 01:03:36]

Hmm the compiler was in trouble. I'll retry.

[commit-bot: I haz the power, 2011-08-10 05:49:16]

Change committed as 96135