Make FaviconService a ProfileKeyedService

Make FaviconService a ProfileKeyedService

BUG=112526
Looks like rlp@ already had a patch for this. My bad for not CC'ing myself to the bug...

[Torne, 2012-08-16 17:45:24]

Looks reasonable to me but someone who wrote some of the favicon code should
really approve.

[sky, 2012-08-16 19:30:06]

Elliot should review this.

[Elliot Glaysher, 2012-08-16 19:37:49]

Did you include modifications to profile_impl.h? You should be able to remove
the scoped_ptr<FaviconService> now.

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
File chrome/browser/favicon/favicon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
chrome/browser/favicon/favicon_service_factory.cc:36: // constructor. See
comment in FaviconService.h.
While I agree this is suboptimal, this is more of a nice to have, no?

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/prof...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/prof...
chrome/browser/profiles/profile_impl.cc:719: FaviconService*
ProfileImpl::GetFaviconService(ServiceAccessType sat) {
This is OK as long as you plan to come back and pull the Profile::Get accessor
out.

[joth, 2012-08-16 21:57:52]

couple comments while passing through...

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
File chrome/browser/favicon/favicon_service.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
chrome/browser/favicon/favicon_service.cc:9: #include
"chrome/browser/profiles/profile.h"
as you don't use the profile (just pass it around) you can remove the include
now.

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
File chrome/browser/favicon/favicon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
chrome/browser/favicon/favicon_service_factory.cc:36: // constructor. See
comment in FaviconService.h.
On 2012/08/16 19:37:49, Elliot Glaysher wrote:
> While I agree this is suboptimal, this is more of a nice to have, no?

AIUI it's  a nice-to-have for this patch, but necessary for some longer term
plans (see also b/6094807)

[andreip3000, 2012-08-17 09:34:57]

Thanks for the review!

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
File chrome/browser/favicon/favicon_service.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
chrome/browser/favicon/favicon_service.cc:9: #include
"chrome/browser/profiles/profile.h"
On 2012/08/16 21:57:52, joth wrote:
> as you don't use the profile (just pass it around) you can remove the include
> now.

Done.

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
File chrome/browser/favicon/favicon_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/favi...
chrome/browser/favicon/favicon_service_factory.cc:36: // constructor. See
comment in FaviconService.h.
On 2012/08/16 21:57:52, joth wrote:
> On 2012/08/16 19:37:49, Elliot Glaysher wrote:
> > While I agree this is suboptimal, this is more of a nice to have, no?
> 
> AIUI it's  a nice-to-have for this patch, but necessary for some longer term
> plans (see also b/6094807)

Yep, I'll look into it after finishing this patch and getting rid of the
GetFaviconService accessors.

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/prof...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10857037/diff/8002/chrome/browser/prof...
chrome/browser/profiles/profile_impl.cc:719: FaviconService*
ProfileImpl::GetFaviconService(ServiceAccessType sat) {
On 2012/08/16 19:37:49, Elliot Glaysher wrote:
> This is OK as long as you plan to come back and pull the Profile::Get accessor
> out.

Yes, that's my next patch. I looked into doing this now but there are about 28
callsites for this accessor. Torne suggested taking them out in a follow-up
patch.

[andreip3000, 2012-08-17 10:25:47]

Looks like rlp@ already had a patch for this. My bad for not CC'ing myself to
the bug...

http://codereview.chromium.org/10828263/