AutoFill Mac preference for enabling Address Book "me" card usage....

AutoFill Mac preference for enabling Address Book "me" card usage.

Adds a boolean preference to specify whether to use the Mac user's Address Book "me" card as a source of information for AutoFill addresses.  This CL adds the Mac UI and the preference setup only.  It does not cover the functionality for fetching the actual Address Book data.  The changes to AutoFillDialog.xib add the checkbox UI and bind the checkbox to the |auxiliaryEnabled| property in the controller class.  The |addressSection_| outlet is set to the spacer following the new checkbox.  Unit tests are included for the addition of the preference at the mac dialog controller level.

BUG=36496
TEST=PersonalDataManagerTest, AutoFillDialogControllerTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40803

[James Hawkins, 2010-03-05 18:41:39]

http://codereview.chromium.org/668171/diff/1/10
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/668171/diff/1/10#newcode23
chrome/browser/autofill/autofill_dialog.h:23: // Returns the boolean preference
value regarding whether to use auxiliary
This still isn't right.  The dialog should be directly setting this pref.

[dhollowa, 2010-03-05 22:04:57]

http://codereview.chromium.org/668171/diff/1/10
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/668171/diff/1/10#newcode23
chrome/browser/autofill/autofill_dialog.h:23: // Returns the boolean preference
value regarding whether to use auxiliary
On 2010/03/05 18:41:39, James Hawkins wrote:
> This still isn't right.  The dialog should be directly setting this pref.

Done.

[James Hawkins, 2010-03-05 22:17:48]

http://codereview.chromium.org/668171/diff/1004/43
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/668171/diff/1004/43#newcode48
chrome/browser/autofill/personal_data_manager.h:48: // AutoFillDialogObserver
implementation.
s/./:/

http://codereview.chromium.org/668171/diff/1004/41
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/668171/diff/1004/41#newcode245
chrome/browser/autofill/personal_data_manager_unittest.cc:245:
TEST_F(PersonalDataManagerTest, AuxiliaryProfilePreference) {
This test has nothing to do with PersonalDataManager; it should be removed.  If
you want to test that the kAutoFillAuxiliaryProfilesEnabled pref is false with a
clean profile, that should be tested in an AutoFillManager test, because that is
where the pref is registered.

[dhollowa, 2010-03-05 22:27:32]

http://codereview.chromium.org/668171/diff/1004/43
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/668171/diff/1004/43#newcode48
chrome/browser/autofill/personal_data_manager.h:48: // AutoFillDialogObserver
implementation.
On 2010/03/05 22:17:48, James Hawkins wrote:
> s/./:/

Done.

http://codereview.chromium.org/668171/diff/1004/41
File chrome/browser/autofill/personal_data_manager_unittest.cc (right):

http://codereview.chromium.org/668171/diff/1004/41#newcode245
chrome/browser/autofill/personal_data_manager_unittest.cc:245:
TEST_F(PersonalDataManagerTest, AuxiliaryProfilePreference) {
On 2010/03/05 22:17:48, James Hawkins wrote:
> This test has nothing to do with PersonalDataManager; it should be removed. 
If
> you want to test that the kAutoFillAuxiliaryProfilesEnabled pref is false with
a
> clean profile, that should be tested in an AutoFillManager test, because that
is
> where the pref is registered.

Done.
Yes, we have coverage of this in the dialog anyway, so removing.

[James Hawkins, 2010-03-05 22:33:54]

LGTM.  Rubber-stamp on the mac UI changes.

[dhollowa, 2010-03-05 22:35:37]

On 2010/03/05 22:33:54, James Hawkins wrote:
> LGTM.  Rubber-stamp on the mac UI changes.

Will get some Mac-eyes on the rest.  Thanks.

[Nico, 2010-03-05 22:45:53]

Basically LG.

A couple style nits, one renaming suggestion, and one somewhat serious question
below.

http://codereview.chromium.org/668171/diff/1021/62
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/668171/diff/1021/62#newcode51
chrome/browser/autofill/autofill_dialog_controller_mac.h:51: @property
(nonatomic) BOOL auxiliaryEnabled;
nonatomic, assign?

Is "useAddressBookData" a better name?

http://codereview.chromium.org/668171/diff/1021/63
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/668171/diff/1021/63#newcode36
chrome/browser/autofill/autofill_dialog_controller_mac.mm:36:
autoFillProfiles:profiles
move this back to the right, so that the ":"s align

http://codereview.chromium.org/668171/diff/1021/63#newcode87
chrome/browser/autofill/autofill_dialog_controller_mac.mm:87:
auxiliaryEnabled_);
Oh, I see. The current name is fine, then.

http://codereview.chromium.org/668171/diff/1021/63#newcode250
chrome/browser/autofill/autofill_dialog_controller_mac.mm:250:
profile:(Profile*)profile {
align :

http://codereview.chromium.org/668171/diff/1021/63#newcode251
chrome/browser/autofill/autofill_dialog_controller_mac.mm:251: DCHECK(profile);
if profile is NULL, you will crash in save: in release builds – so either make
this a CHECK or put an if(profile_) in save:

http://codereview.chromium.org/668171/diff/1021/63#newcode274
chrome/browser/autofill/autofill_dialog_controller_mac.mm:274:
prefs::kAutoFillAuxiliaryProfilesEnabled)];
align :

http://codereview.chromium.org/668171/diff/1021/60
File chrome/browser/autofill/autofill_dialog_mac.mm (right):

http://codereview.chromium.org/668171/diff/1021/60#newcode19
chrome/browser/autofill/autofill_dialog_mac.mm:19: profile:profile];
align :

http://codereview.chromium.org/668171/diff/1021/59
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/668171/diff/1021/59#newcode237
chrome/browser/autofill/autofill_manager.cc:237: tab_contents_->profile());
You probably want to pass profile()->GetOriginalProfile() here, no? Else, this
stores stuff in the incognito profile if it's active.

http://codereview.chromium.org/668171/diff/1021/67
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/668171/diff/1021/67#newcode332
chrome/common/pref_names.cc:332: const wchar_t
kAutoFillAuxiliaryProfilesEnabled[] =
Oh, you're adding this. If there are no plans to let this do anything on
windows/linux in the medium future, I'd name it after what it's actually used
for (kAutoFillUseAddressBookData) instead of some abstract, more general, less
understandable name.

[dhollowa, 2010-03-05 23:20:13]

Added .grd change for resource string that somehow got reverted.

http://codereview.chromium.org/668171/diff/1021/62
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/668171/diff/1021/62#newcode51
chrome/browser/autofill/autofill_dialog_controller_mac.h:51: @property
(nonatomic) BOOL auxiliaryEnabled;
On 2010/03/05 22:45:53, Nico wrote:
> nonatomic, assign?
> 
> Is "useAddressBookData" a better name?

For use in KVO so can't use assign.  Regarding name we're trying to be
consistent across platforms, so keeping generic.

http://codereview.chromium.org/668171/diff/1021/63
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/668171/diff/1021/63#newcode36
chrome/browser/autofill/autofill_dialog_controller_mac.mm:36:
autoFillProfiles:profiles
On 2010/03/05 22:45:53, Nico wrote:
> move this back to the right, so that the ":"s align

Done.

http://codereview.chromium.org/668171/diff/1021/63#newcode250
chrome/browser/autofill/autofill_dialog_controller_mac.mm:250:
profile:(Profile*)profile {
On 2010/03/05 22:45:53, Nico wrote:
> align :

Done.

http://codereview.chromium.org/668171/diff/1021/63#newcode251
chrome/browser/autofill/autofill_dialog_controller_mac.mm:251: DCHECK(profile);
On 2010/03/05 22:45:53, Nico wrote:
> if profile is NULL, you will crash in save: in release builds – so either make
> this a CHECK or put an if(profile_) in save:

Done.

http://codereview.chromium.org/668171/diff/1021/63#newcode274
chrome/browser/autofill/autofill_dialog_controller_mac.mm:274:
prefs::kAutoFillAuxiliaryProfilesEnabled)];
On 2010/03/05 22:45:53, Nico wrote:
> align :
These are different :'s.  Should not be aligned.

http://codereview.chromium.org/668171/diff/1021/60
File chrome/browser/autofill/autofill_dialog_mac.mm (right):

http://codereview.chromium.org/668171/diff/1021/60#newcode19
chrome/browser/autofill/autofill_dialog_mac.mm:19: profile:profile];
On 2010/03/05 22:45:53, Nico wrote:
> align :

Done.

http://codereview.chromium.org/668171/diff/1021/59
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/668171/diff/1021/59#newcode237
chrome/browser/autofill/autofill_manager.cc:237: tab_contents_->profile());
On 2010/03/05 22:45:53, Nico wrote:
> You probably want to pass profile()->GetOriginalProfile() here, no? Else, this
> stores stuff in the incognito profile if it's active.

Done.

http://codereview.chromium.org/668171/diff/1021/67
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/668171/diff/1021/67#newcode332
chrome/common/pref_names.cc:332: const wchar_t
kAutoFillAuxiliaryProfilesEnabled[] =
On 2010/03/05 22:45:53, Nico wrote:
> Oh, you're adding this. If there are no plans to let this do anything on
> windows/linux in the medium future, I'd name it after what it's actually used
> for (kAutoFillUseAddressBookData) instead of some abstract, more general, less
> understandable name.

Prefer to leave as is.  I think James agrees.  Hopefully comment above will
suffice.

[Nico, 2010-03-05 23:25:34]

SLG. Thanks for the changes.