Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924513002:
use user chosen country code to format and validate phone number for addresses. (Closed)
|
Created:
3 years, 6 months ago by wuandy1 Modified:
3 years, 6 months ago Reviewers:
gogerald1 CC:
chromium-reviews, rouslan+payments_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, gogerald+paymentswatch_chromium.org, mahmadi+paymentswatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionuse user chosen country code to format and validate phone number for addresses.
This change will use the country specified by user(as part of address) to format
and validate phone numbers in PaymentRequest UI. Previously it was using
application's locale. This should give user a better experience and eliminate
cases where phone number is mistakenly formatted because application locale
is not determined properly. For example, clank sees fr-CA as fr and formats
Canadian numbers as french numbers.
BUG=723769
Patch Set 1 #Patch Set 2 : looks like most of it is working #Patch Set 3 : add comments #
Total comments: 14
Patch Set 4 : comments addressing #
Total comments: 8
Patch Set 5 : more fixing #
Total comments: 23
Patch Set 6 : fix presubmit complaints #
Total comments: 16
Patch Set 7 : more fixing... #
Messages
Total messages: 46 (29 generated)
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== allow using country code to format and validate phone number this should pass all tests stil add param of country code to c++ phone formatter BUG= ========== to ========== This change will use the country specified by user(as part of address) to format and validate phone numbers in PaymentRequest UI. Previously it was using application's locale. This should give user a better experience and eliminate cases where phone number is mistakenly formatted because application locale is not determined properly. For example, clank sees fr-CA as fr and formats Canadian numbers as french numbers. BUG=723769 ==========
wuandy@chromium.org changed reviewers: + gogerald@chromium.org
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
please take a look
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please also update the CL description as I mentioned in your last CL https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:29: public void setCountryCode(String countryCode) { Add comments for this public interface, https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:87: * @param countryCode The country code used to validate the number. If null, the function will This is incorrect, even the country code is not null, the phonenumberutil will use the number's region code if it starts with '+'. https://cs.chromium.org/chromium/src/third_party/libphonenumber/dist/cpp/src/... https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); mRecentlySelectedCountry is null if the user doesn't change the default selected country, https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2924513002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestFreeShippingTest.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestFreeShippingTest.java:149: "Alice", "Supreme Court", "Airport Road", "Kabul", "1043", "+93202530000"}); why these numbers have to be changed? https://codereview.chromium.org/2924513002/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/phone_number_util_android.cc (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/phone_number_util_android.cc:85: // Formats the given number |jphone_number| * to *
Description was changed from ========== This change will use the country specified by user(as part of address) to format and validate phone numbers in PaymentRequest UI. Previously it was using application's locale. This should give user a better experience and eliminate cases where phone number is mistakenly formatted because application locale is not determined properly. For example, clank sees fr-CA as fr and formats Canadian numbers as french numbers. BUG=723769 ========== to ========== use user chosen country code to format and validate phone number for addresses. This change will use the country specified by user(as part of address) to format and validate phone numbers in PaymentRequest UI. Previously it was using application's locale. This should give user a better experience and eliminate cases where phone number is mistakenly formatted because application locale is not determined properly. For example, clank sees fr-CA as fr and formats Canadian numbers as french numbers. BUG=723769 ==========
https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:29: public void setCountryCode(String countryCode) { On 2017/06/08 16:20:43, gogerald1 wrote: > Add comments for this public interface, Done. https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:87: * @param countryCode The country code used to validate the number. If null, the function will On 2017/06/08 16:20:43, gogerald1 wrote: > This is incorrect, even the country code is not null, the phonenumberutil will > use the number's region code if it starts with '+'. > https://cs.chromium.org/chromium/src/third_party/libphonenumber/dist/cpp/src/... Done. https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); On 2017/06/08 16:20:43, gogerald1 wrote: > mRecentlySelectedCountry is null if the user doesn't change the default selected > country, > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Which is handled by isValidNumber..do you mean a @Nullable here? https://codereview.chromium.org/2924513002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestFreeShippingTest.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestFreeShippingTest.java:149: "Alice", "Supreme Court", "Airport Road", "Kabul", "1043", "+93202530000"}); On 2017/06/08 16:20:43, gogerald1 wrote: > why these numbers have to be changed? Test will hang because that is not a valid Afghanistan number. https://codereview.chromium.org/2924513002/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/phone_number_util_android.cc (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/phone_number_util_android.cc:85: // Formats the given number |jphone_number| On 2017/06/08 16:20:43, gogerald1 wrote: > * to * Done.
https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); On 2017/06/08 20:34:58, wuandy1 wrote: > On 2017/06/08 16:20:43, gogerald1 wrote: > > mRecentlySelectedCountry is null if the user doesn't change the default > selected > > country, > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Which is handled by isValidNumber..do you mean a @Nullable here? NO, the selected country is set according to profile when creating it, which is not the same as mRecentlySelectedCountry. https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:22: * TextWatcher to watch phone number changes so as to format it based on one country code. delete 'one' https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:30: * Update the country code used by this class to format phone numbers. delete 'by this class' https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:91: * the number will be validated against application locale. This is an important information "The region code is from the given phone number if it starts with '+'". Do not delete it. Also add it to formatForDisplay interface comments. https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:95: return countryCode == null ? nativeIsValidNumber(phoneNumber) This could not be null since the country dropdown field in the address editor always has a valid selection,
https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); On 2017/06/09 13:33:46, gogerald1 wrote: > On 2017/06/08 20:34:58, wuandy1 wrote: > > On 2017/06/08 16:20:43, gogerald1 wrote: > > > mRecentlySelectedCountry is null if the user doesn't change the default > > selected > > > country, > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > Which is handled by isValidNumber..do you mean a @Nullable here? > > NO, the selected country is set according to profile when creating it, which is > not the same as mRecentlySelectedCountry. > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... ok i see. what is your suggestion then?
https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); On 2017/06/12 14:15:25, wuandy1 wrote: > On 2017/06/09 13:33:46, gogerald1 wrote: > > On 2017/06/08 20:34:58, wuandy1 wrote: > > > On 2017/06/08 16:20:43, gogerald1 wrote: > > > > mRecentlySelectedCountry is null if the user doesn't change the default > > > selected > > > > country, > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > > > Which is handled by isValidNumber..do you mean a @Nullable here? > > > > NO, the selected country is set according to profile when creating it, which > is > > not the same as mRecentlySelectedCountry. > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > ok i see. what is your suggestion then? What about using 'mCountryField.getValue().toString()' instead of 'mRecentlySelectedCountry'? Please also update the exact behavior after this CL in bug and cc'ed zkoch@ and rouslan@ to confirm.
https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:401: value.toString(), mRecentlySelectedCountry); On 2017/06/12 17:02:11, gogerald1 wrote: > On 2017/06/12 14:15:25, wuandy1 wrote: > > On 2017/06/09 13:33:46, gogerald1 wrote: > > > On 2017/06/08 20:34:58, wuandy1 wrote: > > > > On 2017/06/08 16:20:43, gogerald1 wrote: > > > > > mRecentlySelectedCountry is null if the user doesn't change the default > > > > selected > > > > > country, > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > > > > > Which is handled by isValidNumber..do you mean a @Nullable here? > > > > > > NO, the selected country is set according to profile when creating it, which > > is > > > not the same as mRecentlySelectedCountry. > > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > ok i see. what is your suggestion then? > > What about using 'mCountryField.getValue().toString()' instead of > 'mRecentlySelectedCountry'? > > Please also update the exact behavior after this CL in bug and cc'ed zkoch@ and > rouslan@ to confirm. Done. https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:22: * TextWatcher to watch phone number changes so as to format it based on one country code. On 2017/06/09 13:33:46, gogerald1 wrote: > delete 'one' Done. https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:30: * Update the country code used by this class to format phone numbers. On 2017/06/09 13:33:46, gogerald1 wrote: > delete 'by this class' Done. https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:91: * the number will be validated against application locale. On 2017/06/09 13:33:46, gogerald1 wrote: > This is an important information "The region code is from the given phone number > if it starts with '+'". Do not delete it. Also add it to formatForDisplay > interface comments. Done. https://codereview.chromium.org/2924513002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:95: return countryCode == null ? nativeIsValidNumber(phoneNumber) On 2017/06/09 13:33:46, gogerald1 wrote: > This could not be null since the country dropdown field in the address editor > always has a valid selection, Actually there is another place calling this method at https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... where mCountryField could be null, i had to change to use the country from profile explicitly for this.
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Added rouslan@ for more ideas, https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:65: public static String formatForDisplay(String phoneNumber, String countryCode) { @Nullable countryCode https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:67: ? nativeFormatForDisplay(phoneNumber) For simplicity, you no need two interfaces here, 'null' can be checked in C++ side. After that, please name it 'nativeFormatForDisplay' to be consistent with the 'formatForDisplay' Same for below 'nativeIsValidNumber' https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:90: * @param countryCode The country code used to validate the number against. The region The comments for countryCode should be the same as for 'formatForDisplay' https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:94: public static boolean isValidNumber(String phoneNumber, String countryCode) { @Nullable countryCode? https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:61: * @param phoneNumber The phone number to possibly add. comments: @param countryCode ..... https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:64: @Nullable CharSequence phoneNumber, @Nullable String countryCode) { this countryCode doesn't take effect, since it may not be used in getPhoneValidator https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:127: mEditorDialog.updateCountryOfPhoneFormatter(mRecentlySelectedCountry); This not enough as I mentioned in the last review. onResult is called only after user manually changed selected country. However, we set default selected country when creating the editor dialog (line 135). https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:166: mPhoneNumbers, getPhoneValidator(null), null, mCountryField.getValue()? since we have set default country code above (line 135). https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:396: if (mPhoneValidator == null) { This makes the countryCode of mPhoneValidator is fixed after created, https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:550: profile.getPhoneNumber(), profile.getCountryCode()); We should consider whether this is what we want and how to make it better. These added numbers are used as suggestions. They may not valid in profile.getCountryCode() or application locale, but may valid in the other countries (user can change selected country in the address editor). You can post result or your design on the bug or chat to gain feedback as I mentioned in the last review. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java:574: public void updateCountryOfPhoneFormatter(String country) { It looks not an ideal design to let AddressEditor (controller) set data to EditorDialog (view) directly. It is better to pass data through EditorFieldModel (model). Might move mPhoneFormatter to AddressEditor as mPhoneValidator, and mCardNumberFormatter to CardEditor as well.
On 2017/06/15 17:41:36, gogerald1 wrote: > Added rouslan@ for more ideas, Is there a specific question or I am needed for a review?
On 2017/06/15 17:42:43, rouslan wrote: > On 2017/06/15 17:41:36, gogerald1 wrote: > > Added rouslan@ for more ideas, > > Is there a specific question or I am needed for a review? Had a specific question that: Since we are formatting and validating the phone number according to country code, so it's hard to know whether a number is valid or not when adding it to AddressEditor (for suggestions). After think a little bit, I think we could add all them to AddressEditor without checking or with minimum check of validity, but only set fully validated phone numbers to EditorFieldModel for suggestions according to the selected country.
On 2017/06/15 18:20:27, gogerald1 wrote: > On 2017/06/15 17:42:43, rouslan wrote: > > On 2017/06/15 17:41:36, gogerald1 wrote: > > > Added rouslan@ for more ideas, > > > > Is there a specific question or I am needed for a review? > > Had a specific question that: Since we are formatting and validating the phone > number according to country code, so it's hard to know whether a number is valid > or not when adding it to AddressEditor (for suggestions). > > After think a little bit, I think we could add all them to AddressEditor without > checking or with minimum check of validity, but only set fully validated phone > numbers to EditorFieldModel for suggestions according to the selected country. Sounds good!
rouslan@chromium.org changed reviewers: - rouslan@chromium.org
On 2017/06/15 18:26:01, rouslan wrote: > On 2017/06/15 18:20:27, gogerald1 wrote: > > On 2017/06/15 17:42:43, rouslan wrote: > > > On 2017/06/15 17:41:36, gogerald1 wrote: > > > > Added rouslan@ for more ideas, > > > > > > Is there a specific question or I am needed for a review? > > > > Had a specific question that: Since we are formatting and validating the phone > > number according to country code, so it's hard to know whether a number is > valid > > or not when adding it to AddressEditor (for suggestions). > > > > After think a little bit, I think we could add all them to AddressEditor > without > > checking or with minimum check of validity, but only set fully validated phone > > numbers to EditorFieldModel for suggestions according to the selected country. > > Sounds good! So in AddressEditor.addPhoneNumberIfValid, we don't do the validation there, is my understanding correct?
https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:65: public static String formatForDisplay(String phoneNumber, String countryCode) { On 2017/06/15 17:41:35, gogerald1 wrote: > @Nullable countryCode Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:67: ? nativeFormatForDisplay(phoneNumber) On 2017/06/15 17:41:35, gogerald1 wrote: > For simplicity, you no need two interfaces here, 'null' can be checked in C++ > side. > > After that, please name it 'nativeFormatForDisplay' to be consistent with the > 'formatForDisplay' > > Same for below 'nativeIsValidNumber' Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:90: * @param countryCode The country code used to validate the number against. The region On 2017/06/15 17:41:35, gogerald1 wrote: > The comments for countryCode should be the same as for 'formatForDisplay' Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:94: public static boolean isValidNumber(String phoneNumber, String countryCode) { On 2017/06/15 17:41:35, gogerald1 wrote: > @Nullable countryCode? Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:61: * @param phoneNumber The phone number to possibly add. On 2017/06/15 17:41:35, gogerald1 wrote: > comments: @param countryCode ..... deleted https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:64: @Nullable CharSequence phoneNumber, @Nullable String countryCode) { On 2017/06/15 17:41:35, gogerald1 wrote: > this countryCode doesn't take effect, since it may not be used in > getPhoneValidator removed validation. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:127: mEditorDialog.updateCountryOfPhoneFormatter(mRecentlySelectedCountry); On 2017/06/15 17:41:35, gogerald1 wrote: > This not enough as I mentioned in the last review. onResult is called only after > user manually changed selected country. However, we set default selected country > when creating the editor dialog (line 135). add code to initialize it, not sure this is what you meant though. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:166: mPhoneNumbers, getPhoneValidator(null), null, On 2017/06/15 17:41:35, gogerald1 wrote: > mCountryField.getValue()? since we have set default country code above (line > 135). Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:396: if (mPhoneValidator == null) { On 2017/06/15 17:41:35, gogerald1 wrote: > This makes the countryCode of mPhoneValidator is fixed after created, Done. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:550: profile.getPhoneNumber(), profile.getCountryCode()); On 2017/06/15 17:41:35, gogerald1 wrote: > We should consider whether this is what we want and how to make it better. > > These added numbers are used as suggestions. They may not valid in > profile.getCountryCode() or application locale, but may valid in the other > countries (user can change selected country in the address editor). > > You can post result or your design on the bug or chat to gain feedback as I > mentioned in the last review. as discussed offline, country based validation is removed from here. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java:574: public void updateCountryOfPhoneFormatter(String country) { On 2017/06/15 17:41:35, gogerald1 wrote: > It looks not an ideal design to let AddressEditor (controller) set data to > EditorDialog (view) directly. It is better to pass data through EditorFieldModel > (model). > > Might move mPhoneFormatter to AddressEditor as mPhoneValidator, and > mCardNumberFormatter to CardEditor as well. > separate cl?
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost done, few more comments, BTW, I know you have more important things to take care of, I can help to finish this CL based on your work if you need. https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java (right): https://codereview.chromium.org/2924513002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDialog.java:574: public void updateCountryOfPhoneFormatter(String country) { On 2017/06/19 15:11:14, wuandy1 wrote: > On 2017/06/15 17:41:35, gogerald1 wrote: > > It looks not an ideal design to let AddressEditor (controller) set data to > > EditorDialog (view) directly. It is better to pass data through > EditorFieldModel > > (model). > > > > Might move mPhoneFormatter to AddressEditor as mPhoneValidator, and > > mCardNumberFormatter to CardEditor as well. > > > > separate cl? I would not, this is an important part of your design, https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57: * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] based on Sorry, these comments still looks confusion and incomplete (sometimes comment is harder than code), I would comment like below. /** * Formats the given phone number in INTERNATIONAL format * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] based * on region code, returning the original number if no formatting can be made. * For example, the number of the Google Zürich office will be formatted as * "+41 44 668 1800" in INTERNATIONAL format. * * Note that the region code is from the given phone number if it starts with * '+', otherwise the region code is deduced from the given country code or * from application locale if the given country code is null. * * @param phoneNumber The given phone number. * @param countryCode The given country code. * @return Formatted phone number. */ https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:86: * Checks whether the given phone number matches a valid pattern according to Similarly, I would comment like below: /** * Checks whether the given phone number matches a valid pattern according to * region code. * The region code is from the given phone number if it starts with '+', * otherwise the region code is deduced from the given country code or from * application locale if the given country code is null. * * @param phoneNumber The given phone number. * @param countryCode The given country code. * * @return True if the given number is valid, otherwise return false. */ https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:62: * We don't validate for number format since these are suggestions only. The reason we do not validate it here is not because they are used for suggestions only, but they are expected to have already been validated since we are doing strict Autofill data validation. And here is not validating 'number format' but 'number validity'. I would comment like below: /** * Adds the given phone number to the autocomplete set, if it's valid. * Note that here we consider all non-null and non-empty numbers as valid * since we are doing strict validation of Autofill data. * * @param phoneNumber The phone number to possibly add. */ https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:65: if (phoneNumber != null && !phoneNumber.toString().isEmpty()) { use TextUtils.isEmpty() which do both, https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:399: @Nullable final EditorFieldModel countryCodeField) { Why are you using EditorFieldModel instead of country code directly? Why it is Nullable? https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/phone_number_util_android.cc (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:24: // Formats the |phone_number| to the specified |format| based on a given *for the given country |countryCode| * https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:70: // Formats the given number |jphone_number| for a given country to *for the given country |jcountry_code| * https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:104: // Checks whether the given number |jphone_number| is valid for a given country *for the given country |jcountry_code| *
Addressed most of the comments except the refactor to move formatter off EditorDialog, i have limited time every day..if you can help pick that up it would be great!! Also, Mathieu mentioned he wanted this to be merge back to 60, can you check with him and take care of that as well. A million thanks! https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57: * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] based on On 2017/06/19 21:30:38, gogerald1 wrote: > Sorry, these comments still looks confusion and incomplete (sometimes comment is > harder than code), I would comment like below. > > /** > * Formats the given phone number in INTERNATIONAL format > * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] based > * on region code, returning the original number if no formatting can be made. > * For example, the number of the Google Zürich office will be formatted as > * "+41 44 668 1800" in INTERNATIONAL format. > * > * Note that the region code is from the given phone number if it starts with > * '+', otherwise the region code is deduced from the given country code or > * from application locale if the given country code is null. > * > * @param phoneNumber The given phone number. > * @param countryCode The given country code. > * @return Formatted phone number. > */ Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:86: * Checks whether the given phone number matches a valid pattern according to On 2017/06/19 21:30:38, gogerald1 wrote: > Similarly, I would comment like below: > > /** > * Checks whether the given phone number matches a valid pattern according to > * region code. > * The region code is from the given phone number if it starts with '+', > * otherwise the region code is deduced from the given country code or from > * application locale if the given country code is null. > * > * @param phoneNumber The given phone number. > * @param countryCode The given country code. > * > * @return True if the given number is valid, otherwise return false. > */ Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:62: * We don't validate for number format since these are suggestions only. On 2017/06/19 21:30:38, gogerald1 wrote: > The reason we do not validate it here is not because they are used for > suggestions only, but they are expected to have already been validated since we > are doing strict Autofill data validation. > > And here is not validating 'number format' but 'number validity'. I would > comment like below: > > /** > * Adds the given phone number to the autocomplete set, if it's valid. > * Note that here we consider all non-null and non-empty numbers as valid > * since we are doing strict validation of Autofill data. > * > * @param phoneNumber The phone number to possibly add. > */ Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:65: if (phoneNumber != null && !phoneNumber.toString().isEmpty()) { On 2017/06/19 21:30:38, gogerald1 wrote: > use TextUtils.isEmpty() which do both, > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:399: @Nullable final EditorFieldModel countryCodeField) { On 2017/06/19 21:30:38, gogerald1 wrote: > Why are you using EditorFieldModel instead of country code directly? Why it is > Nullable? because validator can then run validation after user changed the value. it should not be @nullable though. https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/phone_number_util_android.cc (right): https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:24: // Formats the |phone_number| to the specified |format| based on a given On 2017/06/19 21:30:38, gogerald1 wrote: > *for the given country |countryCode| * Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:70: // Formats the given number |jphone_number| for a given country to On 2017/06/19 21:30:38, gogerald1 wrote: > *for the given country |jcountry_code| * Done. https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/phone_number_util_android.cc:104: // Checks whether the given number |jphone_number| is valid for a given country On 2017/06/19 21:30:38, gogerald1 wrote: > *for the given country |jcountry_code| * Done.
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 15:33:27, wuandy1 wrote: > Addressed most of the comments except the refactor to move formatter off > EditorDialog, i have limited time every day..if you can help pick that up it > would be great!! > > Also, Mathieu mentioned he wanted this to be merge back to 60, can you check > with him and take care of that as well. > > A million thanks! > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java > (right): > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57: > * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] based > on > On 2017/06/19 21:30:38, gogerald1 wrote: > > Sorry, these comments still looks confusion and incomplete (sometimes comment > is > > harder than code), I would comment like below. > > > > /** > > * Formats the given phone number in INTERNATIONAL format > > * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] > based > > * on region code, returning the original number if no formatting can be made. > > * For example, the number of the Google Zürich office will be formatted as > > * "+41 44 668 1800" in INTERNATIONAL format. > > * > > * Note that the region code is from the given phone number if it starts with > > * '+', otherwise the region code is deduced from the given country code or > > * from application locale if the given country code is null. > > * > > * @param phoneNumber The given phone number. > > * @param countryCode The given country code. > > * @return Formatted phone number. > > */ > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:86: > * Checks whether the given phone number matches a valid pattern according to > On 2017/06/19 21:30:38, gogerald1 wrote: > > Similarly, I would comment like below: > > > > /** > > * Checks whether the given phone number matches a valid pattern according to > > * region code. > > * The region code is from the given phone number if it starts with '+', > > * otherwise the region code is deduced from the given country code or from > > * application locale if the given country code is null. > > * > > * @param phoneNumber The given phone number. > > * @param countryCode The given country code. > > * > > * @return True if the given number is valid, otherwise return false. > > */ > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java > (right): > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:62: > * We don't validate for number format since these are > suggestions only. > On 2017/06/19 21:30:38, gogerald1 wrote: > > The reason we do not validate it here is not because they are used for > > suggestions only, but they are expected to have already been validated since > we > > are doing strict Autofill data validation. > > > > And here is not validating 'number format' but 'number validity'. I would > > comment like below: > > > > /** > > * Adds the given phone number to the autocomplete set, if it's valid. > > * Note that here we consider all non-null and non-empty numbers as valid > > * since we are doing strict validation of Autofill data. > > * > > * @param phoneNumber The phone number to possibly add. > > */ > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:65: > if (phoneNumber != null && !phoneNumber.toString().isEmpty()) { > On 2017/06/19 21:30:38, gogerald1 wrote: > > use TextUtils.isEmpty() which do both, > > > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:399: > @Nullable final EditorFieldModel countryCodeField) { > On 2017/06/19 21:30:38, gogerald1 wrote: > > Why are you using EditorFieldModel instead of country code directly? Why it is > > Nullable? > > because validator can then run validation after user changed the value. it > should not be @nullable though. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > File chrome/browser/autofill/android/phone_number_util_android.cc (right): > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > chrome/browser/autofill/android/phone_number_util_android.cc:24: // Formats the > |phone_number| to the specified |format| based on a given > On 2017/06/19 21:30:38, gogerald1 wrote: > > *for the given country |countryCode| * > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > chrome/browser/autofill/android/phone_number_util_android.cc:70: // Formats the > given number |jphone_number| for a given country to > On 2017/06/19 21:30:38, gogerald1 wrote: > > *for the given country |jcountry_code| * > > Done. > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > chrome/browser/autofill/android/phone_number_util_android.cc:104: // Checks > whether the given number |jphone_number| is valid for a given country > On 2017/06/19 21:30:38, gogerald1 wrote: > > *for the given country |jcountry_code| * > > Done. The corresponding CL https://codereview.chromium.org/2952673002/ has been landed, you can close this one now.
Message was sent while issue was closed.
On 2017/06/21 14:40:48, gogerald1 wrote: > On 2017/06/20 15:33:27, wuandy1 wrote: > > Addressed most of the comments except the refactor to move formatter off > > EditorDialog, i have limited time every day..if you can help pick that up it > > would be great!! > > > > Also, Mathieu mentioned he wanted this to be merge back to 60, can you check > > with him and take care of that as well. > > > > A million thanks! > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java > > (right): > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:57: > > * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] > based > > on > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > Sorry, these comments still looks confusion and incomplete (sometimes > comment > > is > > > harder than code), I would comment like below. > > > > > > /** > > > * Formats the given phone number in INTERNATIONAL format > > > * [i18n::phonenumbers::PhoneNumberUtil::PhoneNumberFormat::INTERNATIONAL] > > based > > > * on region code, returning the original number if no formatting can be > made. > > > * For example, the number of the Google Zürich office will be formatted as > > > * "+41 44 668 1800" in INTERNATIONAL format. > > > * > > > * Note that the region code is from the given phone number if it starts > with > > > * '+', otherwise the region code is deduced from the given country code or > > > * from application locale if the given country code is null. > > > * > > > * @param phoneNumber The given phone number. > > > * @param countryCode The given country code. > > > * @return Formatted phone number. > > > */ > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/autofill/PhoneNumberUtil.java:86: > > * Checks whether the given phone number matches a valid pattern according to > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > Similarly, I would comment like below: > > > > > > /** > > > * Checks whether the given phone number matches a valid pattern according > to > > > * region code. > > > * The region code is from the given phone number if it starts with '+', > > > * otherwise the region code is deduced from the given country code or from > > > * application locale if the given country code is null. > > > * > > > * @param phoneNumber The given phone number. > > > * @param countryCode The given country code. > > > * > > > * @return True if the given number is valid, otherwise return false. > > > */ > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java > > (right): > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:62: > > * We don't validate for number format since these are > > suggestions only. > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > The reason we do not validate it here is not because they are used for > > > suggestions only, but they are expected to have already been validated since > > we > > > are doing strict Autofill data validation. > > > > > > And here is not validating 'number format' but 'number validity'. I would > > > comment like below: > > > > > > /** > > > * Adds the given phone number to the autocomplete set, if it's valid. > > > * Note that here we consider all non-null and non-empty numbers as valid > > > * since we are doing strict validation of Autofill data. > > > * > > > * @param phoneNumber The phone number to possibly add. > > > */ > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:65: > > if (phoneNumber != null && !phoneNumber.toString().isEmpty()) { > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > use TextUtils.isEmpty() which do both, > > > > > > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:399: > > @Nullable final EditorFieldModel countryCodeField) { > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > Why are you using EditorFieldModel instead of country code directly? Why it > is > > > Nullable? > > > > because validator can then run validation after user changed the value. it > > should not be @nullable though. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > > File chrome/browser/autofill/android/phone_number_util_android.cc (right): > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > > chrome/browser/autofill/android/phone_number_util_android.cc:24: // Formats > the > > |phone_number| to the specified |format| based on a given > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > *for the given country |countryCode| * > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > > chrome/browser/autofill/android/phone_number_util_android.cc:70: // Formats > the > > given number |jphone_number| for a given country to > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > *for the given country |jcountry_code| * > > > > Done. > > > > > https://codereview.chromium.org/2924513002/diff/100001/chrome/browser/autofil... > > chrome/browser/autofill/android/phone_number_util_android.cc:104: // Checks > > whether the given number |jphone_number| is valid for a given country > > On 2017/06/19 21:30:38, gogerald1 wrote: > > > *for the given country |jcountry_code| * > > > > Done. > > The corresponding CL https://codereview.chromium.org/2952673002/ has been > landed, you can close this one now. thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
