Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
381613005:
[Autofill] Autofill fails to fill credit card number when split across fields. (Closed)
|
Created:
6 years, 5 months ago by Pritam Nikam Modified:
6 years, 3 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Autofill] Autofill fails to fill credit card number when split across fields.
With current autofill implementaion for credit card number field on a secure form that splits up the credit card number across multiple fields fails to fill all fields, and instead it just fills first of the credit card number field.
With this patch it solves the problem by book-keeping spilt information to hold the credit card number grouping.
BUG=90280
Committed: https://crrev.com/c011630e025f01ffcc8a926e92b9eb413c920b1a
Cr-Commit-Position: refs/heads/master@{#295200}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Incorporated review inputs. #Patch Set 3 : Incorporated review comments. #
Total comments: 29
Patch Set 4 : Incorporated review comments. #
Total comments: 41
Patch Set 5 : Incorporated review comments. #
Total comments: 9
Patch Set 6 : Incorporated review comments. #Patch Set 7 : Removed const qualifiers from FormField and AutofillScanner implementation. #
Total comments: 10
Patch Set 8 : Incorporated review comments. #
Total comments: 12
Patch Set 9 : Code modified according to review comments. #
Total comments: 2
Patch Set 10 : Modified according to review comments and fixed DCHECK() abort in form_field.cc. #
Total comments: 4
Patch Set 11 : Incorporated review inputs. #Messages
Total messages: 38 (1 generated)
Added a patch for review. Please have a look. Thanks.
The amount of code needed for this looks quite a lot to me, but I have zero knowledge about this actual component so that might be ok. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/credit_card_unittest.cc (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card_unittest.cc:312: EXPECT_EQ(ASCIIToUTF16("432154326543xxxx"), Shouldn't we add a new test case instead of changing an existing one? https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/field_types.h (left): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:57: These changes are unrelevant and useless, please remove (i.e. add back the empty lines).
I was hoping that we could do something more like what we do for phone numbers that are split into many input boxes -- i.e. avoid introducing new field types, but just fill data appropriately into these boxes. Also, your code is very tightly coupled to the notion that a credit card number has exactly 16 digits, which are broken up into four groups. That's not necessarily the case. For example, American Express cards have 15 digits, broken up into three groups, of lengths 4, 6, and 5. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card.h:150: base::string16 number_part4_; // The credit card number part 4 (12-End). These shouldn't be necessary. The card number should still be stored as a single unit, and dynamically split into substrings as needed. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/credit_card_unittest.cc (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card_unittest.cc:312: EXPECT_EQ(ASCIIToUTF16("432154326543xxxx"), On 2014/07/25 00:30:08, kbalazs wrote: > Shouldn't we add a new test case instead of changing an existing one? +1. Your changes should not regress this test. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/field_types.h (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:139: // server. As indicated by this comment, it's not appropriate to make changes to this file without server-side changes. Are you sure that new types are needed? If so, we can potentially add corresponding server types ... though that would require a fairly strong case to be made for these new field types. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:208: HTML_TYPE_CREDIT_CARD_NUMBER_PART4 It's not appropriate to add new HTML field types without changes to the HTML specification. In this case, I don't think the HTML specification will be changing to support these field types, as they do not follow best practices. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/personal_data_manager_unittest.cc:2297: EXPECT_FALSE(non_empty_types.count(CREDIT_CARD_NUMBER_PART4)); Hrm. It might be better to update the card used in this test to have 16 digits.
Thanks Ilya and kbalazs! I have added few easy pickings from review comments and incorporated in patch #2. Please have a look. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card.h:150: base::string16 number_part4_; // The credit card number part 4 (12-End). On 2014/07/25 03:37:49, Ilya Sherman wrote: > These shouldn't be necessary. The card number should still be stored as a > single unit, and dynamically split into substrings as needed. Done. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/credit_card_unittest.cc (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card_unittest.cc:312: EXPECT_EQ(ASCIIToUTF16("432154326543xxxx"), On 2014/07/25 03:37:49, Ilya Sherman wrote: > On 2014/07/25 00:30:08, kbalazs wrote: > > Shouldn't we add a new test case instead of changing an existing one? > > +1. Your changes should not regress this test. Done. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/credit_card_unittest.cc:312: EXPECT_EQ(ASCIIToUTF16("432154326543xxxx"), On 2014/07/25 00:30:08, kbalazs wrote: > Shouldn't we add a new test case instead of changing an existing one? Done. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/field_types.h (left): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:57: On 2014/07/25 00:30:08, kbalazs wrote: > These changes are unrelevant and useless, please remove (i.e. add back the empty > lines). These are aftereffects of CL format. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/field_types.h (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:139: // server. On 2014/07/25 03:37:49, Ilya Sherman wrote: > As indicated by this comment, it's not appropriate to make changes to this file > without server-side changes. Are you sure that new types are needed? If so, we > can potentially add corresponding server types ... though that would require a > fairly strong case to be made for these new field types. In my opinion this is required. I didn't find any easy alternative to identify a split in credit card number field and communicate it back till the end. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/field_types.h:208: HTML_TYPE_CREDIT_CARD_NUMBER_PART4 On 2014/07/25 03:37:49, Ilya Sherman wrote: > It's not appropriate to add new HTML field types without changes to the HTML > specification. In this case, I don't think the HTML specification will be > changing to support these field types, as they do not follow best practices. Done. https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/381613005/diff/80001/components/autofill/core... components/autofill/core/browser/personal_data_manager_unittest.cc:2297: EXPECT_FALSE(non_empty_types.count(CREDIT_CARD_NUMBER_PART4)); On 2014/07/25 03:37:49, Ilya Sherman wrote: > Hrm. It might be better to update the card used in this test to have 16 digits. Done.
On 2014/07/26 11:29:51, Pritam Nikam wrote: > Thanks Ilya and kbalazs! > > I have added few easy pickings from review comments and incorporated in patch > #2. Please have a look. I think Ilya shared some valid concerns about the design of your code so you should rather rework the patch instead of fixing the easier problems. Afaik you should get rid of the new fields and make it work with all kinds of credit card numbers (not just 16 digit).
On 2014/07/26 23:44:50, kbalazs wrote: > On 2014/07/26 11:29:51, Pritam Nikam wrote: > > Thanks Ilya and kbalazs! > > > > I have added few easy pickings from review comments and incorporated in patch > > #2. Please have a look. > > I think Ilya shared some valid concerns about the design of your code so you > should rather rework the patch instead of fixing the easier problems. Afaik you > should get rid of the new fields and make it work with all kinds of credit card > numbers (not just 16 digit). Thanks kbalazs! I'm already experimenting on it. But as I mentioned in earlier comments I didn't find any easy alternative to identify a split in credit card number field and communicate it back till the end. @ Ilya, One place I'm thinking to hack into AutofillManager::ParseForms and make use of FormFieldData::max_length to identify splits. Not sure whether its right way or not.
On 2014/07/27 03:00:56, Pritam Nikam wrote: > On 2014/07/26 23:44:50, kbalazs wrote: > > On 2014/07/26 11:29:51, Pritam Nikam wrote: > > > Thanks Ilya and kbalazs! > > > > > > I have added few easy pickings from review comments and incorporated in > patch > > > #2. Please have a look. > > > > I think Ilya shared some valid concerns about the design of your code so you > > should rather rework the patch instead of fixing the easier problems. Afaik > you > > should get rid of the new fields and make it work with all kinds of credit > card > > numbers (not just 16 digit). > > Thanks kbalazs! > I'm already experimenting on it. But as I mentioned in earlier comments I didn't > find > any easy alternative to identify a split in credit card number field and > communicate > it back till the end. > > @ Ilya, > One place I'm thinking to hack into AutofillManager::ParseForms and make use of > FormFieldData::max_length to identify splits. Not sure whether its right way or > not. Please take a look at [ https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... ] as an example of how phone number filling is implemented. I'd expect the credit card field implementation to be similar.
On 2014/07/28 22:04:50, Ilya Sherman wrote: > On 2014/07/27 03:00:56, Pritam Nikam wrote: > > On 2014/07/26 23:44:50, kbalazs wrote: > > > On 2014/07/26 11:29:51, Pritam Nikam wrote: > > > > Thanks Ilya and kbalazs! > > > > > > > > I have added few easy pickings from review comments and incorporated in > > patch > > > > #2. Please have a look. > > > > > > I think Ilya shared some valid concerns about the design of your code so you > > > should rather rework the patch instead of fixing the easier problems. Afaik > > you > > > should get rid of the new fields and make it work with all kinds of credit > > card > > > numbers (not just 16 digit). > > > > Thanks kbalazs! > > I'm already experimenting on it. But as I mentioned in earlier comments I > didn't > > find > > any easy alternative to identify a split in credit card number field and > > communicate > > it back till the end. > > > > @ Ilya, > > One place I'm thinking to hack into AutofillManager::ParseForms and make use > of > > FormFieldData::max_length to identify splits. Not sure whether its right way > or > > not. > > Please take a look at [ > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > ] as an example of how phone number filling is implemented. I'd expect the > credit card field implementation to be similar. Thanks Ilya for the direction! Sorry for delay, was away for some time. I'll take a look.
Hi Ilya, I've incorporated review comments and uploaded a new patch set (#3) for review. Patch is align with phone number suffix/prefix implementation. Please have a look. With this patch: a. Avoided new ServerFieldType for splits. b. Additional book-keeping with AutofillField for split information on HTML form-structure. c. Unit-tests for different credit card number groupings.
https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:394: FormFieldData* field_data) { nit: Please add documentation for this function. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:400: if (field_data->max_length && field_data->max_length < length && max_length is always set, isn't it? https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:401: number_info) { Is it possible to have the number_info set on a field that doesn't have a max_length? It seems like we should already know how many characters we expect to fill into this field; otherwise, what we fill into subsequent fields probably won't make sense. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:402: base::RemoveChars(cc_number, base::ASCIIToUTF16("- "), &value); Why is this needed? I would have expected that the cc_number is already a digit string at this point. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:403: length = value.length(); Why do you set this again? https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:406: value = value.substr(number_info->start_index_, length); I don't think you need to set the end index manually -- the field will enforce max_length on its own. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:410: } Please move this to be adjacent to the FillPhoneNumberField method. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:437: delete credit_card_number_info_; You should pretty much always prefer a scoped_ptr or other smart pointer to manual resource management. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:31: int start_index_; Why do you need the part in addition to the index? https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_regex_constants.cc.utf8 (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_regex_constants.cc.utf8:134: "card.?number.?\\d*|card.?#|card.?no.?\\d*|acct.?num.?\\d*|\\w*part.?\\d{1}|\\w*q.?\\d{1}|cc.?num.?\\d*" Why did you add ".?\\d*" to these regexes? It should have no effect, since we're looking for substring matches, rather than full string matches. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:99: const_cast<AutofillField::CreditCardNumberInfo*>( Note: If you find yourself using const_cast, that's generally a sign that your code is not const-correct. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:102: last_number_info->start_index_ + last_number_field->max_length; It's possible for this code to assign very large start_index_ values. We should probably have some sort of error correction and bail for such cases. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:108: new AutofillField::CreditCardNumberInfo; If this is always non-null, why use a pointer, rather than just passing and setting by value? https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:213: numbers_.clear(); This is called automatically. Why did you add it explicitly?
Thanks Ilya for your inputs. I've incorporated review comments and uploaded patch set #4 for review. Please have a look. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:394: FormFieldData* field_data) { On 2014/08/07 20:57:25, Ilya Sherman wrote: > nit: Please add documentation for this function. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:400: if (field_data->max_length && field_data->max_length < length && On 2014/08/07 20:57:25, Ilya Sherman wrote: > max_length is always set, isn't it? Done. For HTML forms i didn't notice this being coming as zero. However, there are many test-cases where max_length is not specified, being FormFieldData structure member it's initialize to "0". [https://code.google.com/p/chromium/codesearch#chromium/src/components/autofill/core/common/form_field_data.cc&l=62-70] https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:401: number_info) { On 2014/08/07 20:57:25, Ilya Sherman wrote: > Is it possible to have the number_info set on a field that doesn't have a > max_length? It seems like we should already know how many characters we expect > to fill into this field; otherwise, what we fill into subsequent fields probably > won't make sense. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:402: base::RemoveChars(cc_number, base::ASCIIToUTF16("- "), &value); On 2014/08/07 20:57:25, Ilya Sherman wrote: > Why is this needed? I would have expected that the cc_number is already a digit > string at this point. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:403: length = value.length(); On 2014/08/07 20:57:25, Ilya Sherman wrote: > Why do you set this again? Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:406: value = value.substr(number_info->start_index_, length); On 2014/08/07 20:57:25, Ilya Sherman wrote: > I don't think you need to set the end index manually -- the field will enforce > max_length on its own. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:410: } On 2014/08/07 20:57:25, Ilya Sherman wrote: > Please move this to be adjacent to the FillPhoneNumberField method. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:437: delete credit_card_number_info_; On 2014/08/07 20:57:25, Ilya Sherman wrote: > You should pretty much always prefer a scoped_ptr or other smart pointer to > manual resource management. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:31: int start_index_; On 2014/08/07 20:57:25, Ilya Sherman wrote: > Why do you need the part in addition to the index? Done. Removed this structure and instead book-keeping the start_index only. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/autofill_regex_constants.cc.utf8 (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/autofill_regex_constants.cc.utf8:134: "card.?number.?\\d*|card.?#|card.?no.?\\d*|acct.?num.?\\d*|\\w*part.?\\d{1}|\\w*q.?\\d{1}|cc.?num.?\\d*" On 2014/08/07 20:57:25, Ilya Sherman wrote: > Why did you add ".?\\d*" to these regexes? It should have no effect, since > we're looking for substring matches, rather than full string matches. Done. Restored the original regular expression. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:99: const_cast<AutofillField::CreditCardNumberInfo*>( On 2014/08/07 20:57:25, Ilya Sherman wrote: > Note: If you find yourself using const_cast, that's generally a sign that your > code is not const-correct. ParseField() accepts const Autofill**, and hence we *cannot* get rid of const_cast<>. Moreover, other const_cast<> I've removed. [Ref: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:102: last_number_info->start_index_ + last_number_field->max_length; On 2014/08/07 20:57:25, Ilya Sherman wrote: > It's possible for this code to assign very large start_index_ values. We should > probably have some sort of error correction and bail for such cases. Done. https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:108: new AutofillField::CreditCardNumberInfo; On 2014/08/07 20:57:25, Ilya Sherman wrote: > If this is always non-null, why use a pointer, rather than just passing and > setting by value? Done. std::min(std::numeric_limits<size_t>::max(), last_number_field->max_length+last_number_field->start_index) And added a check inside FillCreditCardNumberField(). https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:213: numbers_.clear(); On 2014/08/07 20:57:25, Ilya Sherman wrote: > This is called automatically. Why did you add it explicitly? Done.
https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:99: const_cast<AutofillField::CreditCardNumberInfo*>( On 2014/08/08 14:14:34, Pritam Nikam wrote: > On 2014/08/07 20:57:25, Ilya Sherman wrote: > > Note: If you find yourself using const_cast, that's generally a sign that your > > code is not const-correct. > > ParseField() accepts const Autofill**, and hence we *cannot* get rid of > const_cast<>. > Moreover, other const_cast<> I've removed. > [Ref: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... The correct change is to modify the signature of ParseField, if need be. Using const_cast subverts the type system, and hence should not be used except in some very rare circumstances. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:307: const base::string16& cc_number, nit: Please don't use the abbreviation "cc". Instead, either spell out "credit_card", or just drop the prefix. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:311: // |filed|'s max_length truncates credit card number to fit within. nit: "filed" -> "field" https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:312: if (field.credit_card_number_start_index() && There's no need to check whether the start index is zero or not, since the functionality is equivalent, and the performance difference is not worth worrying about for this code. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:316: value.length() - field.credit_card_number_start_index()); You can shorten this by dropping the second argument to substr. Unless I'm misreading the code, I believe that should give an equivalent result. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:39: int total_spilts_; nit: Prefer size_t over int for all of the ints in this struct. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:45: for (int i = 0; i < index; i++) nit: Prefer "++i" to "i++". https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:49: }; It looks like this is only used in a single test, so please define this locally to that test. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:535: std::vector<int>(splits1, splits1 + sizeof(splits1) / sizeof(int)); nit: Please use the arraysize macro rather than implementing it yourself. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:537: "8765"}; Why aren't the expected results four digits each? https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:543: // lengths 4, 6, and 5. Rather than writing a single TEST to cover three different cases, I think it would be clearer to simply write three separate TESTs. That way, the logic within each test would be simpler and easier to follow. In general, "dumb" but easy-to-follow test code is better than clever but harder-to-follow test code, since (1) tests are a form of documentation, and so readability matters quite a bit; and (2) simpler tests are less likely to have their own bugs. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:560: for (int i = 0; i < current_case.total_spilts_; i++) { nit: Prefer "++i" to "i++". https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:560: for (int i = 0; i < current_case.total_spilts_; i++) { nit: int -> size_t https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:562: cc_number_part.SetHtmlType(HTML_TYPE_UNKNOWN, HtmlFieldMode()); Does the test fail if you omit this line? If not, please omit it. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:566: str << "card_number_part" << i; If you really need this, please use base::StringPrintf https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:567: cc_number_part.name = ASCIIToUTF16(str.str()); Do you really need to set the name or label? https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:7: #include <stddef.h> nit: Please include a blank line after this one, to separate the C header includes from the C++ ones. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:101: std::min(std::numeric_limits<size_t>::max(), This std::min call does not do anything, since the max size_t value is by definitely the largest possible value, so std::min will always pick the other option (or possibly both, if the two inputs are equal). https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:104: credit_card_field->numbers_.back()->max_length); You should probably check whether the resulting start_index + max_length for the last field is a small value (say, less than or equal to 19, which according to [ https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... ] is the maximum length of a credit card number). At the least, you should check whether the max_length is the default (very large) value, or something smaller. If we see four fields in a row that don't have a max_length set, we should probably bail on parsing the form. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:215: ok = ok && AddClassification(*it, CREDIT_CARD_NUMBER, map); nit: Please add curly braces. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/credit_card_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field_unittest.cc:400: EXPECT_EQ(CREDIT_CARD_NUMBER, field_type_map_[ASCIIToUTF16("number4")]); Please also verify that the start indices are set correctly.
Thanks Ilya for review. I've incorporated review comments and added new patch set (#5) for review. Please have a look. Br. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:307: const base::string16& cc_number, On 2014/08/12 04:23:41, Ilya Sherman wrote: > nit: Please don't use the abbreviation "cc". Instead, either spell out > "credit_card", or just drop the prefix. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:311: // |filed|'s max_length truncates credit card number to fit within. On 2014/08/12 04:23:41, Ilya Sherman wrote: > nit: "filed" -> "field" Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:312: if (field.credit_card_number_start_index() && On 2014/08/12 04:23:41, Ilya Sherman wrote: > There's no need to check whether the start index is zero or not, since the > functionality is equivalent, and the performance difference is not worth > worrying about for this code. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:316: value.length() - field.credit_card_number_start_index()); On 2014/08/12 04:23:41, Ilya Sherman wrote: > You can shorten this by dropping the second argument to substr. Unless I'm > misreading the code, I believe that should give an equivalent result. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:39: int total_spilts_; On 2014/08/12 04:23:41, Ilya Sherman wrote: > nit: Prefer size_t over int for all of the ints in this struct. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:45: for (int i = 0; i < index; i++) On 2014/08/12 04:23:42, Ilya Sherman wrote: > nit: Prefer "++i" to "i++". Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:49: }; On 2014/08/12 04:23:41, Ilya Sherman wrote: > It looks like this is only used in a single test, so please define this locally > to that test. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:535: std::vector<int>(splits1, splits1 + sizeof(splits1) / sizeof(int)); On 2014/08/12 04:23:42, Ilya Sherman wrote: > nit: Please use the arraysize macro rather than implementing it yourself. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:537: "8765"}; On 2014/08/12 04:23:41, Ilya Sherman wrote: > Why aren't the expected results four digits each? As we are not copying exact size substring, instead just moving the start index and leaving the max_length for form field to do rest. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:543: // lengths 4, 6, and 5. On 2014/08/12 04:23:41, Ilya Sherman wrote: > Rather than writing a single TEST to cover three different cases, I think it > would be clearer to simply write three separate TESTs. That way, the logic > within each test would be simpler and easier to follow. In general, "dumb" but > easy-to-follow test code is better than clever but harder-to-follow test code, > since (1) tests are a form of documentation, and so readability matters quite a > bit; and (2) simpler tests are less likely to have their own bugs. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:560: for (int i = 0; i < current_case.total_spilts_; i++) { On 2014/08/12 04:23:41, Ilya Sherman wrote: > nit: Prefer "++i" to "i++". Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:560: for (int i = 0; i < current_case.total_spilts_; i++) { On 2014/08/12 04:23:42, Ilya Sherman wrote: > nit: int -> size_t Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:562: cc_number_part.SetHtmlType(HTML_TYPE_UNKNOWN, HtmlFieldMode()); On 2014/08/12 04:23:41, Ilya Sherman wrote: > Does the test fail if you omit this line? If not, please omit it. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:566: str << "card_number_part" << i; On 2014/08/12 04:23:42, Ilya Sherman wrote: > If you really need this, please use base::StringPrintf Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:567: cc_number_part.name = ASCIIToUTF16(str.str()); On 2014/08/12 04:23:42, Ilya Sherman wrote: > Do you really need to set the name or label? Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:7: #include <stddef.h> On 2014/08/12 04:23:42, Ilya Sherman wrote: > nit: Please include a blank line after this one, to separate the C header > includes from the C++ ones. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:97: const_cast<AutofillField*>(cc_number_field); I didn't get any alternative approach to fix const_cast<> used here. Changing ParseField() signature will have lots of signature changes as its just a wrapper API. And may be doing that will have ripple effect across [form-field, scanner and cursor modules]. I'm kind of blocked here, please guide me how can I solve this problem. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:101: std::min(std::numeric_limits<size_t>::max(), On 2014/08/12 04:23:42, Ilya Sherman wrote: > This std::min call does not do anything, since the max size_t value is by > definitely the largest possible value, so std::min will always pick the other > option (or possibly both, if the two inputs are equal). Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:104: credit_card_field->numbers_.back()->max_length); On 2014/08/12 04:23:42, Ilya Sherman wrote: > You should probably check whether the resulting start_index + max_length for the > last field is a small value (say, less than or equal to 19, which according to [ > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > ] is the maximum length of a credit card number). At the least, you should > check whether the max_length is the default (very large) value, or something > smaller. If we see four fields in a row that don't have a max_length set, we > should probably bail on parsing the form. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:215: ok = ok && AddClassification(*it, CREDIT_CARD_NUMBER, map); On 2014/08/12 04:23:42, Ilya Sherman wrote: > nit: Please add curly braces. Done. https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... File components/autofill/core/browser/credit_card_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/180001/components/autofill/cor... components/autofill/core/browser/credit_card_field_unittest.cc:400: EXPECT_EQ(CREDIT_CARD_NUMBER, field_type_map_[ASCIIToUTF16("number4")]); On 2014/08/12 04:23:42, Ilya Sherman wrote: > Please also verify that the start indices are set correctly. Done.
https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:104: ->credit_card_number_start_index() + style-wise, a lot of this seems off to me. Could you run git cl format? https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:106: if (!last_number_field_size || please don't implicitly convert size_t to boolean. Replace this with last_number_field_size == OU https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:108: return NULL; \n https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:113: const_cast<AutofillField*>(cc_number_field); you still have this const_cast... I agree with Ilya that the signature of ParseField ought to be changed.
Thanks Evan for review. I have incorporated inputs in patch set #6. Please have a look. Br. https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:104: ->credit_card_number_start_index() + On 2014/08/12 16:05:07, Evan Stade wrote: > style-wise, a lot of this seems off to me. Could you run git cl format? It's after git cl format only :) https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:106: if (!last_number_field_size || On 2014/08/12 16:05:07, Evan Stade wrote: > please don't implicitly convert size_t to boolean. Replace this with > last_number_field_size == OU Done. https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:108: return NULL; On 2014/08/12 16:05:07, Evan Stade wrote: > \n Done. https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:113: const_cast<AutofillField*>(cc_number_field); On 2014/08/12 16:05:07, Evan Stade wrote: > you still have this const_cast... I agree with Ilya that the signature of > ParseField ought to be changed. [Same as my previous comments] I didn't get any alternative approach to fix const_cast<> used here. Changing ParseField() signature alone will not help, have to introduce lots of signature changes as it's just a wrapper API. And may be doing that will have ripple effect across [form-field, scanner and cursor modules]. Isn't it an overkill for this issue. I'm kind of blocked here, please guide me how can I solve this problem.
https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:113: const_cast<AutofillField*>(cc_number_field); On 2014/08/12 18:39:11, Pritam Nikam wrote: > On 2014/08/12 16:05:07, Evan Stade wrote: > > you still have this const_cast... I agree with Ilya that the signature of > > ParseField ought to be changed. > > [Same as my previous comments] > I didn't get any alternative approach to fix const_cast<> used here. > > Changing ParseField() signature alone will not help, have to introduce lots of > signature changes as it's just a wrapper API. And may be doing that will have > ripple effect across [form-field, scanner and cursor modules]. What is the cursor module? > Isn't it an > overkill for this issue. the code becomes a lot harder to follow if there are a ton of false consts sprinkled throughout, so no. > > I'm kind of blocked here, please guide me how can I solve this problem. Remove all the consts you need to. I think this will only touch FormField and AutofillScanner.
On 2014/08/12 19:19:55, Evan Stade wrote: > https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... > File components/autofill/core/browser/credit_card_field.cc (right): > > https://codereview.chromium.org/381613005/diff/200001/components/autofill/cor... > components/autofill/core/browser/credit_card_field.cc:113: > const_cast<AutofillField*>(cc_number_field); > On 2014/08/12 18:39:11, Pritam Nikam wrote: > > On 2014/08/12 16:05:07, Evan Stade wrote: > > > you still have this const_cast... I agree with Ilya that the signature of > > > ParseField ought to be changed. > > > > [Same as my previous comments] > > I didn't get any alternative approach to fix const_cast<> used here. > > > > Changing ParseField() signature alone will not help, have to introduce lots > of > > signature changes as it's just a wrapper API. And may be doing that will have > > ripple effect across [form-field, scanner and cursor modules]. > > What is the cursor module? > > > Isn't it an > > overkill for this issue. > > the code becomes a lot harder to follow if there are a ton of false consts > sprinkled throughout, so no. > > > > > I'm kind of blocked here, please guide me how can I solve this problem. > > Remove all the consts you need to. I think this will only touch FormField and > AutofillScanner. Thanks Evan for review. As suggested, I have removed const qualifiers from FormField and AutofillScanner implementation in this patch. I didn't incorporate changes here as it touches other files as well like name, address, phone, email filed. Please have a look. https://codereview.chromium.org/472433002/
Please take a look at Ziran's pending CL, [ https://codereview.chromium.org/442403002/ ]. It's probably wise to update this one to match the changes there; or at least, plan to do so in the near future.
On 2014/08/13 19:59:57, Ilya Sherman wrote: > Please take a look at Ziran's pending CL, [ > https://codereview.chromium.org/442403002/ ]. It's probably wise to update this > one to match the changes there; or at least, plan to do so in the near future. Thanks Ilya & Evan, I've incorporated const qualifier removal changes from FormField and AutofillScanner implementation. Please take a look. I've studied Ziran's CL, & will try to adopt that approach soon once his changes merge to mainline.
Hi Ilya & Evan, I've incorporated const qualifier removal changes from FormField and AutofillScanner implementation. Please take a look. Thanks.
Hi Ilya & Evan, Added a patch for review. Please have a look. Thanks!
sorry for the delay! I made these comments but forgot to send them. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:433: AutofillField::~AutofillField() { did git cl format change this? dang that thing https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:117: size_t credit_card_number_start_index_; nit: s/start_index_/offset_/ https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:107: credit_card_field->numbers_.back()->max_length; nit: \n https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:111: // form fields as invalid and skip autofilling them. Not sure if this would happen in the wild, but it seems like the last input might not have a max size set. In this case, we could still fill in the credit card just by stuffing whatever numbers we have left over from the first n-1 fields into the last field. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:113: last_number_field_size >= kMaxValidCardNumberSize) nit: curlies
Thanks Evan for review comments. I've incorporated concern modifications in new patch set (#8) PTAL. Thanks! https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:433: AutofillField::~AutofillField() { On 2014/08/28 18:18:42, Evan Stade wrote: > did git cl format change this? dang that thing Yes! It's after git cl format. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/autofill_field.h (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/autofill_field.h:117: size_t credit_card_number_start_index_; On 2014/08/28 18:18:42, Evan Stade wrote: > nit: s/start_index_/offset_/ Done. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:107: credit_card_field->numbers_.back()->max_length; On 2014/08/28 18:18:42, Evan Stade wrote: > nit: \n Done. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:111: // form fields as invalid and skip autofilling them. On 2014/08/28 18:18:42, Evan Stade wrote: > Not sure if this would happen in the wild, but it seems like the last input > might not have a max size set. In this case, we could still fill in the credit > card just by stuffing whatever numbers we have left over from the first n-1 > fields into the last field. This is already taken care with current logic. Modified a unit test CreditCardFieldTest::ParseCreditCardNumberWithSplit to reflect this along with a comment. https://codereview.chromium.org/381613005/diff/240001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:113: last_number_field_size >= kMaxValidCardNumberSize) On 2014/08/28 18:18:42, Evan Stade wrote: > nit: curlies Done.
lgtm
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:45: size_t GetNumberOffset(size_t index, TestCase test) { nit: Please pass by const reference. https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:46: size_t ret = 0; nit: "ret" -> "result" https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:587: std::string results[] = {"423456789012345", "56789012345", "12345"}; These values do not make sense to me. Shouldn't they be "4234", "567890", "12345"? I see that you set max_length below. Is it not being respected? The value visible to JavaScript should not be different from the value visible to the user; so if the max_length doesn't actually automatically trim the set values, then we should do so manually. https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:116: credit_card_field->is_valid_card_number_split_ = false; Is it possible to recover from this parsing error? If not, why not bail out early here? https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.h (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/credit_card_field.h:71: bool is_valid_card_number_split_; Why do you need this, rather than just resetting the numbers_ vector?
Thanks Ilya and Evan.
@ Ilya,
Regarding early bail out case when parsing logic for credit card splits found to
be invalid, if we try to break or return early while parsing, control does still
try to parse the rest of the credit card fields as Scanner's cursor position
does not surpasses all form fields related to credit card.
e.g.
number splits = {4, 20, 4, 4} <<- more than max-limit of 19.
and returns after parsing 2nd number field, control try to parse rest of the
fields i.e. 3rd filed onwards in next parse pass.
Even if we try to read/parse all number fields and then maintain an invalid flag
locally, and use it to do vector clear, other fields say card-number &
expiration date still gets fill, and that may not be quite right(?) in my
opinion.
And thats the reason I've added a flag and return from
CreditCardField::ClassifyField().
Please correct my & guide, what shall be the correct work-flow here.
Rest of the comments I've incorporated with patch-set (#9). PTAL.
Thanks!
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
File components/autofill/core/browser/autofill_field_unittest.cc (right):
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
components/autofill/core/browser/autofill_field_unittest.cc:45: size_t
GetNumberOffset(size_t index, TestCase test) {
On 2014/09/03 01:07:51, Ilya Sherman wrote:
> nit: Please pass by const reference.
Done.
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
components/autofill/core/browser/autofill_field_unittest.cc:46: size_t ret = 0;
On 2014/09/03 01:07:51, Ilya Sherman wrote:
> nit: "ret" -> "result"
Done.
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
components/autofill/core/browser/autofill_field_unittest.cc:587: std::string
results[] = {"423456789012345", "56789012345", "12345"};
On 2014/09/03 01:07:51, Ilya Sherman wrote:
> These values do not make sense to me. Shouldn't they be "4234", "567890",
> "12345"? I see that you set max_length below. Is it not being respected?
The
> value visible to JavaScript should not be different from the value visible to
> the user; so if the max_length doesn't actually automatically trim the set
> values, then we should do so manually.
Done.
We leave the field will enforce max_length on its own and avoid truncating
explicitly. And hence we just adjust start index (or an offset) within the
number sub-string.
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (right):
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:116:
credit_card_field->is_valid_card_number_split_ = false;
On 2014/09/03 01:07:51, Ilya Sherman wrote:
> Is it possible to recover from this parsing error? If not, why not bail out
> early here?
I think, I didn't understand this completely.
Moreover, from little debugging, as i understand Parse-Classify cycle runs in
tandem, and based on Scanner's cursor position it try to keep on parsing the
input fields on HTML form.
So in my experiments, if I try to break or return early (i.e. before parsing
rest of the fields related to credit card) it does still invoke
CreditCardField::Parse() followed by CreditCardField::ClassifyField() for rest
of the fields.
Also, I assume if number splits are invalid it's better not to autofill
expiration date and card-holder name as well.
Please correct me and guide what shall be the correct work-flow here.
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.h (right):
https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.h:71: bool
is_valid_card_number_split_;
On 2014/09/03 01:07:51, Ilya Sherman wrote:
> Why do you need this, rather than just resetting the numbers_ vector?
This flag is added to make sure if splits are not valid then not to auto-fill
credit card expiration date & card holder names along with the number, by
returning in CreditCardField::ClassifyField().
By the way, now that I've taken another look at this code, I don't see why we needed to change the method signature of ParseField to drop the constness. It seems like it should be possible to declare "AutofillField* current_number_field;" -- as you do -- without the const modifier, but then pass it into a method that accepts const data. ParseField itself is not modifying the field, so it seems quite unnecessary to drop the const modifier from the method signature. What am I missing? https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:587: std::string results[] = {"423456789012345", "56789012345", "12345"}; On 2014/09/03 12:21:50, Pritam Nikam wrote: > On 2014/09/03 01:07:51, Ilya Sherman wrote: > > These values do not make sense to me. Shouldn't they be "4234", "567890", > > "12345"? I see that you set max_length below. Is it not being respected? > The > > value visible to JavaScript should not be different from the value visible to > > the user; so if the max_length doesn't actually automatically trim the set > > values, then we should do so manually. > > Done. > > We leave the field will enforce max_length on its own and avoid truncating > explicitly. And hence we just adjust start index (or an offset) within the > number sub-string. Have you verified that the max_length is enforced on the renderer side both for the value that's visible to the user and for the value that's visible to JavaScript (document.getElementById('theElementId').value)? https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:116: credit_card_field->is_valid_card_number_split_ = false; On 2014/09/03 12:21:50, Pritam Nikam wrote: > On 2014/09/03 01:07:51, Ilya Sherman wrote: > > Is it possible to recover from this parsing error? If not, why not bail out > > early here? > > I think, I didn't understand this completely. > > Moreover, from little debugging, as i understand Parse-Classify cycle runs in > tandem, and based on Scanner's cursor position it try to keep on parsing the > input fields on HTML form. > So in my experiments, if I try to break or return early (i.e. before parsing > rest of the fields related to credit card) it does still invoke > CreditCardField::Parse() followed by CreditCardField::ClassifyField() for rest > of the fields. > > Also, I assume if number splits are invalid it's better not to autofill > expiration date and card-holder name as well. > > Please correct me and guide what shall be the correct work-flow here. I don't see how this parsing error is different from any other failure, in which we just rewind the scanner and return NULL. Can you explain to me why this case should be treated in a special way? If not, please just clear the numbers_ list and break out of the loop. https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:45: size_t GetNumberOffset(const size_t& index, const TestCase& test) { Sorry for not being clear enough: You should continue to pass the primitive size_t type by copy, but you should pass the TestCase struct by const-reference.
On 2014/09/03 23:25:27, Ilya Sherman wrote: > By the way, now that I've taken another look at this code, I don't see why we > needed to change the method signature of ParseField to drop the constness. It > seems like it should be possible to declare "AutofillField* > current_number_field;" -- as you do -- without the const modifier, but then pass > it into a method that accepts const data. ParseField itself is not modifying > the field, so it seems quite unnecessary to drop the const modifier from the > method signature. What am I missing? const AutofillField* can accept AutofillField* (the in param case), but const AutofillField** cannot accept AutofillField** (the out param case).
On 2014/09/03 23:58:22, Evan Stade wrote: > On 2014/09/03 23:25:27, Ilya Sherman wrote: > > By the way, now that I've taken another look at this code, I don't see why we > > needed to change the method signature of ParseField to drop the constness. It > > seems like it should be possible to declare "AutofillField* > > current_number_field;" -- as you do -- without the const modifier, but then > pass > > it into a method that accepts const data. ParseField itself is not modifying > > the field, so it seems quite unnecessary to drop the const modifier from the > > method signature. What am I missing? > > const AutofillField* can accept AutofillField* (the in param case), but const > AutofillField** cannot accept AutofillField** (the out param case). Ah, right, out param. Thanks for the reminder. Please disregard my concern about this, then :)
On 2014/09/03 23:25:27, Ilya Sherman wrote: > By the way, now that I've taken another look at this code, I don't see why we > needed to change the method signature of ParseField to drop the constness. It > seems like it should be possible to declare "AutofillField* > current_number_field;" -- as you do -- without the const modifier, but then pass > it into a method that accepts const data. ParseField itself is not modifying > the field, so it seems quite unnecessary to drop the const modifier from the > method signature. What am I missing? > > https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... > File components/autofill/core/browser/autofill_field_unittest.cc (right): > > https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... > components/autofill/core/browser/autofill_field_unittest.cc:587: std::string > results[] = {"423456789012345", "56789012345", "12345"}; > On 2014/09/03 12:21:50, Pritam Nikam wrote: > > On 2014/09/03 01:07:51, Ilya Sherman wrote: > > > These values do not make sense to me. Shouldn't they be "4234", "567890", > > > "12345"? I see that you set max_length below. Is it not being respected? > > The > > > value visible to JavaScript should not be different from the value visible > to > > > the user; so if the max_length doesn't actually automatically trim the set > > > values, then we should do so manually. > > > > Done. > > > > We leave the field will enforce max_length on its own and avoid truncating > > explicitly. And hence we just adjust start index (or an offset) within the > > number sub-string. > > Have you verified that the max_length is enforced on the renderer side both for > the value that's visible to the user and for the value that's visible to > JavaScript (document.getElementById('theElementId').value)? Sorry I didn't mention it earlier, but this I've already experimented, and it's working fine in JS. Sample html form I've give below. > > https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... > File components/autofill/core/browser/credit_card_field.cc (right): > > https://codereview.chromium.org/381613005/diff/260001/components/autofill/cor... > components/autofill/core/browser/credit_card_field.cc:116: > credit_card_field->is_valid_card_number_split_ = false; > On 2014/09/03 12:21:50, Pritam Nikam wrote: > > On 2014/09/03 01:07:51, Ilya Sherman wrote: > > > Is it possible to recover from this parsing error? If not, why not bail out > > > early here? > > > > I think, I didn't understand this completely. > > > > Moreover, from little debugging, as i understand Parse-Classify cycle runs in > > tandem, and based on Scanner's cursor position it try to keep on parsing the > > input fields on HTML form. > > So in my experiments, if I try to break or return early (i.e. before parsing > > rest of the fields related to credit card) it does still invoke > > CreditCardField::Parse() followed by CreditCardField::ClassifyField() for rest > > of the fields. > > > > Also, I assume if number splits are invalid it's better not to autofill > > expiration date and card-holder name as well. > > > > Please correct me and guide what shall be the correct work-flow here. > > I don't see how this parsing error is different from any other failure, in which > we just rewind the scanner and return NULL. Can you explain to me why this case > should be treated in a special way? If not, please just clear the numbers_ list > and break out of the loop. As I understand, it has all form input fields related to credit card that make this case different than rest of the parse failures. It's difficult to phrase correctly, but with few experiments it will help me to convey. I've tried below experiments, and so far not able to achieve what it's intended to do, (I could still get the suggestions) (for all experiments splits were {4, 40, 4, NOT-SPECIFIED}) // ---[snap]---// (Sample html form) <body> <form id="testform"> <label for="nameoncard">Name on Card</label> <input size="40" id="CREDIT_CARD_NAME"/> <br> <label for="card_number">Card Number</label> <input id="CREDIT_CARD_NUMBER1" name="card_number1" maxlength=4 /> <input id="CREDIT_CARD_NUMBER2" name="card_number2" maxlength=40 /> <input id="CREDIT_CARD_NUMBER3" name="card_number3" maxlength=4 /> <input id="CREDIT_CARD_NUMBER4" name="card_number4" /> <br> <label>Expiration Date</label> <input type="text" name="cc_expiration_mm" maxlength=2 /> <input type="text" name="cc_expiration_yy" maxlength=2 /> <br> <input type="button" onclick="myFunction()" value="Submit form"> </form> <script> function myFunction() { val1 = document.getElementById('CREDIT_CARD_NUMBER1').value; val2 = document.getElementById('CREDIT_CARD_NUMBER2').value; val3 = document.getElementById('CREDIT_CARD_NUMBER3').value; val4 = document.getElementById('CREDIT_CARD_NUMBER4').value; console.log("Card number: "+val1+"-"+val2+"-"+val3+"-"+val4); } </script> </body> [1] if (last_number_field_size == 0U || last_number_field_size >= kMaxValidCardNumberSize) { is_valid_card_number_split = false; } ... } // end of loop if (!is_valid_card_number_split) { credit_card_field->numbers_.clear(); scanner->Rewind(); return NULL; } [2] if (last_number_field_size == 0U || last_number_field_size >= kMaxValidCardNumberSize) { is_valid_card_number_split = false; } ... } // end of loop if (!is_valid_card_number_split) { credit_card_field->numbers_.clear(); scanner->RewindTo(saved_cursor); return NULL; } [3] if (last_number_field_size == 0U || last_number_field_size >= kMaxValidCardNumberSize) { is_valid_card_number_split = false; break; } ... } // end of loop if (!is_valid_card_number_split) { credit_card_field->numbers_.clear(); scanner->Rewind(); return NULL; } [4] if (last_number_field_size == 0U || last_number_field_size >= kMaxValidCardNumberSize) { is_valid_card_number_split = false; } ... } // end of loop if (!is_valid_card_number_split) { credit_card_field->numbers_.clear(); scanner->RewindTo(saved_cursor); return NULL; } [5] All above experiments skipping Rewind() as well didn't work. Moreover, with this patch (set #10), for invalid number split even we avoid showing autofill suggestions for credit card related fields. > > https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... > File components/autofill/core/browser/autofill_field_unittest.cc (right): > > https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... > components/autofill/core/browser/autofill_field_unittest.cc:45: size_t > GetNumberOffset(const size_t& index, const TestCase& test) { > Sorry for not being clear enough: You should continue to pass the primitive > size_t type by copy, but you should pass the TestCase struct by const-reference.
Thanks Ilya and Evan for review. I've incorporated the review comments. Also fixed DCHECK abort in form_field.cc, for cases where CreditCardField::ClassifyField() returns false for bail out case. [https://code.google.com/p/chromium/codesearch#chromium/src/components/autofill/core/browser/form_field.cc&q=form_field.cc&sq=package:chromium&l=165-166] PTAL, thanks! https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/280001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:45: size_t GetNumberOffset(const size_t& index, const TestCase& test) { On 2014/09/03 23:25:27, Ilya Sherman wrote: > Sorry for not being clear enough: You should continue to pass the primitive > size_t type by copy, but you should pass the TestCase struct by const-reference. Done.
With apologies for the delay of this reply: https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:116: credit_card_field->is_valid_card_number_split_ = false; I'm still not convinced that swallowing all of the credit card fields is appropriate. Even if we don't think that we understand the form well enough to fill the credit card number, we can still fill the name and expiration date. Honestly, I think that a form like <input id="CREDIT_CARD_NUMBER1" name="card_number1" maxlength=4 /> <input id="CREDIT_CARD_NUMBER2" name="card_number2" maxlength=40 /> <input id="CREDIT_CARD_NUMBER3" name="card_number3" maxlength=4 /> <input id="CREDIT_CARD_NUMBER4" name="card_number4" /> <br> is implausible. I could imagine encountering a form like <input id="CREDIT_CARD_NUMBER" name="card_number1" /> <input id="NOT_A_CREDIT_CARD_NUMBER" name="card_number2" /> where our heuristics could incorrectly detect the second field as a credit card number field even though it isn't. I think the reasonable options are either (1) to try to fill the first field(s) that we think are for the credit card number, or (2) to swallow (i.e. refuse to fill) the credit card number fields, but still be willing to fill the remaining credit card fields. In either case, I think that logic can be implemented entirely internally to this function, but setting the contents of |credit_card_field->numbers_| appropriate. https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:240: ok = ok && AddClassification(numbers_.at(index), CREDIT_CARD_NUMBER, map); nit: Please use "numbers_[index]" rather than "numbers_.at(index)", as the latter can throw an exception, whereas Chromium code does not handle exceptions.
Thanks Ilya for inputs. I've adopted 2nd option as you've suggested i.e. to swallow (i.e. refuse to fill) the credit card number fields, but still be willing to fill the remaining credit card fields. Changes are part of patch set #11. PTAL. Thanks! https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:116: credit_card_field->is_valid_card_number_split_ = false; On 2014/09/16 02:35:27, Ilya Sherman wrote: > I'm still not convinced that swallowing all of the credit card fields is > appropriate. Even if we don't think that we understand the form well enough to > fill the credit card number, we can still fill the name and expiration date. > Honestly, I think that a form like > > <input id="CREDIT_CARD_NUMBER1" name="card_number1" maxlength=4 /> > <input id="CREDIT_CARD_NUMBER2" name="card_number2" maxlength=40 /> > <input id="CREDIT_CARD_NUMBER3" name="card_number3" maxlength=4 /> > <input id="CREDIT_CARD_NUMBER4" name="card_number4" /> <br> > > is implausible. I could imagine encountering a form like > > <input id="CREDIT_CARD_NUMBER" name="card_number1" /> > <input id="NOT_A_CREDIT_CARD_NUMBER" name="card_number2" /> > > where our heuristics could incorrectly detect the second field as a credit card > number field even though it isn't. I think the reasonable options are either > (1) to try to fill the first field(s) that we think are for the credit card > number, or (2) to swallow (i.e. refuse to fill) the credit card number fields, > but still be willing to fill the remaining credit card fields. In either case, > I think that logic can be implemented entirely internally to this function, but > setting the contents of |credit_card_field->numbers_| appropriate. Done. Going ahead with option (2) as you suggested i.e. to swallow (i.e. refuse to fill) the credit card number fields, but still be willing to fill the remaining credit card fields. https://codereview.chromium.org/381613005/diff/300001/components/autofill/cor... components/autofill/core/browser/credit_card_field.cc:240: ok = ok && AddClassification(numbers_.at(index), CREDIT_CARD_NUMBER, map); On 2014/09/16 02:35:27, Ilya Sherman wrote: > nit: Please use "numbers_[index]" rather than "numbers_.at(index)", as the > latter can throw an exception, whereas Chromium code does not handle exceptions. 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/patchset/381613005/320001
Message was sent while issue was closed.
Committed patchset #11 (id:320001) as 7b8aab3fe33ac8c5eb34d18b19ef44413d8111f7
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c011630e025f01ffcc8a926e92b9eb413c920b1a Cr-Commit-Position: refs/heads/master@{#295200} |
