Profiles: Move the Spelling system onto a ProfileKeyedServiceFactory.

Profiles: Move the Spelling system onto a ProfileKeyedServiceFactory.

This removes ownership of the SpellCheck stuff from ProfileImpl and moves it into the PKSF model.

BUG=103693
TBR=beng

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

[Elliot Glaysher, 2012-01-25 00:33:10]

Hello,

As part of the move to remove all objects from Profile ownership and their
corresponding accessors, we're moving each service into the ProfileKeyedService
dependency framework.

I do have one question here:

https://chromiumcodereview.appspot.com/9289007/diff/1/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (left):

https://chromiumcodereview.appspot.com/9289007/diff/1/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:382:
GetPrefs()->GetBoolean(prefs::kEnableSpellCheck));
This means that the spelling stuff is never lazily initialized. Is that OK? I've
moved this block to one that's lazily run on first access to the spelling
factory.

[Hajime Morrita, 2012-01-25 01:52:37]

Hi erg, Thanks for doing this.
It basically looks good. I added a small comment about dependency.

https://chromiumcodereview.appspot.com/9289007/diff/2001/chrome/browser/spell...
File chrome/browser/spellchecker/spellcheck_profile.cc (right):

https://chromiumcodereview.appspot.com/9289007/diff/2001/chrome/browser/spell...
chrome/browser/spellchecker/spellcheck_profile.cc:30:
SpellCheckProfile::SpellCheckProfile(Profile* profile)
It's a bit unfortunate that SpellCheckProfile has a reference to Profile.
It looks we need this only for the constructor and ReinializeSpellCheckHost.
Is it possible to push this dependency out to the factory?

[Elliot Glaysher, 2012-01-25 20:33:56]

On 2012/01/25 01:52:37, morrita1 wrote:
> Hi erg, Thanks for doing this.
> It basically looks good. I added a small comment about dependency.
> 
>
https://chromiumcodereview.appspot.com/9289007/diff/2001/chrome/browser/spell...
> File chrome/browser/spellchecker/spellcheck_profile.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9289007/diff/2001/chrome/browser/spell...
> chrome/browser/spellchecker/spellcheck_profile.cc:30:
> SpellCheckProfile::SpellCheckProfile(Profile* profile)
> It's a bit unfortunate that SpellCheckProfile has a reference to Profile.
> It looks we need this only for the constructor and ReinializeSpellCheckHost.
> Is it possible to push this dependency out to the factory?


Whatever owns the PrefChangeRegistrar also needs the Profile so that when it
receives a notification it can then get the URLRequestContextGetter. If the
factory owns the PrefChaneRegistrar, that will require a mapping from the
PrefService to the Profile, and that seems worse. This is comparably simple.

[If this explanation is too short, I'm sorry. I wrote out four paragraphs on why
this is better, went to lunch, and then codereview ate my explanation,
complaining about an invalid XRSF token.]

[Hajime Morrita, 2012-01-25 21:01:13]

> [If this explanation is too short, I'm sorry. I wrote out four paragraphs on
why
> this is better, went to lunch, and then codereview ate my explanation,
> complaining about an invalid XRSF token.]
So I'm writing this in Emacs to avoid such a situation ;-)
Anyway, your point makes sense. I'm convinced to LGTM.
We need to put notification registration stuff somewhere.

When I kicked out Profile object from the class, 
I did it to gain a testability. This change is also fine
because it successfully keeps the test.

[commit-bot: I haz the power, 2012-01-25 21:21:33]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Elliot Glaysher, 2012-01-25 21:28:22]

On 2012/01/25 21:21:33, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

ping sky

[sky, 2012-01-25 22:33:29]

I'm not an owner for any of these directories.

[Elliot Glaysher, 2012-01-25 22:41:09]

-sky, +avi

Avi, you are the closest thing to an OWNER here. You're an owner of the
tab_contents/ directory. morrita reviewed the spell checker. Stamp for
tab_contents/?

[Elliot Glaysher, 2012-01-26 20:10:13]

-avi who is on vacation, +brettw who apparently isn't.

[Elliot Glaysher, 2012-01-26 21:25:31]

I talked with Ben IRL and he told me to just TBR him.

[commit-bot: I haz the power, 2012-01-26 21:25:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9289007/2001

[commit-bot: I haz the power, 2012-01-26 23:58:50]

Try job failure for 9289007-2001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-01-27 00:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/9289007/2001

[commit-bot: I haz the power, 2012-01-27 03:47:30]

Change committed as 119358