Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
622773002:
[Autofill] Autofill fails to show suggestions for credit card split across fields. (Closed)
|
Created:
6 years, 2 months ago by Pritam Nikam Modified:
6 years, 1 month ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Autofill] Autofill fails to show suggestions for suggested input for forms having credit card number split across multiple fields.
BUG=420323
Committed: https://crrev.com/504d780c7e67390d85cc918a6c11617ddd36a495
Cr-Commit-Position: refs/heads/master@{#302703}
Patch Set 1 : Changes aligned with https://codereview.chromium.org/442403002/ #Patch Set 2 : Added handling for card number fields split across. #Patch Set 3 : Added unit-tests. #
Total comments: 13
Patch Set 4 : Incorporate review comments. #Patch Set 5 : #
Total comments: 16
Patch Set 6 : Incorporated review comments. #
Total comments: 2
Patch Set 7 : Removed ObfuscatedCreditCardNumber() from AutofillManager. #
Total comments: 10
Patch Set 8 : Removed unwanted changes and incorporated review comments. #
Total comments: 4
Patch Set 9 : Incorporated review comments. #
Total comments: 5
Patch Set 10 : Added review inputs. #Patch Set 11 : Modified test case. #
Total comments: 4
Patch Set 12 : Incorporated nits. #
Messages
Total messages: 31 (5 generated)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
pritam.nikam@samsung.com changed reviewers: + estade@chromium.org, isherman@chromium.org, ziran.sun@samsung.com
On 2014/10/07 04:20:42, Pritam Nikam wrote: > mailto:pritam.nikam@samsung.com changed reviewers: > + mailto:estade@chromium.org, mailto:isherman@chromium.org, mailto:ziran.sun@samsung.com Hi Ilya, Evan and Ziran, PTAL!
Thanks for the CL. Please add test coverage for the new public method exposed on the AutofillField class. https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:512: // The |field| specifies the |credit_card_number_offset_| to the substring nit: "credit_card_number_offset_" -> "credit_card_number_offset()" https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } Can all of this logic be moved into the PersonalDataManager class, as it is for phone numbers? https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:1181: &guid_pairs); nit: Please add curly braces, since the body spans multiple lines of text.
Thanks Ilya for your review inputs. I've incorporated comments and added a unit-test for AutofillField::GetCreditCardNumberValue(). PTAL. Thanks! https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:512: // The |field| specifies the |credit_card_number_offset_| to the substring On 2014/10/07 21:49:10, Ilya Sherman wrote: > nit: "credit_card_number_offset_" -> "credit_card_number_offset()" Done. https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } On 2014/10/07 21:49:10, Ilya Sherman wrote: > Can all of this logic be moved into the PersonalDataManager class, as it is for > phone numbers? In my opinion, we *cannot* move this logic to PersonalDataManager class straight away. Retrieving card number substring is poised on AutofillField::credit_card_number_offset(). Moreover, changing API signature of PersonalDataManager::GetCreditCardSuggestions() will apparently impact usages from other code excerpts [1] where passing AutofillField reference as argument won't be easy. And as I understand, phone number prefix/suffix suggestions handling logic is part of AutofillManager[2] as of now, and reason is the same mentioned [1]. [1]. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [2]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:1181: &guid_pairs); On 2014/10/07 21:49:10, Ilya Sherman wrote: > nit: Please add curly braces, since the body spans multiple lines of text. Done.
https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:478: field_data->value = AutofillField::GetCreditCardNumberValue(field, value); For consistency it might be worth keeping FillCreditCardNumberField() function, in which you can call GetCreditCardNumberValue().
https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:478: field_data->value = AutofillField::GetCreditCardNumberValue(field, value); On 2014/10/08 16:25:55, ziran.sun wrote: > For consistency it might be worth keeping FillCreditCardNumberField() function, > in which you can call GetCreditCardNumberValue(). Alternately, you could inline the FillPhoneNumberField implementation. I agree that the two should ideally be consistent. https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } On 2014/10/08 05:17:17, Pritam Nikam wrote: > On 2014/10/07 21:49:10, Ilya Sherman wrote: > > Can all of this logic be moved into the PersonalDataManager class, as it is > for > > phone numbers? > > In my opinion, we *cannot* move this logic to PersonalDataManager class straight > away. > Retrieving card number substring is poised on > AutofillField::credit_card_number_offset(). > > Moreover, changing API signature of > PersonalDataManager::GetCreditCardSuggestions() will apparently impact usages > from other code excerpts [1] where passing AutofillField reference as argument > won't be easy. > > And as I understand, phone number prefix/suffix suggestions handling logic is > part of AutofillManager[2] as of now, and reason is the same mentioned [1]. > > [1]. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > [2]. > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Phone numbers are also handled within the PersonalDataManager code [1]. The AutofillManager code that you linked to just adjusts what data will be filled; it's much simpler and clearer than the credit card code that you've added in this CL. I'd prefer to align the two implementations. [1] https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
Thanks Ilya and Ziran for your inputs. I'll upload a patch incorporating these inputs. Thanks! https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_field.cc:478: field_data->value = AutofillField::GetCreditCardNumberValue(field, value); On 2014/10/08 21:04:38, Ilya Sherman wrote: > On 2014/10/08 16:25:55, ziran.sun wrote: > > For consistency it might be worth keeping FillCreditCardNumberField() > function, > > in which you can call GetCreditCardNumberValue(). > > Alternately, you could inline the FillPhoneNumberField implementation. I agree > that the two should ideally be consistent. Done. https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } On 2014/10/08 21:04:38, Ilya Sherman wrote: > On 2014/10/08 05:17:17, Pritam Nikam wrote: > > On 2014/10/07 21:49:10, Ilya Sherman wrote: > > > Can all of this logic be moved into the PersonalDataManager class, as it is > > for > > > phone numbers? > > > > In my opinion, we *cannot* move this logic to PersonalDataManager class > straight > > away. > > Retrieving card number substring is poised on > > AutofillField::credit_card_number_offset(). > > > > Moreover, changing API signature of > > PersonalDataManager::GetCreditCardSuggestions() will apparently impact usages > > from other code excerpts [1] where passing AutofillField reference as argument > > won't be easy. > > > > And as I understand, phone number prefix/suffix suggestions handling logic is > > part of AutofillManager[2] as of now, and reason is the same mentioned [1]. > > > > [1]. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > [2]. > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > Phone numbers are also handled within the PersonalDataManager code [1]. The > AutofillManager code that you linked to just adjusts what data will be filled; > it's much simpler and clearer than the credit card code that you've added in > this CL. I'd prefer to align the two implementations. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Acknowledged. I'll align my changes along these logic i.e. sub-string match and upload patch for review. By I've query related to same, it's a sub-string matching instead of pre-fix matching. But don't we need field match as well? For instance, say we have 3 credit cards saved. /* Name On Card | Card Number | mm | yyyy */ A. {"Miku Hatsune", "4111-1111-1111-1111", "07", "2015"} B. {"Elvis Presley", "4231-2341-1111-1017", "04", "2012"} C. {"Buddy Holly", "5632-1111-4111-2341", "10", "2014"} If suppose, a HTML form having card number split across 4 input fields. And autofilled with card [A] i.e. cc-number "4111-1111-1111-1111". In my opinion, double click on 4'th cc-number input field may bring all 3 card suggestions, and user may be expecting to see only A'th card instead, OR am I missing something here?
Thanks. https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } On 2014/10/09 06:11:11, Pritam Nikam wrote: > On 2014/10/08 21:04:38, Ilya Sherman wrote: > > On 2014/10/08 05:17:17, Pritam Nikam wrote: > > > On 2014/10/07 21:49:10, Ilya Sherman wrote: > > > > Can all of this logic be moved into the PersonalDataManager class, as it > is > > > for > > > > phone numbers? > > > > > > In my opinion, we *cannot* move this logic to PersonalDataManager class > > straight > > > away. > > > Retrieving card number substring is poised on > > > AutofillField::credit_card_number_offset(). > > > > > > Moreover, changing API signature of > > > PersonalDataManager::GetCreditCardSuggestions() will apparently impact > usages > > > from other code excerpts [1] where passing AutofillField reference as > argument > > > won't be easy. > > > > > > And as I understand, phone number prefix/suffix suggestions handling logic > is > > > part of AutofillManager[2] as of now, and reason is the same mentioned [1]. > > > > > > [1]. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > [2]. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > > > Phone numbers are also handled within the PersonalDataManager code [1]. The > > AutofillManager code that you linked to just adjusts what data will be filled; > > it's much simpler and clearer than the credit card code that you've added in > > this CL. I'd prefer to align the two implementations. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > Acknowledged. > > I'll align my changes along these logic i.e. sub-string match and upload patch > for review. > > By I've query related to same, it's a sub-string matching instead of pre-fix > matching. But don't we need field match as well? > > For instance, say we have 3 credit cards saved. > /* Name On Card | Card Number | mm | yyyy */ > A. {"Miku Hatsune", "4111-1111-1111-1111", "07", "2015"} > B. {"Elvis Presley", "4231-2341-1111-1017", "04", "2012"} > C. {"Buddy Holly", "5632-1111-4111-2341", "10", "2014"} > > If suppose, a HTML form having card number split across 4 input fields. And > autofilled with card [A] i.e. cc-number "4111-1111-1111-1111". In my opinion, > double click on 4'th cc-number input field may bring all 3 card suggestions, and > user may be expecting to see only A'th card instead, OR am I missing something > here? That's true. It's such an edge case that I'm honestly not too worried about it -- most users will have at most one credit card saved, and most websites only have a single credit card field anyway. If you really want to handle this case correctly, I'd recommend (a) also improving the handling of phone numbers to match, and (b) extending the PersonalDataManager::GetCreditCardSuggestions() method with the necessary logic, possibly by passing an additional parameter for the current field value.
Thanks Ilya & Ziran for inputs. I've added a patch addressing prefix to sub-string matching for credit card number splits & inline the FillPhoneNumberField(). PTAL! Thanks! https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/80001/components/autofill/core... components/autofill/core/browser/autofill_manager.cc:199: } On 2014/10/09 21:35:12, Ilya Sherman wrote: > On 2014/10/09 06:11:11, Pritam Nikam wrote: > > On 2014/10/08 21:04:38, Ilya Sherman wrote: > > > On 2014/10/08 05:17:17, Pritam Nikam wrote: > > > > On 2014/10/07 21:49:10, Ilya Sherman wrote: > > > > > Can all of this logic be moved into the PersonalDataManager class, as it > > is > > > > for > > > > > phone numbers? > > > > > > > > In my opinion, we *cannot* move this logic to PersonalDataManager class > > > straight > > > > away. > > > > Retrieving card number substring is poised on > > > > AutofillField::credit_card_number_offset(). > > > > > > > > Moreover, changing API signature of > > > > PersonalDataManager::GetCreditCardSuggestions() will apparently impact > > usages > > > > from other code excerpts [1] where passing AutofillField reference as > > argument > > > > won't be easy. > > > > > > > > And as I understand, phone number prefix/suffix suggestions handling logic > > is > > > > part of AutofillManager[2] as of now, and reason is the same mentioned > [1]. > > > > > > > > [1]. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > [2]. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > > > > > Phone numbers are also handled within the PersonalDataManager code [1]. The > > > AutofillManager code that you linked to just adjusts what data will be > filled; > > > it's much simpler and clearer than the credit card code that you've added in > > > this CL. I'd prefer to align the two implementations. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > > > Acknowledged. > > > > I'll align my changes along these logic i.e. sub-string match and upload patch > > for review. > > > > By I've query related to same, it's a sub-string matching instead of pre-fix > > matching. But don't we need field match as well? > > > > For instance, say we have 3 credit cards saved. > > /* Name On Card | Card Number | mm | yyyy */ > > A. {"Miku Hatsune", "4111-1111-1111-1111", "07", "2015"} > > B. {"Elvis Presley", "4231-2341-1111-1017", "04", "2012"} > > C. {"Buddy Holly", "5632-1111-4111-2341", "10", "2014"} > > > > If suppose, a HTML form having card number split across 4 input fields. And > > autofilled with card [A] i.e. cc-number "4111-1111-1111-1111". In my opinion, > > double click on 4'th cc-number input field may bring all 3 card suggestions, > and > > user may be expecting to see only A'th card instead, OR am I missing something > > here? > > That's true. It's such an edge case that I'm honestly not too worried about it > -- most users will have at most one credit card saved, and most websites only > have a single credit card field anyway. If you really want to handle this case > correctly, I'd recommend (a) also improving the handling of phone numbers to > match, and (b) extending the PersonalDataManager::GetCreditCardSuggestions() > method with the necessary logic, possibly by passing an additional parameter for > the current field value. Currently, *current field value* (i.e. const base::string16& field_contents) is getting pass to this function and that seems not enough to get the exact suggestions for matching field input. I'll further investigate on these lines. Moreover, I've added a patch addressing your earlier suggestions i.e. sub-string instead of prefix match.
https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:628: size_t total_spilts = 4; nit: Please use the arraysize macro to compute this. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:641: // Enforce the field's |max_length| constrain. nit: "constrain" -> "constraint". Also, please mention that normally this would be handled by the renderer, which is why it needs to be specially handled in the unit test. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:647: } Please also test the behavior if the offset is equal to the credit card number in size, or larger than it. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:173: } Hmm, I think we should continue to show the full, obfuscated, credit card number in the popup. For example, if my card number is "4111 1111 1111 1111", we should still show "************1111" in the popup, even if I'm for some reason filling from credit_card_number_part3 field. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1167: guid_pairs.swap(suggetion_guid_pairs); Please use .erase() from the original vectors, rather than creating copies and then swapping in the contents. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1169: // Obfuscate the credit card number in suggetions. nit: "number" -> "numbers". https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1171: (*values)[i] = ObfuscatedCreditCardNumber((*values)[i]); nit: Please always use curly braces to contain loop bodies. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:679: if (match_found) { nit: Please write this more like how it was previously written: if (!creditcard_field_value.empty() && (StartsWith(creditcard_field_value, field_contents, false) || (type.GetStorableType() == CREDIT_CARD_NUMBER && base::string16::npos != creditcard_field_value.find(field_contents))))
pritam.nikam@samsung.com changed required reviewers: + isherman@chromium.org
Thanks Ilya for review. I've incorporated review inputs and uploaded a patch #6 PTAL. Thanks! https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:628: size_t total_spilts = 4; On 2014/10/13 23:33:37, Ilya Sherman wrote: > nit: Please use the arraysize macro to compute this. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:641: // Enforce the field's |max_length| constrain. On 2014/10/13 23:33:37, Ilya Sherman wrote: > nit: "constrain" -> "constraint". Also, please mention that normally this would > be handled by the renderer, which is why it needs to be specially handled in the > unit test. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:647: } On 2014/10/13 23:33:37, Ilya Sherman wrote: > Please also test the behavior if the offset is equal to the credit card number > in size, or larger than it. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:173: } On 2014/10/13 23:33:37, Ilya Sherman wrote: > Hmm, I think we should continue to show the full, obfuscated, credit card number > in the popup. For example, if my card number is "4111 1111 1111 1111", we > should still show "************1111" in the popup, even if I'm for some reason > filling from credit_card_number_part3 field. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1167: guid_pairs.swap(suggetion_guid_pairs); On 2014/10/13 23:33:37, Ilya Sherman wrote: > Please use .erase() from the original vectors, rather than creating copies and > then swapping in the contents. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1169: // Obfuscate the credit card number in suggetions. On 2014/10/13 23:33:37, Ilya Sherman wrote: > nit: "number" -> "numbers". Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1171: (*values)[i] = ObfuscatedCreditCardNumber((*values)[i]); On 2014/10/13 23:33:37, Ilya Sherman wrote: > nit: Please always use curly braces to contain loop bodies. Done. https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/622773002/diff/130001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:679: if (match_found) { On 2014/10/13 23:33:37, Ilya Sherman wrote: > nit: Please write this more like how it was previously written: > > if (!creditcard_field_value.empty() && > (StartsWith(creditcard_field_value, field_contents, false) || > (type.GetStorableType() == CREDIT_CARD_NUMBER && > base::string16::npos != creditcard_field_value.find(field_contents)))) Done.
https://codereview.chromium.org/622773002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:173: } Please remove this function. This work should be done by the PersonalDataManager.
Thanks Ilya for review. I've incorporated changes to patch set #7. PTAL. Thanks! https://codereview.chromium.org/622773002/diff/160001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/160001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:173: } On 2014/10/16 23:55:10, Ilya Sherman wrote: > Please remove this function. This work should be done by the > PersonalDataManager. Done.
Thanks. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:96: const base::string16& number); It looks like this change is no longer needed as part of this CL. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1129: const AutofillField& autofill_field, It looks like you can revert this change, since you're only using the field to extract the type. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3040: // getting populated correctly. nit: "getting" -> "are" https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3043: for (size_t i = starts_at; i < (starts_at + total_number_fields); ++i) { Please unroll this loop, so have four repeated sets of checks, rather than generalizing the structure to a loop. In tests, the exact number of iterations is known ahead of time, so there's no need for the expressive power of loops; and straight-line code makes the test expectations easier to read and understand. In fact, for this test, it seems fine to test filling from just one of the middle fields; there's no need to exhaustively test every possible starting point for filling. So, you can just write one set of checks. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3049: : number_field.value.size(); You shouldn't need to do this. Just check that the field starts with the expected substring, if that's all that the code guarantees.
Thanks Ilya for review. Removed unwanted changes from this CL & incorporated your inputs to patch set #8. PTAL. I've moved refactoring related changes to separate CL. [https://codereview.chromium.org/689573002/] Thanks! https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:96: const base::string16& number); On 2014/10/28 21:48:40, Ilya Sherman wrote: > It looks like this change is no longer needed as part of this CL. Done. I've created a follow-up CL for this code refactoring/cleanup. [https://codereview.chromium.org/689573002/] https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1129: const AutofillField& autofill_field, On 2014/10/28 21:48:40, Ilya Sherman wrote: > It looks like you can revert this change, since you're only using the field to > extract the type. Done. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3040: // getting populated correctly. On 2014/10/28 21:48:40, Ilya Sherman wrote: > nit: "getting" -> "are" Done. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3043: for (size_t i = starts_at; i < (starts_at + total_number_fields); ++i) { On 2014/10/28 21:48:40, Ilya Sherman wrote: > Please unroll this loop, so have four repeated sets of checks, rather than > generalizing the structure to a loop. In tests, the exact number of iterations > is known ahead of time, so there's no need for the expressive power of loops; > and straight-line code makes the test expectations easier to read and > understand. > > In fact, for this test, it seems fine to test filling from just one of the > middle fields; there's no need to exhaustively test every possible starting > point for filling. So, you can just write one set of checks. Done. https://codereview.chromium.org/622773002/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3049: : number_field.value.size(); On 2014/10/28 21:48:40, Ilya Sherman wrote: > You shouldn't need to do this. Just check that the field starts with the > expected substring, if that's all that the code guarantees. Done.
https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:240: } Please remove this helper function. https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3046: } Hmm, this test appears to test that if a suggestion is selected, then it's filled correctly. The comment above the test, however, indicates that what you wanted to test was that the suggestion is shown in the first place.
Thanks Ilya. PTAL at new patch set. Thanks! https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:240: } On 2014/10/30 01:00:58, Ilya Sherman wrote: > Please remove this helper function. Done. Merge within test |GetCreditCardSuggestionsForNumberSpitAcrossFields|. https://codereview.chromium.org/622773002/diff/200001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3046: } On 2014/10/30 01:00:58, Ilya Sherman wrote: > Hmm, this test appears to test that if a suggestion is selected, then it's > filled correctly. The comment above the test, however, indicates that what you > wanted to test was that the suggestion is shown in the first place. Done. I've restored the GetAutofillSuggestions() and rest of the code excerpt as it's required to test PersonalDataManager::GetCreditCardSuggestions(). Here, |autofill_values_| is kept private, hence testing it alone against suggestions(expected_values) wont be easy. Only way to test is using helper CheckSuggestions(), that requires expected labels, icons and ids to be supplied in addition.
https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2997: CreateTestCreditCardFormData(&form, true, false); Please remove this call as well, and instead construct the exact form you want for this test. That's clearer than constructing a form that's similar to what you want, and then erasing and inserting fields based on indices. https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3021: // Fill the credit card details. I'm still not sure why you're filling the form as part of this test. I'd prefer to have a more focused test, which just verifies that suggestions are shown correctly. If you also want to test filling, then please move that to a separate test (but I think we already have coverage for the filling case -- do we not?)
Thanks Ilya. I've incorporated review inputs with patch set #10, PTAL. Thanks! https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2997: CreateTestCreditCardFormData(&form, true, false); On 2014/10/30 22:58:55, Ilya Sherman wrote: > Please remove this call as well, and instead construct the exact form you want > for this test. That's clearer than constructing a form that's similar to what > you want, and then erasing and inserting fields based on indices. Done. https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3021: // Fill the credit card details. On 2014/10/30 22:58:55, Ilya Sherman wrote: > I'm still not sure why you're filling the form as part of this test. I'd prefer > to have a more focused test, which just verifies that suggestions are shown > correctly. If you also want to test filling, then please move that to a > separate test (but I think we already have coverage for the filling case -- do > we not?) Yes, you are right, we do have testcases for the filling case. Sorry if I didn't elaborate the problem correctly. The issue here (http://crbug.com/420323, snapshots attached for ref.), is to populate suggestions for one of the middle credit card number fields when autofilled already. If field is empty (not autofilled) then there is no problem to get the autofill suggestions. I would say filled card number field is the per-condition here. Thats the reason the test needs field to be filled first, and the try to populate the suggestions. In other words, without this patch (that is sub-string matching instead of pre-fix matching for card number field), this test shall fail.
https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3021: // Fill the credit card details. On 2014/10/31 06:59:33, Pritam Nikam wrote: > On 2014/10/30 22:58:55, Ilya Sherman wrote: > > I'm still not sure why you're filling the form as part of this test. I'd > prefer > > to have a more focused test, which just verifies that suggestions are shown > > correctly. If you also want to test filling, then please move that to a > > separate test (but I think we already have coverage for the filling case -- do > > we not?) > > Yes, you are right, we do have testcases for the filling case. > > Sorry if I didn't elaborate the problem correctly. The issue here > (http://crbug.com/420323, snapshots attached for ref.), is to populate > suggestions for one of the middle credit card number fields when autofilled > already. If field is empty (not autofilled) then there is no problem to get the > autofill suggestions. I would say filled card number field is the per-condition > here. > > Thats the reason the test needs field to be filled first, and the try to > populate the suggestions. > > In other words, without this patch (that is sub-string matching instead of > pre-fix matching for card number field), this test shall fail. Hmm, do we really need the field to be autofilled? I thought we just needed the field to have a (partial) value entered. Am I misunderstanding what bug you are trying to fix?
On 2014/10/31 22:49:59, Ilya Sherman wrote: > https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... > File components/autofill/core/browser/autofill_manager_unittest.cc (right): > > https://codereview.chromium.org/622773002/diff/220001/components/autofill/cor... > components/autofill/core/browser/autofill_manager_unittest.cc:3021: // Fill the > credit card details. > On 2014/10/31 06:59:33, Pritam Nikam wrote: > > On 2014/10/30 22:58:55, Ilya Sherman wrote: > > > I'm still not sure why you're filling the form as part of this test. I'd > > prefer > > > to have a more focused test, which just verifies that suggestions are shown > > > correctly. If you also want to test filling, then please move that to a > > > separate test (but I think we already have coverage for the filling case -- > do > > > we not?) > > > > Yes, you are right, we do have testcases for the filling case. > > > > Sorry if I didn't elaborate the problem correctly. The issue here > > (http://crbug.com/420323, snapshots attached for ref.), is to populate > > suggestions for one of the middle credit card number fields when autofilled > > already. If field is empty (not autofilled) then there is no problem to get > the > > autofill suggestions. I would say filled card number field is the > per-condition > > here. > > > > Thats the reason the test needs field to be filled first, and the try to > > populate the suggestions. > > > > In other words, without this patch (that is sub-string matching instead of > > pre-fix matching for card number field), this test shall fail. > > Hmm, do we really need the field to be autofilled? I thought we just needed the > field to have a (partial) value entered. Am I misunderstanding what bug you are > trying to fix? Thanks Ilya again for pointing out the crux here. The root-cause boils down to fields partially filled (sub-string matching), & autofilling is not needed as such. Earlier when I've logged the issue; I did autofill card fields and was trying to get suggestions for one of the middle credit card number fields. Apparently test case was aligned to that scenario. I've modified test case now, PTAL. Thanks!
Thanks, Pritam. Just a few small comments left: https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3037: number_field.value = ASCIIToUTF16("9012"); nit: Please test with a partial value, e.g. "901" rather than "9012". https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3042: // No suggestions provided, so send an empty vector as the results. nit: "No suggestions" -> "No autocomplete suggestions"
Thanks Ilya, I've uploaded new patch set with nits that you've suggested. PTAL. Thanks! https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3037: number_field.value = ASCIIToUTF16("9012"); On 2014/11/03 22:55:46, Ilya Sherman wrote: > nit: Please test with a partial value, e.g. "901" rather than "9012". Done. https://codereview.chromium.org/622773002/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3042: // No suggestions provided, so send an empty vector as the results. On 2014/11/03 22:55:46, Ilya Sherman wrote: > nit: "No suggestions" -> "No autocomplete suggestions" Done.
LGTM, thanks.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/622773002/280001
Message was sent while issue was closed.
Committed patchset #12 (id:280001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/504d780c7e67390d85cc918a6c11617ddd36a495 Cr-Commit-Position: refs/heads/master@{#302703} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
