[Autofill] Credit Card Assist Infobar

Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When user accepts the prompt, the credit card form is filled with their credit card data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal): https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI_V2_Exploration/autofill-ahead-infobar#%2F06.png%3Fc=show

Video (public): https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest

Committed: https://crrev.com/f43ced4ebf836fa29f4d4fada46b6f9e924a8d6e
Cr-Commit-Position: refs/heads/master@{#409841}

[Mathieu, 2016-07-22 17:12:45]

Description was changed from

==========
Assist + Android

BUG=
==========

to

==========
Initial implementation of the credit card infobar assist.

Several big TODOs like opening a dialog to get the CVC (hoping to reuse
PaymentRequest code) as well as only proposing the assist when the form is in
the viewport.

BUG=630656
==========

[Mathieu, 2016-07-22 18:09:15]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2016-07-22 18:09:22]

Patchset #1 (id:1) has been deleted

[Mathieu, 2016-07-22 18:15:43]

Patchset #1 (id:40001) has been deleted

[Mathieu, 2016-07-25 13:58:44]

Description was changed from

==========
Initial implementation of the credit card infobar assist.

Several big TODOs like opening a dialog to get the CVC (hoping to reuse
PaymentRequest code) as well as only proposing the assist when the form is in
the viewport.

BUG=630656
==========

to

==========
Initial implementation of the credit card infobar assist.

Several big TODOs like opening a dialog to get the CVC (hoping to reuse
PaymentRequest code) as well as only proposing the assist when the form is in
the viewport (still determining product needs for this).

BUG=630656
==========

[Mathieu, 2016-07-26 20:25:42]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 20:26:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-07-26 20:52:38]

Patchset #2 (id:80001) has been deleted

[Mathieu, 2016-07-26 21:00:05]

mathp@chromium.org changed reviewers:
+ estade@chromium.org, rouslan@chromium.org

[Mathieu, 2016-07-26 21:00:06]

rouslan: Having done the Save Card infobar, would like if you could be main
reviewer for this. Let me know if you'd prefer someone else to do it.

estade: please review for chrome/browser/ui/autofill

Thanks

[Mathieu, 2016-07-27 15:36:41]

On 2016/07/26 21:00:06, Mathieu Perreault wrote:
> rouslan: Having done the Save Card infobar, would like if you could be main
> reviewer for this. Let me know if you'd prefer someone else to do it.
> 
> estade: please review for chrome/browser/ui/autofill
> 
> Thanks

As discussed offline I've added a chrome://flags entry for this. Thanks

[Evan Stade, 2016-07-27 16:42:40]

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
File components/autofill/core/browser/assist_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:31: return false;
do you intend to implement this on desktop?

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:57: base::ASCIIToUTF16(""));
base::string16()

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:212: assist_manager_(new
AssistManager(this)),
does this need to be a unique ptr? Why not store by value?

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1789: if (!cards.empty() &&
!cards[0]->IsExpired(base::Time::Now())) {
nit: no curlies

[please use gerrit instead, 2016-07-27 17:02:26]

Reviewed a bunch of code, but have more to go. Will review when have time on
Friday.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java
(right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:16:
* An infobar for saving credit card information.
This comment is not correct.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:22:
public static class CardDetail {
Let's extract this into a CardDetail.java that can be shared between here and
utofillSaveCardInfoBar.java.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:55:
private final List<CardDetail> mCardDetails = new LinkedList<CardDetail>();
new ArrayList<>();

Android's LinkedLists are inefficient.

There's no need to state "CardDetail" twice.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:77:
* Creates an infobar for saving a credit card.
This comment is not correct.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:98:
* Adds information to the infobar about the credit card that will be saved.
This comment is not correct.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:115:
for (CardDetail detail : mCardDetails) {
Use integer loops, because they are more efficient. Range loops allocate one
iterator object for every iteration. Since mCardDetails is an ArrrayList, it
will be efficient to call mCardDetails.get(i). It might be clearer to do this
inside the loop:

CardDetail detail = mCardDetails.get(i);

Don't worry about allocating an extra object here. This line only creates a new
reference to an existing object, similar to pointers in C++.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:23:
std::unique_ptr<infobars::InfoBar> CreateAssistInfoBar(
Should we put a comment here akin to "Declared in
components/autofill/core/browser/autofill_assist_infobar_mobile.h"?

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:43:
autofill::AutofillAssistInfoBarDelegateMobile* delegate = GetAssistDelegate();
This function is called only once, so you can remove it and simply write:

autofill::AutofillAssistInfoBarDelegateMobile* delegate =
    static_cast<autofill::AutofillAssistInfoBarDelegateMobile*>(GetDelegate());

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:75:
AutofillAssistInfoBar::GetAssistDelegate() {
Can be removed.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.h:32: class AutofillProfile;
Not used anywhere.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.h:33: class CreditCard;
Here there is no need to forward-declare (or include) CreditCard, because it's
mentioned only in implementing the AutofillClient interface. It's the job of
AutofillClient interface to forward declare (or include) the types that it uses.
It should be clear to the reader of this source code that CreditCard on line 63
is forward-declared (or included) in AutofillClient, as stated on line 45.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:26: }
The order of methods here does not match the order of methods in the header
file.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:54: //
TODO(crbug.com/630656): Trigger CVC dialog flow for card filling.
Example of how to trigger the CVC dialog for your next patch.

https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager.h (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:44: AutofillManager*
autofill_manager_;  // Weakly owned.
s/Weakly owned/Weak/ --- because it's not owned.

Add a comment that this AutofillManager owns this AssistManager and is in charge
of deleting it.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager_unittest.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:48: }

}  // namespace

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:52: void SetUp()
override {
Gtests work like this:

running CanShowCreditCardAssist_FeatureOff:
  create AssistManagerTest
  call SetUp
  **** run the test code ****
  call TearDown
  destroy AssistManagerTest 

Therefore, setup code can be in either SetUp or in AssistManagerTest
constructor. It's more common in Chrome and recommended by Gtest to put
initialization code in the constructor, unless using the constructor does not
work for some reason.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:59: void TearDown()
override {
Redundant, because every test case destroys AssistManagerTest at the end.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:24:
AutofillAssistInfoBarDelegateMobile(const CreditCard& card,
The infobar will display only one credit card to fill?

[Mathieu, 2016-07-27 21:35:53]

Thanks to both of you. PTAL

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java
(right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:16:
* An infobar for saving credit card information.
On 2016/07/27 17:02:25, rouslan wrote:
> This comment is not correct.

Done. Sorry, so many files to keep track of :\

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:22:
public static class CardDetail {
On 2016/07/27 17:02:26, rouslan wrote:
> Let's extract this into a CardDetail.java that can be shared between here and
> utofillSaveCardInfoBar.java.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:55:
private final List<CardDetail> mCardDetails = new LinkedList<CardDetail>();
On 2016/07/27 17:02:26, rouslan wrote:
> new ArrayList<>();
> 
> Android's LinkedLists are inefficient.
> 
> There's no need to state "CardDetail" twice.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:77:
* Creates an infobar for saving a credit card.
On 2016/07/27 17:02:25, rouslan wrote:
> This comment is not correct.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:98:
* Adds information to the infobar about the credit card that will be saved.
On 2016/07/27 17:02:25, rouslan wrote:
> This comment is not correct.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:115:
for (CardDetail detail : mCardDetails) {
On 2016/07/27 17:02:26, rouslan wrote:
> Use integer loops, because they are more efficient. Range loops allocate one
> iterator object for every iteration. Since mCardDetails is an ArrrayList, it
> will be efficient to call mCardDetails.get(i). It might be clearer to do this
> inside the loop:
> 
> CardDetail detail = mCardDetails.get(i);
> 
> Don't worry about allocating an extra object here. This line only creates a
new
> reference to an existing object, similar to pointers in C++.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:23:
std::unique_ptr<infobars::InfoBar> CreateAssistInfoBar(
On 2016/07/27 17:02:26, rouslan wrote:
> Should we put a comment here akin to "Declared in
> components/autofill/core/browser/autofill_assist_infobar_mobile.h"?

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:43:
autofill::AutofillAssistInfoBarDelegateMobile* delegate = GetAssistDelegate();
On 2016/07/27 17:02:26, rouslan wrote:
> This function is called only once, so you can remove it and simply write:
> 
> autofill::AutofillAssistInfoBarDelegateMobile* delegate =
>    
static_cast<autofill::AutofillAssistInfoBarDelegateMobile*>(GetDelegate());

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:75:
AutofillAssistInfoBar::GetAssistDelegate() {
On 2016/07/27 17:02:26, rouslan wrote:
> Can be removed.

Acknowledged.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.h (right):

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.h:32: class AutofillProfile;
On 2016/07/27 17:02:26, rouslan wrote:
> Not used anywhere.

Done.

https://codereview.chromium.org/2026353002/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.h:33: class CreditCard;
On 2016/07/27 17:02:26, rouslan wrote:
> Here there is no need to forward-declare (or include) CreditCard, because it's
> mentioned only in implementing the AutofillClient interface. It's the job of
> AutofillClient interface to forward declare (or include) the types that it
uses.
> It should be clear to the reader of this source code that CreditCard on line
63
> is forward-declared (or included) in AutofillClient, as stated on line 45.

Done.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:26: }
On 2016/07/27 17:02:26, rouslan wrote:
> The order of methods here does not match the order of methods in the header
> file.

Good catch!

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:54: //
TODO(crbug.com/630656): Trigger CVC dialog flow for card filling.
On 2016/07/27 17:02:26, rouslan wrote:
> Example of how to trigger the CVC dialog for your next patch.
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...

Thanks!

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager.h (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:44: AutofillManager*
autofill_manager_;  // Weakly owned.
On 2016/07/27 17:02:26, rouslan wrote:
> s/Weakly owned/Weak/ --- because it's not owned.
> 
> Add a comment that this AutofillManager owns this AssistManager and is in
charge
> of deleting it.

Done.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/assist_manager_unittest.cc (right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:48: }
On 2016/07/27 17:02:26, rouslan wrote:
> 
> }  // namespace

Done.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:52: void SetUp()
override {
On 2016/07/27 17:02:26, rouslan wrote:
> Gtests work like this:
> 
> running CanShowCreditCardAssist_FeatureOff:
>   create AssistManagerTest
>   call SetUp
>   **** run the test code ****
>   call TearDown
>   destroy AssistManagerTest 
> 
> Therefore, setup code can be in either SetUp or in AssistManagerTest
> constructor. It's more common in Chrome and recommended by Gtest to put
> initialization code in the constructor, unless using the constructor does not
> work for some reason.
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

Done.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:59: void TearDown()
override {
On 2016/07/27 17:02:26, rouslan wrote:
> Redundant, because every test case destroys AssistManagerTest at the end.

Done.

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:24:
AutofillAssistInfoBarDelegateMobile(const CreditCard& card,
On 2016/07/27 17:02:26, rouslan wrote:
> The infobar will display only one credit card to fill?

Yes, for this patch. Mock calls for a "spinner" eventually where you pick a
card, similar to how you pick a language in the translate infobar.

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
File components/autofill/core/browser/assist_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:31: return false;
On 2016/07/27 16:42:40, Evan Stade wrote:
> do you intend to implement this on desktop?

Android first, desktop is currently a "maybe" according to PM.

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:57: base::ASCIIToUTF16(""));
On 2016/07/27 16:42:40, Evan Stade wrote:
> base::string16()

Done.

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:212: assist_manager_(new
AssistManager(this)),
On 2016/07/27 16:42:40, Evan Stade wrote:
> does this need to be a unique ptr? Why not store by value?

Done.

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1789: if (!cards.empty() &&
!cards[0]->IsExpired(base::Time::Now())) {
On 2016/07/27 16:42:40, Evan Stade wrote:
> nit: no curlies

Done.

[please use gerrit instead, 2016-07-29 16:33:20]

lgtm % a few comments and nits.

https://codereview.chromium.org/2026353002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java
(right):

https://codereview.chromium.org/2026353002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:82:
private final List<CardDetail> mCardDetails = new ArrayList<>();
thx! :-)

https://codereview.chromium.org/2026353002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:169:
for (int i = 0; i < mCardDetails.size(); i++) {
thx again! :-)

https://codereview.chromium.org/2026353002/diff/180001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1485:
FEATURE_VALUE_TYPE(autofill::kAutofillCreditCardAssist)},
I think recently the common practise has been to place kOsAndroid flags inside
of #if defined(OS_ANDROID). This reduces binary size slightly.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill.gy...
components/autofill.gypi:263:
'autofill/core/browser/autofill_assist_infobar_mobile.h',
These sources are not yet used on iOS. Is it wise to place them in OS=="ios"
block now?

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:161:
"autofill_assist_infobar_mobile.h",
Same comment about iOS.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/assist_manager_unittest.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:58: }
Nit: I have not tested this, but this should work as well.

1) Define the member variables on stack:

TestAutofillClient autofill_client_;
testing::NiceMock<TestAutofillDriver> autofill_driver_;
MockAutofillManager autofill_manager_;
AssistManager assist_manager_;

2) Construct them like so:

AssistManagerTest()
    : autofill_manager_(&autofill_driver_, &autofill_client_),
      assist_manager_(&autofill_manager_) {}

This might be more clear to read, though:

AssistManagerTest()
    : autofill_client_(),
      autofill_driver_(),
      autofill_manager_(&autofill_driver_, &autofill_client_),
      assist_manager_(&autofill_manager_),
      message_loop_() {}

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:44: class AutofillProfile;
Not used anywhere in this file.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:24: extern const
base::Feature kAutofillCreditCardAssist;
Sort alphabetically?

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1789: if (!cards.empty() &&
!cards[0]->IsExpired(base::Time::Now()))
Nit: Using .front() instead of [0] would be more idiomatic in this file, because
it uses .back() in several places.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1790:
assist_manager_.ShowAssistForCreditCard(*cards[0]);
Ditto

[Evan Stade, 2016-07-29 17:02:51]

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
File components/autofill/core/browser/assist_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/120001/components/autofill/co...
components/autofill/core/browser/assist_manager.cc:31: return false;
On 2016/07/27 21:35:53, Mathieu Perreault wrote:
> On 2016/07/27 16:42:40, Evan Stade wrote:
> > do you intend to implement this on desktop?
> 
> Android first, desktop is currently a "maybe" according to PM.

can you compile less of this (i.e. the entire class) on non-android then?

[Mathieu, 2016-07-29 20:19:16]

mathp@chromium.org changed reviewers:
+ dfalcantara@chromium.org, jdonnelly@chromium.org, rkaplow@chromium.org,
torne@chromium.org

[Mathieu, 2016-07-29 20:19:17]

Thanks!

Now only compiling on Android.

torne@chromium.org: Please review changes in webview

jdonnelly@chromium.org: Please review changes in iOS

dfalcantara@chromium.org: Please review changes in android UI

rkaplow@chromium.org: metrics

https://codereview.chromium.org/2026353002/diff/180001/chrome/browser/about_f...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/chrome/browser/about_f...
chrome/browser/about_flags.cc:1485:
FEATURE_VALUE_TYPE(autofill::kAutofillCreditCardAssist)},
On 2016/07/29 16:33:20, rouslan wrote:
> I think recently the common practise has been to place kOsAndroid flags inside
> of #if defined(OS_ANDROID). This reduces binary size slightly.

OK, also lumped Save Card in there.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill.gy...
components/autofill.gypi:263:
'autofill/core/browser/autofill_assist_infobar_mobile.h',
On 2016/07/29 16:33:20, rouslan wrote:
> These sources are not yet used on iOS. Is it wise to place them in OS=="ios"
> block now?

Moved to android only block along with assist_manager

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:161:
"autofill_assist_infobar_mobile.h",
On 2016/07/29 16:33:20, rouslan wrote:
> Same comment about iOS.

Done.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/assist_manager_unittest.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/assist_manager_unittest.cc:58: }
On 2016/07/29 16:33:20, rouslan wrote:
> Nit: I have not tested this, but this should work as well.
> 
> 1) Define the member variables on stack:
> 
> TestAutofillClient autofill_client_;
> testing::NiceMock<TestAutofillDriver> autofill_driver_;
> MockAutofillManager autofill_manager_;
> AssistManager assist_manager_;
> 
> 2) Construct them like so:
> 
> AssistManagerTest()
>     : autofill_manager_(&autofill_driver_, &autofill_client_),
>       assist_manager_(&autofill_manager_) {}
> 
> This might be more clear to read, though:
> 
> AssistManagerTest()
>     : autofill_client_(),
>       autofill_driver_(),
>       autofill_manager_(&autofill_driver_, &autofill_client_),
>       assist_manager_(&autofill_manager_),
>       message_loop_() {}

Done.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_client.h (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_client.h:44: class AutofillProfile;
On 2016/07/29 16:33:20, rouslan wrote:
> Not used anywhere in this file.

Done.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_experiments.h:24: extern const
base::Feature kAutofillCreditCardAssist;
On 2016/07/29 16:33:20, rouslan wrote:
> Sort alphabetically? 

Done.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1789: if (!cards.empty() &&
!cards[0]->IsExpired(base::Time::Now()))
On 2016/07/29 16:33:20, rouslan wrote:
> Nit: Using .front() instead of [0] would be more idiomatic in this file,
because
> it uses .back() in several places.

Done.

https://codereview.chromium.org/2026353002/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:1790:
assist_manager_.ShowAssistForCreditCard(*cards[0]);
On 2016/07/29 16:33:20, rouslan wrote:
> Ditto

Done.

[Mathieu, 2016-07-29 20:19:26]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 20:20:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-07-29 20:20:18]

mathp@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mathieu, 2016-07-29 20:20:19]

pkasting@chromium.org: Please review changes in components/infobars

[rkaplow, 2016-07-29 20:22:40]

lgtm

histogram lg

[commit-bot: I haz the power, 2016-07-29 20:34:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 20:34:20]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Justin Donnelly, 2016-07-29 20:57:47]

ios files lgtm

Can you add some more info to the bug and maybe a bit of info to a header file
or two explaining what this feature is and what it's intended to accomplish? I
couldn't actually figure out what it was until I asked rouslan. :) He explained
that it would come up before the user interacted with the form. (On page load?)
Is the idea that users abandon checkout pages before even attempting to focus
the credit card form so this will increase the overall autofill rate?

[Peter Kasting, 2016-07-29 21:26:34]

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.h (right):

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.h:19: // Android
implementation of the infobar for saving credit card information.
Nit: Prefer to give more detail about what this infobar is, assuming the reader
has no idea about any of it.  Something like (I'm making up details here,
replace with the real ones):

An infobar that prompts the user to save credit card information they've just
typed in, for later autofilling.  Appears after submitting a form containing
this information.  Shows the credit card name and number.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:15:
#include "ui/base/window_open_disposition.h"
You don't use this #include.  Check to see if there are other #includes you can
omit as well.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:28:
AutofillMetrics::INFOBAR_SHOWN);
This is a call that should probably be in the Create() function and be prefaced
on whether the AddInfoBar() call succeeds, since otherwise the infobar wasn't
actually shown.  Note that if the infobar was never shown, you probably don't
want to log INFOBAR_IGNORED either.

(This is one reason for Create() functions; as-is, you can't protect against,
say, someone just constructing an instance of this delegate and throwing it away
from corrupting your stats.)

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:58:
return true;
It's very unlikely that you want to override the base class implementation of
this method.  For example, this will mean the infobar hides if the user is
navigated to a different anchor within the page.

The base class already hides the infobar in navigations that feel like they're
"replacing the page content".  You probably just want that.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:74:
assist_callback_.Reset();
Why is this call needed?

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:22:
class AutofillAssistInfoBarDelegateMobile : public ConfirmInfoBarDelegate {
It's strange that the files, classes, and variables here mention "assist" on its
own, which is sort of meaningless (or redundant with the purpose of autofill),
but they don't mention credit cards, even though this is specific to credit card
data.

Consider explicitly mentioning credit cards in these, and also consider either
removing "assist" or replacing/augmenting it so it's clearer what kind of
assistance you mean and why that isn't covered by this being part of autofill.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:24:
AutofillAssistInfoBarDelegateMobile(const CreditCard& card,
Infobar delegates should not expose constructors to the public.

Instead, make this private, and provide a static Create method that takes an
InfoBarManager*, creates the infobar, and adds it to the manager.

Basically, most of what ChromeAutofillClient::ConfirmCreditCardFillAssist() does
should actually be in this create function.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:33:
// ConfirmInfoBarDelegate:
All this stuff should be private if possible.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:47:
// The callback to save credit card if the user accepts the infobar.
Nit: card -> card information?

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:56:
// The label for the card to show in the content of the infobar.
Nit: Comments like this just restate the variable name.  Instead explain what
the label contents are and (if necessary) when they're shown.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_mobile.h (right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_mobile.h:19:
std::unique_ptr<infobars::InfoBar> CreateAssistInfoBar(
Avoid forward-declaring functions that aren't actually defined in components/. 
Instead, prefer to put this on e.g. AutofillClient, so people using this
component are forced to implement it at compile time and don't get missing
symbols at link time.

See how the translate component handles translate infobar creation for example.

https://codereview.chromium.org/2026353002/diff/200001/components/infobars/co...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2026353002/diff/200001/components/infobars/co...
components/infobars/core/infobar_delegate.h:135:
AUTOFILL_CC_ASSIST_INFOBAR_DELEGATE = 65,
Is this used only on Android?  If so, suffix with _ANDROID.

Nit: Try to keep the name here matching your class and file names closely (so at
the moment, omit "CC" -- but see other comments)

[Torne, 2016-08-01 12:00:24]

android_webview lgtm

[Mathieu, 2016-08-01 18:59:37]

Description was changed from

==========
Initial implementation of the credit card infobar assist.

Several big TODOs like opening a dialog to get the CVC (hoping to reuse
PaymentRequest code) as well as only proposing the assist when the form is in
the viewport (still determining product needs for this).

BUG=630656
==========

to

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

BUG=630656
==========

[Mathieu, 2016-08-01 19:03:33]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

BUG=630656
==========

to

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

BUG=630656
==========

[gone, 2016-08-01 20:35:54]

Could you attach screenshots/mocks to the bug?  Hard to gauge what I'm looking
at.

https://chromiumcodereview.appspot.com/2026353002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java
(right):

https://chromiumcodereview.appspot.com/2026353002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:19:
private final long mNativeAutofillAssistInfoBar;
1) Are you planning on using this pointer at some point?
2) It should be cleared when the native side is destroyed.

https://chromiumcodereview.appspot.com/2026353002/diff/200001/chrome/browser/...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.cc (right):

https://chromiumcodereview.appspot.com/2026353002/diff/200001/chrome/browser/...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:36:
AutofillAssistInfoBar::~AutofillAssistInfoBar() {}
Would be good to add a JNI call to the java side to clear the pointer once the
native side is destroyed.

https://chromiumcodereview.appspot.com/2026353002/diff/200001/chrome/browser/...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:57:
java_bitmap.obj(), base::android::ConvertUTF16ToJavaString(
nit: Does it look cleaner if you move the ConvertUTF16ToJavaString call onto the
next line?  Gets you some symmetry with the other string conversions.

https://chromiumcodereview.appspot.com/2026353002/diff/200001/components/auto...
File components/autofill_strings.grdp (right):

https://chromiumcodereview.appspot.com/2026353002/diff/200001/components/auto...
components/autofill_strings.grdp:184: <message
name="IDS_AUTOFILL_CREDIT_CARD_ASSIST_INFOBAR_DENY" desc="Text to show for the
Autofill credit card assist infobar deny button.">
Can you just reuse IDS_NO_THANKS from generated_resources.grd?

[Mathieu, 2016-08-01 21:57:15]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

BUG=630656
==========

to

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
==========

[Mathieu, 2016-08-01 22:14:55]

Patchset #8 (id:220001) has been deleted

[Mathieu, 2016-08-01 22:15:16]

Patchset #8 (id:240001) has been deleted

[Mathieu, 2016-08-01 22:16:14]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 22:16:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-08-01 22:16:58]

Hi Peter and Dan, PTAL. 

Not too happy with adding components/infobars to android_webview DEPS. Can you
see a way around it?

Otherwise I think I covered your comments, thanks!

https://codereview.chromium.org/2026353002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java
(right):

https://codereview.chromium.org/2026353002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillAssistInfoBar.java:19:
private final long mNativeAutofillAssistInfoBar;
On 2016/08/01 20:35:54, dfalcantara wrote:
> 1) Are you planning on using this pointer at some point?
> 2) It should be cleared when the native side is destroyed.

Done.

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.cc (right):

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:36:
AutofillAssistInfoBar::~AutofillAssistInfoBar() {}
On 2016/08/01 20:35:54, dfalcantara wrote:
> Would be good to add a JNI call to the java side to clear the pointer once the
> native side is destroyed.

actually looks like the pointer wasn't being used at all.

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.cc:57:
java_bitmap.obj(), base::android::ConvertUTF16ToJavaString(
On 2016/08/01 20:35:54, dfalcantara wrote:
> nit: Does it look cleaner if you move the ConvertUTF16ToJavaString call onto
the
> next line?  Gets you some symmetry with the other string conversions.

Let's blame `git cl format' :)

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
File chrome/browser/ui/android/infobars/autofill_assist_infobar.h (right):

https://codereview.chromium.org/2026353002/diff/200001/chrome/browser/ui/andr...
chrome/browser/ui/android/infobars/autofill_assist_infobar.h:19: // Android
implementation of the infobar for saving credit card information.
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Nit: Prefer to give more detail about what this infobar is, assuming the
reader
> has no idea about any of it.  Something like (I'm making up details here,
> replace with the real ones):
> 
> An infobar that prompts the user to save credit card information they've just
> typed in, for later autofilling.  Appears after submitting a form containing
> this information.  Shows the credit card name and number.

Done.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc
(right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:15:
#include "ui/base/window_open_disposition.h"
On 2016/07/29 21:26:34, Peter Kasting wrote:
> You don't use this #include.  Check to see if there are other #includes you
can
> omit as well.

Done. Refined the includes, thanks.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:28:
AutofillMetrics::INFOBAR_SHOWN);
On 2016/07/29 21:26:34, Peter Kasting wrote:
> This is a call that should probably be in the Create() function and be
prefaced
> on whether the AddInfoBar() call succeeds, since otherwise the infobar wasn't
> actually shown.  Note that if the infobar was never shown, you probably don't
> want to log INFOBAR_IGNORED either.
> 
> (This is one reason for Create() functions; as-is, you can't protect against,
> say, someone just constructing an instance of this delegate and throwing it
away
> from corrupting your stats.)

Good catch! Now using a boolean to track whether the infobar was shown and only
logging if AddInfoBar was successful).

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:58:
return true;
On 2016/07/29 21:26:34, Peter Kasting wrote:
> It's very unlikely that you want to override the base class implementation of
> this method.  For example, this will mean the infobar hides if the user is
> navigated to a different anchor within the page.
> 
> The base class already hides the infobar in navigations that feel like they're
> "replacing the page content".  You probably just want that.

Base class implementation SGTM, thanks.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.cc:74:
assist_callback_.Reset();
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Why is this call needed?

Removed.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:22:
class AutofillAssistInfoBarDelegateMobile : public ConfirmInfoBarDelegate {
On 2016/07/29 21:26:34, Peter Kasting wrote:
> It's strange that the files, classes, and variables here mention "assist" on
its
> own, which is sort of meaningless (or redundant with the purpose of autofill),
> but they don't mention credit cards, even though this is specific to credit
card
> data.
> 
> Consider explicitly mentioning credit cards in these, and also consider either
> removing "assist" or replacing/augmenting it so it's clearer what kind of
> assistance you mean and why that isn't covered by this being part of autofill.

Renamed to AutofillCreditCardFillingInfoBar

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:24:
AutofillAssistInfoBarDelegateMobile(const CreditCard& card,
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Infobar delegates should not expose constructors to the public.
> 
> Instead, make this private, and provide a static Create method that takes an
> InfoBarManager*, creates the infobar, and adds it to the manager.
> 
> Basically, most of what ChromeAutofillClient::ConfirmCreditCardFillAssist()
does
> should actually be in this create function.

Done.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:33:
// ConfirmInfoBarDelegate:
On 2016/07/29 21:26:34, Peter Kasting wrote:
> All this stuff should be private if possible.

Done, moved some to private.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:47:
// The callback to save credit card if the user accepts the infobar.
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Nit: card -> card information?

Done.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_delegate_mobile.h:56:
// The label for the card to show in the content of the infobar.
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Nit: Comments like this just restate the variable name.  Instead explain what
> the label contents are and (if necessary) when they're shown.

Done.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
File components/autofill/core/browser/autofill_assist_infobar_mobile.h (right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill/co...
components/autofill/core/browser/autofill_assist_infobar_mobile.h:19:
std::unique_ptr<infobars::InfoBar> CreateAssistInfoBar(
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Avoid forward-declaring functions that aren't actually defined in components/.

> Instead, prefer to put this on e.g. AutofillClient, so people using this
> component are forced to implement it at compile time and don't get missing
> symbols at link time.
> 
> See how the translate component handles translate infobar creation for
example.

Done.

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
components/autofill_strings.grdp:184: <message
name="IDS_AUTOFILL_CREDIT_CARD_ASSIST_INFOBAR_DENY" desc="Text to show for the
Autofill credit card assist infobar deny button.">
On 2016/08/01 20:35:54, dfalcantara wrote:
> Can you just reuse IDS_NO_THANKS from generated_resources.grd?

No because I need to access this from components and IDS_NO_THANKS is in
chrome/. However I renamed and reused the string that was used in the Autofill
Save Card infobar.

https://codereview.chromium.org/2026353002/diff/200001/components/infobars/co...
File components/infobars/core/infobar_delegate.h (right):

https://codereview.chromium.org/2026353002/diff/200001/components/infobars/co...
components/infobars/core/infobar_delegate.h:135:
AUTOFILL_CC_ASSIST_INFOBAR_DELEGATE = 65,
On 2016/07/29 21:26:34, Peter Kasting wrote:
> Is this used only on Android?  If so, suffix with _ANDROID.
> 
> Nit: Try to keep the name here matching your class and file names closely (so
at
> the moment, omit "CC" -- but see other comments)

Done.

[Mathieu, 2016-08-01 22:23:20]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
==========

to

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

[commit-bot: I haz the power, 2016-08-01 22:24:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 22:24:38]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mathieu, 2016-08-01 22:26:44]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[Mathieu, 2016-08-01 22:27:01]

Patchset #8 (id:260001) has been deleted

[commit-bot: I haz the power, 2016-08-01 22:27:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-08-01 22:33:54]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 22:34:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 22:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 22:46:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-08-02 00:30:06]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar assist.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

to

==========
Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

[Mathieu, 2016-08-02 00:49:06]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 00:49:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 01:09:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 01:09:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mathieu, 2016-08-02 01:45:40]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 01:45:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 02:37:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 02:37:18]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Mathieu, 2016-08-02 03:03:06]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 03:03:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 04:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 04:16:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2016-08-02 13:46:41]

https://codereview.chromium.org/2026353002/diff/360001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2026353002/diff/360001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:221:
AutofillCreditCardFillingInfoBarDelegateMobile::Create(
is it possible to avoid adding CreateCreditCardFillingInfoBar() to the
interface? It seems a little like a game of hot potato the way you're passing
card and callback back and forth, plus desktop is never going to want an infobar
(I assume it will use some other UI surface like a bubble). I'm ok with a
generic thing like ConfirmCCFillAssist but CreateInfoBar is a bit specific.

Perhaps the easiest way to fix this would be to move the body of
AutofillCreditCardFillingInfoBarDelegateMobile::Create() here.

[Mathieu, 2016-08-02 15:19:33]

Patchset #13 (id:380001) has been deleted

[Mathieu, 2016-08-02 15:25:10]

Patchset #13 (id:400001) has been deleted

[Mathieu, 2016-08-02 15:26:31]

On 2016/08/02 13:46:41, Evan Stade (sorta ooo) wrote:
>
https://codereview.chromium.org/2026353002/diff/360001/chrome/browser/ui/auto...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):
> 
>
https://codereview.chromium.org/2026353002/diff/360001/chrome/browser/ui/auto...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:221:
> AutofillCreditCardFillingInfoBarDelegateMobile::Create(
> is it possible to avoid adding CreateCreditCardFillingInfoBar() to the
> interface? It seems a little like a game of hot potato the way you're passing
> card and callback back and forth, plus desktop is never going to want an
infobar
> (I assume it will use some other UI surface like a bubble). I'm ok with a
> generic thing like ConfirmCCFillAssist but CreateInfoBar is a bit specific.
> 
> Perhaps the easiest way to fix this would be to move the body of
> AutofillCreditCardFillingInfoBarDelegateMobile::Create() here.

Agreed. I moved the logic to ChromeAutofillClient. It makes little sense to
clutter the interface and I can avoid adding components/infobars to the DEPS of
autofill_client implementers.

Cleaner now, PTAL

[Mathieu, 2016-08-02 21:53:11]

ping pkasting@, thanks

[Peter Kasting, 2016-08-03 02:25:41]

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
components/autofill_strings.grdp:184: <message
name="IDS_AUTOFILL_CREDIT_CARD_ASSIST_INFOBAR_DENY" desc="Text to show for the
Autofill credit card assist infobar deny button.">
On 2016/08/01 22:16:57, Mathieu Perreault wrote:
> On 2016/08/01 20:35:54, dfalcantara wrote:
> > Can you just reuse IDS_NO_THANKS from generated_resources.grd?
> 
> No because I need to access this from components and IDS_NO_THANKS is in
> chrome/.

(If it makes sense you can always move strings from chrome to components. 
There's a section for "generic strings" like OK, Not Now, etc.)

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
File
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc
(right):

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc:18:
// In order to use protected methods.
Nit: If you need to declare a subclass just to expose these methods, then just
make those methods public on the base class.

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc:52:
AutofillCreditCardFillingInfoBarDelegateMobileTest::CreateDelegate() {
Nit: If you manage to return to a Create()-based API (see other comments), this
can be eliminated in favor of just calling the real Create() function and having
it return the created delegate.  (A number of existing infobar delegates do
something like this already.)

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:219: #if
defined(OS_ANDROID)
If it's really true that desktop is "never going to want an infobar", as Evan
said, why is AutofillCreditCardFillingInfoBarDelegateMobile even defined in
components/ to begin with?  Why isn't it defined on the embedder side, in
Android-specific files?  This would allow you to implement the Create() function
in the way I asked without having to make a complicated, multi-hop system of
calls back and forth across the component interface, and without having to add
things like set_was_shown().

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:221: base::WrapUnique(new
AutofillCreditCardFillingInfoBar(
Nit: Use MakeUnique in newly-written code (WrapUnique is mostly for
auto-conversions).

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:229:
static_cast<AutofillCreditCardFillingInfoBarDelegateMobile*>(
Nit: Just save this raw pointer off when you first create the delegate, before
passing to the infobar:

  auto infobar_delegate = ...;
  auto* raw_delegate = infobar_delegate.get();
  ...;
  raw_delegate->set_was_shown(true);

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
File components/autofill/core/browser/assist_manager.h (right):

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:22: // assisted filling
mechanisms.
Is this going to be "assisting" lots more things than credit cards?  I still
have the reservations about these file/class names ("assist" is a really vague
word and the meaning is non-obvious, especially compared with "autofill"); you
fixed some of the infobar-specific ones, but ones like this are still
problematic.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:42: // Callback to the user
accepting the infobar.
Nit: The grammar of this sentence is confusing, especially due to the use of
present tense (is the callback happening while the user is still "accepting",
and if so, why is the function name "did accept"?).  Something like "Called by
the infobar delegate when the user accepts the infobar" would be clearer.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
File
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h:37:
void set_was_shown(bool was_shown) { was_shown_ = was_shown; }
Nit: It makes no sense to support set_was_shown(false), and no one does that, so
just have this take no args.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h:43:
protected:
Nit: It looks like you only access about half of these from the test code; the
others should probably be private.

[Mathieu, 2016-08-03 18:32:20]

Patchset #14 (id:440001) has been deleted

[Mathieu, 2016-08-03 18:34:44]

Hi Peter, Evan, PTAL

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2026353002/diff/200001/components/autofill_st...
components/autofill_strings.grdp:184: <message
name="IDS_AUTOFILL_CREDIT_CARD_ASSIST_INFOBAR_DENY" desc="Text to show for the
Autofill credit card assist infobar deny button.">
On 2016/08/03 02:25:40, Peter Kasting wrote:
> On 2016/08/01 22:16:57, Mathieu Perreault wrote:
> > On 2016/08/01 20:35:54, dfalcantara wrote:
> > > Can you just reuse IDS_NO_THANKS from generated_resources.grd?
> > 
> > No because I need to access this from components and IDS_NO_THANKS is in
> > chrome/.
> 
> (If it makes sense you can always move strings from chrome to components. 
> There's a section for "generic strings" like OK, Not Now, etc.)

Actually IDS_NO_THANKS was not used in chrome strings so I removed that in
another CL (https://codereview.chromium.org/2210513002/)

Introduced IDS_NO_THANKS in components_strings.grd in the generic section
(thanks!)

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
File
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc
(right):

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc:18:
// In order to use protected methods.
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: If you need to declare a subclass just to expose these methods, then just
> make those methods public on the base class.

Done.

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/autofil...
chrome/browser/autofill/autofill_credit_card_filling_infobar_delegate_mobile_unittest.cc:52:
AutofillCreditCardFillingInfoBarDelegateMobileTest::CreateDelegate() {
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: If you manage to return to a Create()-based API (see other comments),
this
> can be eliminated in favor of just calling the real Create() function and
having
> it return the created delegate.  (A number of existing infobar delegates do
> something like this already.)

Acknowledged.

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:219: #if
defined(OS_ANDROID)
On 2016/08/03 02:25:40, Peter Kasting wrote:
> If it's really true that desktop is "never going to want an infobar", as Evan
> said, why is AutofillCreditCardFillingInfoBarDelegateMobile even defined in
> components/ to begin with?  Why isn't it defined on the embedder side, in
> Android-specific files?  This would allow you to implement the Create()
function
> in the way I asked without having to make a complicated, multi-hop system of
> calls back and forth across the component interface, and without having to add
> things like set_was_shown().

I would like to keep the delegate inside components/ because it will be reused
for iOS, similar to how the "save card" infobar delegate is reused across mobile
platforms (with the platform-specific infobar instantiation code in their
appropriate directories). I think this way is good.

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:221: base::WrapUnique(new
AutofillCreditCardFillingInfoBar(
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: Use MakeUnique in newly-written code (WrapUnique is mostly for
> auto-conversions).

Done.

https://codereview.chromium.org/2026353002/diff/420001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:229:
static_cast<AutofillCreditCardFillingInfoBarDelegateMobile*>(
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: Just save this raw pointer off when you first create the delegate, before
> passing to the infobar:
> 
>   auto infobar_delegate = ...;
>   auto* raw_delegate = infobar_delegate.get();
>   ...;
>   raw_delegate->set_was_shown(true);

Done.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
File components/autofill/core/browser/assist_manager.h (right):

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:22: // assisted filling
mechanisms.
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Is this going to be "assisting" lots more things than credit cards?  I still
> have the reservations about these file/class names ("assist" is a really vague
> word and the meaning is non-obvious, especially compared with "autofill"); you
> fixed some of the infobar-specific ones, but ones like this are still
> problematic.

Yes it will be an assistant for other things in the future like address forms.
It's an assistant to "autofill" that prompts you and would guide you through the
process so I've changed it to autofill_assistant which is less vague.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/assist_manager.h:42: // Callback to the user
accepting the infobar.
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: The grammar of this sentence is confusing, especially due to the use of
> present tense (is the callback happening while the user is still "accepting",
> and if so, why is the function name "did accept"?).  Something like "Called by
> the infobar delegate when the user accepts the infobar" would be clearer.

Done.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
File
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h
(right):

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h:37:
void set_was_shown(bool was_shown) { was_shown_ = was_shown; }
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: It makes no sense to support set_was_shown(false), and no one does that,
so
> just have this take no args.

Done.

https://codereview.chromium.org/2026353002/diff/420001/components/autofill/co...
components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h:43:
protected:
On 2016/08/03 02:25:40, Peter Kasting wrote:
> Nit: It looks like you only access about half of these from the test code; the
> others should probably be private.

Done.

[gone, 2016-08-03 18:39:57]

Android stuff looks like it's hooked up right, at least, so LGTM.

[Peter Kasting, 2016-08-04 00:51:41]

lgtm

https://chromiumcodereview.appspot.com/2026353002/diff/460001/chrome/browser/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://chromiumcodereview.appspot.com/2026353002/diff/460001/chrome/browser/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:232:
raw_delegate->set_was_shown();
Nit: Can just inline if you want:

    if (InfoBarService::FromWebContents(web_contents())->AddInfoBar(
            std::move(infobar)))
      raw_delegate->set_was_shown();

[Mathieu, 2016-08-04 01:31:50]

Thanks, Evan PTAL

https://chromiumcodereview.appspot.com/2026353002/diff/460001/chrome/browser/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://chromiumcodereview.appspot.com/2026353002/diff/460001/chrome/browser/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:232:
raw_delegate->set_was_shown();
On 2016/08/04 00:51:41, Peter Kasting wrote:
> Nit: Can just inline if you want:
> 
>     if (InfoBarService::FromWebContents(web_contents())->AddInfoBar(
>             std::move(infobar)))
>       raw_delegate->set_was_shown();

Done, thanks

[Mathieu, 2016-08-04 01:36:41]

mathp@chromium.org changed reviewers:
+ caitkp@chromium.org

[Mathieu, 2016-08-04 01:36:42]

caitkp@chromium.org: Please review changes in components_strings.grd

[Mathieu, 2016-08-04 01:36:59]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 01:37:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 01:41:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 01:41:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mathieu, 2016-08-04 02:17:16]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 02:17:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 02:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 02:21:00]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2016-08-04 13:01:49]

my files lgtm

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:226:
std::move(infobar_delegate)));
nit: all these std:: and base:: and autos make this hard to read imo. Seems like
there should be a way to reduce verbosity.

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:228: std::move(infobar)))
nit: curlies

[Cait (Slow), 2016-08-04 14:30:59]

components_strings lgtm

[Mathieu, 2016-08-04 15:28:08]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 15:28:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2016-08-04 15:46:13]

Thanks, will submit

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (right):

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:226:
std::move(infobar_delegate)));
On 2016/08/04 13:01:48, Evan Stade (sorta ooo) wrote:
> nit: all these std:: and base:: and autos make this hard to read imo. Seems
like
> there should be a way to reduce verbosity.

got rid of one std::move and std::unique_ptr<..> by inlining.

https://codereview.chromium.org/2026353002/diff/500001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/chrome_autofill_client.cc:228: std::move(infobar)))
On 2016/08/04 13:01:48, Evan Stade (sorta ooo) wrote:
> nit: curlies

Done.

[Mathieu, 2016-08-04 15:59:01]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2016-08-04 15:59:02]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-08-04 15:59:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, torne@chromium.org, rkaplow@chromium.org,
jdonnelly@chromium.org, pkasting@chromium.org, dfalcantara@chromium.org,
estade@chromium.org, caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/2026353002/#ps540001
(title: "cleaning")

[commit-bot: I haz the power, 2016-08-04 15:59:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 17:09:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 17:09:43]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-08-04 17:36:11]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2016-08-04 17:36:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 18:28:47]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

to

==========
Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

[commit-bot: I haz the power, 2016-08-04 18:28:49]

Committed patchset #18 (id:540001)

[commit-bot: I haz the power, 2016-08-04 18:30:18]

Description was changed from

==========
Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest
==========

to

==========
Initial implementation of the credit card assisted filling infobar.

On page load, this will prompt the user for automatic credit card filling. When
user accepts the prompt, the credit card form is filled with their credit card
data.

Several big TODOs:
* Opening a dialog to get the CVC (hoping to reuse PaymentRequest code)
* Only proposing the assist when the form is in the viewport.
* Offering more than 1 card in the infobar.

Mocks (internal):
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI...

Video (public):
https://drive.google.com/file/d/0B3xzZ-vFr2LRMW1yUkZDYWR4Um8/view?usp=sharing

BUG=630656
TEST=AssistManagerTest,AutofillCreditCardFillingInfoBarDelegateMobileTest

Committed: https://crrev.com/f43ced4ebf836fa29f4d4fada46b6f9e924a8d6e
Cr-Commit-Position: refs/heads/master@{#409841}
==========

[commit-bot: I haz the power, 2016-08-04 18:30:21]

Patchset 18 (id:??) landed as
https://crrev.com/f43ced4ebf836fa29f4d4fada46b6f9e924a8d6e
Cr-Commit-Position: refs/heads/master@{#409841}