Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(101)

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+368 lines, -64 lines) Patch
M components/autofill/core/browser/address_field.h View 1 2 3 4 5 6 1 chunk +9 lines, -9 lines 0 comments Download
M components/autofill/core/browser/address_field_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/autofill_field.h View 1 2 3 4 5 6 7 2 chunks +11 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_field.cc View 1 2 3 4 5 6 7 4 chunks +24 lines, -2 lines 0 comments Download
M components/autofill/core/browser/autofill_field_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +119 lines, -0 lines 0 comments Download
M components/autofill/core/browser/autofill_scanner.h View 1 2 3 4 5 6 3 chunks +6 lines, -6 lines 0 comments Download
M components/autofill/core/browser/autofill_scanner.cc View 1 2 3 4 5 6 2 chunks +2 lines, -3 lines 0 comments Download
M components/autofill/core/browser/credit_card_field.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +9 lines, -8 lines 0 comments Download
M components/autofill/core/browser/credit_card_field.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +48 lines, -6 lines 0 comments Download
M components/autofill/core/browser/credit_card_field_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +113 lines, -1 line 0 comments Download
M components/autofill/core/browser/email_field.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/form_field.h View 1 2 3 4 5 6 4 chunks +5 lines, -6 lines 0 comments Download
M components/autofill/core/browser/form_field.cc View 1 2 3 4 5 6 6 chunks +8 lines, -8 lines 0 comments Download
M components/autofill/core/browser/name_field.cc View 1 2 3 4 5 6 5 chunks +8 lines, -9 lines 0 comments Download
M components/autofill/core/browser/name_field_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/phone_field.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/phone_field.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M components/autofill/core/browser/phone_field_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 38 (1 generated)
Pritam Nikam
Added a patch for review. Please have a look. Thanks.
6 years, 5 months ago (2014-07-24 07:28:29 UTC) #1
kbalazs
The amount of code needed for this looks quite a lot to me, but I ...
6 years, 5 months ago (2014-07-25 00:30:09 UTC) #2
Ilya Sherman
I was hoping that we could do something more like what we do for phone ...
6 years, 5 months ago (2014-07-25 03:37:50 UTC) #3
Pritam Nikam
Thanks Ilya and kbalazs! I have added few easy pickings from review comments and incorporated ...
6 years, 5 months ago (2014-07-26 11:29:51 UTC) #4
kbalazs
On 2014/07/26 11:29:51, Pritam Nikam wrote: > Thanks Ilya and kbalazs! > > I have ...
6 years, 5 months ago (2014-07-26 23:44:50 UTC) #5
Pritam Nikam
On 2014/07/26 23:44:50, kbalazs wrote: > On 2014/07/26 11:29:51, Pritam Nikam wrote: > > Thanks ...
6 years, 5 months ago (2014-07-27 03:00:56 UTC) #6
Ilya Sherman
On 2014/07/27 03:00:56, Pritam Nikam wrote: > On 2014/07/26 23:44:50, kbalazs wrote: > > On ...
6 years, 4 months ago (2014-07-28 22:04:50 UTC) #7
Pritam Nikam
On 2014/07/28 22:04:50, Ilya Sherman wrote: > On 2014/07/27 03:00:56, Pritam Nikam wrote: > > ...
6 years, 4 months ago (2014-08-02 06:46:17 UTC) #8
Pritam Nikam
Hi Ilya, I've incorporated review comments and uploaded a new patch set (#3) for review. ...
6 years, 4 months ago (2014-08-06 16:00:03 UTC) #9
Ilya Sherman
https://codereview.chromium.org/381613005/diff/120001/components/autofill/core/browser/autofill_field.cc File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/core/browser/autofill_field.cc#newcode394 components/autofill/core/browser/autofill_field.cc:394: FormFieldData* field_data) { nit: Please add documentation for this ...
6 years, 4 months ago (2014-08-07 20:57:26 UTC) #10
Pritam Nikam
Thanks Ilya for your inputs. I've incorporated review comments and uploaded patch set #4 for ...
6 years, 4 months ago (2014-08-08 14:14:34 UTC) #11
Ilya Sherman
https://codereview.chromium.org/381613005/diff/120001/components/autofill/core/browser/credit_card_field.cc File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/120001/components/autofill/core/browser/credit_card_field.cc#newcode99 components/autofill/core/browser/credit_card_field.cc:99: const_cast<AutofillField::CreditCardNumberInfo*>( On 2014/08/08 14:14:34, Pritam Nikam wrote: > On ...
6 years, 4 months ago (2014-08-12 04:23:42 UTC) #12
Pritam Nikam
Thanks Ilya for review. I've incorporated review comments and added new patch set (#5) for ...
6 years, 4 months ago (2014-08-12 14:00:38 UTC) #13
Evan Stade
https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc#newcode104 components/autofill/core/browser/credit_card_field.cc:104: ->credit_card_number_start_index() + style-wise, a lot of this seems off ...
6 years, 4 months ago (2014-08-12 16:05:07 UTC) #14
Pritam Nikam
Thanks Evan for review. I have incorporated inputs in patch set #6. Please have a ...
6 years, 4 months ago (2014-08-12 18:39:11 UTC) #15
Evan Stade
https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc#newcode113 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 ...
6 years, 4 months ago (2014-08-12 19:19:55 UTC) #16
Pritam Nikam
On 2014/08/12 19:19:55, Evan Stade wrote: > https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc > File components/autofill/core/browser/credit_card_field.cc (right): > > https://codereview.chromium.org/381613005/diff/200001/components/autofill/core/browser/credit_card_field.cc#newcode113 ...
6 years, 4 months ago (2014-08-13 10:06:14 UTC) #17
Ilya Sherman
Please take a look at Ziran's pending CL, [ https://codereview.chromium.org/442403002/ ]. It's probably wise to ...
6 years, 4 months ago (2014-08-13 19:59:57 UTC) #18
Pritam Nikam
On 2014/08/13 19:59:57, Ilya Sherman wrote: > Please take a look at Ziran's pending CL, ...
6 years, 4 months ago (2014-08-19 14:06:44 UTC) #19
Pritam Nikam
Hi Ilya & Evan, I've incorporated const qualifier removal changes from FormField and AutofillScanner implementation. ...
6 years, 4 months ago (2014-08-20 18:06:17 UTC) #20
Pritam Nikam
Hi Ilya & Evan, Added a patch for review. Please have a look. Thanks!
6 years, 4 months ago (2014-08-22 04:29:13 UTC) #21
Evan Stade
sorry for the delay! I made these comments but forgot to send them. https://codereview.chromium.org/381613005/diff/240001/components/autofill/core/browser/autofill_field.cc File ...
6 years, 3 months ago (2014-08-28 18:18:42 UTC) #22
Pritam Nikam
Thanks Evan for review comments. I've incorporated concern modifications in new patch set (#8) PTAL. ...
6 years, 3 months ago (2014-09-01 09:03:35 UTC) #23
Evan Stade
lgtm
6 years, 3 months ago (2014-09-02 15:50:55 UTC) #24
Ilya Sherman
https://codereview.chromium.org/381613005/diff/260001/components/autofill/core/browser/autofill_field_unittest.cc File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/381613005/diff/260001/components/autofill/core/browser/autofill_field_unittest.cc#newcode45 components/autofill/core/browser/autofill_field_unittest.cc:45: size_t GetNumberOffset(size_t index, TestCase test) { nit: Please pass ...
6 years, 3 months ago (2014-09-03 01:07:51 UTC) #25
Pritam Nikam
Thanks Ilya and Evan. @ Ilya, Regarding early bail out case when parsing logic for ...
6 years, 3 months ago (2014-09-03 12:21:51 UTC) #26
Ilya Sherman
By the way, now that I've taken another look at this code, I don't see ...
6 years, 3 months ago (2014-09-03 23:25:27 UTC) #27
Evan Stade
On 2014/09/03 23:25:27, Ilya Sherman wrote: > By the way, now that I've taken another ...
6 years, 3 months ago (2014-09-03 23:58:22 UTC) #28
Ilya Sherman
On 2014/09/03 23:58:22, Evan Stade wrote: > On 2014/09/03 23:25:27, Ilya Sherman wrote: > > ...
6 years, 3 months ago (2014-09-04 01:28:43 UTC) #29
Pritam Nikam
On 2014/09/03 23:25:27, Ilya Sherman wrote: > By the way, now that I've taken another ...
6 years, 3 months ago (2014-09-04 06:55:56 UTC) #30
Pritam Nikam
Thanks Ilya and Evan for review. I've incorporated the review comments. Also fixed DCHECK abort ...
6 years, 3 months ago (2014-09-04 06:58:03 UTC) #31
Ilya Sherman
With apologies for the delay of this reply: https://codereview.chromium.org/381613005/diff/300001/components/autofill/core/browser/credit_card_field.cc File components/autofill/core/browser/credit_card_field.cc (right): https://codereview.chromium.org/381613005/diff/300001/components/autofill/core/browser/credit_card_field.cc#newcode116 components/autofill/core/browser/credit_card_field.cc:116: credit_card_field->is_valid_card_number_split_ ...
6 years, 3 months ago (2014-09-16 02:35:27 UTC) #32
Pritam Nikam
Thanks Ilya for inputs. I've adopted 2nd option as you've suggested i.e. to swallow (i.e. ...
6 years, 3 months ago (2014-09-16 10:49:36 UTC) #33
Ilya Sherman
LGTM, thanks.
6 years, 3 months ago (2014-09-16 21:55:33 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/381613005/320001
6 years, 3 months ago (2014-09-16 21:57:21 UTC) #36
commit-bot: I haz the power
Committed patchset #11 (id:320001) as 7b8aab3fe33ac8c5eb34d18b19ef44413d8111f7
6 years, 3 months ago (2014-09-17 00:58:22 UTC) #37
commit-bot: I haz the power
6 years, 3 months ago (2014-09-17 00:58:57 UTC) #38
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/c011630e025f01ffcc8a926e92b9eb413c920b1a
Cr-Commit-Position: refs/heads/master@{#295200}

Powered by Google App Engine
This is Rietveld 408576698