AutoFill Profiles dialog implemented according to new mocks on Mac

AutoFill Profiles dialog implemented according to new mocks on Mac

New mocks are attached to bug 44622.  These changes replace the in-place editing of address and credit card records with a table of records and separate sheets for manipulating the record data.  Changes to the layout of fields on the sheets has been done also.
AutoFillDialog.xib changes:  Replaced disclosure based list of address and credit cards with an NSTableView of the same data.  Added buttons for "Add", "Edit", and "Remove".
Replaced AutoFillAddressViewController.xib with sheet-based AutoFillAddressSheetController.xib.
Replaced AutoFillCreditCardViewController.xib with sheet-based AutoFillCreditCardSheetController.xib.

BUG=44621
TEST=AutoFillAddressModelTest,AutoFillAddressSheetControllerTest,AutoFillCreditCardModelTest,AutoFillCreditCardSheetControllerTest,AutoFillDialogControllerTest

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

[Miranda Callahan, 2010-06-07 23:19:19]

Mostly nits, but a couple notes of substance, and a few general points:

* Save should probably be highlighted as the default choice when the "Add an
address" and "Add a credit card" sheets appear.

* It would be nice to be able to delete multiple items. :-)

* Unless this is only going out in en-US (and even then, actually, considering
how many foreign numbers people are apt to dial), I don't think we should check
for US-formatted phone numbers.  Especially since we don't check for proper  zip
codes (which is a good thing!).

* Delete and backspace should work.

* Deleting the first element in a set of addresses or credit cards should not
result in no selection if there are other items remaining in the set.

http://codereview.chromium.org/2673006/diff/1/11
File chrome/browser/autofill/autofill_address_sheet_controller_mac.mm (right):

http://codereview.chromium.org/2673006/diff/1/11#newcode5
chrome/browser/autofill/autofill_address_sheet_controller_mac.mm:5: #import
"chrome/browser/autofill/autofill_address_sheet_controller_mac.h"
nit: empty line after first #import.

http://codereview.chromium.org/2673006/diff/1/12
File chrome/browser/autofill/autofill_address_sheet_controller_mac_unittest.mm
(right):

http://codereview.chromium.org/2673006/diff/1/12#newcode23
chrome/browser/autofill/autofill_address_sheet_controller_mac_unittest.mm:23:
mode:kAutoFillAddressAddMode]);
nit: indent so colons match vertically.

http://codereview.chromium.org/2673006/diff/1/17
File chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h
(right):

http://codereview.chromium.org/2673006/diff/1/17#newcode48
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h:48: // not
hold them as scoped_nsobjects because they are exposed as a KVO
nit: s/as a/a

http://codereview.chromium.org/2673006/diff/1/18
File chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm
(right):

http://codereview.chromium.org/2673006/diff/1/18#newcode5
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm:5: #import
"chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h"
nit: empty line after first #import.

http://codereview.chromium.org/2673006/diff/1/18#newcode104
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm:104: //
Ensures that current selection is valid, if not reset it.
English nit: "Ensures that current selection is valid. If not, reset it."

http://codereview.chromium.org/2673006/diff/1/21
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/2673006/diff/1/21#newcode72
chrome/browser/autofill/autofill_dialog_controller_mac.h:72: // Transient
reference to address "Add" / "Edit" sheed for credit card
nid: sheed

http://codereview.chromium.org/2673006/diff/1/22
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/2673006/diff/1/22#newcode427
chrome/browser/autofill/autofill_dialog_controller_mac.mm:427: return
@"Addresses";
Hmm, shouldn't these strings be localized?

http://codereview.chromium.org/2673006/diff/1/22#newcode450
chrome/browser/autofill/autofill_dialog_controller_mac.mm:450: return @"Credit
Cards";
See remark above about localization.

http://codereview.chromium.org/2673006/diff/1/22#newcode483
chrome/browser/autofill/autofill_dialog_controller_mac.mm:483: if (tableView ==
tableView_)
Add braces since you have two lines (even though one is a comment).

http://codereview.chromium.org/2673006/diff/1/24
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/2673006/diff/1/24#newcode120
chrome/browser/autofill/contact_info.cc:120: } else if (values.size() == 2) {
I think that if we're going to make assumptions about the way that names are
stored, we should perhaps provide a template (the way "New address" shows up in
the label field), showing that names should be entered as "Jane Q. Public Jr.",
or whatever.  Especially if AutoFillType(NAME_SUFFIX), etc., enforce some kind
of encoding on the name parts.  On the other hand, if the name fields needn't
correspond to any actual name part, it would be nice if there were an
AutoFillType(NAME_GENERAL) in order to make that clear in the code -- because
reading this, my first thought is "how will the user know to enter names in
"First Last" order, rather than "Last, First?"

[dhollowa, 2010-06-08 17:40:30]

> * Save should probably be highlighted as the default choice when the "Add an
> address" and "Add a credit card" sheets appear.
> 

I've intentionally disabled the Save button as default due to the presence of
text edit fields that compete with return key.  This is according to HIG
guidelines.

> * It would be nice to be able to delete multiple items. :-)
> 

I've logged bug 46075 on this.  We're moving to a non-modal dialog where the
controller protocol with the WebDB does not support multi-delete.  Will revisit
once we get there.

> * Unless this is only going out in en-US (and even then, actually, considering
> how many foreign numbers people are apt to dial), I don't think we should
check
> for US-formatted phone numbers.  Especially since we don't check for proper 
zip
> codes (which is a good thing!).
> 

I agree.  We have a number of non-international issues at the moment.  We're
planning for M7 to be a i8n milestone.  So will revisit then.

> * Delete and backspace should work.
> 

Done.

> * Deleting the first element in a set of addresses or credit cards should not
> result in no selection if there are other items remaining in the set.

Done.

http://codereview.chromium.org/2673006/diff/1/11
File chrome/browser/autofill/autofill_address_sheet_controller_mac.mm (right):

http://codereview.chromium.org/2673006/diff/1/11#newcode5
chrome/browser/autofill/autofill_address_sheet_controller_mac.mm:5: #import
"chrome/browser/autofill/autofill_address_sheet_controller_mac.h"
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> nit: empty line after first #import.

Done.

http://codereview.chromium.org/2673006/diff/1/12
File chrome/browser/autofill/autofill_address_sheet_controller_mac_unittest.mm
(right):

http://codereview.chromium.org/2673006/diff/1/12#newcode23
chrome/browser/autofill/autofill_address_sheet_controller_mac_unittest.mm:23:
mode:kAutoFillAddressAddMode]);
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> nit: indent so colons match vertically.

Done.

http://codereview.chromium.org/2673006/diff/1/17
File chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h
(right):

http://codereview.chromium.org/2673006/diff/1/17#newcode48
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h:48: // not
hold them as scoped_nsobjects because they are exposed as a KVO
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> nit: s/as a/a

Done.

http://codereview.chromium.org/2673006/diff/1/18
File chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm
(right):

http://codereview.chromium.org/2673006/diff/1/18#newcode5
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm:5: #import
"chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.h"
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> nit: empty line after first #import.

Done.

http://codereview.chromium.org/2673006/diff/1/18#newcode104
chrome/browser/autofill/autofill_credit_card_sheet_controller_mac.mm:104: //
Ensures that current selection is valid, if not reset it.
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> English nit: "Ensures that current selection is valid. If not, reset it."

Done.

http://codereview.chromium.org/2673006/diff/1/21
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/2673006/diff/1/21#newcode72
chrome/browser/autofill/autofill_dialog_controller_mac.h:72: // Transient
reference to address "Add" / "Edit" sheed for credit card
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> nid: sheed

Done.

http://codereview.chromium.org/2673006/diff/1/22
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/2673006/diff/1/22#newcode427
chrome/browser/autofill/autofill_dialog_controller_mac.mm:427: return
@"Addresses";
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> Hmm, shouldn't these strings be localized?

No, these identifiers are not user-visible.

http://codereview.chromium.org/2673006/diff/1/22#newcode450
chrome/browser/autofill/autofill_dialog_controller_mac.mm:450: return @"Credit
Cards";
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> See remark above about localization.

Same.

http://codereview.chromium.org/2673006/diff/1/22#newcode483
chrome/browser/autofill/autofill_dialog_controller_mac.mm:483: if (tableView ==
tableView_)
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> Add braces since you have two lines (even though one is a comment).

Done.

http://codereview.chromium.org/2673006/diff/1/24
File chrome/browser/autofill/contact_info.cc (right):

http://codereview.chromium.org/2673006/diff/1/24#newcode120
chrome/browser/autofill/contact_info.cc:120: } else if (values.size() == 2) {
On 2010/06/07 23:19:19, Miranda Callahan wrote:
> I think that if we're going to make assumptions about the way that names are
> stored, we should perhaps provide a template (the way "New address" shows up
in
> the label field), showing that names should be entered as "Jane Q. Public
Jr.",
> or whatever.  Especially if AutoFillType(NAME_SUFFIX), etc., enforce some kind
> of encoding on the name parts.  On the other hand, if the name fields needn't
> correspond to any actual name part, it would be nice if there were an
> AutoFillType(NAME_GENERAL) in order to make that clear in the code -- because
> reading this, my first thought is "how will the user know to enter names in
> "First Last" order, rather than "Last, First?"

I have added a TODO here.  This was meant to be a stop-gap.  Will review with
folks to formalize this in the very short term.

[Miranda Callahan, 2010-06-08 17:44:20]

Great -- then LGTM!