Allowing PersonalDataManagerFactory to create a service while incognito.

Allowing PersonalDataManagerFactory to create a service while incognito.

R=isherman@chromium.org
BUG=235168
TEST=incognito, show dialog, no asplode.

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

[Ilya Sherman, 2013-04-25 03:17:54]

Are you sure this doesn't also enable saving data in Incognito mode?

[Dan Beam, 2013-04-25 03:21:12]

On 2013/04/25 03:17:54, Ilya Sherman wrote:
> Are you sure this doesn't also enable saving data in Incognito mode?

I think the slew of

  if (profile->IsOffTheRecord())
    return;

in PersonalDataManager::* will ensure this doesn't.  I'll check to make sure
this code flow is being hit.

[Dan Beam, 2013-04-25 03:23:50]

browser_context_->IsOffTheRecord()**
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[Dan Beam, 2013-04-25 03:26:41]

it's possible I'll need to guard the SaveImported*() methods to avoid a
NOTREACHED(),
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[Dan Beam, 2013-04-25 03:47:50]

also removed the NOTREACHED()s from PersonalDataManager::SaveImported*() so we
don't have to guard against this and they now match other methods in this class

[Ilya Sherman, 2013-04-25 04:12:52]

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
File components/autofill/browser/personal_data_manager.cc (right):

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:152: return;
If I'm jumping through code correctly, it looks like the AutofillWebDataService
will be NULL in Incognito mode, which means that we still won't have any
Autofill data available with this CL in place.

If you want to implement this change, please carefully go through the code and
make sure that (a) it actually shows Autofill data in Incognito mode, and (b)
the pursuit of (a) doesn't cause us to allow any data to be written to disk in
Incognito mode.

Keep in mind that it's probably not acceptable to have multiple connections to
the backing database, so you'll need to be careful to avoid that case.

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:523:
prefs::kAutofillAuxiliaryProfilesEnabled)) {
Will this do the right thing if browser_context_ is an Incognito window?

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:675:
prefs::kAutofillEnabled);
Will this do the right thing if browser_context_ is Incognito?

[Dan Beam, 2013-04-25 05:30:34]

added tests

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
File components/autofill/browser/personal_data_manager.cc (right):

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:152: return;
On 2013/04/25 04:12:52, Ilya Sherman wrote:
> If I'm jumping through code correctly, it looks like the
AutofillWebDataService
> will be NULL in Incognito mode, which means that we still won't have any
> Autofill data available with this CL in place.
> 
> If you want to implement this change, please carefully go through the code and
> make sure that (a) it actually shows Autofill data in Incognito mode, and (b)
> the pursuit of (a) doesn't cause us to allow any data to be written to disk in
> Incognito mode.
> 
> Keep in mind that it's probably not acceptable to have multiple connections to
> the backing database, so you'll need to be careful to avoid that case.

so, if the first part of this maybe isn't right (as our dialog works for me with
just this patch), do I need to worry about the rest of what you said?

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:523:
prefs::kAutofillAuxiliaryProfilesEnabled)) {
On 2013/04/25 04:12:52, Ilya Sherman wrote:
> Will this do the right thing if browser_context_ is an Incognito window?

this seems to be agnostic to whether the profile is OTR or not

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:675:
prefs::kAutofillEnabled);
On 2013/04/25 04:12:52, Ilya Sherman wrote:
> Will this do the right thing if browser_context_ is Incognito?

this code seems to agnostic to OTR-ness as well, additionally our dialog doesn't
show when autofill is disabled -- it just errors out and dispatches an event.

[Ilya Sherman, 2013-04-25 06:10:01]

LGTM with one nit.  Thanks for double-checking *all the things* :)

https://chromiumcodereview.appspot.com/14392005/diff/10001/components/autofil...
File components/autofill/browser/personal_data_manager.cc (right):

https://chromiumcodereview.appspot.com/14392005/diff/10001/components/autofil...
components/autofill/browser/personal_data_manager.cc:145:
metric_logger_->LogIsAutofillEnabledAtStartup(IsAutofillEnabled());
nit: Let's not log this in Incognito mode.

[Dan Beam, 2013-04-25 06:33:34]

https://chromiumcodereview.appspot.com/14392005/diff/10001/components/autofil...
File components/autofill/browser/personal_data_manager.cc (right):

https://chromiumcodereview.appspot.com/14392005/diff/10001/components/autofil...
components/autofill/browser/personal_data_manager.cc:145:
metric_logger_->LogIsAutofillEnabledAtStartup(IsAutofillEnabled());
On 2013/04/25 06:10:02, Ilya Sherman wrote:
> nit: Let's not log this in Incognito mode.

Done.

[Dan Beam, 2013-04-25 06:37:10]

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
File components/autofill/browser/personal_data_manager.cc (right):

https://chromiumcodereview.appspot.com/14392005/diff/6001/components/autofill...
components/autofill/browser/personal_data_manager.cc:152: return;
On 2013/04/25 05:30:34, Dan Beam wrote:
> On 2013/04/25 04:12:52, Ilya Sherman wrote:
> > If I'm jumping through code correctly, it looks like the
> AutofillWebDataService
> > will be NULL in Incognito mode, which means that we still won't have any
> > Autofill data available with this CL in place.
> > 
> > If you want to implement this change, please carefully go through the code
and
> > make sure that (a) it actually shows Autofill data in Incognito mode, and
(b)
> > the pursuit of (a) doesn't cause us to allow any data to be written to disk
in
> > Incognito mode.
> > 
> > Keep in mind that it's probably not acceptable to have multiple connections
to
> > the backing database, so you'll need to be careful to avoid that case.

so if you're not worried about ^ this, CQ da patch, else I'll worry about it
tomorrow.

> 
> so, if the first part of this maybe isn't right (as our dialog works for me
with
> just this patch), do I need to worry about the rest of what you said?

[commit-bot: I haz the power, 2013-04-25 06:39:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14392005/10003

[commit-bot: I haz the power, 2013-04-25 08:41:49]

Change committed as 196360