Highlight fields we know to be invalid but have no way of locally checking.

Highlight fields we know to be invalid but have no way of locally checking.

In this specific case, expired Online Wallet instruments.

R=groby@chromium.org,estade@chromium.org
TBR=aruslan@chromium.org
BUG=168680
TEST=unit_tests and manual

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

[Dan Beam, 2013-05-31 03:24:02]

i'll add a test soon, let's see how much blood I can get from your eyes first...

[groby-ooo-7-16, 2013-05-31 04:12:51]

LGTM w/ nits. Bonus points for a unit test.

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

https://codereview.chromium.org/15961007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1246: bool
invalid_exp = false;
nit: invalid_expiration. invalid_date. Something that is actually a fully formed
word :)

https://codereview.chromium.org/15961007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1255:
std::map<DialogSection, bool>::const_iterator it =
You can skip the "find" hoops - just section_editing_state_[SECTION_CC_BILLING]

If there is no entry yet, it'll create one and default it to "false". Which,
AFAICT, is the right thing to do. Less STL noise is good.

[Dan Beam, 2013-05-31 05:08:50]

i have written you a unit test AND a haiku!

+estade@ for c/b/ui/views and to nitpick as he pleases

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

https://codereview.chromium.org/15961007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1246: bool
invalid_exp = false;
On 2013/05/31 04:12:51, groby wrote:
> nit: invalid_expiration. invalid_date. Something that is actually a fully
formed
> word :)

Done.

https://codereview.chromium.org/15961007/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1255:
std::map<DialogSection, bool>::const_iterator it =
On 2013/05/31 04:12:51, groby wrote:
> You can skip the "find" hoops - just
section_editing_state_[SECTION_CC_BILLING]
> 
> If there is no entry yet, it'll create one and default it to "false". Which,
> AFAICT, is the right thing to do. Less STL noise is good.

const, the hoops must stay
std::set, a better way
refactor someday

[tfarina, 2013-05-31 05:17:07]

please, update the TEST line before landing this.

[Dan Beam, 2013-05-31 05:18:21]

On 2013/05/31 05:17:07, tfarina wrote:
> please, update the TEST line before landing this.

Done.

[Evan Stade, 2013-05-31 22:50:09]

I think changing to a set is a fine idea.

[Evan Stade, 2013-05-31 23:06:45]

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller.h:40: enum ValidationType
{
this belongs in autofill_dialog_types.h

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller.h:41: VALIDATE_SELECT,  //
Validate existing suggestions (card might've expired).
I can't see the link between VALIDATE_SELECT and the comment. Should it be
VALIDATE_SUGGESTION?

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

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1253: } else if
(validation_type == VALIDATE_SELECT && IsPayingWithWallet()) {
instead of VALIDATE_SELECT, can you check if inputs has the same values as those
found in the masked instrument?

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1253: } else if
(validation_type == VALIDATE_SELECT && IsPayingWithWallet()) {
It also might be easier to read if you moved the if else if block into a utility
fn.

[Dan Beam, 2013-06-01 00:23:06]

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller.h:40: enum ValidationType
{
On 2013/05/31 23:06:45, Evan Stade wrote:
> this belongs in autofill_dialog_types.h

Done.

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller.h:41: VALIDATE_SELECT,  //
Validate existing suggestions (card might've expired).
On 2013/05/31 23:06:45, Evan Stade wrote:
> I can't see the link between VALIDATE_SELECT and the comment. Should it be
> VALIDATE_SUGGESTION?

Done.

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

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1253: } else if
(validation_type == VALIDATE_SELECT && IsPayingWithWallet()) {
On 2013/05/31 23:06:45, Evan Stade wrote:
> instead of VALIDATE_SELECT, can you check if inputs has the same values as
those
> found in the masked instrument?

Done.

https://codereview.chromium.org/15961007/diff/5/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1253: } else if
(validation_type == VALIDATE_SELECT && IsPayingWithWallet()) {
On 2013/05/31 23:06:45, Evan Stade wrote:
> It also might be easier to read if you moved the if else if block into a
utility
> fn.

Done.

[Dan Beam, 2013-06-01 00:25:55]

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2419: bool
AutofillDialogControllerImpl::IsCCExpirationValid(
whoops, return values are inverted, fixing

[Evan Stade, 2013-06-01 00:39:18]

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1231:
validation_type == VALIDATE_SUGGESTION) && iter->second.empty()) {
I thought you suggested we could get rid of this

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2419: bool
AutofillDialogControllerImpl::IsCCExpirationValid(
nit: IsCreditCardExpirationValid (preferred) or IsCcExpirationValid

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2431:
base::StringToInt(model->GetItemKeyForCheckedItem(), &instrument_index);
I see a lot of uses of StringToInt in this file for converting from section to
active instrument. Can you pull it into a helper function?

[Dan Beam, 2013-06-01 01:15:04]

did everything but helper function

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/15961007/diff/17001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1231:
validation_type == VALIDATE_SUGGESTION) && iter->second.empty()) {
On 2013/06/01 00:39:19, Evan Stade wrote:
> I thought you suggested we could get rid of this

Done.

[Dan Beam, 2013-06-03 09:00:55]

also did helper function, ptal

[Evan Stade, 2013-06-03 21:24:46]

lgtm

https://chromiumcodereview.appspot.com/15961007/diff/49002/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/15961007/diff/49002/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1020:
ActiveInstrument() const {
could you make an ActiveShippingAddress() while you're at it?

[Dan Beam, 2013-06-03 22:04:18]

https://chromiumcodereview.appspot.com/15961007/diff/49002/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/15961007/diff/49002/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1020:
ActiveInstrument() const {
On 2013/06/03 21:24:46, Evan Stade wrote:
> could you make an ActiveShippingAddress() while you're at it?

Done.

[commit-bot: I haz the power, 2013-06-03 22:06:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/15961007/56001

[commit-bot: I haz the power, 2013-06-03 22:52:50]

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

[commit-bot: I haz the power, 2013-06-03 23:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/15961007/56001

[commit-bot: I haz the power, 2013-06-04 08:33:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/15961007/2002

[commit-bot: I haz the power, 2013-06-04 13:51:41]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-04 16:43:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/15961007/2002

[commit-bot: I haz the power, 2013-06-04 19:19:31]

Change committed as 204012