Change GetProfileToUse method from Profile to content::BrowserContext

Change GetProfileToUse method from Profile to content::BrowserContext

This drops a dependency on Profile in the interface of ProfileKeyedService
infrastructure, and adds helper functions that make it explicit what is being
done (it's not obvious just from "return context" for example that this will
result in the service having its own instance in Incognito mode instead
of no instance).

BUG=227219
R=erg@chromium.org, joi@chromium.org

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

[Paweł Hajdan Jr., 2013-04-29 23:37:05]

This is work in progress, but shows what I intend to do - the rest would be
mechanical changes.

Please take a look and say what you think about the design. After it's good,
I'll make the remaining mechanical changes.

[Elliot Glaysher, 2013-04-30 00:11:12]

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
File chrome/browser/profiles/incognito_helpers.cc (right):

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
chrome/browser/profiles/incognito_helpers.cc:13: return
static_cast<Profile*>(context)->GetOriginalProfile();
The only thing that I worry about is the possibility of something not a Profile
being passed in here.

[Paweł Hajdan Jr., 2013-04-30 18:07:55]

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
File chrome/browser/profiles/incognito_helpers.cc (right):

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
chrome/browser/profiles/incognito_helpers.cc:13: return
static_cast<Profile*>(context)->GetOriginalProfile();
On 2013/04/30 00:11:12, Elliot Glaysher wrote:
> The only thing that I worry about is the possibility of something not a
Profile
> being passed in here.

Right, this is a reasonable concern. Note however, that this is the part
intended to stay in chrome, while most of other files will be pushed down to
content.

In chrome, Profile is the only subclass of BrowserContext:

$ git grep 'public BrowserContext'
content/browser/download/download_manager_impl_unittest.cc:class
MockBrowserContext : public BrowserContext {
content/public/test/test_browser_context.h:class TestBrowserContext : public
BrowserContext {
content/shell/shell_browser_context.h:class ShellBrowserContext : public
BrowserContext {
$ git grep 'public content::BrowserContext'
android_webview/browser/aw_browser_context.h:class AwBrowserContext : public
content::BrowserContext,
chrome/browser/profiles/profile.h:class Profile : public content::BrowserContext
{

Now the publicly exposed TestBrowserContext in content is a bit suspicious - I'm
going to give it a try to make it non-public.

Note that TestBrowserContext used not to be in content/public, but got moved in
https://chromiumcodereview.appspot.com/10492009 (and I didn't fully realize the
long-term consequences of exposing another subclass of BrowserContext which is
not Profile).

[Paweł Hajdan Jr., 2013-04-30 18:21:09]

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
File chrome/browser/profiles/incognito_helpers.cc (right):

https://codereview.chromium.org/13865012/diff/1/chrome/browser/profiles/incog...
chrome/browser/profiles/incognito_helpers.cc:13: return
static_cast<Profile*>(context)->GetOriginalProfile();
On 2013/04/30 18:07:56, Paweł Hajdan Jr. wrote:
> Now the publicly exposed TestBrowserContext in content is a bit suspicious -
I'm
> going to give it a try to make it non-public.
> 
> Note that TestBrowserContext used not to be in content/public, but got moved
in
> https://chromiumcodereview.appspot.com/10492009 (and I didn't fully realize
the
> long-term consequences of exposing another subclass of BrowserContext which is
> not Profile).

Actually it's not going to be easy because of usage out of content:

ash/test/test_shell_delegate.cc:#include
"content/public/test/test_browser_context.h"
chrome/browser/extensions/event_listener_map_unittest.cc:#include
"content/public/test/test_browser_context.h"
components/visitedlink/test/visitedlink_unittest.cc:#include
"content/public/test/test_browser_context.h"

[Jói, 2013-05-02 17:10:52]

LGTM, I think. Wonder if we could e.g. DCHECK if concrete BrowserContext objects
or specializations of BrowserContext other than Profile are being instantiated
in a Chrome process in debug builds?

https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_system_factory.cc (right):

https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_system_factory.cc:83:
content::BrowserContext* ExtensionSystemSharedFactory::GetBrowserContextToUse(
ExtensionSystemSharedFactory -> ExtensionSystemFactory

[Jói, 2013-05-02 17:11:52]

Oh, and sorry for the delay, I was on vacation for a few days.

On Thu, May 2, 2013 at 5:10 PM,  <joi@chromium.org> wrote:
> LGTM, I think. Wonder if we could e.g. DCHECK if concrete BrowserContext
> objects
> or specializations of BrowserContext other than Profile are being
> instantiated
> in a Chrome process in debug builds?
>
>
>
https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
> File chrome/browser/extensions/extension_system_factory.cc (right):
>
>
https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_system_factory.cc:83:
> content::BrowserContext*
> ExtensionSystemSharedFactory::GetBrowserContextToUse(
> ExtensionSystemSharedFactory -> ExtensionSystemFactory
>
> https://codereview.chromium.org/13865012/

[Paweł Hajdan Jr., 2013-05-03 17:34:41]

PTAL - this is now ready for review.

https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_system_factory.cc (right):

https://codereview.chromium.org/13865012/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_system_factory.cc:83:
content::BrowserContext* ExtensionSystemSharedFactory::GetBrowserContextToUse(
On 2013/05/02 17:10:52, Jói wrote:
> ExtensionSystemSharedFactory -> ExtensionSystemFactory

Done.

[Jói, 2013-05-03 17:45:14]

SLGTM

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_helpers.h (right):

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_helpers.h:20: content::BrowserContext*
GetBrowserContextOwnInstanceInIncognito(
Seeing as this is basically a no-op, is it even needed?

[Elliot Glaysher, 2013-05-03 18:04:46]

slgtm

[Paweł Hajdan Jr., 2013-05-03 18:08:19]

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_helpers.h (right):

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_helpers.h:20: content::BrowserContext*
GetBrowserContextOwnInstanceInIncognito(
On 2013/05/03 17:45:14, Jói wrote:
> Seeing as this is basically a no-op, is it even needed?

Yes - mostly to make code readable and obvious.

It looks like a no-op but actually _does_ a thing (and in a pretty sensitive
area, i.e. Incognito mode). 

Without it the service would not be created in Incognito mode. With this helper
used, it does get created. This is not obvious from just reading the code (as
your comment above confirms), and it's used in many places throughout the
codebase (where it's even easier to miss the context).

Also, making this a function makes it more greppable, which IMO is also very
important.

[Jói, 2013-05-03 18:54:08]

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
File chrome/browser/profiles/incognito_helpers.h (right):

https://codereview.chromium.org/13865012/diff/10001/chrome/browser/profiles/i...
chrome/browser/profiles/incognito_helpers.h:20: content::BrowserContext*
GetBrowserContextOwnInstanceInIncognito(
On 2013/05/03 18:08:19, Paweł Hajdan Jr. wrote:
> On 2013/05/03 17:45:14, Jói wrote:
> > Seeing as this is basically a no-op, is it even needed?
> 
> Yes - mostly to make code readable and obvious.
> 
> It looks like a no-op but actually _does_ a thing (and in a pretty sensitive
> area, i.e. Incognito mode). 
> 
> Without it the service would not be created in Incognito mode. With this
helper
> used, it does get created. This is not obvious from just reading the code (as
> your comment above confirms), and it's used in many places throughout the
> codebase (where it's even easier to miss the context).
> 
> Also, making this a function makes it more greppable, which IMO is also very
> important.

Hmm, it just returns the parameter passed to it.  Are there any side effects to
that?  If I'm missing something please document.

Regardless, I don't mind if you keep the function, it serves as documentation
for what's happening in the individual PKSs.

[Paweł Hajdan Jr., 2013-05-03 23:18:43]

Committed patchset #3 manually as r198232.