Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680143002:
Use dropdown list for admin areas in pr form. (Closed)
DescriptionUse drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country's sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the network connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
Review-Url: https://codereview.chromium.org/2680143002
Cr-Commit-Position: refs/heads/master@{#462611}
Committed: https://chromium.googlesource.com/chromium/src/+/69b196d676ecbbb986009aaf9f5627fd6e332de1
Patch Set 1 : wip #
Total comments: 19
Patch Set 2 : The one where I modify the tests (and later will undo them!) #Patch Set 3 : The one where I merged the cl with seb's. #Patch Set 4 : Touch ups #
Total comments: 36
Patch Set 5 : The one where I merged the cl with seb's again. #
Total comments: 16
Patch Set 6 : The one where I applied seb's comments. #
Total comments: 2
Patch Set 7 : The one where I fixed a line (an error from sync!) #
Total comments: 34
Patch Set 8 : The one where I applied Math's comments #
Total comments: 8
Patch Set 9 : The one with some clean ups, suggested by Math. #
Total comments: 4
Patch Set 10 : Another one with addressing Math's comments. #
Total comments: 38
Patch Set 11 : The one with Rouslan's comments. (this was a wrong patch) #
Total comments: 10
Patch Set 12 : Rouslan follow up. #Patch Set 13 : Show Progress Dialogue formats. #
Total comments: 2
Patch Set 14 : nit #
Total comments: 44
Patch Set 15 : Dan review. #Patch Set 16 : generalize time out cancelation #
Total comments: 2
Patch Set 17 : nit #Patch Set 18 : Supress error-- suggested by clank team #Messages
Total messages: 235 (193 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
parastoog@google.com changed reviewers: + sebsg@chromium.org
Description was changed from ========== wip-use dropdown list for admin areas in pr form. BUG= ========== to ========== Use dropdown list for admin areas in payment request form. BUG= ==========
Description was changed from ========== Use dropdown list for admin areas in payment request form. BUG= ========== to ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. If the server doesn't respond, there is a time-out, or no admin areas are recorded for that country, show text-field. BUG= ==========
Great start :) https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:286: if (mIsUpdate) { I don't fully understand what update means. Could you explain it in a comment? Thanks. https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:310: // used to check if the callback is called (for time-out). Nit: Capitalize first letter. https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:369: mEditor.addField(mPhoneField); I don't fully understand the full concept, like why phone is added here. Could you link a bug in the description in which the new behavior is fully described? Thanks :) https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:109: adminAreas.add(new DropdownKeyValue(s, s)); // TODO (@parastoog): show names, save keys. The new format for TODOs is to create a bug in crbug and then write a comment as follows: // TODO(crbug.com/XXXX): Show names, save keys. That way, someone else can get a sense of what needs to be done by reading the bug a maybe fix it :) https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:837: void PersonalDataManagerAndroid::OnGettingSubKeysRulesLoaded( You probably don't need a second function here. When the rules are loaded, you can go through your 2 maps inside a single function. https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:860: requester->OnRulesSuccessfullyLoaded(); Since both RegionSubKeys and ValidationRules requester are implementing the Delegate interface, you can probably create the "requester" here but then use a common method for the rest of the function, which identical to StartAddressNormalization. https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:357: void OnGettingSubKeysRulesLoaded(const std::string& region_code, As mentioned, this is probably not necessary. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:65: auto rules = supplier_->GetRulesForRegion(region_code); Can you add DCHECK(AreRulesLoadedForRegion(region_code)); as the first line? DCHECKs are useful in cases like this where it should always be the case, but you want to make sure you know if there is ever a case where it's not. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:51: virtual void OnGettingSubKeysRulesLoaded(const std::string& region_code, As mentionned in the other comments, this can probably be removed. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:99: virtual std::vector<std::string> GetRegionSubKeys( Please add a comment describing this function.
Description was changed from ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. If the server doesn't respond, there is a time-out, or no admin areas are recorded for that country, show text-field. BUG= ========== to ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. If the server doesn't respond, there is a time-out, or if no admin areas are recorded for that country, show text-field. To keep the fields in order, add all the fields to the form, when admin areas are loaded. Otherwise, admin areas would show up at the end (since it's fetched asynchronously.) BUG= ==========
https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:286: if (mIsUpdate) { On 2017/02/10 16:00:32, sebsg wrote: > I don't fully understand what update means. Could you explain it in a comment? > Thanks. Done. https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:310: // used to check if the callback is called (for time-out). On 2017/02/10 16:00:32, sebsg wrote: > Nit: Capitalize first letter. Done. https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:369: mEditor.addField(mPhoneField); On 2017/02/10 16:00:31, sebsg wrote: > I don't fully understand the full concept, like why phone is added here. Could > you link a bug in the description in which the new behavior is fully described? > Thanks :) Done. https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:109: adminAreas.add(new DropdownKeyValue(s, s)); // TODO (@parastoog): show names, save keys. On 2017/02/10 16:00:32, sebsg wrote: > The new format for TODOs is to create a bug in crbug and then write a comment as > follows: > > // TODO(crbug.com/XXXX): Show names, save keys. > > That way, someone else can get a sense of what needs to be done by reading the > bug a maybe fix it :) Done. https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:837: void PersonalDataManagerAndroid::OnGettingSubKeysRulesLoaded( On 2017/02/10 16:00:32, sebsg wrote: > You probably don't need a second function here. When the rules are loaded, you > can go through your 2 maps inside a single function. As discussed, we will keep the functions separated, since we will need them both for further changes (loading rules from non-aggregate server for sub-keys requests.) https://codereview.chromium.org/2680143002/diff/60001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:860: requester->OnRulesSuccessfullyLoaded(); On 2017/02/10 16:00:32, sebsg wrote: > Since both RegionSubKeys and ValidationRules requester are implementing the > Delegate interface, you can probably create the "requester" here but then use a > common method for the rest of the function, which identical to > StartAddressNormalization. Same here. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:65: auto rules = supplier_->GetRulesForRegion(region_code); On 2017/02/10 16:00:32, sebsg wrote: > Can you add DCHECK(AreRulesLoadedForRegion(region_code)); as the first line? > DCHECKs are useful in cases like this where it should always be the case, but > you want to make sure you know if there is ever a case where it's not. Done. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:51: virtual void OnGettingSubKeysRulesLoaded(const std::string& region_code, On 2017/02/10 16:00:32, sebsg wrote: > As mentionned in the other comments, this can probably be removed. Acknowledged. https://codereview.chromium.org/2680143002/diff/60001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:99: virtual std::vector<std::string> GetRegionSubKeys( On 2017/02/10 16:00:32, sebsg wrote: > Please add a comment describing this function. Done.
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #2 (id:80001) has been deleted
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by parastoog@google.com 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...
Patchset #2 (id:100001) has been deleted
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 parastoog@google.com to run a CQ dry run
Patchset #2 (id:120001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. If the server doesn't respond, there is a time-out, or if no admin areas are recorded for that country, show text-field. To keep the fields in order, add all the fields to the form, when admin areas are loaded. Otherwise, admin areas would show up at the end (since it's fetched asynchronously.) BUG= ========== to ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). - For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. - If the server doesn't respond, or there is a time-out, or if no admin areas are recorded for that country, show text-field. - Change the PaymentRequestTest accordingly. To keep the fields in order, add all the fields to the form, when admin areas are loaded. Otherwise, admin areas would show up at the end (since it's fetched asynchronously.) BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Description was changed from ========== Use drop-down list for admin areas (state, province, ..) in the payment request form (instead of the previous text-field). - For this purpose, the rules of the specified country are loaded from the i18n aggregate server, and the country sub-keys are retrieved from the right rule. - If the server doesn't respond, or there is a time-out, or if no admin areas are recorded for that country, show text-field. - Change the PaymentRequestTest accordingly. To keep the fields in order, add all the fields to the form, when admin areas are loaded. Otherwise, admin areas would show up at the end (since it's fetched asynchronously.) BUG= ========== to ========== [Payment Request Form] Use drop-down list for admin areas (state, province, ..). - Previously, there was a text-field for that. - For this purpose, the rules of the specified country are loaded from the i18n aggregate server => the country sub-keys are retrieved from the right rule. - If the server doesn't respond, or there is a time-out, or if no admin areas are recorded for that country => show text-field. - Change the Payment Request Tests accordingly. To keep the fields in order, add all the fields to the form, when admin areas are loaded. Otherwise, admin areas would show up at the end (since it's fetched asynchronously.) BUG= ==========
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
- Change the PaymentRequestTest accordingly.
To keep the fields in order, add all the fields to the form, when admin areas
are loaded. Otherwise, admin areas would show up at the end (since it's fetched
asynchronously.)
BUG=
==========
to
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- To keep the fields in order, add all the fields to the form, only when
admin areas are loaded. Otherwise, admin areas would show up at the end (since
they are fetched asynchronously.)
- Change the PaymentRequestTest accordingly.
BUG=
==========
As discussed offline, let's rebase and re-upload the patch :)
The CQ bit was checked by parastoog@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #4 (id:180001) has been deleted
The CQ bit was checked by parastoog@google.com 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by parastoog@google.com 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...
Patchset #4 (id:200001) has been deleted
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 parastoog@google.com 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 parastoog@google.com 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.
First run of comments Good job! https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:827: * Starts loading the address validation rules for the specified {@code regionCode}. s/address validation/ subkeys ? https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:324: public void onAdminAreasLoading() { General question. Is it possible to query two different countries at the same time, of sequentially fast enough before the timer runs out? If you can have more than one, you will cancel both queries at the same time with the current implementation. Also if it's only one at the time, you can simplify how you keep track of the pending queries in Personal_data_manager_android. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:330: PersonalDataManager.getInstance().cancelPendingGetSubKeys(); You also only need to call this if the rules are not done loading. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:344: // In terms of order, country must be the first field. nit: Country must be filled first. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:370: // In terms of order, phone number must be the last field. nit: Phone number must be the last field. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:168: * an autofill profile Nit: put back the period https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:254: Nit: remove new-line https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:340: Nit: Remove new-line https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:86: /** The expiration year dropdown index for the California. */ expiration year? https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:89: /** The expiration year dropdown index for the California. */ ditto https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:92: /** The expiration year dropdown index for the United States. */ ditto https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:553: /** Selects the spinner value in the editor UI for credit cards. */ For credit cards? https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:8: #include <map> Needed? https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:413: AddressValidator address_validator_; Add comment that this is used for the sub keys. https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:416: std::map<std::string, std::vector<std::unique_ptr<Delegate>>> Like mentioned in another comment, maybe we can simplify this. https://codereview.chromium.org/2680143002/diff/240001/components/payments/ad... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2680143002/diff/240001/components/payments/ad... components/payments/address_normalizer.h:8: #include <map> needed? https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:162: if (load_rules_listener_) { no need for brackets. https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Can you add unittests for your GetRegionSubKeys method? Like testing the content you receive? It will also help future devs to understand what to expect from the function.
The CQ bit was checked by parastoog@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by parastoog@google.com 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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by parastoog@google.com 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 checked by parastoog@google.com 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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #10 (id:340001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:360001) has been deleted
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by parastoog@google.com 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 checked by parastoog@google.com 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 checked by parastoog@google.com to run a CQ dry run
Patchset #5 (id:380001) has been deleted
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...)
Patchset #5 (id:380001) has been deleted
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #5 (id:400001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:420001) has been deleted
Patchset #5 (id:440001) has been deleted
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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 parastoog@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #8 (id:520001) has been deleted
Patchset #7 (id:500001) has been deleted
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:827: * Starts loading the address validation rules for the specified {@code regionCode}. On 2017/02/28 16:13:59, sebsg wrote: > s/address validation/ subkeys ? Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:324: public void onAdminAreasLoading() { On 2017/02/28 16:13:59, sebsg wrote: > General question. Is it possible to query two different countries at the same > time, of sequentially fast enough before the timer runs out? > > If you can have more than one, you will cancel both queries at the same time > with the current implementation. > > Also if it's only one at the time, you can simplify how you keep track of the > pending queries in Personal_data_manager_android. Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:330: PersonalDataManager.getInstance().cancelPendingGetSubKeys(); On 2017/02/28 16:13:59, sebsg wrote: > You also only need to call this if the rules are not done loading. Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:344: // In terms of order, country must be the first field. On 2017/02/28 16:13:59, sebsg wrote: > nit: Country must be filled first. This means that in terms of order of appearance, country comes before other ones. So, I am adding the *field* here. (Not about *filling* the field.) https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:370: // In terms of order, phone number must be the last field. On 2017/02/28 16:13:59, sebsg wrote: > nit: Phone number must be the last field. Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:168: * an autofill profile On 2017/02/28 16:13:59, sebsg wrote: > Nit: put back the period Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:254: On 2017/02/28 16:13:59, sebsg wrote: > Nit: remove new-line Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:340: On 2017/02/28 16:13:59, sebsg wrote: > Nit: Remove new-line Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:86: /** The expiration year dropdown index for the California. */ On 2017/02/28 16:13:59, sebsg wrote: > expiration year? Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:89: /** The expiration year dropdown index for the California. */ On 2017/02/28 16:13:59, sebsg wrote: > ditto Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:92: /** The expiration year dropdown index for the United States. */ On 2017/02/28 16:13:59, sebsg wrote: > ditto Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:553: /** Selects the spinner value in the editor UI for credit cards. */ On 2017/02/28 16:13:59, sebsg wrote: > For credit cards? Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:8: #include <map> On 2017/02/28 16:14:00, sebsg wrote: > Needed? Not really. It was either suggested by git cl format or lint. Anyway, I'll remove them. https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:413: AddressValidator address_validator_; On 2017/02/28 16:14:00, sebsg wrote: > Add comment that this is used for the sub keys. Done. https://codereview.chromium.org/2680143002/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:416: std::map<std::string, std::vector<std::unique_ptr<Delegate>>> On 2017/02/28 16:14:00, sebsg wrote: > Like mentioned in another comment, maybe we can simplify this. Done. https://codereview.chromium.org/2680143002/diff/240001/components/payments/ad... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2680143002/diff/240001/components/payments/ad... components/payments/address_normalizer.h:8: #include <map> On 2017/02/28 16:14:00, sebsg wrote: > needed? => git cl lint error. [build/include_what_you_see] I don't know how seriously I should take the lint's comments. Anyway, I removed them for now. Done. https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:162: if (load_rules_listener_) { On 2017/02/28 16:14:00, sebsg wrote: > no need for brackets. Done. https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/240001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/02/28 16:14:00, sebsg wrote: > Can you add unittests for your GetRegionSubKeys method? > > Like testing the content you receive? It will also help future devs to > understand what to expect from the function. Done.
Good progress! https://codereview.chromium.org/2680143002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:536: private static int sNormalizationTimeoutSeconds = 5000; I use this in my address normalizer as seconds, so changing this is not possible. Maybe you can *1000 when you use it in getNormalizationTimeoutMS?Thanks! https://codereview.chromium.org/2680143002/diff/540001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:253: nit: Remove new-line https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:357: I18N_ADDRESS_VALIDATION_DATA_URL, Can you open a bug and put a TODO here to re-use the same source and storage for both our address normalizer/validator ? Thanks https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:800: // Check if there is any sub-key request. please add "for that region code" to the comment. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_region_code_ = ""; You should probably delete the delegate too. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: // The address validator used for sub key request. I think this comment is for your new address_validator_. Please move it there and put back the old comment here. https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.h:100: // This function is called when the rules are ready. nit: "This function should be called when the rules are loaded." https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:80: if (!country_code.compare("US")) { This should be in it's own test case instead of in this helper class. Thanks!
Patchset #5 (id:460001) has been deleted
Patchset #5 (id:480001) has been deleted
The CQ bit was checked by parastoog@google.com 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...
https://codereview.chromium.org/2680143002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:536: private static int sNormalizationTimeoutSeconds = 5000; On 2017/03/21 15:29:51, sebsg wrote: > I use this in my address normalizer as seconds, so changing this is not > possible. Maybe you can *1000 when you use it in > getNormalizationTimeoutMS?Thanks! Done. https://codereview.chromium.org/2680143002/diff/540001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestBillingAddressTest.java:253: On 2017/03/21 15:29:51, sebsg wrote: > nit: Remove new-line Done. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:357: I18N_ADDRESS_VALIDATION_DATA_URL, On 2017/03/21 15:29:52, sebsg wrote: > Can you open a bug and put a TODO here to re-use the same source and storage for > both our address normalizer/validator ? Thanks Acknowledged. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:800: // Check if there is any sub-key request. On 2017/03/21 15:29:51, sebsg wrote: > please add "for that region code" to the comment. Done. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_region_code_ = ""; On 2017/03/21 15:29:51, sebsg wrote: > You should probably delete the delegate too. Done. https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/540001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: // The address validator used for sub key request. On 2017/03/21 15:29:52, sebsg wrote: > I think this comment is for your new address_validator_. Please move it there > and put back the old comment here. :P Done. https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.h:100: // This function is called when the rules are ready. On 2017/03/21 15:29:52, sebsg wrote: > nit: "This function should be called when the rules are loaded." Done. https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/540001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:80: if (!country_code.compare("US")) { On 2017/03/21 15:29:52, sebsg wrote: > This should be in it's own test case instead of in this helper class. Thanks! Done.
I just have one more question. :) https://codereview.chromium.org/2680143002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:279: || PersonalDataManager.getInstance().isOnTestingMode()) { I'm not sure I understand the need for this? Could you explain? Thank you!
https://codereview.chromium.org/2680143002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:279: || PersonalDataManager.getInstance().isOnTestingMode()) { On 2017/03/21 20:49:31, sebsg wrote: > I'm not sure I understand the need for this? Could you explain? Thank you! If we are on the testing mode (we are testing for payment request), we would show the text field instead of the drop down in all cases, this way, the tests would be consistent regardless of the internet connection status. (I am not using the time out concept here, as we discussed previously, that won't work.)
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...)
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- To keep the fields in order, add all the fields to the form, only when
admin areas are loaded. Otherwise, admin areas would show up at the end (since
they are fetched asynchronously.)
- Change the PaymentRequestTest accordingly.
BUG=
==========
to
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded. This way
we can have the fields in order. Otherwise, admin areas would show up at the end
(since they are fetched asynchronously.)
- In the Payment Request tests, the admin area would always be a text field (and
not a drop down). This way, the tests would be independent of the state of the
internet connection.
- Tests were added to the AddressValidatorTest according to the changes.
BUG=
==========
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded. This way
we can have the fields in order. Otherwise, admin areas would show up at the end
(since they are fetched asynchronously.)
- In the Payment Request tests, the admin area would always be a text field (and
not a drop down). This way, the tests would be independent of the state of the
internet connection.
- Tests were added to the AddressValidatorTest according to the changes.
BUG=
==========
to
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded. This way
we can have the fields in order. Otherwise, admin areas would show up at the end
(since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a text
field (and not a drop down). This way, the tests would be independent of the
internet connection state.
- Tests were added to the AddressValidatorTest according to the changes.
BUG=
==========
Patchset #7 (id:580001) has been deleted
The CQ bit was checked by parastoog@google.com 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.
parastoog@google.com changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:931: public static int getTimeoutMS() { Would call this getRequestTimeoutMs https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:942: extra line here https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:52: private Pair<String, Runnable> mEventData; What's EventData? Would recommend a descriptive comment https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:165: mAdminAreasLoaded = true; Can we add a comment saying why we are setting this to true here? https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:268: } need extra line after this https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); can we have a special value of normalization timeout which is essentially == isTestingMode? https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:87: protected static final int CA = 8; is this based on fake data, or are we going to network?? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:308: class GetSubKeysRequester : public PersonalDataManagerAndroid::Delegate, SubKeysRequester? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:807: void PersonalDataManagerAndroid::StartGettingRegionSubKeys( nit: StartRegionSubKeysRequest? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:829: pending_subkey_region_code_ = ""; should we reset the requester? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:829: pending_subkey_region_code_ = ""; I don't think it's necessary to clear the region code. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:28: class Delegate { SubKeyRequestDelegate? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:412: std::unique_ptr<Delegate> pending_subkey_request_; it's not a request, it's a delegate. So I would change the variable name https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:71: if (rule_iterator != rules.end()) { if (rule_iterator != rules.end() && rule_iterator->second) return rule_iterator->second->GetSubKeys(); return std::vector<std::string>(); https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:115: std::string country_code = "US"; const to both
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #8 (id:620001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:931: public static int getTimeoutMS() { On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > Would call this getRequestTimeoutMs Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:942: On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > extra line here Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:52: private Pair<String, Runnable> mEventData; On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > What's EventData? Would recommend a descriptive comment I broke it into two variables with descriptive names. + a comment in the function. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:165: mAdminAreasLoaded = true; On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > Can we add a comment saying why we are setting this to true here? Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:268: } On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > need extra line after this Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > can we have a special value of normalization timeout which is essentially == > isTestingMode? Like checking what the timeout is set to, and based on that decide if it's on the testing mode? Like this: if (PDM.getInstance().getRequestTimeout() == 1) { // Do this } I consider this as a bad design. Or in setNormalizationTimeoutForTesting(int t) { if (t == 1) then setToTestingMode() } How about in setToTestingMode, setting the timeout to 1? And then removing the setNormalizationTimeoutForTesting()? https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:87: protected static final int CA = 8; On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > is this based on fake data, or are we going to network?? These lines are useless now that I have made the tests independent of the network connection. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:308: class GetSubKeysRequester : public PersonalDataManagerAndroid::Delegate, On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > SubKeysRequester? SubKeyRequester is the name of the class I added in the next CL (parallel to the AddressNormalizer class). This one is parallel to AndroidAddressNormalizerDelegate. So I'll change this to AndroidSubKeyRequesterDelegate. What do you think? https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:807: void PersonalDataManagerAndroid::StartGettingRegionSubKeys( On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > nit: StartRegionSubKeysRequest? Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:829: pending_subkey_region_code_ = ""; On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > should we reset the requester? Yes, I did this in the next CL. Too lazy to change this. :P I just did. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:829: pending_subkey_region_code_ = ""; On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > I don't think it's necessary to clear the region code. In OnAddressRulesLoaded we check the code AND THEN BASED ON THAT use the request. + I think it's a good practice to reset both. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:28: class Delegate { On 2017/03/24 17:24:33, Mathieu (slow - travel) wrote: > SubKeyRequestDelegate? Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:412: std::unique_ptr<Delegate> pending_subkey_request_; On 2017/03/24 17:24:33, Mathieu (slow - travel) wrote: > it's not a request, it's a delegate. So I would change the variable name It starts the sub key request. A pointer to an object of the sub key request delegate class. Parallel to the other request in the function. https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:71: if (rule_iterator != rules.end()) { On 2017/03/24 17:24:33, Mathieu (slow - travel) wrote: > if (rule_iterator != rules.end() && rule_iterator->second) > return rule_iterator->second->GetSubKeys(); > > return std::vector<std::string>(); Done. https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2680143002/diff/600001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:115: std::string country_code = "US"; On 2017/03/24 17:24:33, Mathieu (slow - travel) wrote: > const to both Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Are there tests? I only see PaymentRequestTestBase.java being updated. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); On 2017/03/24 22:42:23, Parastoo wrote: > On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > > can we have a special value of normalization timeout which is essentially == > > isTestingMode? > > Like checking what the timeout is set to, and based on that decide if it's on > the testing mode? Like this: > > if (PDM.getInstance().getRequestTimeout() == 1) { > // Do this > } > > I consider this as a bad design. > > Or in setNormalizationTimeoutForTesting(int t) { > if (t == 1) then setToTestingMode() > } > > > How about in setToTestingMode, setting the timeout to 1? And then removing the > setNormalizationTimeoutForTesting()? I find it weird to have two calls related to testing. I don't think we use the pattern "testing mode" in many places in the code. Ideally you would setNormalizationTimeoutForTesting(0) which would tell the code to not even make the request and return fake/no data right away. Ideally there would be no concept of "testing mode" https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:51: private boolean mOnCountryChange; do you need this variable and could you not instead check for mRecentlySelectedCountry != null https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:53: private Runnable mCountryChangedCB; mCountryChangedCallback https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:173: /* if onSubKeysReceived is already called, This is still slightly confusing to me. Consider phrasing: "This makes sure that onSubKeysReceived returns early if it's ever called after Cancel has occurred." https://codereview.chromium.org/2680143002/diff/640001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/640001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:324: ~AndroidSubKeyReqeusterDelegate() override {} *Requester
The CQ bit was checked by parastoog@google.com 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 checked by parastoog@google.com to run a CQ dry run
Patchset #9 (id:660001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:51: private boolean mOnCountryChange; On 2017/03/27 19:09:07, Mathieu wrote: > do you need this variable and could you not instead check for > mRecentlySelectedCountry != null Done. https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:53: private Runnable mCountryChangedCB; On 2017/03/27 19:09:06, Mathieu wrote: > mCountryChangedCallback Done. https://codereview.chromium.org/2680143002/diff/640001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:173: /* if onSubKeysReceived is already called, On 2017/03/27 19:09:07, Mathieu wrote: > This is still slightly confusing to me. Consider phrasing: > > "This makes sure that onSubKeysReceived returns early if it's ever called after > Cancel has occurred." Done. https://codereview.chromium.org/2680143002/diff/640001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/640001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:324: ~AndroidSubKeyReqeusterDelegate() override {} On 2017/03/27 19:09:07, Mathieu wrote: > *Requester Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); On 2017/03/27 19:09:06, Mathieu wrote: > On 2017/03/24 22:42:23, Parastoo wrote: > > On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > > > can we have a special value of normalization timeout which is essentially == > > > isTestingMode? > > > > Like checking what the timeout is set to, and based on that decide if it's on > > the testing mode? Like this: > > > > if (PDM.getInstance().getRequestTimeout() == 1) { > > // Do this > > } > > > > I consider this as a bad design. > > > > Or in setNormalizationTimeoutForTesting(int t) { > > if (t == 1) then setToTestingMode() > > } > > > > > > How about in setToTestingMode, setting the timeout to 1? And then removing the > > setNormalizationTimeoutForTesting()? > > I find it weird to have two calls related to testing. I don't think we use the > pattern "testing mode" in many places in the code. Ideally you would > setNormalizationTimeoutForTesting(0) which would tell the code to not even make > the request and return fake/no data right away. Ideally there would be no > concept of "testing mode" Still need to address this. https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:52: private Runnable mCountryChangeCallBack; nit: mCountryChangeCallback https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:322: public void loadAdminAreas() { make private? Rename loadAdminAreasForCountry(String countryCode), it will make the API clearer
The CQ bit was checked by parastoog@google.com 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: Exceeded global retry quota
The CQ bit was checked by parastoog@google.com 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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by parastoog@google.com 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...
Patchset #10 (id:700001) has been deleted
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...)
Patchset #10 (id:720001) has been deleted
The CQ bit was checked by parastoog@google.com
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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...
https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); On 2017/03/28 14:43:32, Mathieu wrote: > On 2017/03/27 19:09:06, Mathieu wrote: > > On 2017/03/24 22:42:23, Parastoo wrote: > > > On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > > > > can we have a special value of normalization timeout which is essentially > == > > > > isTestingMode? > > > > > > Like checking what the timeout is set to, and based on that decide if it's > on > > > the testing mode? Like this: > > > > > > if (PDM.getInstance().getRequestTimeout() == 1) { > > > // Do this > > > } > > > > > > I consider this as a bad design. > > > > > > Or in setNormalizationTimeoutForTesting(int t) { > > > if (t == 1) then setToTestingMode() > > > } > > > > > > > > > How about in setToTestingMode, setting the timeout to 1? And then removing > the > > > setNormalizationTimeoutForTesting()? > > > > I find it weird to have two calls related to testing. I don't think we use the > > pattern "testing mode" in many places in the code. Ideally you would > > setNormalizationTimeoutForTesting(0) which would tell the code to not even > make > > the request and return fake/no data right away. Ideally there would be no > > concept of "testing mode" > > Still need to address this. Done. https://codereview.chromium.org/2680143002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:27: setNormalizationTimeoutForTesting(); On 2017/03/27 19:09:06, Mathieu wrote: > On 2017/03/24 22:42:23, Parastoo wrote: > > On 2017/03/24 17:24:32, Mathieu (slow - travel) wrote: > > > can we have a special value of normalization timeout which is essentially == > > > isTestingMode? > > > > Like checking what the timeout is set to, and based on that decide if it's on > > the testing mode? Like this: > > > > if (PDM.getInstance().getRequestTimeout() == 1) { > > // Do this > > } > > > > I consider this as a bad design. > > > > Or in setNormalizationTimeoutForTesting(int t) { > > if (t == 1) then setToTestingMode() > > } > > > > > > How about in setToTestingMode, setting the timeout to 1? And then removing the > > setNormalizationTimeoutForTesting()? > > I find it weird to have two calls related to testing. I don't think we use the > pattern "testing mode" in many places in the code. Ideally you would > setNormalizationTimeoutForTesting(0) which would tell the code to not even make > the request and return fake/no data right away. Ideally there would be no > concept of "testing mode" Done. https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:52: private Runnable mCountryChangeCallBack; On 2017/03/28 14:43:32, Mathieu wrote: > nit: mCountryChangeCallback Done. https://codereview.chromium.org/2680143002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:322: public void loadAdminAreas() { On 2017/03/28 14:43:32, Mathieu wrote: > make private? Rename loadAdminAreasForCountry(String countryCode), it will make > the API clearer Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + rouslan@chromium.org
This lgtm but I'm a leeetle bit out of my comfort zone reviewing Java UI code. Rouslan PTAL
Before I start reviewing this change, please set the BUG number and format the CL description as follows: <At most 64 characters of title.> <Paragraphs of at most 72 characters each.> <Paragraphs of at most 72 characters each.> <Paragraphs of at most 72 characters each.> <BUG=XYZ> The reason for this formatting is to look better in "git log" on people's narrow terminals. Although most terminals are 80 character wide, the possible reverts and relands will prepend each line with "> ", which will make the lines wider. The even shorter requirement for the title is to accommodate the git revision hash and possible [Merge M-59] type tags. The website does not accommodate reflowing your comments easily, but you can use your favorite editor instead by running "git cl format" from command line. Thank you!
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..) in the payment request
form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from the i18n
aggregate server, and the country sub-keys are retrieved from the right rule.
- If the server doesn't respond, or there is a time-out, or if no admin areas
are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded. This way
we can have the fields in order. Otherwise, admin areas would show up at the end
(since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a text
field (and not a drop down). This way, the tests would be independent of the
internet connection state.
- Tests were added to the AddressValidatorTest according to the changes.
BUG=
==========
to
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country sub-keys are retrieved from
the right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the internet connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
==========
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country sub-keys are retrieved from
the right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the internet connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
==========
to
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country sub-keys are retrieved from
the right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the internet connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
==========
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country sub-keys are retrieved from
the right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the internet connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
==========
to
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country's sub-keys are retrieved from the
right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the network connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
==========
Good job! https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:55: /** Need a newline above. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:120: mEditor.removeAllFields(); Should there be a spinner or "Loading..." text while the rules are loading? https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:107: for (String s : keys) { Range loops are expensive on Android. Let's use a "for (int i = 0..." type loop. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:118: if (result == 0) result = lhs.getKey().compareTo(rhs.getKey()); Why use a collator for the value, but not for the key? https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:324: ~AndroidSubKeyRequesterDelegate() override {} This object has a private destructor and never deletes itself. Does it live on forever? https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:326: void OnRulesSuccessfullyLoaded() override { Add a comment that indicates which class you're overriding. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:338: JNIEnv* env_; No need to save this. You should get new ones from base::android::AttachCurrentThread();. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:356: std::unique_ptr<::i18n::addressinput::Source>( Use base::MakeUnique. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_request_->OnRulesSuccessfullyLoaded(); Need to check for |success| == false. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:815: new AndroidSubKeyRequesterDelegate(env, jdelegate, use base::MakeUnique. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:831: pending_subkey_region_code_ = ""; .clear(); https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:34: }; Let's put the interface first in the class (before statics) and separate it by space from the statics. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: payments::AddressNormalizer address_normalizer_; payments::AddressNormalizer has an AddressValidator inside of it. AddressValidator contains a huge amount of data in memory. Therefore, let's not have two of these variables in memory. There're several approaches to take here. Let me know if you need help. https://codereview.chromium.org/2680143002/diff/740001/components/payments/co... File components/payments/core/address_normalizer.h (left): https://codereview.chromium.org/2680143002/diff/740001/components/payments/co... components/payments/core/address_normalizer.h:11: #include <vector> Deleted by mistake? https://codereview.chromium.org/2680143002/diff/740001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/740001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: return (rule_iterator->second)->GetSubKeys(); No need for ().
https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:55: /** On 2017/03/30 18:48:58, ಠ_ಠwrote: > Need a newline above. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:120: mEditor.removeAllFields(); On 2017/03/30 18:48:58, ಠ_ಠwrote: > Should there be a spinner or "Loading..." text while the rules are loading? I don't know. But, later, the sub-keys will be fetched from the data/ssl-address (rather than data/ssl-aggregate-address), which means that they all will take some milisecs to load. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:107: for (String s : keys) { On 2017/03/30 18:48:58, ಠ_ಠwrote: > Range loops are expensive on Android. Let's use a "for (int i = 0..." type loop. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:118: if (result == 0) result = lhs.getKey().compareTo(rhs.getKey()); On 2017/03/30 18:48:58, ಠ_ಠwrote: > Why use a collator for the value, but not for the key? Well, the second line (if (result == 0) is redundant. At the end, it should be according to what appears on the list, which is admin area's value (such as Quebec, and not QC). Right now, key == value. (We show QC on the list.) https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:118: if (result == 0) result = lhs.getKey().compareTo(rhs.getKey()); On 2017/03/30 18:48:58, ಠ_ಠwrote: > Why use a collator for the value, but not for the key? Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:324: ~AndroidSubKeyRequesterDelegate() override {} On 2017/03/30 18:48:58, ಠ_ಠwrote: > This object has a private destructor and never deletes itself. Does it live on > forever? Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:326: void OnRulesSuccessfullyLoaded() override { On 2017/03/30 18:48:58, ಠ_ಠwrote: > Add a comment that indicates which class you're overriding. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:338: JNIEnv* env_; On 2017/03/30 18:48:58, ಠ_ಠwrote: > No need to save this. You should get new ones from > base::android::AttachCurrentThread();. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:356: std::unique_ptr<::i18n::addressinput::Source>( On 2017/03/30 18:48:58, ಠ_ಠwrote: > Use base::MakeUnique. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_request_->OnRulesSuccessfullyLoaded(); On 2017/03/30 18:48:58, ಠ_ಠwrote: > Need to check for |success| == false. An empty vector will be returned, even if false. => success makes no difference. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:815: new AndroidSubKeyRequesterDelegate(env, jdelegate, On 2017/03/30 18:48:58, ಠ_ಠwrote: > use base::MakeUnique. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:831: pending_subkey_region_code_ = ""; On 2017/03/30 18:48:58, ಠ_ಠwrote: > .clear(); Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:34: }; On 2017/03/30 18:48:58, ಠ_ಠwrote: > Let's put the interface first in the class (before statics) and separate it by > space from the statics. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: payments::AddressNormalizer address_normalizer_; On 2017/03/30 18:48:58, ಠ_ಠwrote: > payments::AddressNormalizer has an AddressValidator inside of it. > AddressValidator contains a huge amount of data in memory. Therefore, let's not > have two of these variables in memory. There're several approaches to take here. > Let me know if you need help. In the next CL (that is almost done), all these codes are moved to another class (SubKeyRequester) that is consistent with the AddressNormalizer. SubKeyRequester will have its own address_validator_. Do you mean "address_normalizer_" and "subkey_requester" should share the same address_validator_? They have a separate storage, and will have different sources + the callback to OnAddressRulesLoaded. Or you mean another approach? https://codereview.chromium.org/2680143002/diff/740001/components/payments/co... File components/payments/core/address_normalizer.h (left): https://codereview.chromium.org/2680143002/diff/740001/components/payments/co... components/payments/core/address_normalizer.h:11: #include <vector> On 2017/03/30 18:48:59, ಠ_ಠwrote: > Deleted by mistake? Done. https://codereview.chromium.org/2680143002/diff/740001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/740001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: return (rule_iterator->second)->GetSubKeys(); On 2017/03/30 18:48:59, ಠ_ಠwrote: > No need for (). Done.
Thank you for quick response! https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:120: mEditor.removeAllFields(); On 2017/03/31 15:44:14, Parastoo wrote: > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > Should there be a spinner or "Loading..." text while the rules are loading? > > I don't know. But, later, the sub-keys will be fetched from the data/ssl-address > (rather than data/ssl-aggregate-address), which means that they all will take > some milisecs to load. Let's put a "Loading..." text in the editor view when .removeAllFields() is called. Pro-tip: Connect to wifi that simulates 2G or 3G network to see how your UI behaves on slow networks. go/google-c. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_request_->OnRulesSuccessfullyLoaded(); On 2017/03/31 15:44:14, Parastoo wrote: > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > Need to check for |success| == false. > > An empty vector will be returned, even if false. => success makes no difference. This is not immediately clear from reading the code, so please add a comment about it. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: payments::AddressNormalizer address_normalizer_; On 2017/03/31 15:44:14, Parastoo wrote: > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > payments::AddressNormalizer has an AddressValidator inside of it. > > AddressValidator contains a huge amount of data in memory. Therefore, let's > not > > have two of these variables in memory. There're several approaches to take > here. > > Let me know if you need help. > > In the next CL (that is almost done), all these codes are moved to another class > (SubKeyRequester) that is consistent with the AddressNormalizer. SubKeyRequester > will have its own address_validator_. Do you mean "address_normalizer_" and > "subkey_requester" should share the same address_validator_? They have a > separate storage, and will have different sources + the callback to > OnAddressRulesLoaded. Or you mean another approach? I'm fairly certain that disk storage will be shared. See usages of ValidationRulesStorageFactory::CreateStorage(): https://cs.chromium.org/chromium/src/chrome/browser/autofill/validation_rules... It's a singleton that's used in both AddressNormalizer and AddressValidator. Since AddressNormalizer downloads aggregated data, but you plan to download non-aggregated data, the data from these two objects can collapse on disk, if it uses the same keys. I have not verified this, but you should. Essentially, you should put LOG(ERROR) in ChromeStorageImpl::Put/Get and ChromeMetadataSource::Download to make sure that correct data is being written down and then read from storage. https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:56: /** Need a newline between "private..." and "/**" lines. https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:120: int result = collator.compare(lhs.getValue(), rhs.getValue()); No need for temporary variable "result". Save a line, return directly. https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:309: class AndroidSubKeyRequesterDelegate Use the prefix "SelfDeleting" for the class name. Also add a class comment that describes when the object will delete itself. https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:357: std::unique_ptr<::i18n::addressinput::Source>( No need for std::unique_ptr<>() wrapper for base::MakeUnique. https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:815: std::unique_ptr<AndroidSubKeyRequesterDelegate> requester( Please use "requester = " instead of "requester(". This is more clear, IMHO.
The CQ bit was checked by parastoog@google.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #12 (id:780001) has been deleted
https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:120: mEditor.removeAllFields(); On 2017/03/31 16:06:20, ಠ_ಠwrote: > On 2017/03/31 15:44:14, Parastoo wrote: > > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > > Should there be a spinner or "Loading..." text while the rules are loading? > > > > I don't know. But, later, the sub-keys will be fetched from the > data/ssl-address > > (rather than data/ssl-aggregate-address), which means that they all will take > > some milisecs to load. > > Let's put a "Loading..." text in the editor view when .removeAllFields() is > called. Pro-tip: Connect to wifi that simulates 2G or 3G network to see how your > UI behaves on slow networks. go/google-c. Note: There is a limit of 5 secs, if the data is not fetched (which is the case for China + Brazil when fetched for the first time, or a slow network connection), a textfield would appear instead. => 0 < latency < 5secs. https://codereview.chromium.org/2680143002/diff/740001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:120: mEditor.removeAllFields(); On 2017/03/31 16:06:20, ಠ_ಠwrote: > On 2017/03/31 15:44:14, Parastoo wrote: > > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > > Should there be a spinner or "Loading..." text while the rules are loading? > > > > I don't know. But, later, the sub-keys will be fetched from the > data/ssl-address > > (rather than data/ssl-aggregate-address), which means that they all will take > > some milisecs to load. > > Let's put a "Loading..." text in the editor view when .removeAllFields() is > called. Pro-tip: Connect to wifi that simulates 2G or 3G network to see how your > UI behaves on slow networks. go/google-c. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:803: pending_subkey_request_->OnRulesSuccessfullyLoaded(); On 2017/03/31 16:06:20, ಠ_ಠwrote: > On 2017/03/31 15:44:14, Parastoo wrote: > > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > > Need to check for |success| == false. > > > > An empty vector will be returned, even if false. => success makes no > difference. > > This is not immediately clear from reading the code, so please add a comment > about it. Done. https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/740001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: payments::AddressNormalizer address_normalizer_; On 2017/03/31 16:06:20, ಠ_ಠwrote: > On 2017/03/31 15:44:14, Parastoo wrote: > > On 2017/03/30 18:48:58, ಠ_ಠwrote: > > > payments::AddressNormalizer has an AddressValidator inside of it. > > > AddressValidator contains a huge amount of data in memory. Therefore, let's > > not > > > have two of these variables in memory. There're several approaches to take > > here. > > > Let me know if you need help. > > > > In the next CL (that is almost done), all these codes are moved to another > class > > (SubKeyRequester) that is consistent with the AddressNormalizer. > SubKeyRequester > > will have its own address_validator_. Do you mean "address_normalizer_" and > > "subkey_requester" should share the same address_validator_? They have a > > separate storage, and will have different sources + the callback to > > OnAddressRulesLoaded. Or you mean another approach? > > I'm fairly certain that disk storage will be shared. See usages of > ValidationRulesStorageFactory::CreateStorage(): > > https://cs.chromium.org/chromium/src/chrome/browser/autofill/validation_rules... > > It's a singleton that's used in both AddressNormalizer and AddressValidator. > Since AddressNormalizer downloads aggregated data, but you plan to download > non-aggregated data, the data from these two objects can collapse on disk, if it > uses the same keys. I have not verified this, but you should. Essentially, you > should put LOG(ERROR) in ChromeStorageImpl::Put/Get and > ChromeMetadataSource::Download to make sure that correct data is being written > down and then read from storage. Acknowledged. https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:56: /** On 2017/03/31 16:06:20, ಠ_ಠwrote: > Need a newline between "private..." and "/**" lines. Done. https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:120: int result = collator.compare(lhs.getValue(), rhs.getValue()); On 2017/03/31 16:06:20, ಠ_ಠwrote: > No need for temporary variable "result". Save a line, return directly. Done. https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:309: class AndroidSubKeyRequesterDelegate On 2017/03/31 16:06:20, ಠ_ಠwrote: > Use the prefix "SelfDeleting" for the class name. Also add a class comment that > describes when the object will delete itself. It is not self-deleting in this CL, this was a mistake in this patch (wrong patch as I said.) https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:357: std::unique_ptr<::i18n::addressinput::Source>( On 2017/03/31 16:06:20, ಠ_ಠwrote: > No need for std::unique_ptr<>() wrapper for base::MakeUnique. Done. https://codereview.chromium.org/2680143002/diff/760001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:815: std::unique_ptr<AndroidSubKeyRequesterDelegate> requester( On 2017/03/31 16:06:20, ಠ_ಠwrote: > Please use "requester = " instead of "requester(". This is more clear, IMHO. Done.
The CQ bit was checked by parastoog@google.com 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...
LGTM % nit https://codereview.chromium.org/2680143002/diff/820001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/820001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:800: // if |success| == false, an empty list of sub-keys will be returned. Clarify the comment by naming the method that returns the empty list of sub-keys.
The CQ bit was checked by parastoog@google.com to run a CQ dry run
https://codereview.chromium.org/2680143002/diff/820001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/820001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:800: // if |success| == false, an empty list of sub-keys will be returned. On 2017/04/03 18:47:39, ಠ_ಠwrote: > Clarify the comment by naming the method that returns the empty list of > sub-keys. Done.
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 parastoog@google.com
The CQ bit was checked by parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2680143002/#ps840001 (title: "nit")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
parastoog@google.com changed reviewers: + dfalcantara@chromium.org
dfalcantara@chromium.org: Please review changes in
I don't know what I'm supposed to be looking at. I'd trust that rouslan@ understands most of the logic. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:868: } Put this next to getRegionSubKeys instead of splitting it up with normalizeAddress. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:931: return sNormalizationTimeoutSeconds * 1000; return TimeUnit.SECONDS.toMillis(sNormalizationTimeoutSeconds); https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:117: * If the selected country on the country dropdown list is changed, 1) Javadoc comments start with /** and end with */. Fix here and everywhere else in your CL. 2) Indent at 100, not at 80. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:178: /* This makes sure that onSubKeysReceived returns early if it's Java-land uses // for non-javadoc comments. See the rest of the file. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:297: /* Callback of the sub keys request that is called when the sub keys are loaded. /** * Callback of the sub keys request that is called when the sub keys are loaded. * Here the sub keys are admin areas (sub keys of the region=country). */ Is this different from how the other onSubKeysReceived calls are implemented? Does this comment apply to everywhere that makes this callback? If so, this detail should be put in the interface itself. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:304: // If can't get admin areas from server or there is no admin area on the 1) be consistent about whether you use "Admin Areas" or "admin areas". 2) If Chrome can't get admin areas... then use the text field drop-down https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:312: /* Admin Areas need to be fetched in two cases. 1) Use //. 2) two cases. -> two cases: https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:315: mRecentlySelectedCountry is not null <=> the 2nd case */ What does this line mean specifically? Expand your "<=>". https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:342: // In each rule, admin area keys are saved under sub keys of country. sub-keys https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:348: /* In case the the admin areas are not loaded yet, starts a timer to cancel the request. */ If it fits on a single line, then: /** In case the the admin areas are not loaded yet, starts a timer to cancel the request. */ Otherwise: /** * In case the the admin areas are not loaded yet, starts a timer to cancel the request. */ https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:349: public void onAdminAreasLoading() { save a level of indentation by early returning. if (mAdminAreasLoaded) return; https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:358: if (!mAdminAreasLoaded) { This is exactly the code above with a delay. Don't duplicate it; make another function and pull it out. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:364: } One of these pathways is asynchronous and one is not. Doesn't that complicate the pathway? Can you just always post? https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:201: public static EditorFieldModel createDropdown() { public methods need javadocs. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:339: public boolean isTextField() { public methods -> javadocs https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:108: // TODO (@crbug.com/691643): show names, save keys. // TODO(parastoog): Show names, save keys. crbug.com/691643 https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:802: // Check if there is any sub-key request for that region code. This comment confuses me. You don't make a call to GetRegionSubKeys() here, nor do you check |success|. What is it referring to? If you don't check success, why pass it in? https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: // The address validator used to normalize address. grammatically weird now. why'd you switch? https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:408: // The address validator used for sub key request. be consistent: you use "sub-key" below and "sub key" here https://codereview.chromium.org/2680143002/diff/840001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/840001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:71: if (rule_iterator != rules.end() && rule_iterator->second) might be more readable if you treat all error conditions as early exits, mirroring what you do on line 65. if (rule_iterator == rules.end() || !rule_iterator->second) return std::vector<std::string>(); return rule_iterator->second->GetSubKeys();
The CQ bit was checked by parastoog@google.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by parastoog@google.com 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 parastoog@google.com
Patchset #16 (id:880001) has been deleted
Patchset #15 (id:860001) has been deleted
Patchset #15 (id:900001) has been deleted
The CQ bit was checked by parastoog@google.com 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...
https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:868: } On 2017/04/05 00:14:20, dfalcantara (load balance plz) wrote: > Put this next to getRegionSubKeys instead of splitting it up with > normalizeAddress. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:931: return sNormalizationTimeoutSeconds * 1000; On 2017/04/05 00:14:20, dfalcantara (load balance plz) wrote: > return TimeUnit.SECONDS.toMillis(sNormalizationTimeoutSeconds); Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:117: * If the selected country on the country dropdown list is changed, On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > 1) Javadoc comments start with /** and end with */. Fix here and everywhere > else in your CL. > > 2) Indent at 100, not at 80. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:178: /* This makes sure that onSubKeysReceived returns early if it's On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > Java-land uses // for non-javadoc comments. See the rest of the file. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:297: /* Callback of the sub keys request that is called when the sub keys are loaded. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > /** > * Callback of the sub keys request that is called when the sub keys are loaded. > * Here the sub keys are admin areas (sub keys of the region=country). > */ > > Is this different from how the other onSubKeysReceived calls are implemented? > Does this comment apply to everywhere that makes this callback? If so, this > detail should be put in the interface itself. It isn't different. The comment was moved to the interface. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:304: // If can't get admin areas from server or there is no admin area on the On 2017/04/05 00:14:20, dfalcantara (load balance plz) wrote: > 1) be consistent about whether you use "Admin Areas" or "admin areas". > 2) If Chrome can't get admin areas... > then use the text field > drop-down Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:312: /* Admin Areas need to be fetched in two cases. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > 1) Use //. > 2) two cases. -> two cases: Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:315: mRecentlySelectedCountry is not null <=> the 2nd case */ On 2017/04/05 00:14:20, dfalcantara (load balance plz) wrote: > What does this line mean specifically? Expand your "<=>". Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:342: // In each rule, admin area keys are saved under sub keys of country. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > sub-keys Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:348: /* In case the the admin areas are not loaded yet, starts a timer to cancel the request. */ On 2017/04/05 00:14:20, dfalcantara (load balance plz) wrote: > If it fits on a single line, then: > > /** In case the the admin areas are not loaded yet, starts a timer to cancel the > request. */ > > Otherwise: > > /** > * In case the the admin areas are not loaded yet, starts a timer to cancel the > request. > */ Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:349: public void onAdminAreasLoading() { On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > save a level of indentation by early returning. > > if (mAdminAreasLoaded) return; Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:358: if (!mAdminAreasLoaded) { On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > This is exactly the code above with a delay. Don't duplicate it; make another > function and pull it out. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:364: } On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > One of these pathways is asynchronous and one is not. Doesn't that complicate > the pathway? Can you just always post? Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:201: public static EditorFieldModel createDropdown() { On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > public methods need javadocs. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:339: public boolean isTextField() { On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > public methods -> javadocs Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:108: // TODO (@crbug.com/691643): show names, save keys. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > // TODO(parastoog): Show names, save keys. crbug.com/691643 I was initially told to do otherwise, would you please show me a reference I can look up/show for future reference? Thanks. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:802: // Check if there is any sub-key request for that region code. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > This comment confuses me. You don't make a call to GetRegionSubKeys() here, nor > do you check |success|. What is it referring to? If you don't check success, > why pass it in? Why pass it? The function (LoadRulesListner::OnAddressRulesLoaded) is shared with the AddressNormalizer class. In address normalization, success is used. AddressValidator::GetRegionSubKeys is called from AndroidSubKeyRequesterDelegate::OnRulesSuccessfullyLoaded. https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:405: // The address validator used to normalize address. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > grammatically weird now. why'd you switch? typo. Done. https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:408: // The address validator used for sub key request. On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > be consistent: you use "sub-key" below and "sub key" here Done. https://codereview.chromium.org/2680143002/diff/840001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2680143002/diff/840001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:71: if (rule_iterator != rules.end() && rule_iterator->second) On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > might be more readable if you treat all error conditions as early exits, > mirroring what you do on line 65. > > if (rule_iterator == rules.end() || !rule_iterator->second) > return std::vector<std::string>(); > > return rule_iterator->second->GetSubKeys(); Done.
https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:108: // TODO (@crbug.com/691643): show names, save keys. On 2017/04/05 19:49:07, Parastoo wrote: > On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > > // TODO(parastoog): Show names, save keys. crbug.com/691643 > > I was initially told to do otherwise, would you please show me a reference I can > look up/show for future reference? Thanks. > > Done. git grep "TODO(.*)" https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:408: // The address validator used for sub key request. On 2017/04/05 19:49:07, Parastoo wrote: > On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > > be consistent: you use "sub-key" below and "sub key" here > > Done. You didn't change anything here. https://codereview.chromium.org/2680143002/diff/940001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/940001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:78: */ Double-javadocing doesn't work. Combine this comment appropriately.
The CQ bit was checked by parastoog@google.com 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...
Thank you for your quick response! https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:108: // TODO (@crbug.com/691643): show names, save keys. On 2017/04/05 20:21:38, dfalcantara (load balance plz) wrote: > On 2017/04/05 19:49:07, Parastoo wrote: > > On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > > > // TODO(parastoog): Show names, save keys. crbug.com/691643 > > > > I was initially told to do otherwise, would you please show me a reference I > can > > look up/show for future reference? Thanks. > > > > Done. > > git grep "TODO(.*)" Thanks. https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2680143002/diff/840001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:408: // The address validator used for sub key request. On 2017/04/05 20:21:38, dfalcantara (load balance plz) wrote: > On 2017/04/05 19:49:07, Parastoo wrote: > > On 2017/04/05 00:14:21, dfalcantara (load balance plz) wrote: > > > be consistent: you use "sub-key" below and "sub key" here > > > > Done. > > You didn't change anything here. Done. https://codereview.chromium.org/2680143002/diff/940001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2680143002/diff/940001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:78: */ On 2017/04/05 20:21:38, dfalcantara (load balance plz) wrote: > Double-javadocing doesn't work. Combine this comment appropriately. Done.
lol yeah... I think yours was review #7 or 8 yesterday. queue was relatively empty today. lgtm
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2680143002/#ps960001 (title: "nit")
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
On 2017/04/05 21:55:44, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) Don't know why you're triggering this error; it's not on a line you touched: FindBugs reported the following issues: LI_LAZY_INIT_STATIC: Incorrect lazy initialization of static field In class org.chromium.chrome.browser.autofill.PersonalDataManager In method org.chromium.chrome.browser.autofill.PersonalDataManager.getInstance() On field org.chromium.chrome.browser.autofill.PersonalDataManager.sManager At PersonalDataManager.java:[lines 532-533]
Patchset #18 (id:980001) has been deleted
The CQ bit was checked by parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2680143002/#ps1000001 (title: "Supress error-- suggested by clank team")
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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...
On 2017/04/05 22:06:23, dfalcantara (load balance plz) wrote: > On 2017/04/05 21:55:44, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) > > Don't know why you're triggering this error; it's not on a line you touched: > > FindBugs reported the following issues: > LI_LAZY_INIT_STATIC: Incorrect lazy initialization of static field > In class org.chromium.chrome.browser.autofill.PersonalDataManager > In method > org.chromium.chrome.browser.autofill.PersonalDataManager.getInstance() > On field org.chromium.chrome.browser.autofill.PersonalDataManager.sManager > At PersonalDataManager.java:[lines 532-533] According to the clank-team@, "it doesn't understand the thread assertions (it doesn't understand the methods that are called with the lock already held, neither.)" And the suggested solution was to add a suppression.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Go for it. still lgtm
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1000001, "attempt_start_ts": 1491508757637550,
"parent_rev": "46a8dce41297331415c5d0ecbfe05acd181d1e80", "commit_rev":
"69b196d676ecbbb986009aaf9f5627fd6e332de1"}
Message was sent while issue was closed.
Description was changed from
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country's sub-keys are retrieved from the
right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the network connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
==========
to
==========
Use drop-down list for admin areas (state, province, ..)
-- In the payment request form (instead of the previous text-field).
- For this purpose, the rules of the specified country are loaded from
the i18n aggregate server, and the country's sub-keys are retrieved from the
right rule.
- If the server doesn't respond, or there is a time-out, or if no
admin areas are recorded for that country, show text-field.
-- Add all the fields to the form, only when admin areas are loaded.
This way we can have the fields in order. Otherwise, admin areas
would show up at the end (since they are fetched asynchronously.)
- In the Payment Request tests, the admin area field would always be a
text field (and not a drop down). This way, the tests would be
independent of the network connection state.
- Tests were added to the AddressValidatorTest according to the
changes.
BUG=675634
Review-Url: https://codereview.chromium.org/2680143002
Cr-Commit-Position: refs/heads/master@{#462611}
Committed:
https://chromium.googlesource.com/chromium/src/+/69b196d676ecbbb986009aaf9f56...
==========
Message was sent while issue was closed.
Committed patchset #18 (id:1000001) as https://chromium.googlesource.com/chromium/src/+/69b196d676ecbbb986009aaf9f56...
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:1000001) has been created in https://codereview.chromium.org/2800013004/ by hanxi@chromium.org. The reason for reverting is: The patch causes chrome_public_test_apk failing on chromium.android/Marshmallow Tablet Tester: crbug.com/709491..
Message was sent while issue was closed.
Patchset #19 (id:1020001) has been deleted
Message was sent while issue was closed.
Patchset #19 (id:1040001) has been deleted |
