Make autofill stop depending on InfoBarService

Make autofill stop depending on InfoBarService

The goal is AutofillCCInfoBarDelegate will stay in c/b/autofill, while most of
autofill code move to components/autofill


BUG=140037
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186516

[kaiwang, 2013-03-02 02:31:34]

Please lemme know whether you feel this approach looks good :)

[Jói, 2013-03-02 16:21:03]

The approach looks good.  Perhaps you could update the DEPS file to disallow use
of chrome/browser/api/infobars except for the files that still need it (via
specific_include_rules)?

Cheers,
Jói

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_manager_delegate.h:91:
scoped_ptr<AutofillCCImportConfirmationDelegate> delegate) = 0;
Perhaps add something like this to the documentation: "Ownership of delegate is
passed to the method."

[Ilya Sherman, 2013-03-04 21:07:34]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
I don't think this interface is really necessary to separate out from the
implementation class.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:27: virtual
void DidShowConfirmation() OVERRIDE;
nit: IMO it would be fine to go with the shorter name "DidShow()"

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:29: virtual
void DidDeny() OVERRIDE;
nit: IMO DidDismiss() would be a better name for this.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:31: virtual
void DidIgnore() OVERRIDE;
Rather than DidIgnore(), define DidClose().

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:62: bool
had_user_interaction_;
IMO you should move this into the delegate as well.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_manager_delegate.h:89: // inforbar, etc...).
nit: "inforbar" -> "infobar"

[kaiwang, 2013-03-05 01:48:56]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
I think there are 2 benefits to define such an interface
1. The engineers working on UI don't need to care about the impl detail about
this delegate. Looking at such clean interface is easier for them.
2. Impl of this interface (AutofillCCImportConfirmationHandler) may have other
public functions added later to interact with other part of autofill. UI code
should not know those functions.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:27: virtual
void DidShowConfirmation() OVERRIDE;
On 2013/03/04 21:07:34, Ilya Sherman wrote:
> nit: IMO it would be fine to go with the shorter name "DidShow()"

Done.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:29: virtual
void DidDeny() OVERRIDE;
On 2013/03/04 21:07:34, Ilya Sherman wrote:
> nit: IMO DidDismiss() would be a better name for this.

Done.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:31: virtual
void DidIgnore() OVERRIDE;
I think it's more cleaner to just move the logic to destructor, if
AutofillCCInfoBarDelegate does not care about "ignore"

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:62: bool
had_user_interaction_;
On 2013/03/04 21:07:34, Ilya Sherman wrote:
> IMO you should move this into the delegate as well.

Done.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_manager_delegate.h:89: // inforbar, etc...).
On 2013/03/04 21:07:34, Ilya Sherman wrote:
> nit: "inforbar" -> "infobar"

Done.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_manager_delegate.h:91:
scoped_ptr<AutofillCCImportConfirmationDelegate> delegate) = 0;
On 2013/03/02 16:21:04, Jói wrote:
> Perhaps add something like this to the documentation: "Ownership of delegate
is
> passed to the method."

Done.

[Ilya Sherman, 2013-03-05 02:50:38]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
On 2013/03/05 01:48:56, kaiwang wrote:
> I think there are 2 benefits to define such an interface
> 1. The engineers working on UI don't need to care about the impl detail about
> this delegate. Looking at such clean interface is easier for them.

I rather disagree.  About 50% of the time when I look at a class's API, I
double-check the implementation file to make sure I'm understanding what the
code does.  Separating the header files just makes this a little harder.  IMO
what's helpful is having a small public API, which is the case other way.

> 2. Impl of this interface (AutofillCCImportConfirmationHandler) may have other
> public functions added later to interact with other part of autofill. UI code
> should not know those functions.

We can cross that bridge when we come to it.  Right now that's not a concern.

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:31: virtual
void DidIgnore() OVERRIDE;
On 2013/03/05 01:48:56, kaiwang wrote:
> I think it's more cleaner to just move the logic to destructor, if
> AutofillCCInfoBarDelegate does not care about "ignore"

Yeah, that's fine :)

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc (right):

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:22: if
(!had_user_interaction_)
nit: Curly braces

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:28: LOG(INFO)
<< "DidShow";
nit: Remove this.

[kaiwang, 2013-03-05 03:57:22]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
Seems the only CONs is about code reading. I really think the PROs it gains are
more important.
IMO, chromium code has many "unnecessary" interfaces. E.g. BrowserProcess vs
BrowserProcessImpl, TaskRunner vs MessageLoop, etc. Even in autofill, there are
AutofillPopupDelegate, AutofillDialogView, etc

I do feel harder to read code in this way. But the good thing is they helps a
lot to decouple components and prevent future dependency problems. For this CL,
I'm trying to  decouple components/autofill and chrome/browser/autofill.

With an interface, it's also very easy to write a mock and do testing, while
mocking an concrete class is always hard and easy to cause further problems (any
changes to the class  may break testing).

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc (right):

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:22: if
(!had_user_interaction_)
On 2013/03/05 02:50:39, Ilya Sherman wrote:
> nit: Curly braces

Done.

https://codereview.chromium.org/12378055/diff/12001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:28: LOG(INFO)
<< "DidShow";
On 2013/03/05 02:50:39, Ilya Sherman wrote:
> nit: Remove this.

Done.

[Ilya Sherman, 2013-03-05 04:37:39]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
On 2013/03/05 03:57:23, kaiwang wrote:
> Seems the only CONs is about code reading. I really think the PROs it gains
are
> more important.
> IMO, chromium code has many "unnecessary" interfaces. E.g. BrowserProcess vs
> BrowserProcessImpl, TaskRunner vs MessageLoop, etc. Even in autofill, there
are
> AutofillPopupDelegate, AutofillDialogView, etc

I'm less familiar with the history of the non-Autofill interfaces, but the
Autofill ones all serve some specific purpose.  The two you listed both exist
because they are implemented by multiple classes each.

> I do feel harder to read code in this way. But the good thing is they helps a
> lot to decouple components and prevent future dependency problems. For this
CL,
> I'm trying to  decouple components/autofill and chrome/browser/autofill.

Could you explain how the pure interface helps decouple these components, vs.
combining this with the single class that implements the interface?  Maybe I'm
not following what the game plan is...  Is the plan to export this interface
from the component, while keeping the implementation class private to the
component?

> With an interface, it's also very easy to write a mock and do testing, while
> mocking an concrete class is always hard and easy to cause further problems
(any
> changes to the class  may break testing).

You can use exactly the same mock code to mock the concrete class as you would
to mock out this interface, because the public interfaces of the two classes are
identical.

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:22: if
(!had_user_interaction_)
nit: Still missing curly braces...

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.h (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:17: class
AutofillCCImportConfirmationHandler
To potentially save a round-trip in reviewing: If you keep this class distinct
from AutofillCCImportConfirmationDelegate, please name it
AutofillCCImportConfirmationDelegateImpl.

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:32: // Only for unit
tests.
Why did you remove the #if defined(UNIT_TEST)?  Preferably, keep that syntax. 
If that's not possible, name this something like CreateForTesting().

[kaiwang, 2013-03-05 18:38:16]

BTW, do you think I should rename the metrics names? e.g.
INFOBAR_SHOWN -> AUTOFILL_CC_CONFIRMATION_SHOWN?

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
On 2013/03/05 04:37:39, Ilya Sherman wrote:
> On 2013/03/05 03:57:23, kaiwang wrote:
> > Seems the only CONs is about code reading. I really think the PROs it gains
> are
> > more important.
> > IMO, chromium code has many "unnecessary" interfaces. E.g. BrowserProcess vs
> > BrowserProcessImpl, TaskRunner vs MessageLoop, etc. Even in autofill, there
> are
> > AutofillPopupDelegate, AutofillDialogView, etc
> 
> I'm less familiar with the history of the non-Autofill interfaces, but the
> Autofill ones all serve some specific purpose.  The two you listed both exist
> because they are implemented by multiple classes each.

BrowserProcess has only BrowserProcessImpl and testing subclasses,right?

> 
> > I do feel harder to read code in this way. But the good thing is they helps
a
> > lot to decouple components and prevent future dependency problems. For this
> CL,
> > I'm trying to  decouple components/autofill and chrome/browser/autofill.
> 
> Could you explain how the pure interface helps decouple these components, vs.
> combining this with the single class that implements the interface?  Maybe I'm
> not following what the game plan is...  Is the plan to export this interface
> from the component, while keeping the implementation class private to the
> component?

By decoupling, I mean "depend as less as possible". Interface is less than a
concrete class :)
I think the further problem is as I mentioned, the concrete class may grow out
of our expectation. As the result, the other component will be actually
depending on more and more.

> 
> > With an interface, it's also very easy to write a mock and do testing, while
> > mocking an concrete class is always hard and easy to cause further problems
> (any
> > changes to the class  may break testing).
> 
> You can use exactly the same mock code to mock the concrete class as you would
> to mock out this interface, because the public interfaces of the two classes
are
> identical.

Yes, we can. Just when a concrete class grows larger, the mock will be hard to
write and understand. E.g. see the old AutofillExternalDelegate mocks I deleted
in https://chromiumcodereview.appspot.com/12340065/

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.cc:22: if
(!had_user_interaction_)
On 2013/03/05 04:37:39, Ilya Sherman wrote:
> nit: Still missing curly braces...

Oops, forgot to commit

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_import_confirmation_handler.h (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_import_confirmation_handler.h:17: class
AutofillCCImportConfirmationHandler
Sounds good, thanks!

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/9003/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:32: // Only for unit
tests.
Using the ifdef, the function implementation has to be in .h file.
I'm adding some code to this function, so I think it's better to keep .h file
clean and use the common "CreateForTesting" style.

[Ilya Sherman, 2013-03-05 22:56:57]

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h (right):

https://codereview.chromium.org/12378055/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h:21: };
On 2013/03/05 18:38:16, kaiwang wrote:
> On 2013/03/05 04:37:39, Ilya Sherman wrote:
> > On 2013/03/05 03:57:23, kaiwang wrote:
> > > Seems the only CONs is about code reading. I really think the PROs it
gains
> > are
> > > more important.
> > > IMO, chromium code has many "unnecessary" interfaces. E.g. BrowserProcess
vs
> > > BrowserProcessImpl, TaskRunner vs MessageLoop, etc. Even in autofill,
there
> > are
> > > AutofillPopupDelegate, AutofillDialogView, etc
> > 
> > I'm less familiar with the history of the non-Autofill interfaces, but the
> > Autofill ones all serve some specific purpose.  The two you listed both
exist
> > because they are implemented by multiple classes each.
> 
> BrowserProcess has only BrowserProcessImpl and testing subclasses,right?

Like I said, I'm not familiar with the design decisions for this interface. 
But, the BrowserProcess interface is pretty large on its own, and the
BrowserProcessImpl interface is also quite large.  That's pretty different from
the interface we're discussing here.

> > > I do feel harder to read code in this way. But the good thing is they
helps
> a
> > > lot to decouple components and prevent future dependency problems. For
this
> > CL,
> > > I'm trying to  decouple components/autofill and chrome/browser/autofill.
> > 
> > Could you explain how the pure interface helps decouple these components,
vs.
> > combining this with the single class that implements the interface?  Maybe
I'm
> > not following what the game plan is...  Is the plan to export this interface
> > from the component, while keeping the implementation class private to the
> > component?
> 
> By decoupling, I mean "depend as less as possible". Interface is less than a
> concrete class :)
> I think the further problem is as I mentioned, the concrete class may grow out
> of our expectation. As the result, the other component will be actually
> depending on more and more.

There are two reasons why I don't find this reasoning compelling in this case:
(1) This code is very simple, and not under development.  There's very little
reason to expect the API to change.
(2) This interface's single purpose *is* to glue together the two components. 
There's no reason to expect the Impl interface to change without corresponding
changes in the abstract interface.  Hence, even if the interface does change,
separating the two won't help keep the code less coupled.

If the primary motivation for separating out the interface is that the concrete
class *might* change in the future, then I strongly disagree that there is
compelling reason to keep the classes separate.

> > > With an interface, it's also very easy to write a mock and do testing,
while
> > > mocking an concrete class is always hard and easy to cause further
problems
> > (any
> > > changes to the class  may break testing).
> > 
> > You can use exactly the same mock code to mock the concrete class as you
would
> > to mock out this interface, because the public interfaces of the two classes
> are
> > identical.
> 
> Yes, we can. Just when a concrete class grows larger, the mock will be hard to
> write and understand. E.g. see the old AutofillExternalDelegate mocks I
deleted
> in https://chromiumcodereview.appspot.com/12340065/
>

[Ilya Sherman, 2013-03-05 22:59:00]

On 2013/03/05 18:38:16, kaiwang wrote:
> BTW, do you think I should rename the metrics names? e.g.
> INFOBAR_SHOWN -> AUTOFILL_CC_CONFIRMATION_SHOWN?

No, I don't think renaming the metrics is helpful.  Maybe that's an indication
that these metrics shouldn't be moved into the autofill component, and should
instead remain in chrome/browser/autofill.

[kaiwang, 2013-03-06 03:46:34]

Please take another look. THanks!

[Ilya Sherman, 2013-03-06 09:13:24]

LGTM with nits.  Thanks!

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_infobar_delegate.cc (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.cc:27:
metric_logger->LogCreditCardInfoBarMetric(AutofillMetrics::INFOBAR_SHOWN);
Hmm, why did you move this out of the constructor?

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:31: const base::Closure&
import_callback);
nit: import_callback -> save_card_callback or something like that

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:62: // The callback to do
credit card importing.
nit: "The callback to save the credit card if the user accepts the infobar." 
(In general, prefer "save" to "import" for this.)

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_delegate.h:87: virtual void
ConfirmImportCC(const AutofillMetrics& metric_logger,
nit: Please name this something closer to "ConfirmSaveCreditCard"

[kaiwang, 2013-03-06 18:25:04]

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_infobar_delegate.cc (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.cc:27:
metric_logger->LogCreditCardInfoBarMetric(AutofillMetrics::INFOBAR_SHOWN);
Creating an infobar delegate does not mean showing it. Just try to make it more
accurate.

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_cc_infobar_delegate.h (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:31: const base::Closure&
import_callback);
On 2013/03/06 09:13:24, Ilya Sherman wrote:
> nit: import_callback -> save_card_callback or something like that

Done.

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_cc_infobar_delegate.h:62: // The callback to do
credit card importing.
On 2013/03/06 09:13:24, Ilya Sherman wrote:
> nit: "The callback to save the credit card if the user accepts the infobar." 
> (In general, prefer "save" to "import" for this.)

Done.

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://codereview.chromium.org/12378055/diff/30002/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_delegate.h:87: virtual void
ConfirmImportCC(const AutofillMetrics& metric_logger,
On 2013/03/06 09:13:24, Ilya Sherman wrote:
> nit: Please name this something closer to "ConfirmSaveCreditCard"

Done.

[commit-bot: I haz the power, 2013-03-06 18:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12378055/38001

[commit-bot: I haz the power, 2013-03-06 21:47:26]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-03-06 21:53:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaiwang@chromium.org/12378055/38001

[commit-bot: I haz the power, 2013-03-06 22:07:20]

Change committed as 186516

[Justin Donnelly, 2016-03-09 03:33:34]

jdonnelly@chromium.org changed reviewers:
+ jdonnelly@chromium.org

[Justin Donnelly, 2016-03-09 03:33:35]

https://chromiumcodereview.appspot.com/12378055/diff/38001/chrome/browser/aut...
File chrome/browser/autofill/autofill_cc_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/12378055/diff/38001/chrome/browser/aut...
chrome/browser/autofill/autofill_cc_infobar_delegate.cc:94:
save_card_callback_.Reset();
Question arose about this arose in https://codereview.chromium.org/1780443002/.
What's the motivation for the reset here? If it's serving some useful purpose,
would it make sense to reset in Cancel() also?