Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1623803002:
[Autofill] Fix phone upload bug. (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. |
DescriptionWhen determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise.
BUG=580629
TEST=AutofillManagerTest
Committed: https://crrev.com/03c71f5c105e04f15a5b761f67dbcb9e74d178d0
Cr-Commit-Position: refs/heads/master@{#371867}
Patch Set 1 #Patch Set 2 : Fixed metrics tests #
Total comments: 8
Patch Set 3 : Addressed comments #
Total comments: 10
Patch Set 4 : Nits #
Messages
Total messages: 22 (13 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Autofill] Fix phone upload bug (WIP) BUG= ========== to ========== [Autofill] Fix phone upload bug (WIP) BUG=580629 ==========
Patchset #1 (id:40001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
PTAL
nice tests :) just a few comments https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... File components/autofill/core/browser/phone_number.cc (right): https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:149: void PhoneNumber::GetMatchingTypes(const base::string16& text, Since you have the context in your head, please enhance comments in this method for each code block (I see that you have started and it's a good idea). I would like to see clearly described examples of phone numbers (WHOLE/HOME_NUMBER/CITY_AND_NUMBER), what are the values at each step of the way (maybe before/after i18n::NormalizePhoneNumber), etc. Think of a reader who has little context on this and make it clear to them. https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:167: // To match whole phone numbers with the "+" in front of the country code. is the + always present? https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:173: if (normalized_number == whole_number) { revert change to add {} https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:178: // If both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER, it means nit: sentence is missing a few words
Also, please update the CL description and make it descriptive! Also, mention TEST=
Description was changed from ========== [Autofill] Fix phone upload bug (WIP) BUG=580629 ========== to ========== [Autofill] Fix phone upload bug (WIP) BUG=580629 TEST=AutofillManagerTest ==========
Description was changed from ========== [Autofill] Fix phone upload bug (WIP) BUG=580629 TEST=AutofillManagerTest ========== to ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number.In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ==========
Patchset #3 (id:100001) has been deleted
Thanks for the comments! https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... File components/autofill/core/browser/phone_number.cc (right): https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:149: void PhoneNumber::GetMatchingTypes(const base::string16& text, On 2016/01/25 20:32:16, Mathieu Perreault wrote: > Since you have the context in your head, please enhance comments in this method > for each code block (I see that you have started and it's a good idea). I would > like to see clearly described examples of phone numbers > (WHOLE/HOME_NUMBER/CITY_AND_NUMBER), what are the values at each step of the way > (maybe before/after i18n::NormalizePhoneNumber), etc. Think of a reader who has > little context on this and make it clear to them. Done. https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:167: // To match whole phone numbers with the "+" in front of the country code. On 2016/01/25 20:32:16, Mathieu Perreault wrote: > is the + always present? I put a better comment. https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:173: if (normalized_number == whole_number) { On 2016/01/25 20:32:16, Mathieu Perreault wrote: > revert change to add {} Done. https://codereview.chromium.org/1623803002/diff/80001/components/autofill/cor... components/autofill/core/browser/phone_number.cc:178: // If both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER, it means On 2016/01/25 20:32:16, Mathieu Perreault wrote: > nit: sentence is missing a few words Done.
lgtm with nits https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... File components/autofill/core/browser/phone_number.cc (right): https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:154: // would become the stripped text "5141211523". Mention that FormGroup::GetMatchingTypes will only do simple canonicalization in order to match against the stored data, hence why we need more domain-specific knowledge here and cover special cases below. https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:170: // TODO(crbug.com/581391): Investigate the use of phonenumberutil when *PhoneNumberUtil https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:173: // whole number. For example, the number from France "33 2 49 19 70 70" would *the France number and *the US number https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:182: matching_types->insert(PHONE_HOME_WHOLE_NUMBER); I think there's a case where we could be doubly adding PHOME_HOME_WHOLE_NUMBER here (say it was matched in FormGroup::GetMatchingTypes). I guess it doesn't matter because |matching_types| is an std::set, but perhaps let's add a check on line 178 for matching_types->find(PHONE_HOME_WHOLE_NUMBER) == matching_types->end() https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:186: // it means there is no country code in the profile's phone number.In that nit: space after .
Description was changed from ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number.In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ========== to ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ==========
Thanks! https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... File components/autofill/core/browser/phone_number.cc (right): https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:154: // would become the stripped text "5141211523". On 2016/01/27 18:48:35, Mathieu Perreault wrote: > Mention that FormGroup::GetMatchingTypes will only do simple canonicalization in > order to match against the stored data, hence why we need more domain-specific > knowledge here and cover special cases below. Done. https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:170: // TODO(crbug.com/581391): Investigate the use of phonenumberutil when On 2016/01/27 18:48:35, Mathieu Perreault wrote: > *PhoneNumberUtil Done. https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:173: // whole number. For example, the number from France "33 2 49 19 70 70" would On 2016/01/27 18:48:35, Mathieu Perreault wrote: > *the France number and *the US number Done. https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:182: matching_types->insert(PHONE_HOME_WHOLE_NUMBER); On 2016/01/27 18:48:35, Mathieu Perreault wrote: > I think there's a case where we could be doubly adding PHOME_HOME_WHOLE_NUMBER > here (say it was matched in FormGroup::GetMatchingTypes). I guess it doesn't > matter because |matching_types| is an std::set, but perhaps let's add a check on > line 178 for matching_types->find(PHONE_HOME_WHOLE_NUMBER) == > matching_types->end() Done. https://codereview.chromium.org/1623803002/diff/120001/components/autofill/co... components/autofill/core/browser/phone_number.cc:186: // it means there is no country code in the profile's phone number.In that On 2016/01/27 18:48:35, Mathieu Perreault wrote: > nit: space after . Done.
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/1623803002/#ps140001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1623803002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1623803002/140001
Message was sent while issue was closed.
Description was changed from ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ========== to ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest ========== to ========== When determining what field type to upload, if both PHONE_HOME_CITY_AND_NUMBER and PHONE_HOME_WHOLE_NUMBER are matched, it means there is no country code in the profile's phone number. In that case, only match PHONE_HOME_CITY_AND_NUMBER because it's more precise. BUG=580629 TEST=AutofillManagerTest Committed: https://crrev.com/03c71f5c105e04f15a5b761f67dbcb9e74d178d0 Cr-Commit-Position: refs/heads/master@{#371867} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/03c71f5c105e04f15a5b761f67dbcb9e74d178d0 Cr-Commit-Position: refs/heads/master@{#371867} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
