Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639563002:
[Autofill] Fill from the last digits when filling a phone number with a maximum length. (Closed)
|
Created:
4 years, 11 months ago by sebsg Modified:
4 years, 11 months ago Reviewers:
Mathieu 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. |
DescriptionFor phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits.
BUG=579674
TEST=AutofillFieldTest
Committed: https://crrev.com/1aea64bdff908c94c4c032935d3c3b9a906afd91
Cr-Commit-Position: refs/heads/master@{#371946}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Rebase #Patch Set 4 : Nits #
Messages
Total messages: 19 (11 generated)
Description was changed from ========== [Autofill] Fill from the last digits when filling a phone number with a maximum length. BUG=579674 ========== to ========== [Autofill] Fill from the last digits when filling a phone number with a maximum length. BUG=579674 TEST=AutofillFieldTest ==========
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Description was changed from ========== [Autofill] Fill from the last digits when filling a phone number with a maximum length. BUG=579674 TEST=AutofillFieldTest ========== to ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits (so generally removing the country code). BUG=579674 TEST=AutofillFieldTest ==========
Description was changed from ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits (so generally removing the country code). BUG=579674 TEST=AutofillFieldTest ========== to ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits. BUG=579674 TEST=AutofillFieldTest ==========
Patchset #1 (id:1) has been deleted
PTAL
https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:560: if (number.length() == In which cases will the phone number size be 7? I'm thinking almost never. This appears to be well intentioned logic that may never work. Let's put a TODO to investigate if libphonenumber can help here. It can be done in a subsequent CL since you are just refactoring this block. https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:578: if (field_data.max_length == PhoneNumber::kCityAndNumberLength && Why should we check for max_length == 10? It feels like it could be applicable to other situations such as european phone numbers. https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:262: TEST(AutofillFieldTest, FillPhoneNumber) { Maybe I'm spoiled by your other changes, but can we refactor these tests to be done with TestCase structs? Sounds like HtmlType, value to fill, expected value, max_length are the things that keep coming back from test to test. It will give us velocity later on. https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:287: field.SetHtmlType(HTML_TYPE_UNSPECIFIED, HtmlFieldMode()); HTML_TYPE_TEL? https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
Patchset #2 (id:40001) has been deleted
Thanks for your comments! https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:560: if (number.length() == On 2016/01/26 14:43:01, Mathieu Perreault wrote: > In which cases will the phone number size be 7? I'm thinking almost never. This > appears to be well intentioned logic that may never work. Let's put a TODO to > investigate if libphonenumber can help here. It can be done in a subsequent CL > since you are just refactoring this block. Done. https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:578: if (field_data.max_length == PhoneNumber::kCityAndNumberLength && On 2016/01/26 14:43:02, Mathieu Perreault wrote: > Why should we check for max_length == 10? It feels like it could be applicable > to other situations such as european phone numbers. I thought it was safer that way, to only cut the front part when we are pretty sure it's gonna help. However by looking at the test, I realize that the result would be bad anyway if we cut the from. However the current way is pretty much always wrong (cutting the back). We might event catch some nice use case in Europe like you mentioned now. Thanks https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:262: TEST(AutofillFieldTest, FillPhoneNumber) { On 2016/01/26 14:43:02, Mathieu Perreault wrote: > Maybe I'm spoiled by your other changes, but can we refactor these tests to be > done with TestCase structs? Sounds like HtmlType, value to fill, expected value, > max_length are the things that keep coming back from test to test. > > It will give us velocity later on. It's a good habit to take. It's so much easier to understand and to add tests. Plus it made me realize a couple of improvements that could be made. Plus I added test cases. Thanks! https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:287: field.SetHtmlType(HTML_TYPE_UNSPECIFIED, HtmlFieldMode()); On 2016/01/26 14:43:02, Mathieu Perreault wrote: > HTML_TYPE_TEL? > > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Done.
Just nits about comments. Otherwise lgtm https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:262: TEST(AutofillFieldTest, FillPhoneNumber) { On 2016/01/26 23:56:26, sebsg wrote: > On 2016/01/26 14:43:02, Mathieu Perreault wrote: > > Maybe I'm spoiled by your other changes, but can we refactor these tests to be > > done with TestCase structs? Sounds like HtmlType, value to fill, expected > value, > > max_length are the things that keep coming back from test to test. > > > > It will give us velocity later on. > > It's a good habit to take. It's so much easier to understand and to add tests. > Plus it made me realize a couple of improvements that could be made. Plus I > added test cases. Thanks! Thanks! https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:560: // Check to see if the size field matches the "prefix" or "suffix" size or if size field -> |field| size https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2476: // In one form, rely on the maxlength attribute to imply phone number parts. imply US phone number parts. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2478: FormData form_with_maxlength; Rename to |form_with_us_number_max_length| https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2556: // We should not be able to fill prefix and suffix fields for international // We should not be able to fill international numbers correctly in a form containing fields with US max_length. Question for seb: we should probably detect those cases and just give up? Let's file a bug about this. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2558: // max lenght specified, starting from the right. *length https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2571: EXPECT_EQ(ASCIIToUTF16("4"), response_data3.fields[0].value); This is weird (a few digits truncated from each field). We should really revisit international phone numbers soon, as mentioned above.
Patchset #4 (id:100001) has been deleted
Thanks! https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:262: TEST(AutofillFieldTest, FillPhoneNumber) { On 2016/01/27 14:30:49, Mathieu Perreault wrote: > On 2016/01/26 23:56:26, sebsg wrote: > > On 2016/01/26 14:43:02, Mathieu Perreault wrote: > > > Maybe I'm spoiled by your other changes, but can we refactor these tests to > be > > > done with TestCase structs? Sounds like HtmlType, value to fill, expected > > value, > > > max_length are the things that keep coming back from test to test. > > > > > > It will give us velocity later on. > > > > It's a good habit to take. It's so much easier to understand and to add tests. > > Plus it made me realize a couple of improvements that could be made. Plus I > > added test cases. Thanks! > > Thanks! Acknowledged. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_field.cc (right): https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_field.cc:560: // Check to see if the size field matches the "prefix" or "suffix" size or if On 2016/01/27 14:30:49, Mathieu Perreault wrote: > size field -> |field| size Done. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2476: // In one form, rely on the maxlength attribute to imply phone number parts. On 2016/01/27 14:30:49, Mathieu Perreault wrote: > imply US phone number parts. Done. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2478: FormData form_with_maxlength; On 2016/01/27 14:30:49, Mathieu Perreault wrote: > Rename to |form_with_us_number_max_length| Done. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2556: // We should not be able to fill prefix and suffix fields for international On 2016/01/27 14:30:49, Mathieu Perreault wrote: > // We should not be able to fill international numbers correctly in a form > containing fields with US max_length. > > Question for seb: we should probably detect those cases and just give up? Let's > file a bug about this. Done. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2558: // max lenght specified, starting from the right. On 2016/01/27 14:30:49, Mathieu Perreault wrote: > *length Done. https://codereview.chromium.org/1639563002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:2571: EXPECT_EQ(ASCIIToUTF16("4"), response_data3.fields[0].value); On 2016/01/27 14:30:49, Mathieu Perreault wrote: > This is weird (a few digits truncated from each field). We should really revisit > international phone numbers soon, as mentioned above. Acknowledged.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/1639563002/#ps120001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639563002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639563002/120001
Message was sent while issue was closed.
Description was changed from ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits. BUG=579674 TEST=AutofillFieldTest ========== to ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits. BUG=579674 TEST=AutofillFieldTest ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits. BUG=579674 TEST=AutofillFieldTest ========== to ========== For phone fields that have a maxLength attribute of 10 (city code + phone number), if the value of the phone number is larger, fill the field with the last 10 digits. BUG=579674 TEST=AutofillFieldTest Committed: https://crrev.com/1aea64bdff908c94c4c032935d3c3b9a906afd91 Cr-Commit-Position: refs/heads/master@{#371946} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1aea64bdff908c94c4c032935d3c3b9a906afd91 Cr-Commit-Position: refs/heads/master@{#371946} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
