Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141843005:
rAc: fix some validation issues (Closed)
|
Created:
6 years, 10 months ago by Evan Stade Modified:
6 years, 10 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionrAc: fix some validation issues
1) hide bubble after fixing a missing month or year
2) CVC validity should be based on card type best guess
3) cross-field validation shouldn't override single-field validation, but fields that are validly individually shouldn't skip cross-field validation just because their partner field is invalid (necessary for \#2)
BUG=336998
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248571
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #Patch Set 3 : not const after all #Patch Set 4 : re-up #Patch Set 5 : merge #Patch Set 6 : merge #
Messages
Total messages: 57 (0 generated)
https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1764: // If the field is valid but can be invalidated by inter-field validation, example of the problem this caused: 1) CC is invalid (but type can be determined as Visa) 2) CVC is invalid (4 digits) thus) CC is not in field_values, so CVC doesn't get cross-validated. Since it is non-empty, it's marked as valid.
lgtm but groby@ should take a look as well https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_types.cc (right): https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_types.cc:140: MessageMap::iterator iter = messages_.find(field); nit: const_iterator
I'd prefer tests for the new behavior. https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1764: // If the field is valid but can be invalidated by inter-field validation, On 2014/01/31 00:22:13, Evan Stade wrote: > example of the problem this caused: > > 1) CC is invalid (but type can be determined as Visa) > 2) CVC is invalid (4 digits) > thus) CC is not in field_values, so CVC doesn't get cross-validated. Since it is > non-empty, it's marked as valid. I'm not sure this addresses that issue without side effects. For one, this will now treat the month as invalid if no year is picked yet. I'm also not sure what sure = false does in the phone number case. Do we want inter-field validation to _always_ occur? In which case, yay for deleting code.
LGTM if you're fine with "inter-field happens always"
https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/141843005/diff/1/chrome/browser/ui/autofill/a... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1764: // If the field is valid but can be invalidated by inter-field validation, On 2014/01/31 19:59:59, groby wrote: > On 2014/01/31 00:22:13, Evan Stade wrote: > > example of the problem this caused: > > > > 1) CC is invalid (but type can be determined as Visa) > > 2) CVC is invalid (4 digits) > > thus) CC is not in field_values, so CVC doesn't get cross-validated. Since it > is > > non-empty, it's marked as valid. > > I'm not sure this addresses that issue without side effects. For one, this will > now treat the month as invalid if no year is picked yet. I'm also not sure what > sure = false does in the phone number case. > > Do we want inter-field validation to _always_ occur? In which case, yay for > deleting code. yes, we want it to always occur, so that we can validate field X based on the value of field Y even if field Y is already known to be invalid. In the expiration date case, the InputWasEdited checks prevent what you describe from happening.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_aura for step(s) app_list_unittests, aura_unittests, browser_tests, compositor_unittests, content_browsertests, content_unittests, events_unittests, interactive_ui_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/60002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/60002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/60002
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
Failed to trigger a try job on linux_clang HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/920001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/920001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Hunk #1 FAILED at 1745.
Hunk #2 succeeded at 1843 (offset 86 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej
Patch: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
4005cd637740de78e4b893536150352e6f725c0c..994fbbed790f85022c3f510b1aee56ed8e3de0bb
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -1745,7 +1745,6 @@ ValidityMessages
AutofillDialogControllerImpl::InputsAreValid(
DialogSection section,
const FieldValueMap& inputs) {
ValidityMessages messages;
- FieldValueMap field_values;
for (FieldValueMap::const_iterator iter = inputs.begin();
iter != inputs.end(); ++iter) {
const ServerFieldType type = iter->first;
@@ -1757,23 +1756,11 @@ ValidityMessages
AutofillDialogControllerImpl::InputsAreValid(
// it to be valid unless later proven otherwise.
bool sure = InputWasEdited(type, iter->second) ||
AutofillType(type).GetStorableType() == ADDRESS_HOME_COUNTRY;
-
- // Consider only individually valid fields for inter-field validation.
- if (text.empty()) {
- field_values[type] = iter->second;
- // If the field is valid but can be invalidated by inter-field
validation,
- // assume it to be unsure.
- if (type == CREDIT_CARD_EXP_4_DIGIT_YEAR ||
- type == CREDIT_CARD_EXP_MONTH ||
- type == CREDIT_CARD_VERIFICATION_CODE ||
- type == PHONE_HOME_WHOLE_NUMBER ||
- type == PHONE_BILLING_WHOLE_NUMBER) {
- sure = false;
- }
- }
messages.Set(type, ValidityMessage(text, sure));
}
+ // For the convenience of using operator[].
+ FieldValueMap& field_values = const_cast<FieldValueMap&>(inputs);
// Validate the date formed by month and year field. (Autofill dialog is
// never supposed to have 2-digit years, so not checked).
if (field_values.count(CREDIT_CARD_EXP_4_DIGIT_YEAR) &&
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/1190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/141843005/1240004
Message was sent while issue was closed.
Change committed as 248571
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
