AutoFill profile shouldn't be saved when cancelled during initial setup.

AutoFill profile shouldn't be saved when cancelled during initial setup.

For first encounter with fillable form, the AutoFillManager::OnInfoBarAccepted()
call now passes the new profile and credit card information to the dialog directly
instead of saving it to the database and then invoking the dialog.  This facilitates
"Cancel" in the dialog where the new information is not persisted.

This was a good opportunity to refactor the deferred PersonalDataManager::Observer() logic
out of the preferences dialog and into the AutoFillDialogController itself.

This also consolidates the Windows, Mac, and Linux interfaces for the ShowAutoFillDialog()
call.  More work is required on Linux and Windows to properly conform to this interface and
fix bug 41010.  The Linux and Windows implementations will need to respect the new input
parameters |imported_profile| and |imported_credit_card|.

BUG=41010
TEST=AutoFillDialogControllerTest.WaitForDataToLoad, AutoFillDialogControllerTest.ImportedParameters

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

[James Hawkins, 2010-05-04 21:05:30]

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

http://codereview.chromium.org/1930002/diff/1/2#newcode44
chrome/browser/autofill/autofill_dialog.h:44: void
ShowAutoFillDialog(gfx::NativeView parent,
Can you add a comment for |parent|?  For example, I'm don't see why you changed
it to NativeView over NativeWindow.

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

http://codereview.chromium.org/1930002/diff/1/4#newcode35
chrome/browser/autofill/autofill_dialog_controller_mac.mm:35: //
PersonalDataManager data before showing the auto fill settings data to the
s/auto fill/AutoFill/

http://codereview.chromium.org/1930002/diff/1/4#newcode88
chrome/browser/autofill/autofill_dialog_controller_mac.mm:88: //
|showAutoFillSettings:| to try again now knowing that the
Is this comment still relevant vis a vis |showAutoFillSettings:|?

http://codereview.chromium.org/1930002/diff/1/4#newcode619
chrome/browser/autofill/autofill_dialog_controller_mac.mm:619: // Add initial
profile if we don't yet have any.
No initial profile/CC.  If the user has no profiles and opens the AutoFill
dialog, each list (profiles, CCs) should be empty.

http://codereview.chromium.org/1930002/diff/1/4#newcode620
chrome/browser/autofill/autofill_dialog_controller_mac.mm:620: AutoFillProfile
autofillProfile(ASCIIToUTF16(""), 0);
FTR, ASCIIToUTF16("") should be string16().

http://codereview.chromium.org/1930002/diff/1/5
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/1/5#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:47:
kWebDataLoadDelayMilliseconds);
Do we need this to be delayed at all? 10ms seems arbitrary.

http://codereview.chromium.org/1930002/diff/1/5#newcode70
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:70: virtual
~ProfileMock() {
nit: virtual ~ProfileMock() {}

http://codereview.chromium.org/1930002/diff/1/5#newcode89
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:89: virtual
~BrowserMock() {
nit: same as above.

http://codereview.chromium.org/1930002/diff/1/5#newcode103
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:103: // Mock
observer for the autofill settings dialog.
s/autofill/AutoFill/

http://codereview.chromium.org/1930002/diff/1/6
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/1930002/diff/1/6#newcode1073
chrome/browser/autofill/autofill_dialog_gtk.cc:1073: void
ShowAutoFillDialog(gfx::NativeView parent,
A TODO would be helpful.

http://codereview.chromium.org/1930002/diff/1/8
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1930002/diff/1/8#newcode267
chrome/browser/autofill/autofill_manager.cc:267: void
AutoFillManager::OnPersonalDataLoaded() {
If we're loading the profiles in the dialog, why keep this method around?

http://codereview.chromium.org/1930002/diff/1/8#newcode296
chrome/browser/autofill/autofill_manager.cc:296:
personal_data_->GetImportedFormData(&profile, &credit_card);
Now that we aren't immediately Saving the imported form data, we should store
the profile and CC in the AFM instead of the PDM.

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

http://codereview.chromium.org/1930002/diff/1/10#newcode74
chrome/browser/autofill/personal_data_manager.h:74: void
GetImportedFormData(AutoFillProfile** profile, CreditCard** credit_card);
Remove this method and store the imported profile and CC in the AFM.

[dhollowa, 2010-05-04 21:52:10]

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

http://codereview.chromium.org/1930002/diff/1/2#newcode44
chrome/browser/autofill/autofill_dialog.h:44: void
ShowAutoFillDialog(gfx::NativeView parent,
On 2010/05/04 21:05:30, James Hawkins wrote:
> Can you add a comment for |parent|?  For example, I'm don't see why you
changed
> it to NativeView over NativeWindow.

At call sites (autofill_manager.cc:297 for example) the input is a native view
and not a window.  It looks like they're equivalent on Windows but the compiler
complains about type mismatch on Mac.

tab_contents_->GetContentNativeView() is what's passed in.

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

http://codereview.chromium.org/1930002/diff/1/4#newcode35
chrome/browser/autofill/autofill_dialog_controller_mac.mm:35: //
PersonalDataManager data before showing the auto fill settings data to the
On 2010/05/04 21:05:30, James Hawkins wrote:
> s/auto fill/AutoFill/

Done.

http://codereview.chromium.org/1930002/diff/1/4#newcode88
chrome/browser/autofill/autofill_dialog_controller_mac.mm:88: //
|showAutoFillSettings:| to try again now knowing that the
On 2010/05/04 21:05:30, James Hawkins wrote:
> Is this comment still relevant vis a vis |showAutoFillSettings:|?

Done.  Fixed the comment.

http://codereview.chromium.org/1930002/diff/1/4#newcode619
chrome/browser/autofill/autofill_dialog_controller_mac.mm:619: // Add initial
profile if we don't yet have any.
On 2010/05/04 21:05:30, James Hawkins wrote:
> No initial profile/CC.  If the user has no profiles and opens the AutoFill
> dialog, each list (profiles, CCs) should be empty.

I had left this in here because it was requested for old-style dialog.  But will
remove now that we're moving to new-style that doesn't have initial empty
records.

http://codereview.chromium.org/1930002/diff/1/4#newcode620
chrome/browser/autofill/autofill_dialog_controller_mac.mm:620: AutoFillProfile
autofillProfile(ASCIIToUTF16(""), 0);
On 2010/05/04 21:05:30, James Hawkins wrote:
> FTR, ASCIIToUTF16("") should be string16().

Noted, yes.

http://codereview.chromium.org/1930002/diff/1/5
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/1/5#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:47:
kWebDataLoadDelayMilliseconds);
On 2010/05/04 21:05:30, James Hawkins wrote:
> Do we need this to be delayed at all? 10ms seems arbitrary.

I do need a delay to properly test, yes.  Because it allows the UI loop to run
and display intermediate results.  Then when notified the UI updates with the
correct data.  10ms is a nice short amount of time to let things happen but not
slow down the tests too much.

http://codereview.chromium.org/1930002/diff/1/5#newcode70
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:70: virtual
~ProfileMock() {
On 2010/05/04 21:05:30, James Hawkins wrote:
> nit: virtual ~ProfileMock() {}

Done.

http://codereview.chromium.org/1930002/diff/1/5#newcode89
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:89: virtual
~BrowserMock() {
On 2010/05/04 21:05:30, James Hawkins wrote:
> nit: same as above.

Done.

http://codereview.chromium.org/1930002/diff/1/5#newcode103
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:103: // Mock
observer for the autofill settings dialog.
On 2010/05/04 21:05:30, James Hawkins wrote:
> s/autofill/AutoFill/

Done.

http://codereview.chromium.org/1930002/diff/1/6
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/1930002/diff/1/6#newcode1073
chrome/browser/autofill/autofill_dialog_gtk.cc:1073: void
ShowAutoFillDialog(gfx::NativeView parent,
On 2010/05/04 21:05:30, James Hawkins wrote:
> A TODO would be helpful.

Done.

http://codereview.chromium.org/1930002/diff/1/8
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/1930002/diff/1/8#newcode267
chrome/browser/autofill/autofill_manager.cc:267: void
AutoFillManager::OnPersonalDataLoaded() {
On 2010/05/04 21:05:30, James Hawkins wrote:
> If we're loading the profiles in the dialog, why keep this method around?

Yes.  Removed.

http://codereview.chromium.org/1930002/diff/1/8#newcode296
chrome/browser/autofill/autofill_manager.cc:296:
personal_data_->GetImportedFormData(&profile, &credit_card);
On 2010/05/04 21:05:30, James Hawkins wrote:
> Now that we aren't immediately Saving the imported form data, we should store
> the profile and CC in the AFM instead of the PDM.

I've added a TODO for this.  Would prefer to do this in a separate CL.

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

http://codereview.chromium.org/1930002/diff/1/10#newcode74
chrome/browser/autofill/personal_data_manager.h:74: void
GetImportedFormData(AutoFillProfile** profile, CreditCard** credit_card);
On 2010/05/04 21:05:30, James Hawkins wrote:
> Remove this method and store the imported profile and CC in the AFM.

Added a TODO to refactor this.  Prefer to tackle in separate CL.

[James Hawkins, 2010-05-04 22:34:20]

http://codereview.chromium.org/1930002/diff/1/5
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/1/5#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:47:
kWebDataLoadDelayMilliseconds);
On 2010/05/04 21:52:10, dhollowa wrote:
> On 2010/05/04 21:05:30, James Hawkins wrote:
> > Do we need this to be delayed at all? 10ms seems arbitrary.
> 
> I do need a delay to properly test, yes.  Because it allows the UI loop to run
> and display intermediate results.  Then when notified the UI updates with the
> correct data.  10ms is a nice short amount of time to let things happen but
not
> slow down the tests too much.

Please add a comment that describes this need.

http://codereview.chromium.org/1930002/diff/1/6
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/1930002/diff/1/6#newcode1073
chrome/browser/autofill/autofill_dialog_gtk.cc:1073: void
ShowAutoFillDialog(gfx::NativeView parent,
On 2010/05/04 21:52:10, dhollowa wrote:
> On 2010/05/04 21:05:30, James Hawkins wrote:
> > A TODO would be helpful.
> 
> Done.

s/dhollowa/jhawkins/ since this file is my responsibility.  Probably the same
(georgey) for the Win file.

http://codereview.chromium.org/1930002/diff/9001/10001
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/1930002/diff/9001/10001#newcode44
chrome/browser/autofill/autofill_dialog.h:44: void
ShowAutoFillDialog(gfx::NativeView parent,
Can you add a comment for |parent|?

http://codereview.chromium.org/1930002/diff/9001/10003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/1930002/diff/9001/10003#newcode618
chrome/browser/autofill/autofill_dialog_controller_mac.mm:618:
std::vector<AutoFillProfile*>::const_iterator i;
nit: Please put the iterator inside the for loop scope, that way you can re-use
|i|, and change it to |iter| while you're at it.

http://codereview.chromium.org/1930002/diff/9001/10015
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/1930002/diff/9001/10015#newcode1437
chrome/browser/views/autofill_profiles_view_win.cc:1437: // Declared in
"chrome/browser/autofill/autofill_dialog.h"
Please add TODO(georgey).

[dhollowa, 2010-05-04 23:00:40]

http://codereview.chromium.org/1930002/diff/1/5
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/1/5#newcode47
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:47:
kWebDataLoadDelayMilliseconds);
On 2010/05/04 22:34:20, James Hawkins wrote:
> On 2010/05/04 21:52:10, dhollowa wrote:
> > On 2010/05/04 21:05:30, James Hawkins wrote:
> > > Do we need this to be delayed at all? 10ms seems arbitrary.
> > 
> > I do need a delay to properly test, yes.  Because it allows the UI loop to
run
> > and display intermediate results.  Then when notified the UI updates with
the
> > correct data.  10ms is a nice short amount of time to let things happen but
> not
> > slow down the tests too much.
> 
> Please add a comment that describes this need.

Done.

http://codereview.chromium.org/1930002/diff/1/6
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/1930002/diff/1/6#newcode1073
chrome/browser/autofill/autofill_dialog_gtk.cc:1073: void
ShowAutoFillDialog(gfx::NativeView parent,
On 2010/05/04 22:34:20, James Hawkins wrote:
> On 2010/05/04 21:52:10, dhollowa wrote:
> > On 2010/05/04 21:05:30, James Hawkins wrote:
> > > A TODO would be helpful.
> > 
> > Done.
> 
> s/dhollowa/jhawkins/ since this file is my responsibility.  Probably the same
> (georgey) for the Win file.

Done.  Also in views/autofill_profiles_view_win.cc

http://codereview.chromium.org/1930002/diff/9001/10001
File chrome/browser/autofill/autofill_dialog.h (right):

http://codereview.chromium.org/1930002/diff/9001/10001#newcode44
chrome/browser/autofill/autofill_dialog.h:44: void
ShowAutoFillDialog(gfx::NativeView parent,
On 2010/05/04 22:34:20, James Hawkins wrote:
> Can you add a comment for |parent|?

Done.

http://codereview.chromium.org/1930002/diff/9001/10003
File chrome/browser/autofill/autofill_dialog_controller_mac.mm (right):

http://codereview.chromium.org/1930002/diff/9001/10003#newcode618
chrome/browser/autofill/autofill_dialog_controller_mac.mm:618:
std::vector<AutoFillProfile*>::const_iterator i;
On 2010/05/04 22:34:20, James Hawkins wrote:
> nit: Please put the iterator inside the for loop scope, that way you can
re-use
> |i|, and change it to |iter| while you're at it.

Done.

http://codereview.chromium.org/1930002/diff/9001/10015
File chrome/browser/views/autofill_profiles_view_win.cc (right):

http://codereview.chromium.org/1930002/diff/9001/10015#newcode1437
chrome/browser/views/autofill_profiles_view_win.cc:1437: // Declared in
"chrome/browser/autofill/autofill_dialog.h"
On 2010/05/04 22:34:20, James Hawkins wrote:
> Please add TODO(georgey).

Done.

[James Hawkins, 2010-05-04 23:28:46]

LGTM

http://codereview.chromium.org/1930002/diff/21001/2006
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/21001/2006#newcode22
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:22: // This
delay allows the UI loop to run and display intermediate results.  Then
This comment should be along the lines of "This is the timeout value for such
and such".  The comment you currently have here should go down where the timeout
actually is called.

[dhollowa, 2010-05-04 23:37:58]

http://codereview.chromium.org/1930002/diff/21001/2006
File chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm (right):

http://codereview.chromium.org/1930002/diff/21001/2006#newcode22
chrome/browser/autofill/autofill_dialog_controller_mac_unittest.mm:22: // This
delay allows the UI loop to run and display intermediate results.  Then
On 2010/05/04 23:28:47, James Hawkins wrote:
> This comment should be along the lines of "This is the timeout value for such
> and such".  The comment you currently have here should go down where the
timeout
> actually is called.

Done.