AutoFill Mac dialog should be non-modal

AutoFill Mac dialog should be non-modal

Changes the AutoFill dialog on Mac to be modeless.  Changes to preferences and AutoFill profile and credit card information is now saved immediately.
Nib changes: Removes Save and Cancel buttons, activates Close and Minimize controls, and compacts dialog to account for new vertical space.

BUG=49935
TEST=AutoFillDialogControllerTest.*

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

[dhollowa, 2010-09-08 18:53:55]

jhawkins for AutoFill logic.
pinkerton for Mac specifics.

[James Hawkins, 2010-09-08 18:57:30]

Mostly rubber-stamp LGTM beside 80 col nit, on account of me not being up to
snuff on obj-c.

[James Hawkins, 2010-09-08 18:58:30]

http://codereview.chromium.org/3302015/diff/2001/3002
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3302015/diff/2001/3002#newcode127
chrome/browser/autofill/autofill_dialog_controller_mac.h:127: // modeless
dialog.  The controller autoreleases itself when the dialog is closed.
80 cols

[dhollowa, 2010-09-08 19:02:29]

http://codereview.chromium.org/3302015/diff/2001/3002
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3302015/diff/2001/3002#newcode127
chrome/browser/autofill/autofill_dialog_controller_mac.h:127: // modeless
dialog.  The controller autoreleases itself when the dialog is closed.
On 2010/09/08 18:58:31, James Hawkins wrote:
> 80 cols

Done.

[pink (ping after 24hrs), 2010-09-08 19:22:27]

Some minor questions.

http://codereview.chromium.org/3302015/diff/2001/3002
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3302015/diff/2001/3002#newcode136
chrome/browser/autofill/autofill_dialog_controller_mac.h:136:
profile:(Profile*)profile;
line up the colons

http://codereview.chromium.org/3302015/diff/2001/3003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3302015/diff/2001/3003#newcode106
chrome/browser/autofill/autofill_dialog_controller_mac.mm:106: -
(void)onPreferenceChanged:(const std::string&)preferenceName;
usually in cocoa these are named something like "preferenceDidChange". Do you
want to be consistent with that naming?

http://codereview.chromium.org/3302015/diff/2001/3003#newcode239
chrome/browser/autofill/autofill_dialog_controller_mac.mm:239: @dynamic
autoFillEnabled;
I don't think you actually need to call these out explicitly as you are
providing the methods in this file. @dynamic, according to the docs, is for when
the compiler can't find the method definitions because they come from somewhere
else, such as dynamically loaded code. This is a pretty straightforward
definition and so you can omit it unless you really want to.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode287
chrome/browser/autofill/autofill_dialog_controller_mac.mm:287:
ProfileControllerMap* map = Singleton<ProfileControllerMap>::get();
can there be multiple autofill dialogs? Is this looking forward for when we can
have multiple profiles? Is there UI to clearly distinguish which profile this
dialog is for?

http://codereview.chromium.org/3302015/diff/2001/3003#newcode652
chrome/browser/autofill/autofill_dialog_controller_mac.mm:652: return
autoFillEnabled_.GetValue() ? YES : NO;
The conversion isn't technically needed, see the obj-c style guide section on
BOOL and bool interchangeability.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode657
chrome/browser/autofill/autofill_dialog_controller_mac.mm:657:
autoFillEnabled_.SetValueIfNotManaged(value ? true : false);
same here

http://codereview.chromium.org/3302015/diff/2001/3003#newcode678
chrome/browser/autofill/autofill_dialog_controller_mac.mm:678:
profile:(Profile*)profile {
can you line up the colons?

http://codereview.chromium.org/3302015/diff/2001/3003#newcode690
chrome/browser/autofill/autofill_dialog_controller_mac.mm:690:
AutoFillDialogController* controller =
indent looks wrong here.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode746
chrome/browser/autofill/autofill_dialog_controller_mac.mm:746:
sizeSaver_.reset([[WindowSizeAutosaver alloc]
how will this take into account if you run in one language, then switch
languages such that the text is longer? We've had problems with this in other
dialogs (prefs) where we need to only save/restore the origin, not the full
size.

[dhollowa, 2010-09-08 19:59:03]

http://codereview.chromium.org/3302015/diff/2001/3002
File chrome/browser/autofill/autofill_dialog_controller_mac.h (right):

http://codereview.chromium.org/3302015/diff/2001/3002#newcode136
chrome/browser/autofill/autofill_dialog_controller_mac.h:136:
profile:(Profile*)profile;
On 2010/09/08 19:22:27, pink wrote:
> line up the colons

Done.

http://codereview.chromium.org/3302015/diff/2001/3003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3302015/diff/2001/3003#newcode106
chrome/browser/autofill/autofill_dialog_controller_mac.mm:106: -
(void)onPreferenceChanged:(const std::string&)preferenceName;
On 2010/09/08 19:22:27, pink wrote:
> usually in cocoa these are named something like "preferenceDidChange". Do you
> want to be consistent with that naming?

Yes, Done.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode239
chrome/browser/autofill/autofill_dialog_controller_mac.mm:239: @dynamic
autoFillEnabled;
On 2010/09/08 19:22:27, pink wrote:
> I don't think you actually need to call these out explicitly as you are
> providing the methods in this file. @dynamic, according to the docs, is for
when
> the compiler can't find the method definitions because they come from
somewhere
> else, such as dynamically loaded code. This is a pretty straightforward
> definition and so you can omit it unless you really want to.

Yes, I had put these here for doc purposes.  But a curious mind will find the
getters/setters pretty easily, so I'll remove.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode287
chrome/browser/autofill/autofill_dialog_controller_mac.mm:287:
ProfileControllerMap* map = Singleton<ProfileControllerMap>::get();
On 2010/09/08 19:22:27, pink wrote:
> can there be multiple autofill dialogs? Is this looking forward for when we
can
> have multiple profiles? Is there UI to clearly distinguish which profile this
> dialog is for?

There is no distinguishing UI currently, no.  My original plan was to create a
single instance (not profile specific), but upon inspection of
clear_browsing_data_controller.mm:42 and keyword_editor_cocoa_controller.mm:102
I decided to follow suit.  For consistency and future-proofness.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode652
chrome/browser/autofill/autofill_dialog_controller_mac.mm:652: return
autoFillEnabled_.GetValue() ? YES : NO;
On 2010/09/08 19:22:27, pink wrote:
> The conversion isn't technically needed, see the obj-c style guide section on
> BOOL and bool interchangeability. 

Done.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode657
chrome/browser/autofill/autofill_dialog_controller_mac.mm:657:
autoFillEnabled_.SetValueIfNotManaged(value ? true : false);
On 2010/09/08 19:22:27, pink wrote:
> same here

Done.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode678
chrome/browser/autofill/autofill_dialog_controller_mac.mm:678:
profile:(Profile*)profile {
On 2010/09/08 19:22:27, pink wrote:
> can you line up the colons?

Done.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode690
chrome/browser/autofill/autofill_dialog_controller_mac.mm:690:
AutoFillDialogController* controller =
On 2010/09/08 19:22:27, pink wrote:
> indent looks wrong here.

Done.

http://codereview.chromium.org/3302015/diff/2001/3003#newcode746
chrome/browser/autofill/autofill_dialog_controller_mac.mm:746:
sizeSaver_.reset([[WindowSizeAutosaver alloc]
On 2010/09/08 19:22:27, pink wrote:
> how will this take into account if you run in one language, then switch
> languages such that the text is longer? We've had problems with this in other
> dialogs (prefs) where we need to only save/restore the origin, not the full
> size.

Since this dialog is not resizable I believe we're ok.  The logic in the
WindowSizeAutosaver checks NSResizableWindowMask and only saves x,y in this
case.

[pink (ping after 24hrs), 2010-09-08 20:07:36]

lgtm.

http://codereview.chromium.org/3302015/diff/12001/13003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3302015/diff/12001/13003#newcode675
chrome/browser/autofill/autofill_dialog_controller_mac.mm:675:
(AutoFillDialogObserver*)observer
This might flow better as:

+ (AutoFillDialogController*)
    controllerWithObserver:(AutoFillDialogObserver*)observer
                   profile:(Profile*)profile {

A little easier to read, i think.

[dhollowa, 2010-09-08 20:12:30]

http://codereview.chromium.org/3302015/diff/12001/13003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/3302015/diff/12001/13003#newcode675
chrome/browser/autofill/autofill_dialog_controller_mac.mm:675:
(AutoFillDialogObserver*)observer
On 2010/09/08 20:07:36, pink wrote:
> This might flow better as:
> 
> + (AutoFillDialogController*)
>     controllerWithObserver:(AutoFillDialogObserver*)observer
>                    profile:(Profile*)profile {
> 
> A little easier to read, i think.

Ah yes.  Better.  Thanks.