[Autofill] Update credit card type detection logic.

[Autofill] Update credit card type detection logic.

* Remove Solo card support, as Solo cards have been decommissioned permanently since March 31, 2011.
* Update the BIN range for DiscoverCard to include 644-649.
* Correct the BIN ranges for Diner's Club and JCB cards.
* Add test coverage

BUG=246497
TEST=CreditCardTest.GetCreditCardType
R=estade@chromium.org
TBR=joi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207809

[benquan, 2013-06-08 07:27:57]

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
File components/autofill/browser/credit_card.cc (right):

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:166: return kGenericCard;
There are tons of card types, I think kGenericCard means all other types that we
do not really need to care about or know about. But they are valid types.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:172: //
http://www.beachnet.com/~hstiles/cardtype.html
this link is dead, remove it?

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:242: return kGenericCard;
Card type should be determined by BIN (the first 6 digits of a card), not
necessary  the length of the card. It's a common practice in payment industry.
Number of digits should be used to validate the card.

For example: 

CreditCard c;
c.SetNumber("34123");

now, "c" should be considered as an Invalid kAmericanExpressCard, not an Invalid
kGenericCard;

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
File components/autofill/browser/credit_card_unittest.cc (right):

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card_unittest.cc:448: }
Please add a test for kGenericCard and some negative tests, like wrong length of
card number.

[Evan Stade, 2013-06-10 18:59:47]

https://codereview.chromium.org/16254010/diff/5001/components/autofill/browse...
File components/autofill/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/16254010/diff/5001/components/autofill/browse...
components/autofill/browser/credit_card_unittest.cc:409:
EXPECT_EQ(kAmericanExpressCard, GetCreditCardType("378282246310005"));
I think the most common way to do this in tests is to have a struct of test
cases and then loop through them. It makes it a bit easier to read individual
test cases since they look like:

...
{ kDinersCard, "38520000023237" }
...

plus the code is all together (instead of pulling out related code into a
utility function at the other end of the file)

[Ilya Sherman, 2013-06-18 03:37:38]

Hokay, I've re-written this CL to separate out credit card type detection from
validation.  AFAICT, no existing code was explicitly relying on
CreditCard::GetCreditCardType() to handle both, though I might have missed
something...

I also caught a few more instances of the Solo card, which I've nixed in the
latest patch set.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
File components/autofill/browser/credit_card.cc (right):

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:166: return kGenericCard;
On 2013/06/08 07:27:57, benquan wrote:
> There are tons of card types, I think kGenericCard means all other types that
we
> do not really need to care about or know about. But they are valid types.

Done.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:172: //
http://www.beachnet.com/~hstiles/cardtype.html
On 2013/06/08 07:27:57, benquan wrote:
> this link is dead, remove it?

I've kept the link, because it seems to be where the length checks were derived
from.  I've also added a link to an archived version of the page, so that the
dead link isn't so disappointing.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card.cc:242: return kGenericCard;
On 2013/06/08 07:27:57, benquan wrote:
> Card type should be determined by BIN (the first 6 digits of a card), not
> necessary  the length of the card. It's a common practice in payment industry.
> Number of digits should be used to validate the card.
> 
> For example: 
> 
> CreditCard c;
> c.SetNumber("34123");
> 
> now, "c" should be considered as an Invalid kAmericanExpressCard, not an
Invalid
> kGenericCard;

Done.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
File components/autofill/browser/credit_card_unittest.cc (right):

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card_unittest.cc:409:
EXPECT_EQ(kAmericanExpressCard, GetCreditCardType("378282246310005"));
On 2013/06/10 18:59:47, Evan Stade wrote:
> I think the most common way to do this in tests is to have a struct of test
> cases and then loop through them. It makes it a bit easier to read individual
> test cases since they look like:
> 
> ...
> { kDinersCard, "38520000023237" }
> ...
> 
> plus the code is all together (instead of pulling out related code into a
> utility function at the other end of the file)

Done.

https://chromiumcodereview.appspot.com/16254010/diff/5001/components/autofill...
components/autofill/browser/credit_card_unittest.cc:448: }
On 2013/06/08 07:27:57, benquan wrote:
> Please add a test for kGenericCard and some negative tests, like wrong length
of
> card number.

Good call -- done.

[Evan Stade, 2013-06-19 00:43:35]

lgtm

[Ilya Sherman, 2013-06-19 00:46:39]

+jamesr for webkit/ OWNER

[jamesr, 2013-06-19 18:09:06]

lgtm

[commit-bot: I haz the power, 2013-06-21 03:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/16254010/41001

[commit-bot: I haz the power, 2013-06-21 03:59:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ilya Sherman, 2013-06-21 04:04:34]

TBR Jói for diff to component_strings.grd

[commit-bot: I haz the power, 2013-06-21 04:04:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/16254010/41001

[Jói, 2013-06-21 11:27:31]

LGTM component_strings.grd

On Fri, Jun 21, 2013 at 4:04 AM,  <isherman@chromium.org> wrote:
> TBR Jói for diff to component_strings.grd
>
> https://chromiumcodereview.appspot.com/16254010/

[commit-bot: I haz the power, 2013-06-21 14:30:27]

Change committed as 207809