Autofill: Better recognize credit card type fields.

Autofill: Better recognize credit card type fields.

BUG=464002, 466685, 469012
Committed: https://crrev.com/e2f6955f2d3c01c6ec05a54a5848b4827201397a
Cr-Commit-Position: refs/heads/master@{#322325}

[Lei Zhang, 2015-03-25 05:05:50]

Patchset #2 (id:20001) has been deleted

[Lei Zhang, 2015-03-25 18:37:08]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Lei Zhang, 2015-03-25 18:37:08]

https://codereview.chromium.org/1001193002/ doesn't apply at ToT anymore, so I
started a new CL with the most useful parts of that CL.

[Evan Stade, 2015-03-25 21:25:51]

mostly lg

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:529: bool
AutofillField::FindValueInCreditTypeSelectControl(
nothing about this looks credit card specific (?)

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:542:
base::RemoveChars(base::StringToLowerASCII(field.option_contents[i]),
I don't think assuming this is ASCII and failing on all non-ascii stuff is
correct; you can strip whitespace but after that compare should probably use
base::CaseInsensitiveCompare

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (left):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:95: MATCH_DEFAULT |
MATCH_SELECT,
so we'll no longer fill text inputs?

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:251: const char* const
kCardBrands[] = {
nit: this is a little over-engineered imo. You can just call
FindValueInCreditTypeSelectControl twice.

Also, instead of new constants, how about CreditCard::TypeForFill

[Lei Zhang, 2015-03-25 22:18:48]

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:529: bool
AutofillField::FindValueInCreditTypeSelectControl(
On 2015/03/25 21:25:51, Evan Stade wrote:
> nothing about this looks credit card specific (?)

Done.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:542:
base::RemoveChars(base::StringToLowerASCII(field.option_contents[i]),
On 2015/03/25 21:25:51, Evan Stade wrote:
> I don't think assuming this is ASCII and failing on all non-ascii stuff is
> correct; you can strip whitespace but after that compare should probably use
> base::CaseInsensitiveCompare

base::StringToLowerASCII() doesn't fail if it encounters non-ASCII characters.
It only converts ASCII chars to lower case. Non-ASCII chars are unmodified. i.e.
ABCйцу becomes abcйцу.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (left):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:95: MATCH_DEFAULT |
MATCH_SELECT,
On 2015/03/25 21:25:51, Evan Stade wrote:
> so we'll no longer fill text inputs?

Correct. None of the websites in our heuristics tests make the user type in the
type of their card into text input.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:251: const char* const
kCardBrands[] = {
On 2015/03/25 21:25:51, Evan Stade wrote:
> nit: this is a little over-engineered imo. You can just call
> FindValueInCreditTypeSelectControl twice.
> 
> Also, instead of new constants, how about CreditCard::TypeForFill

I'm just going to use IDS_AUTOFILL_CC_FOO since TypeForFill() is in an anonymous
namespace within credit_card.cc.

[Evan Stade, 2015-03-25 22:23:08]

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:542:
base::RemoveChars(base::StringToLowerASCII(field.option_contents[i]),
On 2015/03/25 22:18:48, Lei Zhang wrote:
> On 2015/03/25 21:25:51, Evan Stade wrote:
> > I don't think assuming this is ASCII and failing on all non-ascii stuff is
> > correct; you can strip whitespace but after that compare should probably use
> > base::CaseInsensitiveCompare
> 
> base::StringToLowerASCII() doesn't fail if it encounters non-ASCII characters.
> It only converts ASCII chars to lower case. Non-ASCII chars are unmodified.
i.e.
> ABCйцу becomes abcйцу.

my point is that upper case accented character won't get converted to lower case
accented character.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (left):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:95: MATCH_DEFAULT |
MATCH_SELECT,
On 2015/03/25 22:18:48, Lei Zhang wrote:
> On 2015/03/25 21:25:51, Evan Stade wrote:
> > so we'll no longer fill text inputs?
> 
> Correct. None of the websites in our heuristics tests make the user type in
the
> type of their card into text input.

k, seems fine. Can you put a comment to that effect? I believe that is unique to
this datatype.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:251: const char* const
kCardBrands[] = {
On 2015/03/25 22:18:48, Lei Zhang wrote:
> On 2015/03/25 21:25:51, Evan Stade wrote:
> > nit: this is a little over-engineered imo. You can just call
> > FindValueInCreditTypeSelectControl twice.
> > 
> > Also, instead of new constants, how about CreditCard::TypeForFill
> 
> I'm just going to use IDS_AUTOFILL_CC_FOO since TypeForFill() is in an
anonymous
> namespace within credit_card.cc.

sgtm

[Lei Zhang, 2015-03-25 22:39:03]

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:542:
base::RemoveChars(base::StringToLowerASCII(field.option_contents[i]),
On 2015/03/25 22:23:08, Evan Stade wrote:
> On 2015/03/25 22:18:48, Lei Zhang wrote:
> > On 2015/03/25 21:25:51, Evan Stade wrote:
> > > I don't think assuming this is ASCII and failing on all non-ascii stuff is
> > > correct; you can strip whitespace but after that compare should probably
use
> > > base::CaseInsensitiveCompare
> > 
> > base::StringToLowerASCII() doesn't fail if it encounters non-ASCII
characters.
> > It only converts ASCII chars to lower case. Non-ASCII chars are unmodified.
> i.e.
> > ABCйцу becomes abcйцу.
> 
> my point is that upper case accented character won't get converted to lower
case
> accented character.

Sure, that's a valid point, but I just want to move some code from
FillCreditCardTypeSelectControl() in this CL. Let's work on the non-ASCII lower
case issue separately.

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card_field.cc (left):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card_field.cc:95: MATCH_DEFAULT |
MATCH_SELECT,
On 2015/03/25 22:23:08, Evan Stade wrote:
> On 2015/03/25 22:18:48, Lei Zhang wrote:
> > On 2015/03/25 21:25:51, Evan Stade wrote:
> > > so we'll no longer fill text inputs?
> > 
> > Correct. None of the websites in our heuristics tests make the user type in
> the
> > type of their card into text input.
> 
> k, seems fine. Can you put a comment to that effect? I believe that is unique
to
> this datatype.

Done.

[Evan Stade, 2015-03-25 22:39:54]

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:542:
base::RemoveChars(base::StringToLowerASCII(field.option_contents[i]),
On 2015/03/25 22:39:03, Lei Zhang wrote:
> On 2015/03/25 22:23:08, Evan Stade wrote:
> > On 2015/03/25 22:18:48, Lei Zhang wrote:
> > > On 2015/03/25 21:25:51, Evan Stade wrote:
> > > > I don't think assuming this is ASCII and failing on all non-ascii stuff
is
> > > > correct; you can strip whitespace but after that compare should probably
> use
> > > > base::CaseInsensitiveCompare
> > > 
> > > base::StringToLowerASCII() doesn't fail if it encounters non-ASCII
> characters.
> > > It only converts ASCII chars to lower case. Non-ASCII chars are
unmodified.
> > i.e.
> > > ABCйцу becomes abcйцу.
> > 
> > my point is that upper case accented character won't get converted to lower
> case
> > accented character.
> 
> Sure, that's a valid point, but I just want to move some code from
> FillCreditCardTypeSelectControl() in this CL. Let's work on the non-ASCII
lower
> case issue separately.

seems like it would only take about 5 minutes of effort. Is there something
tricky to it that I'm missing?

[Lei Zhang, 2015-03-25 22:48:33]

On 2015/03/25 22:39:54, Evan Stade wrote:
> seems like it would only take about 5 minutes of effort. Is there something
> tricky to it that I'm missing?

It's a separate issue. I just don't see why it has to be in this CL. We can also
add a unit test for FindValueInSelectControl() and it's a perfectly good CL on
its own.

[Evan Stade, 2015-03-25 22:53:12]

On 2015/03/25 22:48:33, Lei Zhang wrote:
> On 2015/03/25 22:39:54, Evan Stade wrote:
> > seems like it would only take about 5 minutes of effort. Is there something
> > tricky to it that I'm missing?
> 
> It's a separate issue. I just don't see why it has to be in this CL. We can
also
> add a unit test for FindValueInSelectControl() and it's a perfectly good CL on
> its own.

you're refactoring old code. Taking a look at it and improving it is a natural
corollary. I would agree it's worthy of its own CL if it took more than 5
minutes. Adding a unit test would take more than 5 minutes. So if you want to
file a bug against yourself instead of doing it now, I think that's valid.

[Lei Zhang, 2015-03-25 23:41:54]

On 2015/03/25 22:53:12, Evan Stade wrote:
> you're refactoring old code. Taking a look at it and improving it is a natural
> corollary. I would agree it's worthy of its own CL if it took more than 5
> minutes. Adding a unit test would take more than 5 minutes. So if you want to
> file a bug against yourself instead of doing it now, I think that's valid.

Filed http://crbug.com/470726. Shall we get on with this CL as is?

[Evan Stade, 2015-03-25 23:44:15]

lgtm

https://codereview.chromium.org/1030073003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:532: // Try stripping off
spaces.
add link to bug

[Lei Zhang, 2015-03-26 02:12:36]

https://codereview.chromium.org/1030073003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/1030073003/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_field.cc:532: // Try stripping off
spaces.
On 2015/03/25 23:44:15, Evan Stade wrote:
> add link to bug

Done.

[Lei Zhang, 2015-03-26 04:13:57]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-03-26 04:13:57]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1030073003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-26 04:14:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1030073003/120001

[commit-bot: I haz the power, 2015-03-26 05:09:36]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-03-26 05:10:33]

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