Use BrowserContext as key in API. Switch Autofill to use BC in place of Profile.

Use BrowserContext as key in API. Switch Autofill to use BC in place of Profile.

All that Autofill needed from Profile, apart from looking up services
keyed on Profile, was stuff already in BrowserContext
e.g. IsIncognitoMode(). Once a base class for ProfileSyncService
migrates to API, we should be able to remove the profile.h includes in
Autofill.

TBR=mechanicalowners@chromium.org
BUG=140037
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156620

[erikwright (departed), 2012-09-04 19:42:14]

Main question: what is the long term plan for each of the cases where  a
static_cast is introduced?

Also, what is GetOriginalBrowserContext for? I didn't see a client.

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS#n...
chrome/browser/autofill/DEPS:28: # Needs abstract base.
As a side comment, I am always confused when I see "abstract base" but it turns
out to be pure-virtual (possibly with exception of a static ForContext etc.,
which is not really an exception). I think of Abstract as implying that there is
a partial implementation.

Unless I'm wrong about terminology, I wonder if we could use "pure-virtual
interface" instead. Or "component interface". The former might be a detail that
one could argue is technically not required (though will probably always be
true) and the latter is lacking a formal meaning in object-oriented design and
thus has no baggage.

I realize, in this case, that this particular comment is already removed in a
pipelined CL.

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:196: tab_contents_->profile());
Presumably you have determined that this change from profile() to
profile->GetOriginalProfile() has no impact?

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
chrome/browser/autofill/personal_data_manager.cc:931:
static_cast<Profile*>(browser_context_));
Will this be permanent?

http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:119: return
static_cast<Profile*>(context)->GetPrefs();
Do you think this static cast will be permanent?

[Jói, 2012-09-04 20:10:24]

Thanks for the review.

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS#n...
chrome/browser/autofill/DEPS:28: # Needs abstract base.
On 2012/09/04 19:42:14, erikwright wrote:
> As a side comment, I am always confused when I see "abstract base" but it
turns
> out to be pure-virtual (possibly with exception of a static ForContext etc.,
> which is not really an exception). I think of Abstract as implying that there
is
> a partial implementation.
> 
> Unless I'm wrong about terminology, I wonder if we could use "pure-virtual
> interface" instead. Or "component interface". The former might be a detail
that
> one could argue is technically not required (though will probably always be
> true) and the latter is lacking a formal meaning in object-oriented design and
> thus has no baggage.
> 
> I realize, in this case, that this particular comment is already removed in a
> pipelined CL.

We could just say "API" since that's what we're calling the directory housing
the extracted APIs.

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
chrome/browser/autofill/autofill_manager.cc:196: tab_contents_->profile());
On 2012/09/04 19:42:14, erikwright wrote:
> Presumably you have determined that this change from profile() to
> profile->GetOriginalProfile() has no impact?

No, this was a mistake.  Thanks for noticing.

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
chrome/browser/autofill/personal_data_manager.cc:931:
static_cast<Profile*>(browser_context_));
On 2012/09/04 19:42:15, erikwright wrote:
> Will this be permanent?

This goes away in the next change after, where instead we do
ProfileSyncServiceBase::ForContext(browser_context_);

http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:119: return
static_cast<Profile*>(context)->GetPrefs();
On 2012/09/04 19:42:15, erikwright wrote:
> Do you think this static cast will be permanent?

As permanent as the PrefServiceBase::ForContext function, which as we've
discussed can go away once we have dependency injection everywhere, or transform
into a ForContextForTesting once it's only used by tests.

[erikwright (departed), 2012-09-06 18:08:18]

On 2012/09/04 20:10:24, Jói wrote:
> Thanks for the review.
> 
> http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS
> File chrome/browser/autofill/DEPS (right):
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/DEPS#n...
> chrome/browser/autofill/DEPS:28: # Needs abstract base.
> On 2012/09/04 19:42:14, erikwright wrote:
> > As a side comment, I am always confused when I see "abstract base" but it
> turns
> > out to be pure-virtual (possibly with exception of a static ForContext etc.,
> > which is not really an exception). I think of Abstract as implying that
there
> is
> > a partial implementation.
> > 
> > Unless I'm wrong about terminology, I wonder if we could use "pure-virtual
> > interface" instead. Or "component interface". The former might be a detail
> that
> > one could argue is technically not required (though will probably always be
> > true) and the latter is lacking a formal meaning in object-oriented design
and
> > thus has no baggage.
> > 
> > I realize, in this case, that this particular comment is already removed in
a
> > pipelined CL.
> 
> We could just say "API" since that's what we're calling the directory housing
> the extracted APIs.
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
> File chrome/browser/autofill/autofill_manager.cc (right):
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/autofi...
> chrome/browser/autofill/autofill_manager.cc:196: tab_contents_->profile());
> On 2012/09/04 19:42:14, erikwright wrote:
> > Presumably you have determined that this change from profile() to
> > profile->GetOriginalProfile() has no impact?
> 
> No, this was a mistake.  Thanks for noticing.
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
> File chrome/browser/autofill/personal_data_manager.cc (right):
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/autofill/person...
> chrome/browser/autofill/personal_data_manager.cc:931:
> static_cast<Profile*>(browser_context_));
> On 2012/09/04 19:42:15, erikwright wrote:
> > Will this be permanent?
> 
> This goes away in the next change after, where instead we do
> ProfileSyncServiceBase::ForContext(browser_context_);
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
> File chrome/browser/prefs/pref_service.cc (right):
> 
>
http://codereview.chromium.org/10919066/diff/1/chrome/browser/prefs/pref_serv...
> chrome/browser/prefs/pref_service.cc:119: return
> static_cast<Profile*>(context)->GetPrefs();
> On 2012/09/04 19:42:15, erikwright wrote:
> > Do you think this static cast will be permanent?
> 
> As permanent as the PrefServiceBase::ForContext function, which as we've
> discussed can go away once we have dependency injection everywhere, or
transform
> into a ForContextForTesting once it's only used by tests.

LGTM.

[commit-bot: I haz the power, 2012-09-13 17:20:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10919066/10001

[commit-bot: I haz the power, 2012-09-13 17:20:54]

Presubmit check for 10919066-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/prefs
    chrome/browser/ui/webui/options

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2012-09-13 17:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10919066/10001

[commit-bot: I haz the power, 2012-09-13 20:27:04]

Change committed as 156620