Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1811543002:
[Autofill] Fill text state fields that have a max length of 2 with the state abbreviation. (Closed)
|
Created:
4 years, 9 months ago by sebsg Modified:
4 years, 9 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY.
BUG=595104
TEST=AutofillFieldTest
Committed: https://crrev.com/f72852358bb018390318b4b95775294f0a429eb8
Cr-Commit-Position: refs/heads/master@{#381792}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed comments and made code more robust #
Total comments: 2
Patch Set 3 : Nit #
Messages
Total messages: 20 (10 generated)
sebsg@chromium.org changed reviewers: + mathp@chromium.org, vabr@chromium.org
Could you please take a look? Thanks!
Description was changed from ========== [Autofill] Fill text state fields that have a max length of 2 with the state abbreviation. Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest ========== to ========== Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest ==========
Hi Sebastien, LGTM with comments. Cheers, Vaclav https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field.cc:422: if (field->max_length != 2) { The "2" here is related to the data used in GetAbbreviationForName. If we ever extend that to non-U.S. states, we might want to allow for different lengths of abbreviations. So what about: if (field->max_length >= value.size()) { field->value = value; } else { base::string16 abbreviation; state_names::GetNameAndAbbreviation(value, nullptr, &abbreviation); if (field->max_length >= abbreviation.size()) field->value = base::i18n::ToUpper(abbreviation); } https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field.cc:428: state_names::GetNameAndAbbreviation(value, &full, &abbreviation); &full -> nullptr https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field_unittest.cc:901: for (TestCase test_case : test_cases) { nit: const TestCase& to avoid copies
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks for the review! Could you take another look when you get the chance? Thanks! https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field.cc:422: if (field->max_length != 2) { On 2016/03/16 16:38:45, vabr (Chromium) wrote: > The "2" here is related to the data used in GetAbbreviationForName. If we ever > extend that to non-U.S. states, we might want to allow for different lengths of > abbreviations. So what about: > > if (field->max_length >= value.size()) { > field->value = value; > } else { > base::string16 abbreviation; > state_names::GetNameAndAbbreviation(value, nullptr, &abbreviation); > if (field->max_length >= abbreviation.size()) > field->value = base::i18n::ToUpper(abbreviation); > } Good idea, I also added a check to 0 (default value) and changed the method to return false if we don't fill. That value will be returned by AutofillField::FillFormField to indicate whether filling was successful or not. Thanks! https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field.cc:428: state_names::GetNameAndAbbreviation(value, &full, &abbreviation); On 2016/03/16 16:38:45, vabr (Chromium) wrote: > &full -> nullptr Didn't think of that, thanks! https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field_unittest.cc:901: for (TestCase test_case : test_cases) { On 2016/03/16 16:38:45, vabr (Chromium) wrote: > nit: const TestCase& to avoid copies Done.
LGTM, thank you! Vaclav https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1811543002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_field.cc:422: if (field->max_length != 2) { On 2016/03/16 23:41:01, sebsg wrote: > On 2016/03/16 16:38:45, vabr (Chromium) wrote: > > The "2" here is related to the data used in GetAbbreviationForName. If we ever > > extend that to non-U.S. states, we might want to allow for different lengths > of > > abbreviations. So what about: > > > > if (field->max_length >= value.size()) { > > field->value = value; > > } else { > > base::string16 abbreviation; > > state_names::GetNameAndAbbreviation(value, nullptr, &abbreviation); > > if (field->max_length >= abbreviation.size()) > > field->value = base::i18n::ToUpper(abbreviation); > > } > > Good idea, I also added a check to 0 (default value) and changed the method to > return false if we don't fill. That value will be returned by > AutofillField::FillFormField to indicate whether filling was successful or not. > Thanks! Acknowledged, both your improvements SGTM. https://codereview.chromium.org/1811543002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1811543002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:431: if (abbreviation.size() != 0 && field->max_length >= abbreviation.size()) { optional nit: "!abbreviation.empty()" instead of "size() != 0"; the reason being better readability, and also only asking for the least information needed (avoiding potential performance cost, although negligible here).
Thanks! https://codereview.chromium.org/1811543002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1811543002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:431: if (abbreviation.size() != 0 && field->max_length >= abbreviation.size()) { On 2016/03/17 09:03:37, vabr (Chromium) wrote: > optional nit: "!abbreviation.empty()" instead of "size() != 0"; the reason being > better readability, and also only asking for the least information needed > (avoiding potential performance cost, although negligible here). Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1811543002/#ps80001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811543002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811543002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by sebsg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811543002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811543002/80001
Message was sent while issue was closed.
Description was changed from ========== Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest ========== to ========== Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest ========== to ========== Some text state fields have a maxlength of two. This can cause problem for some states. For example New York is filled as NE (Nebraska) instead of NY. BUG=595104 TEST=AutofillFieldTest Committed: https://crrev.com/f72852358bb018390318b4b95775294f0a429eb8 Cr-Commit-Position: refs/heads/master@{#381792} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f72852358bb018390318b4b95775294f0a429eb8 Cr-Commit-Position: refs/heads/master@{#381792} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
