[components] Switch {RefCounted}ProfileKeyedService to use BrowserContext

[components] Switch {RefCounted}ProfileKeyedService to use BrowserContext

instead of Profile. This will help move it out of chrome.

BUG=227219
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196777

[Paweł Hajdan Jr., 2013-04-18 18:32:59]

Please review. This is a work-in-progress change (some highlights in comments
below).

Most of the action is in chrome/browser/profiles, and the rest is just
adjustments.

I've yet to fix e.g. 80cols issues - in case I'd need to throw out this CL.

https://codereview.chromium.org/14141006/diff/1/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14141006/diff/1/build/linux/system.gyp#newcode46
build/linux/system.gyp:46: 'all_dependent_settings': {
Suggestions welcome how to do this better. clipboard header in src/ui includes
GTK+ headers, and that happens or not to work depending where that gets
transitively included.

To make it work reliably I've done this.

https://codereview.chromium.org/14141006/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://codereview.chromium.org/14141006/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:41: profile =
profile->GetOriginalProfile();
There is no GetOriginalProfile or equivalent in BrowserContext, and I'm not
quite sure what to do about this.

John Abd-El-Malek generally objected to adding GetOriginalProfile concept to
content.

I've commented out usages of GetProfileToUse in this CL just to get it to
compile, but this will need a solution before I land this.

[Jói, 2013-04-19 10:39:25]

Hi Paweł,

I didn't look at much beyond //chrome/browser/profiles.  I think this approach
should work, see feedback below regarding incognito.

Cheers,
Jói

https://codereview.chromium.org/14141006/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://codereview.chromium.org/14141006/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_keyed_base_factory.cc:41: profile =
profile->GetOriginalProfile();
On 2013/04/18 18:33:00, Paweł Hajdan Jr. wrote:
> There is no GetOriginalProfile or equivalent in BrowserContext, and I'm not
> quite sure what to do about this.
> 
> John Abd-El-Malek generally objected to adding GetOriginalProfile concept to
> content.
> 
> I've commented out usages of GetProfileToUse in this CL just to get it to
> compile, but this will need a solution before I land this.

Perhaps the ProfileDependencyManager should receive a delegate as an
initialization parameter; that delegate could implement GetContextToUse for a
given BrowserContext*.

Alternatively, perhaps it is worth looking into having a BrowserContext-based
PKSF that knows nothing about incognito (and thus always response with the same
pointer when GetContextToUse is called) and a specialization within Chrome that
is keyed off of Profile that can implement it in terms of a Profile object.  I'm
not sure this is better though, and it might be a bunch of work to figure out
when it's applicable, although I guess that you only need to know about
incognito if ServiceRedirectedInIncognito returns true?

[Elliot Glaysher, 2013-04-22 19:17:26]

Given that BrowserContext::IsOffTheRecord() exists, why does jam not want the
rest of incognito state to be plumed through?

[Paweł Hajdan Jr., 2013-04-22 19:26:23]

On 2013/04/22 19:17:26, Elliot Glaysher wrote:
> Given that BrowserContext::IsOffTheRecord() exists, why does jam not want the
> rest of incognito state to be plumed through?

Thank you for taking a look! Quoting John:

"I'm facing this too when I look at extensions.

I don't think this should go into content. The concept of two profiles
associated with each other is a chrome thing. I'd prefer this doesn't leak into
content, since content doesn't need to know this. one of the guiding principles
of the content refactoring is that the only stuff in the content module is stuff
that content needs to know about."

I think I have a solution for this though - just wanted to check whether current
changes could raise any objections. I'm taking that as no major objections about
the direction of this CL, and will work on the above remaining part.

I'll send another message when it's ready for final review.

[Paweł Hajdan Jr., 2013-04-23 18:29:51]

PTAL - this is now ready for your review.

I've left GetProfileToUse as-is for now - I'll do a second pass and convert
ServiceRedirectedInIncognito and others to something more generic (in interface
- I think I'll still manage to have a shared and uniform implementation in
chrome).

[Jói, 2013-04-23 22:45:21]

LGTM with a question and a nit.

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp#new...
build/linux/system.gyp:46: 'all_dependent_settings': {
Were the changes in this file meant to be part of this changelist?

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_apitest.cc:183:
ProfileKeyedService* IdentityAPITestFactory(content::BrowserContext* profile) {
nit: Might be nice to rename |profile| to |context|, here and elsewhere. I don't
mind if that's done as a separate follow-up change.

[Paweł Hajdan Jr., 2013-04-23 22:58:10]

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp#new...
build/linux/system.gyp:46: 'all_dependent_settings': {
On 2013/04/23 22:45:22, Jói wrote:
> Were the changes in this file meant to be part of this changelist?

Unfortunately, yes. Otherwise it doesn't compile.

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_apitest.cc:183:
ProfileKeyedService* IdentityAPITestFactory(content::BrowserContext* profile) {
On 2013/04/23 22:45:22, Jói wrote:
> nit: Might be nice to rename |profile| to |context|, here and elsewhere. I
don't
> mind if that's done as a separate follow-up change.

I think I'll be changing that later, also including ProfileKeyedService ->
BrowserContextKeyedService etc.

[Jói, 2013-04-24 09:09:09]

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp#new...
build/linux/system.gyp:46: 'all_dependent_settings': {
On 2013/04/23 22:58:11, Paweł Hajdan Jr. wrote:
> On 2013/04/23 22:45:22, Jói wrote:
> > Were the changes in this file meant to be part of this changelist?
> 
> Unfortunately, yes. Otherwise it doesn't compile.

Hmm, do you know why?

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/identity/identity_apitest.cc (right):

https://codereview.chromium.org/14141006/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/identity/identity_apitest.cc:183:
ProfileKeyedService* IdentityAPITestFactory(content::BrowserContext* profile) {
On 2013/04/23 22:58:11, Paweł Hajdan Jr. wrote:
> On 2013/04/23 22:45:22, Jói wrote:
> > nit: Might be nice to rename |profile| to |context|, here and elsewhere. I
> don't
> > mind if that's done as a separate follow-up change.
> 
> I think I'll be changing that later, also including ProfileKeyedService ->
> BrowserContextKeyedService etc.

OK, fine by me.

[Paweł Hajdan Jr., 2013-04-25 17:51:42]

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/14141006/diff/8001/build/linux/system.gyp#new...
build/linux/system.gyp:46: 'all_dependent_settings': {
On 2013/04/24 09:09:10, Jói wrote:
> On 2013/04/23 22:58:11, Paweł Hajdan Jr. wrote:
> > On 2013/04/23 22:45:22, Jói wrote:
> > > Were the changes in this file meant to be part of this changelist?
> > 
> > Unfortunately, yes. Otherwise it doesn't compile.
> 
> Hmm, do you know why?

The error message is:

  CXX(target) out/Debug/obj.target/apps/apps/app_restore_service_factory.o
In file included from ./content/public/browser/browser_context.h:13:0,
                 from ./chrome/browser/profiles/profile.h:16,
                 from apps/app_restore_service_factory.cc:8:
./ui/base/clipboard/clipboard.h:23:21: fatal error: gdk/gdk.h: No such file or
directory

And it's caused by GTK+ include path not being propagated to the build target
that contains apps/app_restore_service_factory.cc . The problem is: why should
apps gyp target explicitly include a gtk+ dependency, while it exists there only
due to transitive header inclusion?

[Elliot Glaysher, 2013-04-25 22:43:23]

profiles lgtm

[Jói, 2013-04-26 12:34:38]

OK, LGTM.

On Thu, Apr 25, 2013 at 10:43 PM,  <erg@chromium.org> wrote:
> profiles lgtm
>
>
> https://codereview.chromium.org/14141006/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Paweł Hajdan Jr., 2013-04-26 18:09:13]

Committed patchset #6 manually as r196777.