[Autofill] Credit Card validation for rAc.

[Autofill] Credit Card validation for rAc.

Validate all things credit card. (Number, CSC, Expiration)
BUG=170467
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181861

[groby-ooo-7-16, 2013-02-07 23:14:47]

Technically, the validation for expiration dates in the rAc dialog is
unnecessary, since the fields are selects that are pre-populated with valid
values. If I hadn't worked with a cold-fogged mind yesterday, I would've noticed
that before writing the code :)

The only one error we could catch right now is the month, and that would require
us to know about the year. The code as is doesn't like cross-field dependencies,
though.

Questions:

1) Do I still leave the validation in the controller?
2) If I don't, do I leave the validation code in validation.cc?

[Evan Stade, 2013-02-08 15:48:14]

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:71: size_t separator =
text.find(kSeparatorSlash);
Seems like you're doing a lot of work to parse something g that should be in a
very predictable format and not need parsing (since it comes from a combobox)

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:73: year_text = text.substr(separator+1);
Spaces around operators.

https://codereview.chromium.org/12213077/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12213077/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:445: case
CREDIT_CARD_EXP_2_DIGIT_YEAR:
You only need to handle the types in Show()

[groby-ooo-7-16, 2013-02-09 00:01:48]

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:71: size_t separator =
text.find(kSeparatorSlash);
Killing it, since we don't validate fields of that type.
On 2013/02/08 15:48:14, Evan Stade wrote:
> Seems like you're doing a lot of work to parse something g that should be in a
> very predictable format and not need parsing (since it comes from a combobox)

https://codereview.chromium.org/12213077/diff/1/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:73: year_text = text.substr(separator+1);
It's dead, Jim.
On 2013/02/08 15:48:14, Evan Stade wrote:
> Spaces around operators.

https://codereview.chromium.org/12213077/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12213077/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:445: case
CREDIT_CARD_EXP_2_DIGIT_YEAR:
Done. 

Added NOTREACHED default to make clear this can't be validated.

On 2013/02/08 15:48:14, Evan Stade wrote:
> You only need to handle the types in Show()

[Evan Stade, 2013-02-11 00:25:14]

Lgtm

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:436: using
base::Time;
No longer needed

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:440: case
EMAIL_ADDRESS: break;
I would prefer you put break on its own line (or at least be consistent about
it)

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:447: NOTREACHED()
<< "Validation is not called for <select>";
Can we remove this block and let the default case handle it?

[Ilya Sherman, 2013-02-11 05:39:51]

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.cc:10: #include
"base/utf_string_conversions.h"
nit: Not needed?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation.h (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.h:14: }  // namespace base;
nit: Not needed?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.h:23: bool IsValidCreditCardCSC(const
string16& text);
nit: Let's spell out "(card) security code" rather than using "CSC".

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation_unittest.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation_unittest.cc:5: #include "base/time.h"
nit: No longer needed?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation_unittest.cc:16: };
nit: No longer used?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:462: NOTREACHED()
<< "Trying to validate unknown field";
nit: Please remove the logging text.  You can include it as a comment above this
line if you'd like.

[groby-ooo-7-16, 2013-02-11 22:47:53]

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.cc:10: #include
"base/utf_string_conversions.h"
Left over from patch 1 - removed.
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: Not needed?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation.h (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.h:14: }  // namespace base;
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation.h:23: bool IsValidCreditCardCSC(const
string16& text);
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: Let's spell out "(card) security code" rather than using "CSC".

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
File chrome/browser/autofill/validation_unittest.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation_unittest.cc:5: #include "base/time.h"
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: No longer needed?

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/autofill/va...
chrome/browser/autofill/validation_unittest.cc:16: };
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: No longer used?

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:436: using
base::Time;
On 2013/02/11 00:25:14, Evan Stade wrote:
> No longer needed

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:440: case
EMAIL_ADDRESS: break;
On 2013/02/11 00:25:14, Evan Stade wrote:
> I would prefer you put break on its own line (or at least be consistent about
> it)

Done.

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:447: NOTREACHED()
<< "Validation is not called for <select>";
I'd prefer to leave this NOTREACHED. If program flow gets here, something
substantially different from the default flow is broken.

If you still insist, I'm not super-tied to it :)
On 2013/02/11 00:25:14, Evan Stade wrote:
> Can we remove this block and let the default case handle it?

https://codereview.chromium.org/12213077/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:462: NOTREACHED()
<< "Trying to validate unknown field";
On 2013/02/11 05:39:51, Ilya Sherman wrote:
> nit: Please remove the logging text.  You can include it as a comment above
this
> line if you'd like.

Done.

[Ilya Sherman, 2013-02-11 22:55:10]

LGTM with a nit.  Thanks.

https://codereview.chromium.org/12213077/diff/8/chrome/browser/autofill/valid...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/8/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:9: #include "base/time.h"
nit: Both of these #includes look like they are not needed.

[groby-ooo-7-16, 2013-02-11 23:27:07]

https://codereview.chromium.org/12213077/diff/8/chrome/browser/autofill/valid...
File chrome/browser/autofill/validation.cc (right):

https://codereview.chromium.org/12213077/diff/8/chrome/browser/autofill/valid...
chrome/browser/autofill/validation.cc:9: #include "base/time.h"
Oops, sorry. Done.
On 2013/02/11 22:55:10, Ilya Sherman wrote:
> nit: Both of these #includes look like they are not needed.

[commit-bot: I haz the power, 2013-02-11 23:27:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/12213077/10006

[commit-bot: I haz the power, 2013-02-12 05:06:36]

Change committed as 181861