Autofill dialog for the Mac. This is UI only at this point. The UI is not h...

Autofill dialog for the Mac.  This is UI only at this point.  The UI is not hooked up to the back end yet.  The UI demonstrates manipulation of one address and one credit card record.  Eventually buttons will be added to add and remove additional records.  The additions in this CL are:
  - Preferences dialog has a new "Change autofill settings" button that triggers an autofill settings dialog.
  - The autofill settings dialog now exists and allows the user to manipulate form autofill data.  Specifically address information and credit card information.
  - Each address or credit card record is presented in a disclosure view to allow for summary or detailed views of each record.
  - The autofill dialog is layed out dynamically in a vertical list (ordered by y) using the VerticalLayoutView.
  - Sections are delimited visually with the SectionSeparatorView.  There are currently two sections, one for addresses and one for credit cards.
  - Unit tests are present that exercise the invocation of the dialog and check basic functionality.  Checks are performed to see that data is flowing from core profile and credit card data structures into Cocoa model data structures used for bindings internally by the UI.
  - There are three .xib files (AutoFillDialog.xib, AutoFillAddressFormView.xib, and AutoFillCreditCardFormView.xib) that partition the dialog UI into distinct views, controllers, and model objects.
  - Cocoa databinding is utilized to syncronize dependent parts of the UI.
  - All strings are stored in internationalized form in .grd files and .xib files (with one small TODO execption, see below).

The things remaining to do are:

  - Hook the UI up to the backend model, specifically the PersonalDataManager data.
  - Add support for arbitrary number of address and credit card records.  I.e. Add and Delete buttons.
  - Scroll-to-Point support for autoscrolling when tabbing between fields.
  - Billing and shipping address popups in the credit card section.
  - Any validation of input (need to circle back with UI folks on this).
  - Input validation unit tests.
  - String concatenation of the summary label needs to be internationalized.

BUG=33029
TEST=none


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

[pink (ping after 24hrs), 2010-02-01 15:34:31]

I only looked at about half, there's a lot that needs cleanup throughout the
entire CL (mostly style issues). How about you take a crack at that cleanup,
which will make the remaining files much easier to read, and then I'll re-review
from the start.

http://codereview.chromium.org/558066/diff/1/23
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/1/23#newcode36
chrome/browser/autofill/autofill_address_model_mac.h:36: @property (readonly)
NSString* summary;
what does this return? what's the format? I'd say rather than making this a
property, just make it a normal method call.

http://codereview.chromium.org/558066/diff/1/21
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/21#newcode29
chrome/browser/autofill/autofill_address_model_mac.mm:29: - (NSString *)summary
{
Chrome uses the style NSString*, not NSString *. Please make this change
throughout.

http://codereview.chromium.org/558066/diff/1/21#newcode30
chrome/browser/autofill/autofill_address_model_mac.mm:30: 
omit blank line after opening brace. Please make this change throughout.

http://codereview.chromium.org/558066/diff/1/21#newcode31
chrome/browser/autofill/autofill_address_model_mac.mm:31: BOOL haveFirstName =
firstName_ != nil && ![firstName_ isEqualToString:@""];
you don't have to test obj-c objects against nil before sending them a message.
The runtime will do that for you.

http://codereview.chromium.org/558066/diff/1/21#newcode31
chrome/browser/autofill/autofill_address_model_mac.mm:31: BOOL haveFirstName =
firstName_ != nil && ![firstName_ isEqualToString:@""];
rather than checking for != "", how about just checking the length. THat's
probably a bit faster and easier to read since it's positive logic.

http://codereview.chromium.org/558066/diff/1/21#newcode31
chrome/browser/autofill/autofill_address_model_mac.mm:31: BOOL haveFirstName =
firstName_ != nil && ![firstName_ isEqualToString:@""];
rather than having all this special-case code for these fields being null, why
not just initialize all the strings to be @"" in an init call?

http://codereview.chromium.org/558066/diff/1/21#newcode40
chrome/browser/autofill/autofill_address_model_mac.mm:40: return [NSString
stringWithFormat:@"%@ %@%@ %@",
this can't be localized. The format string needs to be in the grd file.

http://codereview.chromium.org/558066/diff/1/21#newcode57
chrome/browser/autofill/autofill_address_model_mac.mm:57: 
omit blank line.

http://codereview.chromium.org/558066/diff/1/11
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/1/11#newcode23
chrome/browser/autofill/autofill_address_view_controller_mac.h:23: // Exposed
for unit tests.
You can do this in a separate category below so that it's not in the middle of
other methods that are part of the public API. AFter the @end below, just do

@interface AutoFillAddressViewController(ForTestingOnly)
@property (nonatomic, assign) AutoFillAddressModel* addressModel;
@end

http://codereview.chromium.org/558066/diff/1/11#newcode31
chrome/browser/autofill/autofill_address_view_controller_mac.h:31: -
(void)copyModelToProfile:(AutoFillProfile&)profile;
non-const references are not allowed by the google style guide. this needs to be
a  pointer.

http://codereview.chromium.org/558066/diff/1/15
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/15#newcode6
chrome/browser/autofill/autofill_address_view_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
put this below, alphabetized with the others.

http://codereview.chromium.org/558066/diff/1/15#newcode17
chrome/browser/autofill/autofill_address_view_controller_mac.mm:17: 
2 blank lines is enough

http://codereview.chromium.org/558066/diff/1/15#newcode22
chrome/browser/autofill/autofill_address_view_controller_mac.mm:22: 
omit blank line

http://codereview.chromium.org/558066/diff/1/15#newcode29
chrome/browser/autofill/autofill_address_view_controller_mac.mm:29: // pull the
view in now so it, and it's model, are available.
s/it's/its. Capitalize (and punctuate) sentences in comments. Please fix
throughout.

http://codereview.chromium.org/558066/diff/1/15#newcode90
chrome/browser/autofill/autofill_address_view_controller_mac.mm:90:
setLabel:SysUTF16ToNSString(profile.Label())];
indent continuation lines 4 additional spaces. Fix throughout.

http://codereview.chromium.org/558066/diff/1/12
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/1/12#newcode26
chrome/browser/autofill/autofill_credit_card_model_mac.h:26: @property
(readonly) NSString* summary;
same comment on summary property from previous file.

http://codereview.chromium.org/558066/diff/1/24
File chrome/browser/autofill/autofill_credit_card_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/24#newcode18
chrome/browser/autofill/autofill_credit_card_model_mac.mm:18: - (NSString
*)summary {
the same comments here from the other model class all apply as well.

http://codereview.chromium.org/558066/diff/1/24#newcode30
chrome/browser/autofill/autofill_credit_card_model_mac.mm:30: return [NSString
stringWithFormat:@"%@%@ %@%@%@",
l10n issue.

http://codereview.chromium.org/558066/diff/1/14
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/1/14#newcode23
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:23: //
todo(dhollowa): temporary to disable until implementend.
per style guide, todo's are in all caps, eg: TODO(dhollowa): ...

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

http://codereview.chromium.org/558066/diff/1/18#newcode6
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_credit_card_model_mac.h"
move down with others.

http://codereview.chromium.org/558066/diff/1/18#newcode17
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:17: 
omit one of these blank lines

http://codereview.chromium.org/558066/diff/1/18#newcode37
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:37: //
todo(dhollowa): not yet implemented, disabling controls for now.
TODO

http://codereview.chromium.org/558066/diff/1/18#newcode68
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:68: -
(void)dealloc {
usually if there's nothing to do in dealloc, it's omitted.

http://codereview.chromium.org/558066/diff/1/18#newcode78
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:78: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard {
who creates creditCardModel_? Is it an object in the xib? You should probably
comment where this object comes from so it's clear in the code.

In fact, rather than having the controller marshall all of this data in and out
of the model, why not just have the controller create/destroy the model (ie, not
have it live in the nib) and init the model object with the appropriate
CreditCard object in its init?

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

http://codereview.chromium.org/558066/diff/1/22#newcode11
chrome/browser/autofill/autofill_dialog_controller_mac.h:11: #include
"chrome/browser/autofill/autofill_dialog.h"
do you need all of these includes or can you use "class" forward decls to pull
in what's needed?

http://codereview.chromium.org/558066/diff/1/22#newcode20
chrome/browser/autofill/autofill_dialog_controller_mac.h:20: // Modally presents
a dialog allowing the user to store personal
window modal or app modal?

http://codereview.chromium.org/558066/diff/1/22#newcode34
chrome/browser/autofill/autofill_dialog_controller_mac.h:34:
AutoFillAddressViewController* addressFormViewController_;
if these are strong refs, why not use scoped_nsobject?

http://codereview.chromium.org/558066/diff/1/22#newcode37
chrome/browser/autofill/autofill_dialog_controller_mac.h:37:
AutoFillDialogObserver* observer_; // (weak) not retained
2 spaces after semicolon for same-line comments.

http://codereview.chromium.org/558066/diff/1/22#newcode38
chrome/browser/autofill/autofill_dialog_controller_mac.h:38:
std::vector<AutoFillProfile> profiles_; // (strong) working copy
this isn't necessarily a strong reference, as it's a vector of objects, not a
vector of pointers.

http://codereview.chromium.org/558066/diff/1/22#newcode39
chrome/browser/autofill/autofill_dialog_controller_mac.h:39:
std::vector<CreditCard> creditCards_; // (strong) working copy
same as above re: strong.

http://codereview.chromium.org/558066/diff/1/22#newcode42
chrome/browser/autofill/autofill_dialog_controller_mac.h:42: @property
(nonatomic, retain) AutoFillAddressViewController*
do these need to be public? does someone outside the dialog controller need
access to these?

http://codereview.chromium.org/558066/diff/1/22#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac.h:47: // Main interface
for popping up a modal autofill dialog on screen.
app modal?

http://codereview.chromium.org/558066/diff/1/22#newcode54
chrome/browser/autofill/autofill_dialog_controller_mac.h:54: // Returns an
instance of AutoFillDialogController.  See |-initWithObserver|
why would you call one over the other? Do both need to be public?

http://codereview.chromium.org/558066/diff/1/22#newcode62
chrome/browser/autofill/autofill_dialog_controller_mac.h:62: // IBActions for
the dialog buttons
period at end of sentence

http://codereview.chromium.org/558066/diff/1/22#newcode71
chrome/browser/autofill/autofill_dialog_controller_mac.h:71:
autoFillProfiles:(const std::vector<AutoFillProfile*>&)profiles
line up colons if possible

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

http://codereview.chromium.org/558066/diff/1/13#newcode6
chrome/browser/autofill/autofill_dialog_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
mix these with includes below

http://codereview.chromium.org/558066/diff/1/13#newcode35
chrome/browser/autofill/autofill_dialog_controller_mac.mm:35:
controllerWithObserver:observer
line up colons

http://codereview.chromium.org/558066/diff/1/13#newcode50
chrome/browser/autofill/autofill_dialog_controller_mac.mm:50: [[self alloc]
initWithObserver:observer
indent 4

http://codereview.chromium.org/558066/diff/1/13#newcode52
chrome/browser/autofill/autofill_dialog_controller_mac.mm:52:
creditCards:creditCards];
line up colons

http://codereview.chromium.org/558066/diff/1/13#newcode123
chrome/browser/autofill/autofill_dialog_controller_mac.mm:123: [[self window]
orderOut:self];
the right way to do this is to call [window close] and then hold off on
autoreleasing until you get back the windowWillClose notification as the
window's delegate.

http://codereview.chromium.org/558066/diff/1/13#newcode148
chrome/browser/autofill/autofill_dialog_controller_mac.mm:148:
AutoFillAddressViewController *avc =
use scoped_nsobject

http://codereview.chromium.org/558066/diff/1/13#newcode155
chrome/browser/autofill/autofill_dialog_controller_mac.mm:155:
positioned:NSWindowBelow relativeTo:addressSection_];
align colons

http://codereview.chromium.org/558066/diff/1/13#newcode160
chrome/browser/autofill/autofill_dialog_controller_mac.mm:160:
AutoFillCreditCardViewController *ccvc =
scoped_nsobject

[dhollowa, 2010-02-02 00:38:59]

Thanks for the detailed review.  I think I've got all the formatting sorted out
now.  I also took the time to properly internationalize the "summary" strings. 
Had forgotten to flag this as TODO work in the last round.

http://codereview.chromium.org/558066/diff/1/23
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/1/23#newcode36
chrome/browser/autofill/autofill_address_model_mac.h:36: @property (readonly)
NSString* summary;
The reason I've made it a property is so that I can bind to it using KVO.  It's
value is dependent on the values of other properties so this the most direct way
to hook up to the KVO machinery.  Using a method would be a bit more involved I
think.


On 2010/02/01 15:34:31, pink wrote:
> what does this return? what's the format? I'd say rather than making this a
> property, just make it a normal method call.

http://codereview.chromium.org/558066/diff/1/21
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/21#newcode29
chrome/browser/autofill/autofill_address_model_mac.mm:29: - (NSString *)summary
{
On 2010/02/01 15:34:31, pink wrote:
> Chrome uses the style NSString*, not NSString *. Please make this change
> throughout.

Done.

http://codereview.chromium.org/558066/diff/1/21#newcode30
chrome/browser/autofill/autofill_address_model_mac.mm:30: 
On 2010/02/01 15:34:31, pink wrote:
> omit blank line after opening brace. Please make this change throughout.

Done.

http://codereview.chromium.org/558066/diff/1/21#newcode31
chrome/browser/autofill/autofill_address_model_mac.mm:31: BOOL haveFirstName =
firstName_ != nil && ![firstName_ isEqualToString:@""];
Yes, this was not my intent.  But is now moot following your other suggestions
for fixup.  Please see below.

On 2010/02/01 15:34:31, pink wrote:
> you don't have to test obj-c objects against nil before sending them a
message.
> The runtime will do that for you.

http://codereview.chromium.org/558066/diff/1/21#newcode40
chrome/browser/autofill/autofill_address_model_mac.mm:40: return [NSString
stringWithFormat:@"%@ %@%@ %@",
On 2010/02/01 15:34:31, pink wrote:
> this can't be localized. The format string needs to be in the grd file.

Done.

http://codereview.chromium.org/558066/diff/1/21#newcode57
chrome/browser/autofill/autofill_address_model_mac.mm:57: 
On 2010/02/01 15:34:31, pink wrote:
> omit blank line.

Done.

http://codereview.chromium.org/558066/diff/1/11
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/1/11#newcode23
chrome/browser/autofill/autofill_address_view_controller_mac.h:23: // Exposed
for unit tests.
Properties are not allowed in separate categories.  I tried what you suggest and
the compiler balked.

On 2010/02/01 15:34:31, pink wrote:
> You can do this in a separate category below so that it's not in the middle of
> other methods that are part of the public API. AFter the @end below, just do
> 
> @interface AutoFillAddressViewController(ForTestingOnly)
> @property (nonatomic, assign) AutoFillAddressModel* addressModel;
> @end

http://codereview.chromium.org/558066/diff/1/11#newcode31
chrome/browser/autofill/autofill_address_view_controller_mac.h:31: -
(void)copyModelToProfile:(AutoFillProfile&)profile;
On 2010/02/01 15:34:31, pink wrote:
> non-const references are not allowed by the google style guide. this needs to
be
> a  pointer.

Done.

http://codereview.chromium.org/558066/diff/1/15
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/15#newcode6
chrome/browser/autofill/autofill_address_view_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
On 2010/02/01 15:34:31, pink wrote:
> put this below, alphabetized with the others.

Done.

http://codereview.chromium.org/558066/diff/1/15#newcode17
chrome/browser/autofill/autofill_address_view_controller_mac.mm:17: 
On 2010/02/01 15:34:31, pink wrote:
> 2 blank lines is enough

Done.

http://codereview.chromium.org/558066/diff/1/15#newcode22
chrome/browser/autofill/autofill_address_view_controller_mac.mm:22: 
On 2010/02/01 15:34:31, pink wrote:
> omit blank line

Done.

http://codereview.chromium.org/558066/diff/1/15#newcode29
chrome/browser/autofill/autofill_address_view_controller_mac.mm:29: // pull the
view in now so it, and it's model, are available.
On 2010/02/01 15:34:31, pink wrote:
> s/it's/its. Capitalize (and punctuate) sentences in comments. Please fix
> throughout.

Done.

http://codereview.chromium.org/558066/diff/1/15#newcode90
chrome/browser/autofill/autofill_address_view_controller_mac.mm:90:
setLabel:SysUTF16ToNSString(profile.Label())];
On 2010/02/01 15:34:31, pink wrote:
> indent continuation lines 4 additional spaces. Fix throughout.

Done.

http://codereview.chromium.org/558066/diff/1/12
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/1/12#newcode26
chrome/browser/autofill/autofill_credit_card_model_mac.h:26: @property
(readonly) NSString* summary;
(Same reply.)  Would prefer to keep this as a property as it plays more nicely
with the KVO machinery I'm using.  Let me know.  Thanks.

On 2010/02/01 15:34:31, pink wrote:
> same comment on summary property from previous file.

http://codereview.chromium.org/558066/diff/1/24
File chrome/browser/autofill/autofill_credit_card_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/1/24#newcode18
chrome/browser/autofill/autofill_credit_card_model_mac.mm:18: - (NSString
*)summary {
On 2010/02/01 15:34:31, pink wrote:
> the same comments here from the other model class all apply as well.

Done.

http://codereview.chromium.org/558066/diff/1/24#newcode30
chrome/browser/autofill/autofill_credit_card_model_mac.mm:30: return [NSString
stringWithFormat:@"%@%@ %@%@%@",
On 2010/02/01 15:34:31, pink wrote:
> l10n issue.

Done.

http://codereview.chromium.org/558066/diff/1/14
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/1/14#newcode23
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:23: //
todo(dhollowa): temporary to disable until implementend.
On 2010/02/01 15:34:31, pink wrote:
> per style guide, todo's are in all caps, eg: TODO(dhollowa): ...

Done.

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

http://codereview.chromium.org/558066/diff/1/18#newcode6
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_credit_card_model_mac.h"
On 2010/02/01 15:34:31, pink wrote:
> move down with others.

Done.

http://codereview.chromium.org/558066/diff/1/18#newcode17
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:17: 
On 2010/02/01 15:34:31, pink wrote:
> omit one of these blank lines

Done.

http://codereview.chromium.org/558066/diff/1/18#newcode37
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:37: //
todo(dhollowa): not yet implemented, disabling controls for now.
On 2010/02/01 15:34:31, pink wrote:
> TODO

Done.

http://codereview.chromium.org/558066/diff/1/18#newcode68
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:68: -
(void)dealloc {
I've been leaving these in so for retain/release verification in the debugger. 
But I'll take them out.

On 2010/02/01 15:34:31, pink wrote:
> usually if there's nothing to do in dealloc, it's omitted.

http://codereview.chromium.org/558066/diff/1/18#newcode78
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:78: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard {
The nib creates the creditCardModel_.  I would prefer to keep it that way as it
eases the wiring up of the KVO/KVC stuff.  But I agree with you about the
marshaling.  I have moved the to/from data transfer over to the model object and
simply call through to the appropriate method from the controller.  Much
cleaner.  My original motivation was "smart controller, dumb model" mantra.  But
that doesn't make so much sense for pure data transfer.  Better to have that in
the model as you've pointed out.

I've also done this for the address model.  And I've commented the controllers'
outlets to the model objects to be clear that they are created from the nib.

On 2010/02/01 15:34:31, pink wrote:
> who creates creditCardModel_? Is it an object in the xib? You should probably
> comment where this object comes from so it's clear in the code.
> 
> In fact, rather than having the controller marshall all of this data in and
out
> of the model, why not just have the controller create/destroy the model (ie,
not
> have it live in the nib) and init the model object with the appropriate
> CreditCard object in its init?

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

http://codereview.chromium.org/558066/diff/1/22#newcode11
chrome/browser/autofill/autofill_dialog_controller_mac.h:11: #include
"chrome/browser/autofill/autofill_dialog.h"

I need the includes because I'm using lists of objects (not object pointers) in
my class.

On 2010/02/01 15:34:31, pink wrote:
> do you need all of these includes or can you use "class" forward decls to pull
> in what's needed?

http://codereview.chromium.org/558066/diff/1/22#newcode20
chrome/browser/autofill/autofill_dialog_controller_mac.h:20: // Modally presents
a dialog allowing the user to store personal
App modal.  Comment is fixed.
On 2010/02/01 15:34:31, pink wrote:
> window modal or app modal?

http://codereview.chromium.org/558066/diff/1/22#newcode34
chrome/browser/autofill/autofill_dialog_controller_mac.h:34:
AutoFillAddressViewController* addressFormViewController_;
On 2010/02/01 15:34:31, pink wrote:
> if these are strong refs, why not use scoped_nsobject?

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode37
chrome/browser/autofill/autofill_dialog_controller_mac.h:37:
AutoFillDialogObserver* observer_; // (weak) not retained
On 2010/02/01 15:34:31, pink wrote:
> 2 spaces after semicolon for same-line comments.

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode38
chrome/browser/autofill/autofill_dialog_controller_mac.h:38:
std::vector<AutoFillProfile> profiles_; // (strong) working copy
On 2010/02/01 15:34:31, pink wrote:
> this isn't necessarily a strong reference, as it's a vector of objects, not a
> vector of pointers.

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode38
chrome/browser/autofill/autofill_dialog_controller_mac.h:38:
std::vector<AutoFillProfile> profiles_; // (strong) working copy
Right.  Removed comment.

On 2010/02/01 15:34:31, pink wrote:
> this isn't necessarily a strong reference, as it's a vector of objects, not a
> vector of pointers.

http://codereview.chromium.org/558066/diff/1/22#newcode39
chrome/browser/autofill/autofill_dialog_controller_mac.h:39:
std::vector<CreditCard> creditCards_; // (strong) working copy
On 2010/02/01 15:34:31, pink wrote:
> same as above re: strong.

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode39
chrome/browser/autofill/autofill_dialog_controller_mac.h:39:
std::vector<CreditCard> creditCards_; // (strong) working copy
On 2010/02/01 15:34:31, pink wrote:
> same as above re: strong.

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode42
chrome/browser/autofill/autofill_dialog_controller_mac.h:42: @property
(nonatomic, retain) AutoFillAddressViewController*
I exposed these for unit testing purposes.  Due to the restrictions on
properties in separate categories I exposed them in the main class.  I have
changed this now though: I removed the properties and created accessor methods
in the ExposedForUnitTests category.  I'm using the scoped_nsobject members now
internally.

On 2010/02/01 15:34:31, pink wrote:
> do these need to be public? does someone outside the dialog controller need
> access to these?

http://codereview.chromium.org/558066/diff/1/22#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac.h:47: // Main interface
for popping up a modal autofill dialog on screen.
Yes.  App modal.  Comment fixed.
On 2010/02/01 15:34:31, pink wrote:
> app modal?

http://codereview.chromium.org/558066/diff/1/22#newcode54
chrome/browser/autofill/autofill_dialog_controller_mac.h:54: // Returns an
instance of AutoFillDialogController.  See |-initWithObserver|
I created the controllerWithObserver method for clarity in unit testing, but
forgot to move it into the ExposedForUnitTests category.  This is done now.

On 2010/02/01 15:34:31, pink wrote:
> why would you call one over the other? Do both need to be public?

http://codereview.chromium.org/558066/diff/1/22#newcode62
chrome/browser/autofill/autofill_dialog_controller_mac.h:62: // IBActions for
the dialog buttons
On 2010/02/01 15:34:31, pink wrote:
> period at end of sentence

Done.

http://codereview.chromium.org/558066/diff/1/22#newcode71
chrome/browser/autofill/autofill_dialog_controller_mac.h:71:
autoFillProfiles:(const std::vector<AutoFillProfile*>&)profiles
On 2010/02/01 15:34:31, pink wrote:
> line up colons if possible

Done.

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

http://codereview.chromium.org/558066/diff/1/13#newcode6
chrome/browser/autofill/autofill_dialog_controller_mac.mm:6: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
On 2010/02/01 15:34:31, pink wrote:
> mix these with includes below

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode35
chrome/browser/autofill/autofill_dialog_controller_mac.mm:35:
controllerWithObserver:observer
On 2010/02/01 15:34:31, pink wrote:
> line up colons

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode50
chrome/browser/autofill/autofill_dialog_controller_mac.mm:50: [[self alloc]
initWithObserver:observer
On 2010/02/01 15:34:31, pink wrote:
> indent 4

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode52
chrome/browser/autofill/autofill_dialog_controller_mac.mm:52:
creditCards:creditCards];
On 2010/02/01 15:34:31, pink wrote:
> line up colons

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode123
chrome/browser/autofill/autofill_dialog_controller_mac.mm:123: [[self window]
orderOut:self];
Done.  This was copy/paste bug.  The source was ClearBrowsingDataController. 
Shall I fix this there too?

On 2010/02/01 15:34:31, pink wrote:
> the right way to do this is to call [window close] and then hold off on
> autoreleasing until you get back the windowWillClose notification as the
> window's delegate.

http://codereview.chromium.org/558066/diff/1/13#newcode148
chrome/browser/autofill/autofill_dialog_controller_mac.mm:148:
AutoFillAddressViewController *avc =
On 2010/02/01 15:34:31, pink wrote:
> use scoped_nsobject

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode155
chrome/browser/autofill/autofill_dialog_controller_mac.mm:155:
positioned:NSWindowBelow relativeTo:addressSection_];
On 2010/02/01 15:34:31, pink wrote:
> align colons

Done.

http://codereview.chromium.org/558066/diff/1/13#newcode160
chrome/browser/autofill/autofill_dialog_controller_mac.mm:160:
AutoFillCreditCardViewController *ccvc =
On 2010/02/01 15:34:31, pink wrote:
> scoped_nsobject

Done.

[pink (ping after 24hrs), 2010-02-02 15:24:19]

Comments on a few comments:

> http://codereview.chromium.org/558066/diff/1/11#newcode23
> chrome/browser/autofill/autofill_address_view_controller_mac.h:23: //
> Exposed for unit tests.
> Properties are not allowed in separate categories.  I tried what you
> suggest and the compiler balked.

The category has to be un-named. That should work. Someone just made
that change for me in a readability review, which is why we know it
only works in certain situations. You just have to comment that it's
private.

> http://codereview.chromium.org/558066/diff/1/18#newcode78
> chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:78:
> - (void)copyCreditCardToModel:(const CreditCard&)creditCard {
> The nib creates the creditCardModel_.  I would prefer to keep it that
> way as it eases the wiring up of the KVO/KVC stuff.

Have you double-checked that the model is being dealloc'd properly?
The nib holds an extra strong reference to the model object in this
case, which means that it probably is not ever going away.

> http://codereview.chromium.org/558066/diff/1/13#newcode123
> chrome/browser/autofill/autofill_dialog_controller_mac.mm:123: [[self
> window] orderOut:self];
> Done.  This was copy/paste bug.  The source was
> ClearBrowsingDataController.  Shall I fix this there too?

Probably.

I'll take another look at the CL later today from the start. Thanks
for making these changes!

-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[dhollowa, 2010-02-02 19:10:07]

On 2010/02/02 15:24:19, pink wrote:
> Comments on a few comments:
> 
> > http://codereview.chromium.org/558066/diff/1/11#newcode23
> > chrome/browser/autofill/autofill_address_view_controller_mac.h:23: //
> > Exposed for unit tests.
> > Properties are not allowed in separate categories.  I tried what you
> > suggest and the compiler balked.
> 
> The category has to be un-named. That should work. Someone just made
> that change for me in a readability review, which is why we know it
> only works in certain situations. You just have to comment that it's
> private.
> 

Done.  It worked as you said.  If I used an unnamed category and kept the
property synthesize within the main implementation.  Creating a separate unnamed
category implementation for the synthesize didn't work.

Also fixed this for the credit card view controller.


> > http://codereview.chromium.org/558066/diff/1/18#newcode78
> > chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:78:
> > - (void)copyCreditCardToModel:(const CreditCard&)creditCard {
> > The nib creates the creditCardModel_.  I would prefer to keep it that
> > way as it eases the wiring up of the KVO/KVC stuff.
> 
> Have you double-checked that the model is being dealloc'd properly?
> The nib holds an extra strong reference to the model object in this
> case, which means that it probably is not ever going away.
> 

Yes, I double checked.  The NSBinder/NSBindingInfo owns the model object and it
deallocates it when the controller autoreleases upon window close.

> > http://codereview.chromium.org/558066/diff/1/13#newcode123
> > chrome/browser/autofill/autofill_dialog_controller_mac.mm:123: [[self
> > window] orderOut:self];
> > Done.  This was copy/paste bug.  The source was
> > ClearBrowsingDataController.  Shall I fix this there too?
> 
> Probably.
> 
> I'll take another look at the CL later today from the start. Thanks
> for making these changes!
> 
> -- 
> Mike Pinkerton
> Mac Weenie
> mailto:pinkerton@google.com
>

[dhollowa, 2010-02-02 19:11:26]

On 2010/02/02 19:10:07, dhollowa wrote:
> On 2010/02/02 15:24:19, pink wrote:
> > Comments on a few comments:
> > 
> > > http://codereview.chromium.org/558066/diff/1/11#newcode23
> > > chrome/browser/autofill/autofill_address_view_controller_mac.h:23: //
> > > Exposed for unit tests.
> > > Properties are not allowed in separate categories.  I tried what you
> > > suggest and the compiler balked.
> > 
> > The category has to be un-named. That should work. Someone just made
> > that change for me in a readability review, which is why we know it
> > only works in certain situations. You just have to comment that it's
> > private.
> > 
> 
> Done.  It worked as you said.  If I used an unnamed category and kept the
> property synthesize within the main implementation.  Creating a separate
unnamed
> category implementation for the synthesize didn't work.
> 
> Also fixed this for the credit card view controller.
> 
> 
> > > http://codereview.chromium.org/558066/diff/1/18#newcode78
> > > chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:78:
> > > - (void)copyCreditCardToModel:(const CreditCard&)creditCard {
> > > The nib creates the creditCardModel_.  I would prefer to keep it that
> > > way as it eases the wiring up of the KVO/KVC stuff.
> > 
> > Have you double-checked that the model is being dealloc'd properly?
> > The nib holds an extra strong reference to the model object in this
> > case, which means that it probably is not ever going away.
> > 
> 
> Yes, I double checked.  The NSBinder/NSBindingInfo owns the model object and
it
> deallocates it when the controller autoreleases upon window close.
> 
> > > http://codereview.chromium.org/558066/diff/1/13#newcode123
> > > chrome/browser/autofill/autofill_dialog_controller_mac.mm:123: [[self
> > > window] orderOut:self];
> > > Done.  This was copy/paste bug.  The source was
> > > ClearBrowsingDataController.  Shall I fix this there too?
> > 
> > Probably.
> > 

Let me know.  I'll create a different CL for that though to keep things
separate.

> > I'll take another look at the CL later today from the start. Thanks
> > for making these changes!
> > 
> > -- 
> > Mike Pinkerton
> > Mac Weenie
> > mailto:pinkerton@google.com
> >

[dhollowa, 2010-02-02 19:11:43]

http://codereview.chromium.org/558066/diff/1/11
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/1/11#newcode23
chrome/browser/autofill/autofill_address_view_controller_mac.h:23: // Exposed
for unit tests.
Done.  Created an unnamed category.  Also noted that the property has to be
synthesized in the main implementation and not a separate implementation for the
unnamed category.

Did same fix for autofill_creditcard_view_controller_mac.h

On 2010/02/02 00:38:59, dhollowa wrote:
> Properties are not allowed in separate categories.  I tried what you suggest
and
> the compiler balked.
> 
> On 2010/02/01 15:34:31, pink wrote:
> > You can do this in a separate category below so that it's not in the middle
of
> > other methods that are part of the public API. AFter the @end below, just do
> > 
> > @interface AutoFillAddressViewController(ForTestingOnly)
> > @property (nonatomic, assign) AutoFillAddressModel* addressModel;
> > @end
> 
>

[pink (ping after 24hrs), 2010-02-02 22:14:22]

Much easier to read now! Thanks for that first pass. More comments below.

http://codereview.chromium.org/558066/diff/15036/15058
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15058#newcode18
chrome/browser/autofill/autofill_address_model_mac.h:18: NSString* label_;
you might want to mention that these are not scoped_nsobjects because we intend
them to be used via KVO/bindings.

http://codereview.chromium.org/558066/diff/15036/15058#newcode61
chrome/browser/autofill/autofill_address_model_mac.h:61: -
(void)copyModelToProfile:(AutoFillProfile*)profile;
If these are part of the public api, they need a method comment.

http://codereview.chromium.org/558066/diff/15036/15058#newcode62
chrome/browser/autofill/autofill_address_model_mac.h:62: -
(void)copyProfileToModel:(const AutoFillProfile&)profile;
You should probably explain somewhere what state the object is in if you don't
call this, given that it's possible to use the object "uninitialized" with no
profile.

http://codereview.chromium.org/558066/diff/15036/15056
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15056#newcode7
chrome/browser/autofill/autofill_address_model_mac.mm:7: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
it's ok if this one goes first, just the others have to be alphabetized after
it. Or you can mix it in with the others.

http://codereview.chromium.org/558066/diff/15036/15056#newcode33
chrome/browser/autofill/autofill_address_model_mac.mm:33: - (NSString*)summary {
i'd move this down below dealloc. Usually put the class methods first, then
init/dealloc, then everything else.

http://codereview.chromium.org/558066/diff/15036/15056#newcode96
chrome/browser/autofill/autofill_address_model_mac.mm:96: -
(void)copyModelToProfile:(AutoFillProfile*)profile {
DCHECK(profile)? should you return early if it is NULL?

http://codereview.chromium.org/558066/diff/15036/15046
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15046#newcode29
chrome/browser/autofill/autofill_address_view_controller_mac.h:29: // Copy data
from internal model to |profile|.
Can |profile| be NULL?

http://codereview.chromium.org/558066/diff/15036/15050
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15050#newcode40
chrome/browser/autofill/autofill_address_view_controller_mac.mm:40: -
(void)copyProfileToModel:(const AutoFillProfile&)profile {
is this really necessary to abstract this for a 1-line function? The file would
be about 50% smaller if you didn't (thus easier to read).

http://codereview.chromium.org/558066/diff/15036/15047
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15047#newcode41
chrome/browser/autofill/autofill_credit_card_model_mac.h:41: -
(void)copyModelToCreditCard:(CreditCard*)creditCard;
need comments as part of public api

http://codereview.chromium.org/558066/diff/15036/15059
File chrome/browser/autofill/autofill_credit_card_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15059#newcode25
chrome/browser/autofill/autofill_credit_card_model_mac.mm:25: -
(NSString*)summary {
move down below dealloc

http://codereview.chromium.org/558066/diff/15036/15059#newcode33
chrome/browser/autofill/autofill_credit_card_model_mac.mm:33: BOOL haveExpYear =
[expirationYear_ length] > 0;
do we need to do any validation on this string to ensure it's numeric/etc? Does
the CreditCard object already do all that for us?

http://codereview.chromium.org/558066/diff/15036/15059#newcode80
chrome/browser/autofill/autofill_credit_card_model_mac.mm:80: -
(void)copyModelToCreditCard:(CreditCard*)creditCard {
DCHECK(creditCard) and probably return early if NULL

http://codereview.chromium.org/558066/diff/15036/15049
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15049#newcode24
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:24: //
TODO(dhollowa): temporary to disable until implementend.
is there a bug to do this, and if so, can you note it here?

http://codereview.chromium.org/558066/diff/15036/15053
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/15036/15053#newcode12
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:12: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard;
again, i'd just get rid of this altogether.

http://codereview.chromium.org/558066/diff/15036/15053#newcode30
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:30: //
TODO(dhollowa): not yet implemented, disabling controls for now.
bug?

http://codereview.chromium.org/558066/diff/15036/15057
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15057#newcode44
chrome/browser/autofill/autofill_dialog_controller_mac.h:44: // Main interface
for popping up a application modal autofill dialog on screen.
an application modal

http://codereview.chromium.org/558066/diff/15036/15057#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac.h:47: +
(void)showAutoFillDialogWithObserver:(AutoFillDialogObserver*)observer
can observer be NULL?

http://codereview.chromium.org/558066/diff/15036/15057#newcode57
chrome/browser/autofill/autofill_dialog_controller_mac.h:57: // Inteface exposed
for unit testing.
Interface

http://codereview.chromium.org/558066/diff/15036/15048
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15048#newcode30
chrome/browser/autofill/autofill_dialog_controller_mac.mm:30: if (![controller
isWindowLoaded]) {
i don't follow what this is doing. If the nib hasn't been loaded, run it as a
modal? Why don't you always want this?

http://codereview.chromium.org/558066/diff/15036/15048#newcode78
chrome/browser/autofill/autofill_dialog_controller_mac.mm:78: // Use
initWithWindowNibPath:: instead of initWithWindowNibName: so we
s/::/:

http://codereview.chromium.org/558066/diff/15036/15048#newcode84
chrome/browser/autofill/autofill_dialog_controller_mac.mm:84: // Make local
reference of |observer|.
I think this is self-explanatory. Omit the comment.

http://codereview.chromium.org/558066/diff/15036/15048#newcode89
chrome/browser/autofill/autofill_dialog_controller_mac.mm:89: profiles_.clear();
this is an init. is the clear really necessary?

http://codereview.chromium.org/558066/diff/15036/15048#newcode90
chrome/browser/autofill/autofill_dialog_controller_mac.mm:90: for
(i=profiles.begin(); i != profiles.end(); i++)
spaces around = and use pre-increment for iterators, per google style guide.

http://codereview.chromium.org/558066/diff/15036/15048#newcode96
chrome/browser/autofill/autofill_dialog_controller_mac.mm:96: for
(j=creditCards.begin(); j != creditCards.end(); j++)
same as above

http://codereview.chromium.org/558066/diff/15036/15048#newcode104
chrome/browser/autofill/autofill_dialog_controller_mac.mm:104: [self
autorelease];
don't autorelease here, autorelease in the windowWillClose delegate method.

http://codereview.chromium.org/558066/diff/15036/15048#newcode137
chrome/browser/autofill/autofill_dialog_controller_mac.mm:137: // Embed the
child view into our (owned by us) target view.
This confuses me greatly. The "child view" you're embedding is not |childView_|
correct? What's the "target view"? Is that |childView|? If so, this object does
not own |childView_|, the view hierarchy does. We only have a weak reference to
it.

http://codereview.chromium.org/558066/diff/15036/15048#newcode149
chrome/browser/autofill/autofill_dialog_controller_mac.mm:149: // Embed the
child view into our (owned by us) target view.
Same as above.

http://codereview.chromium.org/558066/diff/15036/15054
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/558066/diff/15036/15054#newcode23
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:23:
std::vector<CreditCard>* creditCards) {
in C++, use C++ naming, which is underscores (credit_cards).

http://codereview.chromium.org/558066/diff/15036/15054#newcode28
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:28: for
(i=profiles->begin(); i != profiles->end(); i++)
space around ='s, pre-increment on iterator.

http://codereview.chromium.org/558066/diff/15036/15054#newcode33
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:33: for
(j=creditCards->begin(); j != creditCards->end(); j++)
same as above

http://codereview.chromium.org/558066/diff/15036/15054#newcode42
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:42: virtual
~AutoFillDialogObserverTester() {}
why declare an additional public section for the dtor? Just put it up next to
the ctor.

http://codereview.chromium.org/558066/diff/15036/15054#newcode48
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:48: {}
put on the same line as the decl

http://codereview.chromium.org/558066/diff/15036/15054#newcode50
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:50: virtual
void SetUp() {
is there any need for these if they just call the parent?

http://codereview.chromium.org/558066/diff/15036/15054#newcode61
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:61:
autoFillProfiles:profiles_
align colons

http://codereview.chromium.org/558066/diff/15036/15054#newcode68
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:68:
AutoFillDialogController* controller_;
weak ref

http://codereview.chromium.org/558066/diff/15036/15054#newcode143
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:143:
AutoFillType(EMAIL_ADDRESS),
why the break after the open paren?

http://codereview.chromium.org/558066/diff/15036/15054#newcode150
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:150:
AutoFillType(ADDRESS_HOME_CITY),
same here

http://codereview.chromium.org/558066/diff/15036/15051
File chrome/browser/autofill/autofill_dialog_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15051#newcode8
chrome/browser/autofill/autofill_dialog_mac.mm:8: void
ShowAutoFillDialog(AutoFillDialogObserver* observer,
needs a comment

http://codereview.chromium.org/558066/diff/15036/15066
File chrome/browser/cocoa/disclosure_view_controller.h (right):

http://codereview.chromium.org/558066/diff/15036/15066#newcode15
chrome/browser/cocoa/disclosure_view_controller.h:15: // of the view.
|disclosureState| is initialized as "on".
"on", I assume, means "open" or "expanded"?

http://codereview.chromium.org/558066/diff/15036/15062
File chrome/browser/cocoa/disclosure_view_controller.mm (right):

http://codereview.chromium.org/558066/diff/15036/15062#newcode37
chrome/browser/cocoa/disclosure_view_controller.mm:37: // Set up initial model
state before we trigger listener, we don't want
grammar seems odd here.

http://codereview.chromium.org/558066/diff/15036/15062#newcode61
chrome/browser/cocoa/disclosure_view_controller.mm:61: -
(void)initDisclosureState:(NSCellStateValue)state {
needs a method-level comment, as do all methods from here down.

http://codereview.chromium.org/558066/diff/15036/15062#newcode76
chrome/browser/cocoa/disclosure_view_controller.mm:76: startFrame.origin.y,
this should probably be indented 4 spaces more, it's a continuation of the
previous expression, not the next parameter in the NSMakeRect call. Also, the
last param should be on a new line as well.

http://codereview.chromium.org/558066/diff/15036/15062#newcode83
chrome/browser/cocoa/disclosure_view_controller.mm:83: startFrame.origin.y,
same here

http://codereview.chromium.org/558066/diff/15036/15062#newcode92
chrome/browser/cocoa/disclosure_view_controller.mm:92: NSViewAnimation*
animation;
declare this where it is initialized

http://codereview.chromium.org/558066/diff/15036/15062#newcode96
chrome/browser/cocoa/disclosure_view_controller.mm:96: {
why scope this?

http://codereview.chromium.org/558066/diff/15036/15062#newcode98
chrome/browser/cocoa/disclosure_view_controller.mm:98: dictionary =
[NSMutableDictionary dictionaryWithCapacity:2];
just use NSDictionary dictionaryWithObjectsAndKeys:...

http://codereview.chromium.org/558066/diff/15036/15062#newcode113
chrome/browser/cocoa/disclosure_view_controller.mm:113: animation =
[[NSViewAnimation alloc] initWithViewAnimations:
scoped_nsobject

http://codereview.chromium.org/558066/diff/15036/15062#newcode114
chrome/browser/cocoa/disclosure_view_controller.mm:114: [NSArray
arrayWithObjects:dictionary, nil]];
use NSArray arrayWithObject: since there is only 1

http://codereview.chromium.org/558066/diff/15036/15062#newcode163
chrome/browser/cocoa/disclosure_view_controller.mm:163: @interface
DisclosureViewState : NSObject {
Needs a class comment. What is this for? How is it used? Why is it here?

http://codereview.chromium.org/558066/diff/15036/15062#newcode164
chrome/browser/cocoa/disclosure_view_controller.mm:164: @private
1-space indent

http://codereview.chromium.org/558066/diff/15036/15065
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/558066/diff/15036/15065#newcode1158
chrome/browser/cocoa/preferences_window_controller.mm:1158: // todo(dhollowa):
need "n" of these.  create single entry for now.
TODO

http://codereview.chromium.org/558066/diff/15036/15065#newcode1163
chrome/browser/cocoa/preferences_window_controller.mm:1163: // todo(dhollowa):
need "n" of these.  create single entry for now.
TODO

http://codereview.chromium.org/558066/diff/15036/15064
File chrome/browser/cocoa/section_separator_view.h (right):

http://codereview.chromium.org/558066/diff/15036/15064#newcode1
chrome/browser/cocoa/section_separator_view.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
2010?

http://codereview.chromium.org/558066/diff/15036/15064#newcode30
chrome/browser/cocoa/section_separator_view.h:30: // Designated initializer.
this is inherited as a NSView, no need to re-declare.

http://codereview.chromium.org/558066/diff/15036/15063
File chrome/browser/cocoa/section_separator_view.mm (right):

http://codereview.chromium.org/558066/diff/15036/15063#newcode1
chrome/browser/cocoa/section_separator_view.mm:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
2010?

http://codereview.chromium.org/558066/diff/15036/15063#newcode23
chrome/browser/cocoa/section_separator_view.mm:23: [self setShowBaseLine:TRUE];
TRUE is not objective-C. YES is.

http://codereview.chromium.org/558066/diff/15036/15063#newcode25
chrome/browser/cocoa/section_separator_view.mm:25: [self setShowTopLine:TRUE];
same here

http://codereview.chromium.org/558066/diff/15036/15063#newcode58
chrome/browser/cocoa/section_separator_view.mm:58: -
(void)drawGradientRect:(NSRect)rect {
Needs a method-level comment

http://codereview.chromium.org/558066/diff/15036/15063#newcode71
chrome/browser/cocoa/section_separator_view.mm:71: size_t stopCount = 2;
const?

http://codereview.chromium.org/558066/diff/15036/15063#newcode72
chrome/browser/cocoa/section_separator_view.mm:72: CGFloat stops[2] = { 0.0, 1.0
};
use stopCount here?

http://codereview.chromium.org/558066/diff/15036/15061
File chrome/browser/cocoa/vertical_layout_view.h (right):

http://codereview.chromium.org/558066/diff/15036/15061#newcode20
chrome/browser/cocoa/vertical_layout_view.h:20: // Designated initializer.
you can omit this

http://codereview.chromium.org/558066/diff/15036/15060
File chrome/browser/cocoa/vertical_layout_view.mm (right):

http://codereview.chromium.org/558066/diff/15036/15060#newcode14
chrome/browser/cocoa/vertical_layout_view.mm:14: - (BOOL)isFlipped {
move below initWithFrame

http://codereview.chromium.org/558066/diff/15036/15060#newcode48
chrome/browser/cocoa/vertical_layout_view.mm:48: [[NSNotificationCenter
defaultCenter]
just as a precaution, you might want to make sure you remove self from all
notifications in dealloc.

http://codereview.chromium.org/558066/diff/15036/15060#newcode58
chrome/browser/cocoa/vertical_layout_view.mm:58: -
(void)onChildFrameChanged:(NSNotification*)notification {
these all need comments

http://codereview.chromium.org/558066/diff/15036/15060#newcode66
chrome/browser/cocoa/vertical_layout_view.mm:66: int i;
declare |i| in for loop where it's used.

http://codereview.chromium.org/558066/diff/15036/15060#newcode68
chrome/browser/cocoa/vertical_layout_view.mm:68: for(i = childCount-1, yPosition
= 0.0f; i >= 0; i--) {
space after |for|

http://codereview.chromium.org/558066/diff/15036/15060#newcode81
chrome/browser/cocoa/vertical_layout_view.mm:81: @end
blank line before @end

[dhollowa, 2010-02-03 19:20:35]

The credit card summary code was changed in the trunk.  So I've put a TODO there
for this CL.  I'll be integrating the new summary code in a future CL.

Note also, the vertical_layout_view.mm implementation has been simplified
considerably.  I noticed a problem with the old implementation and was able to
find a simpler and more robust approach.  It no longer installs view frame
change notification listeners and now relies on the |-viewWillDraw| override to
handle the layout of the children.

http://codereview.chromium.org/558066/diff/15036/15058
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15058#newcode18
chrome/browser/autofill/autofill_address_model_mac.h:18: NSString* label_;
On 2010/02/02 22:14:22, pink wrote:
> you might want to mention that these are not scoped_nsobjects because we
intend
> them to be used via KVO/bindings.

Done.
Added same comment to AutoFillCreditCardModel.

http://codereview.chromium.org/558066/diff/15036/15058#newcode61
chrome/browser/autofill/autofill_address_model_mac.h:61: -
(void)copyModelToProfile:(AutoFillProfile*)profile;
On 2010/02/02 22:14:22, pink wrote:
> If these are part of the public api, they need a method comment.

Done.
Added similar comments to AutoFillCreditCardModel.

http://codereview.chromium.org/558066/diff/15036/15058#newcode62
chrome/browser/autofill/autofill_address_model_mac.h:62: -
(void)copyProfileToModel:(const AutoFillProfile&)profile;
On 2010/02/02 22:14:22, pink wrote:
> You should probably explain somewhere what state the object is in if you don't
> call this, given that it's possible to use the object "uninitialized" with no
> profile.

Done.
Added similar comments to AutoFillCreditCardModel.

http://codereview.chromium.org/558066/diff/15036/15056
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15056#newcode7
chrome/browser/autofill/autofill_address_model_mac.mm:7: #import
"chrome/browser/autofill/autofill_address_model_mac.h"
On 2010/02/02 22:14:22, pink wrote:
> it's ok if this one goes first, just the others have to be alphabetized after
> it. Or you can mix it in with the others.

I will change these throughout.  I read the reasoning here and having a rule
that implicitly debugs dependent headers seems very worthwhile.

http://codereview.chromium.org/558066/diff/15036/15056#newcode33
chrome/browser/autofill/autofill_address_model_mac.mm:33: - (NSString*)summary {
On 2010/02/02 22:14:22, pink wrote:
> i'd move this down below dealloc. Usually put the class methods first, then
> init/dealloc, then everything else.

Done.
Will remember this for the future.

http://codereview.chromium.org/558066/diff/15036/15056#newcode96
chrome/browser/autofill/autofill_address_model_mac.mm:96: -
(void)copyModelToProfile:(AutoFillProfile*)profile {
On 2010/02/02 22:14:22, pink wrote:
> DCHECK(profile)? should you return early if it is NULL?

Done.
Added DCHECKs to this and AutoFillCreditCardModel as well.

http://codereview.chromium.org/558066/diff/15036/15046
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15046#newcode29
chrome/browser/autofill/autofill_address_view_controller_mac.h:29: // Copy data
from internal model to |profile|.
On 2010/02/02 22:14:22, pink wrote:
> Can |profile| be NULL?

Done.
Added comment banning NULL input, and DCHECK in implementation.  Also done for
AutoFillCreditCardModel.

http://codereview.chromium.org/558066/diff/15036/15050
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15050#newcode40
chrome/browser/autofill/autofill_address_view_controller_mac.mm:40: -
(void)copyProfileToModel:(const AutoFillProfile&)profile {
On 2010/02/02 22:14:22, pink wrote:
> is this really necessary to abstract this for a 1-line function? The file
would
> be about 50% smaller if you didn't (thus easier to read).

Done.

http://codereview.chromium.org/558066/diff/15036/15047
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15047#newcode41
chrome/browser/autofill/autofill_credit_card_model_mac.h:41: -
(void)copyModelToCreditCard:(CreditCard*)creditCard;
On 2010/02/02 22:14:22, pink wrote:
> need comments as part of public api

Done.

http://codereview.chromium.org/558066/diff/15036/15059
File chrome/browser/autofill/autofill_credit_card_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15059#newcode25
chrome/browser/autofill/autofill_credit_card_model_mac.mm:25: -
(NSString*)summary {
On 2010/02/02 22:14:22, pink wrote:
> move down below dealloc

Done.

http://codereview.chromium.org/558066/diff/15036/15059#newcode33
chrome/browser/autofill/autofill_credit_card_model_mac.mm:33: BOOL haveExpYear =
[expirationYear_ length] > 0;
On 2010/02/02 22:14:22, pink wrote:
> do we need to do any validation on this string to ensure it's numeric/etc?
Does
> the CreditCard object already do all that for us?

I spoke at length with James about this.  I had the same question.  His guidance
was that no validation is needed at the UI level.  The fields can be quite free
form.  Validation would happen by the checks in the target webpage itself.  He
does do some small massaging of phone number data to strip non-numerics as it
flows into the back end.  This could be a little weird for the user.  But I have
it on my TODO list to review this with the UX folks.

http://codereview.chromium.org/558066/diff/15036/15059#newcode80
chrome/browser/autofill/autofill_credit_card_model_mac.mm:80: -
(void)copyModelToCreditCard:(CreditCard*)creditCard {
On 2010/02/02 22:14:22, pink wrote:
> DCHECK(creditCard) and probably return early if NULL

Done.

http://codereview.chromium.org/558066/diff/15036/15049
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15049#newcode24
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:24: //
TODO(dhollowa): temporary to disable until implementend.
On 2010/02/02 22:14:22, pink wrote:
> is there a bug to do this, and if so, can you note it here?

Yes.  Bug 33029.  It tracks the Mac UI for AutoFill.  I'll update that bug when
all the outstanding TODOs are complete and I've got sign-off from UX.  Good?

http://codereview.chromium.org/558066/diff/15036/15053
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/15036/15053#newcode12
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:12: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard;
On 2010/02/02 22:14:22, pink wrote:
> again, i'd just get rid of this altogether.

Done.

http://codereview.chromium.org/558066/diff/15036/15053#newcode30
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:30: //
TODO(dhollowa): not yet implemented, disabling controls for now.
On 2010/02/02 22:14:22, pink wrote:
> bug?
Yes.  558066.

http://codereview.chromium.org/558066/diff/15036/15057
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/15036/15057#newcode44
chrome/browser/autofill/autofill_dialog_controller_mac.h:44: // Main interface
for popping up a application modal autofill dialog on screen.
On 2010/02/02 22:14:22, pink wrote:
> an application modal

Wow!  Good eyes!  :-)

http://codereview.chromium.org/558066/diff/15036/15057#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac.h:47: +
(void)showAutoFillDialogWithObserver:(AutoFillDialogObserver*)observer
On 2010/02/02 22:14:22, pink wrote:
> can observer be NULL?

Yes.  I have elaborated in the method's comments.  Also added comments about
when the observer's callback is invoked.

http://codereview.chromium.org/558066/diff/15036/15057#newcode57
chrome/browser/autofill/autofill_dialog_controller_mac.h:57: // Inteface exposed
for unit testing.
On 2010/02/02 22:14:22, pink wrote:
> Interface

Done.

http://codereview.chromium.org/558066/diff/15036/15048
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15048#newcode30
chrome/browser/autofill/autofill_dialog_controller_mac.mm:30: if (![controller
isWindowLoaded]) {
On 2010/02/02 22:14:22, pink wrote:
> i don't follow what this is doing. If the nib hasn't been loaded, run it as a
> modal? Why don't you always want this?

My read of this is that it prevents a second autofill dialog to be created. 
This was copy/paste from showClearBrowsingDialogForProfile.  Does this not
achieve the desired result?

http://codereview.chromium.org/558066/diff/15036/15048#newcode78
chrome/browser/autofill/autofill_dialog_controller_mac.mm:78: // Use
initWithWindowNibPath:: instead of initWithWindowNibName: so we
On 2010/02/02 22:14:22, pink wrote:
> s/::/:

Done.

http://codereview.chromium.org/558066/diff/15036/15048#newcode84
chrome/browser/autofill/autofill_dialog_controller_mac.mm:84: // Make local
reference of |observer|.
On 2010/02/02 22:14:22, pink wrote:
> I think this is self-explanatory. Omit the comment.

Done.

http://codereview.chromium.org/558066/diff/15036/15048#newcode89
chrome/browser/autofill/autofill_dialog_controller_mac.mm:89: profiles_.clear();
On 2010/02/02 22:14:22, pink wrote:
> this is an init. is the clear really necessary?

Removed.

http://codereview.chromium.org/558066/diff/15036/15048#newcode90
chrome/browser/autofill/autofill_dialog_controller_mac.mm:90: for
(i=profiles.begin(); i != profiles.end(); i++)
On 2010/02/02 22:14:22, pink wrote:
> spaces around = and use pre-increment for iterators, per google style guide.

Done.

http://codereview.chromium.org/558066/diff/15036/15048#newcode96
chrome/browser/autofill/autofill_dialog_controller_mac.mm:96: for
(j=creditCards.begin(); j != creditCards.end(); j++)
On 2010/02/02 22:14:22, pink wrote:
> same as above

Done.

http://codereview.chromium.org/558066/diff/15036/15048#newcode104
chrome/browser/autofill/autofill_dialog_controller_mac.mm:104: [self
autorelease];
On 2010/02/02 22:14:22, pink wrote:
> don't autorelease here, autorelease in the windowWillClose delegate method.

Done.
I installed |self| (the controller) as the delegate of the window in the
|awakeFromNib| method to achieve this.

Note: Putting this on the cleanup list for ClearBrowsingDataController as well.

http://codereview.chromium.org/558066/diff/15036/15048#newcode137
chrome/browser/autofill/autofill_dialog_controller_mac.mm:137: // Embed the
child view into our (owned by us) target view.
On 2010/02/02 22:14:22, pink wrote:
> This confuses me greatly. The "child view" you're embedding is not
|childView_|

Correct.  I'm inserting the addressFormViewController's view into the dialog's
|childView_|.

At this point point:
  - the dialog retains the addressFormViewController,
  - the addressFormViewController retains its view
  - the |childView_| *also* retains the addressFormViewController's view



> correct? What's the "target view"? Is that |childView|? If so, this object
does
> not own |childView_|, the view hierarchy does. We only have a weak reference
to
> it.

Right.  I could explicitly transfer ownership from the addressFormViewController
to |childView_| but this didn't seem necessary.  I'd need to reverify the
deallocs if I did that.

One clarification that might not be clear is that |childView_| is the
VerticalLayoutView that lives within the scrolling view within this window.  I
used the |childView_| outlet here to simplify the placement of my disclosure
views.

http://codereview.chromium.org/558066/diff/15036/15054
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/558066/diff/15036/15054#newcode23
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:23:
std::vector<CreditCard>* creditCards) {
On 2010/02/02 22:14:22, pink wrote:
> in C++, use C++ naming, which is underscores (credit_cards).

Done.
(I re-read the C++ style guide and have absorbed the details a little more
closely this time around.  Thanks for your patience.)

http://codereview.chromium.org/558066/diff/15036/15054#newcode28
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:28: for
(i=profiles->begin(); i != profiles->end(); i++)
On 2010/02/02 22:14:22, pink wrote:
> space around ='s, pre-increment on iterator.

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode33
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:33: for
(j=creditCards->begin(); j != creditCards->end(); j++)
On 2010/02/02 22:14:22, pink wrote:
> same as above

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode42
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:42: virtual
~AutoFillDialogObserverTester() {}
On 2010/02/02 22:14:22, pink wrote:
> why declare an additional public section for the dtor? Just put it up next to
> the ctor.

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode48
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:48: {}
On 2010/02/02 22:14:22, pink wrote:
> put on the same line as the decl

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode50
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:50: virtual
void SetUp() {
On 2010/02/02 22:14:22, pink wrote:
> is there any need for these if they just call the parent?

Removed.
These were vestigial.  I'd left them in to remind myself how.  I'll put them
back if/when I need them.

http://codereview.chromium.org/558066/diff/15036/15054#newcode61
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:61:
autoFillProfiles:profiles_
On 2010/02/02 22:14:22, pink wrote:
> align colons

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode143
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:143:
AutoFillType(EMAIL_ADDRESS),
On 2010/02/02 22:14:22, pink wrote:
> why the break after the open paren?

Done.

http://codereview.chromium.org/558066/diff/15036/15054#newcode150
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:150:
AutoFillType(ADDRESS_HOME_CITY),
On 2010/02/02 22:14:22, pink wrote:
> same here

Done.

http://codereview.chromium.org/558066/diff/15036/15051
File chrome/browser/autofill/autofill_dialog_mac.mm (right):

http://codereview.chromium.org/558066/diff/15036/15051#newcode8
chrome/browser/autofill/autofill_dialog_mac.mm:8: void
ShowAutoFillDialog(AutoFillDialogObserver* observer,
On 2010/02/02 22:14:22, pink wrote:
> needs a comment

There is a fairly extensive comment in autofill_dialog.h.  Are you looking for
something speaking to the nature of this "glue" code?  Thanks.

http://codereview.chromium.org/558066/diff/15036/15066
File chrome/browser/cocoa/disclosure_view_controller.h (right):

http://codereview.chromium.org/558066/diff/15036/15066#newcode15
chrome/browser/cocoa/disclosure_view_controller.h:15: // of the view.
|disclosureState| is initialized as "on".
On 2010/02/02 22:14:22, pink wrote:
> "on", I assume, means "open" or "expanded"?

Yes.  I've added clarity to the comment.

http://codereview.chromium.org/558066/diff/15036/15062
File chrome/browser/cocoa/disclosure_view_controller.mm (right):

http://codereview.chromium.org/558066/diff/15036/15062#newcode37
chrome/browser/cocoa/disclosure_view_controller.mm:37: // Set up initial model
state before we trigger listener, we don't want
On 2010/02/02 22:14:22, pink wrote:
> grammar seems odd here.

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode61
chrome/browser/cocoa/disclosure_view_controller.mm:61: -
(void)initDisclosureState:(NSCellStateValue)state {
On 2010/02/02 22:14:22, pink wrote:
> needs a method-level comment, as do all methods from here down.

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode76
chrome/browser/cocoa/disclosure_view_controller.mm:76: startFrame.origin.y,
On 2010/02/02 22:14:22, pink wrote:
> this should probably be indented 4 spaces more, it's a continuation of the
> previous expression, not the next parameter in the NSMakeRect call. Also, the
> last param should be on a new line as well.

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode83
chrome/browser/cocoa/disclosure_view_controller.mm:83: startFrame.origin.y,
On 2010/02/02 22:14:22, pink wrote:
> same here

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode92
chrome/browser/cocoa/disclosure_view_controller.mm:92: NSViewAnimation*
animation;
On 2010/02/02 22:14:22, pink wrote:
> declare this where it is initialized

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode96
chrome/browser/cocoa/disclosure_view_controller.mm:96: {
On 2010/02/02 22:14:22, pink wrote:
> why scope this?

Old habit.  Scoping the comment.  I did notice the C++ guide says this is Ok. 
But I've removed it.

http://codereview.chromium.org/558066/diff/15036/15062#newcode98
chrome/browser/cocoa/disclosure_view_controller.mm:98: dictionary =
[NSMutableDictionary dictionaryWithCapacity:2];
On 2010/02/02 22:14:22, pink wrote:
> just use NSDictionary dictionaryWithObjectsAndKeys:...

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode113
chrome/browser/cocoa/disclosure_view_controller.mm:113: animation =
[[NSViewAnimation alloc] initWithViewAnimations:
On 2010/02/02 22:14:22, pink wrote:
> scoped_nsobject

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode114
chrome/browser/cocoa/disclosure_view_controller.mm:114: [NSArray
arrayWithObjects:dictionary, nil]];
On 2010/02/02 22:14:22, pink wrote:
> use NSArray arrayWithObject: since there is only 1

Done.

http://codereview.chromium.org/558066/diff/15036/15062#newcode163
chrome/browser/cocoa/disclosure_view_controller.mm:163: @interface
DisclosureViewState : NSObject {
On 2010/02/02 22:14:22, pink wrote:
> Needs a class comment. What is this for? How is it used? Why is it here?

Done.
Please see updated code for details.  I notice that the style guide explicitly
mentions breaking out model objects.  I wasn't sure if that implied breaking it
out into a separate source file and header.  I have left it here assuming this
is not necessary for such a simple "model".  But let me know if I should.

http://codereview.chromium.org/558066/diff/15036/15062#newcode164
chrome/browser/cocoa/disclosure_view_controller.mm:164: @private
On 2010/02/02 22:14:22, pink wrote:
> 1-space indent

Done.

http://codereview.chromium.org/558066/diff/15036/15065
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/558066/diff/15036/15065#newcode1158
chrome/browser/cocoa/preferences_window_controller.mm:1158: // todo(dhollowa):
need "n" of these.  create single entry for now.
On 2010/02/02 22:14:22, pink wrote:
> TODO

Done.
Crap.  Missed these.

http://codereview.chromium.org/558066/diff/15036/15065#newcode1163
chrome/browser/cocoa/preferences_window_controller.mm:1163: // todo(dhollowa):
need "n" of these.  create single entry for now.
On 2010/02/02 22:14:22, pink wrote:
> TODO

Done.

http://codereview.chromium.org/558066/diff/15036/15064
File chrome/browser/cocoa/section_separator_view.h (right):

http://codereview.chromium.org/558066/diff/15036/15064#newcode30
chrome/browser/cocoa/section_separator_view.h:30: // Designated initializer.
On 2010/02/02 22:14:22, pink wrote:
> this is inherited as a NSView, no need to re-declare.

Done.

http://codereview.chromium.org/558066/diff/15036/15063
File chrome/browser/cocoa/section_separator_view.mm (right):

http://codereview.chromium.org/558066/diff/15036/15063#newcode1
chrome/browser/cocoa/section_separator_view.mm:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/02/02 22:14:22, pink wrote:
> 2010?

Done.

http://codereview.chromium.org/558066/diff/15036/15063#newcode23
chrome/browser/cocoa/section_separator_view.mm:23: [self setShowBaseLine:TRUE];
On 2010/02/02 22:14:22, pink wrote:
> TRUE is not objective-C. YES is.

Done.

http://codereview.chromium.org/558066/diff/15036/15063#newcode25
chrome/browser/cocoa/section_separator_view.mm:25: [self setShowTopLine:TRUE];
On 2010/02/02 22:14:22, pink wrote:
> same here

Done.

http://codereview.chromium.org/558066/diff/15036/15063#newcode58
chrome/browser/cocoa/section_separator_view.mm:58: -
(void)drawGradientRect:(NSRect)rect {
On 2010/02/02 22:14:22, pink wrote:
> Needs a method-level comment

Done.

http://codereview.chromium.org/558066/diff/15036/15063#newcode71
chrome/browser/cocoa/section_separator_view.mm:71: size_t stopCount = 2;
On 2010/02/02 22:14:22, pink wrote:
> const?

Done.

http://codereview.chromium.org/558066/diff/15036/15063#newcode72
chrome/browser/cocoa/section_separator_view.mm:72: CGFloat stops[2] = { 0.0, 1.0
};
On 2010/02/02 22:14:22, pink wrote:
> use stopCount here?

Done.

http://codereview.chromium.org/558066/diff/15036/15061
File chrome/browser/cocoa/vertical_layout_view.h (right):

http://codereview.chromium.org/558066/diff/15036/15061#newcode20
chrome/browser/cocoa/vertical_layout_view.h:20: // Designated initializer.
On 2010/02/02 22:14:22, pink wrote:
> you can omit this

Done.

http://codereview.chromium.org/558066/diff/15036/15060
File chrome/browser/cocoa/vertical_layout_view.mm (right):

http://codereview.chromium.org/558066/diff/15036/15060#newcode14
chrome/browser/cocoa/vertical_layout_view.mm:14: - (BOOL)isFlipped {
On 2010/02/02 22:14:22, pink wrote:
> move below initWithFrame

Done.

http://codereview.chromium.org/558066/diff/15036/15060#newcode48
chrome/browser/cocoa/vertical_layout_view.mm:48: [[NSNotificationCenter
defaultCenter]
On 2010/02/02 22:14:22, pink wrote:
> just as a precaution, you might want to make sure you remove self from all
> notifications in dealloc.

I have reworked this class to avoid relying on notifications to re-layout.  So
this is no longer needed.  Removing.

http://codereview.chromium.org/558066/diff/15036/15060#newcode58
chrome/browser/cocoa/vertical_layout_view.mm:58: -
(void)onChildFrameChanged:(NSNotification*)notification {
On 2010/02/02 22:14:22, pink wrote:
> these all need comments

Done.

http://codereview.chromium.org/558066/diff/15036/15060#newcode66
chrome/browser/cocoa/vertical_layout_view.mm:66: int i;
On 2010/02/02 22:14:22, pink wrote:
> declare |i| in for loop where it's used.

Done.

http://codereview.chromium.org/558066/diff/15036/15060#newcode68
chrome/browser/cocoa/vertical_layout_view.mm:68: for(i = childCount-1, yPosition
= 0.0f; i >= 0; i--) {
On 2010/02/02 22:14:22, pink wrote:
> space after |for|

Done.

http://codereview.chromium.org/558066/diff/15036/15060#newcode81
chrome/browser/cocoa/vertical_layout_view.mm:81: @end
On 2010/02/02 22:14:22, pink wrote:
> blank line before @end

Done.

[dhollowa, 2010-02-05 03:49:02]

Reworked the way model objects are handled.  Changed them from being nib-based
with a NSObjectController acting as a controller-delegate to a system where the
controller exposes the model via a property, the controls bind directly to this
property, and the lifetime of the model is managed directly by the controller.

[pink (ping after 24hrs), 2010-02-08 14:58:21]

Almost there.

http://codereview.chromium.org/558066/diff/16003/8042
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8042#newcode16
chrome/browser/autofill/autofill_address_model_mac.h:16: // It is typical to
initialize newly created objects using |copyProfileToModel|.
see below about initializer.

http://codereview.chromium.org/558066/diff/16003/8042#newcode72
chrome/browser/autofill/autofill_address_model_mac.h:72: -
(void)copyProfileToModel:(const AutoFillProfile&)profile;
this should be the designated initializer, called "initWithModel:"

http://codereview.chromium.org/558066/diff/16003/8040
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8040#newcode69
chrome/browser/autofill/autofill_address_model_mac.mm:69: firstName_ =
(firstName_ == nil) ? @"" : firstName_;
I know it's more compact, but it seems silly to set the variable to itself on
success. Why not just have an if like:

if (!firstName_) firstName_ = @"";

http://codereview.chromium.org/558066/diff/16003/8034
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8034#newcode30
chrome/browser/autofill/autofill_address_view_controller_mac.mm:30:
[addressModel_ copyProfileToModel:profile];
this should all just be part of the init call for the model.

http://codereview.chromium.org/558066/diff/16003/8031
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8031#newcode54
chrome/browser/autofill/autofill_credit_card_model_mac.h:54: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard;
same as before, this should be the designated initializer. initWithCreditCard:

http://codereview.chromium.org/558066/diff/16003/8033
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8033#newcode20
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:20: //
TODO(dhollowa): temporary to disable until implementend.
bug #?

http://codereview.chromium.org/558066/diff/16003/8037
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/16003/8037#newcode31
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:31:
[creditCardModel_ copyCreditCardToModel:creditCard];
make part of model's init.

http://codereview.chromium.org/558066/diff/16003/8037#newcode34
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:34: //
TODO(dhollowa): not yet implemented, disabling controls for now.
bug# for TODO?

http://codereview.chromium.org/558066/diff/16003/8041
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8041#newcode29
chrome/browser/autofill/autofill_dialog_controller_mac.h:29: // TODO(dhollowa):
one each of these for now.  Will be n of each
bug# for TODO?

http://codereview.chromium.org/558066/diff/16003/8032
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8032#newcode37
chrome/browser/autofill/autofill_dialog_controller_mac.mm:37: [[self window]
setDelegate:self];
can you not do this in the nib?

http://codereview.chromium.org/558066/diff/16003/8049
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/558066/diff/16003/8049#newcode1158
chrome/browser/cocoa/preferences_window_controller.mm:1158: // TODO(dhollowa):
Need "n" of these.  Create single entry for now.
bug# for TODOs

http://codereview.chromium.org/558066/diff/16003/8048
File chrome/browser/cocoa/section_separator_view.h (right):

http://codereview.chromium.org/558066/diff/16003/8048#newcode17
chrome/browser/cocoa/section_separator_view.h:17: @interface
SectionSeparatorView : NSView {
do we have anything like this already in GTM? Should we add this there so others
can use it?

http://codereview.chromium.org/558066/diff/16003/8044
File chrome/browser/cocoa/vertical_layout_view.mm (right):

http://codereview.chromium.org/558066/diff/16003/8044#newcode33
chrome/browser/cocoa/vertical_layout_view.mm:33: - (void)viewWillDraw {
do you want to do this every time you draw? Doesn't that layout every time
something is exposed, or with double-buffering does that actually not happen
that often? Can you put a log here to see how often this actually gets called?

[dhollowa, 2010-02-10 21:34:00]

Next round.  Thanks.  Hopefully this'll be it!

http://codereview.chromium.org/558066/diff/16003/8042
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8042#newcode16
chrome/browser/autofill/autofill_address_model_mac.h:16: // It is typical to
initialize newly created objects using |copyProfileToModel|.
On 2010/02/08 14:58:22, pink wrote:
> see below about initializer.

Done.

http://codereview.chromium.org/558066/diff/16003/8042#newcode72
chrome/browser/autofill/autofill_address_model_mac.h:72: -
(void)copyProfileToModel:(const AutoFillProfile&)profile;
On 2010/02/08 14:58:22, pink wrote:
> this should be the designated initializer, called "initWithModel:"

Done.

Yes, better.

http://codereview.chromium.org/558066/diff/16003/8040
File chrome/browser/autofill/autofill_address_model_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8040#newcode69
chrome/browser/autofill/autofill_address_model_mac.mm:69: firstName_ =
(firstName_ == nil) ? @"" : firstName_;
On 2010/02/08 14:58:22, pink wrote:
> I know it's more compact, but it seems silly to set the variable to itself on
> success. Why not just have an if like:
> 
> if (!firstName_) firstName_ = @"";

Done.

http://codereview.chromium.org/558066/diff/16003/8034
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8034#newcode30
chrome/browser/autofill/autofill_address_view_controller_mac.mm:30:
[addressModel_ copyProfileToModel:profile];
On 2010/02/08 14:58:22, pink wrote:
> this should all just be part of the init call for the model.

Done.
Also removed the declaration to |copyProfileToModel| above.  No longer needed.

http://codereview.chromium.org/558066/diff/16003/8031
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8031#newcode54
chrome/browser/autofill/autofill_credit_card_model_mac.h:54: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard;
On 2010/02/08 14:58:22, pink wrote:
> same as before, this should be the designated initializer. initWithCreditCard:

Done.

http://codereview.chromium.org/558066/diff/16003/8033
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8033#newcode20
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:20: //
TODO(dhollowa): temporary to disable until implementend.
On 2010/02/08 14:58:22, pink wrote:
> bug #?

Done.
My understanding is that TODOs should have a bug number inline in the code.  So
I have added "See crbug.com/33029", next to the TODO.  Correct?

http://codereview.chromium.org/558066/diff/16003/8037
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/16003/8037#newcode12
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:12: -
(void)copyCreditCardToModel:(const CreditCard&)creditCard;
Removed this.  No longer needed now that we've moved to using designated
initializers.

http://codereview.chromium.org/558066/diff/16003/8037#newcode31
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:31:
[creditCardModel_ copyCreditCardToModel:creditCard];
On 2010/02/08 14:58:22, pink wrote:
> make part of model's init.

Done.

http://codereview.chromium.org/558066/diff/16003/8037#newcode34
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:34: //
TODO(dhollowa): not yet implemented, disabling controls for now.
On 2010/02/08 14:58:22, pink wrote:
> bug# for TODO?

Done.

http://codereview.chromium.org/558066/diff/16003/8041
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/16003/8041#newcode29
chrome/browser/autofill/autofill_dialog_controller_mac.h:29: // TODO(dhollowa):
one each of these for now.  Will be n of each
On 2010/02/08 14:58:22, pink wrote:
> bug# for TODO?

Done.

http://codereview.chromium.org/558066/diff/16003/8032
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/16003/8032#newcode37
chrome/browser/autofill/autofill_dialog_controller_mac.mm:37: [[self window]
setDelegate:self];
On 2010/02/08 14:58:22, pink wrote:
> can you not do this in the nib?

Yes.  Done.

http://codereview.chromium.org/558066/diff/16003/8049
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/558066/diff/16003/8049#newcode1158
chrome/browser/cocoa/preferences_window_controller.mm:1158: // TODO(dhollowa):
Need "n" of these.  Create single entry for now.
On 2010/02/08 14:58:22, pink wrote:
> bug# for TODOs

Done.

http://codereview.chromium.org/558066/diff/16003/8048
File chrome/browser/cocoa/section_separator_view.h (right):

http://codereview.chromium.org/558066/diff/16003/8048#newcode17
chrome/browser/cocoa/section_separator_view.h:17: @interface
SectionSeparatorView : NSView {
On 2010/02/08 14:58:22, pink wrote:
> do we have anything like this already in GTM? Should we add this there so
others
> can use it?

I don't see anything like this in GTM.  I'll talk to MacLachlan about it and see
who might do this and how.  The VerticalLayoutView seems generally handy too.

http://codereview.chromium.org/558066/diff/16003/8044
File chrome/browser/cocoa/vertical_layout_view.mm (right):

http://codereview.chromium.org/558066/diff/16003/8044#newcode33
chrome/browser/cocoa/vertical_layout_view.mm:33: - (void)viewWillDraw {
On 2010/02/08 14:58:22, pink wrote:
> do you want to do this every time you draw? Doesn't that layout every time
> something is exposed, or with double-buffering does that actually not happen
> that often? Can you put a log here to see how often this actually gets called?

I considered this, yes.  It does get called for any view change that requires a
redraw in any of its children, and any frame change in its containing views. 
But this does not happen terribly often, typically only when user interaction is
going on, such as a window resize or a disclosure triangle toggle.  And we need
to actually perform the layout in these cases anyway.  The cases where we have
potential to avoid some computation is where view changes happen that don't
affect the frame sizes of the children, but detecting these cases is nearly as
costly as running through the full layout.  I did some measurements and the time
for layout is in the .5 millisecond range (at worst).  This would put us in the
2000fps range.  So not a problem IMO.  :-)

[pink (ping after 24hrs), 2010-02-11 17:09:16]

LGTM with some nit-picks. No need for re-review.

http://codereview.chromium.org/558066/diff/28001/28023
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28023#newcode17
chrome/browser/autofill/autofill_address_model_mac.h:17: // initializer
|initWithProfile|.
initWithProfile: (note the colon)

http://codereview.chromium.org/558066/diff/28001/28011
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28011#newcode16
chrome/browser/autofill/autofill_address_view_controller_mac.h:16: //
|initWithProfile| is the designated initializer.  It takes |profile|
initWithProfile:

http://codereview.chromium.org/558066/diff/28001/28015
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/28001/28015#newcode23
chrome/browser/autofill/autofill_address_view_controller_mac.mm:23: [self
setAddressModel:[[[AutoFillAddressModel alloc]
probably cleaner to just write:

addressModel_ = [[AutoFillAddressModel alloc] initWithProfile:profile];

*shrug* no big deal.

http://codereview.chromium.org/558066/diff/28001/28012
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28012#newcode16
chrome/browser/autofill/autofill_credit_card_model_mac.h:16: // Model objects
are initialized from a given creditCard using the designated
either "credit card" or "CreditCard".

http://codereview.chromium.org/558066/diff/28001/28012#newcode17
chrome/browser/autofill/autofill_credit_card_model_mac.h:17: // initializer
|initWithCreditCard|.
initWithCreditCard:

http://codereview.chromium.org/558066/diff/28001/28014
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28014#newcode16
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:16: //
|initWithCreditCard| is the designated initializer.  It takes |creditCard|
initWithCreditCard:

http://codereview.chromium.org/558066/diff/28001/28018
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/28001/28018#newcode23
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:23: [self
setCreditCardModel:[[[AutoFillCreditCardModel alloc]
same comment as before about just assigning directly.

http://codereview.chromium.org/558066/diff/28001/28013
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/28001/28013#newcode30
chrome/browser/autofill/autofill_dialog_controller_mac.mm:30: if (![controller
isWindowLoaded]) {
comment about the purpose of this line.

http://codereview.chromium.org/558066/diff/28001/28013#newcode71
chrome/browser/autofill/autofill_dialog_controller_mac.mm:71:
AutoFillDialogController* controller =
comment here about intentionally "leaking" because controller will release
itself when dialog closes.

[dhollowa, 2010-02-11 17:32:45]

Awesome.  Thanks for the detailed review!  I have integrated these last round of
changes.  Now on to the commit...

http://codereview.chromium.org/558066/diff/28001/28023
File chrome/browser/autofill/autofill_address_model_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28023#newcode17
chrome/browser/autofill/autofill_address_model_mac.h:17: // initializer
|initWithProfile|.
On 2010/02/11 17:09:17, pink wrote:
> initWithProfile: (note the colon)

Done.

http://codereview.chromium.org/558066/diff/28001/28011
File chrome/browser/autofill/autofill_address_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28011#newcode16
chrome/browser/autofill/autofill_address_view_controller_mac.h:16: //
|initWithProfile| is the designated initializer.  It takes |profile|
On 2010/02/11 17:09:17, pink wrote:
> initWithProfile:

Done.

http://codereview.chromium.org/558066/diff/28001/28015
File chrome/browser/autofill/autofill_address_view_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/28001/28015#newcode23
chrome/browser/autofill/autofill_address_view_controller_mac.mm:23: [self
setAddressModel:[[[AutoFillAddressModel alloc]
On 2010/02/11 17:09:17, pink wrote:
> probably cleaner to just write:
> 
> addressModel_ = [[AutoFillAddressModel alloc] initWithProfile:profile];
> 
> *shrug* no big deal.

Due to KVO notification I need to use the setter explicitly here.

http://codereview.chromium.org/558066/diff/28001/28012
File chrome/browser/autofill/autofill_credit_card_model_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28012#newcode16
chrome/browser/autofill/autofill_credit_card_model_mac.h:16: // Model objects
are initialized from a given creditCard using the designated
On 2010/02/11 17:09:17, pink wrote:
> either "credit card" or "CreditCard".
Made it specific |creditCard|.

http://codereview.chromium.org/558066/diff/28001/28012#newcode17
chrome/browser/autofill/autofill_credit_card_model_mac.h:17: // initializer
|initWithCreditCard|.
On 2010/02/11 17:09:17, pink wrote:
> initWithCreditCard:

Done.

http://codereview.chromium.org/558066/diff/28001/28014
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.h (right):

http://codereview.chromium.org/558066/diff/28001/28014#newcode16
chrome/browser/autofill/autofill_credit_card_view_controller_mac.h:16: //
|initWithCreditCard| is the designated initializer.  It takes |creditCard|
On 2010/02/11 17:09:17, pink wrote:
> initWithCreditCard:

Done.
Same below.

http://codereview.chromium.org/558066/diff/28001/28018
File chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm
(right):

http://codereview.chromium.org/558066/diff/28001/28018#newcode23
chrome/browser/autofill/autofill_credit_card_view_controller_mac.mm:23: [self
setCreditCardModel:[[[AutoFillCreditCardModel alloc]
On 2010/02/11 17:09:17, pink wrote:
> same comment as before about just assigning directly.

Same reply.  Due to KVO notification I need to explicitly use the setter.

http://codereview.chromium.org/558066/diff/28001/28013
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/558066/diff/28001/28013#newcode30
chrome/browser/autofill/autofill_dialog_controller_mac.mm:30: if (![controller
isWindowLoaded]) {
On 2010/02/11 17:09:17, pink wrote:
> comment about the purpose of this line.

Done.

http://codereview.chromium.org/558066/diff/28001/28013#newcode71
chrome/browser/autofill/autofill_dialog_controller_mac.mm:71:
AutoFillDialogController* controller =
On 2010/02/11 17:09:17, pink wrote:
> comment here about intentionally "leaking" because controller will release
> itself when dialog closes.

Done.

[vandebo (ex-Chrome), 2010-02-12 18:33:29]

This change may have caused a valgrind report.  Can you please look into it?
http://chrome-buildbot.corp.google.com:8014/builders/Chromium%20Mac%20(valgri...