Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966103002:
[Payments] Show admin area complete names on PR form. (Closed)
Description- In payment request form,
for the admin area drop-down field,
show admin areas' complete names rather than codes.
- If available, show the names in the device's language,
otherwise, show in the default language for that country.
Exp: Show Quebec and not QC, and show Québec
if the device's language is set to French.
- If the name of the admin area for a country
is not available, show the admin area code.
- Change the unit-tests accordingly.
BUG=691643
Review-Url: https://codereview.chromium.org/2966103002
Cr-Commit-Position: refs/heads/master@{#485338}
Committed: https://chromium.googlesource.com/chromium/src/+/51456a9609172d3f8a803c056c1789a55b52fd60
Patch Set 1 : Keys to names #
Total comments: 7
Patch Set 2 : Show the name in the appropriate language. #
Total comments: 12
Patch Set 3 : Unit-tests added. #
Total comments: 9
Patch Set 4 : Small Changes. #Messages
Total messages: 71 (56 generated)
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_tsan_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 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...) 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 #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== keys to names BUG= ========== to ========== In payment request form, for the admin area field, show admin areas' complete names rather than codes. Change the unit-tests accordingly. BUG=691643 ==========
Description was changed from ========== In payment request form, for the admin area field, show admin areas' complete names rather than codes. Change the unit-tests accordingly. BUG=691643 ========== to ========== In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. Exp: Show Quebec and not QC. Change the unit-tests accordingly. BUG=691643 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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 #1 (id:40001) has been deleted
parastoog@google.com changed reviewers: + mathp@chromium.org
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 #1 (id:60001) 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...
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
parastoog@google.com changed reviewers: + dtrainor@chromium.org
dtrainor@chromium.org: Please review changes in
lgtm https://codereview.chromium.org/2966103002/diff/100001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2966103002/diff/100001/components/payments/co... components/payments/core/subkey_requester.h:13: using SubKeyReceiverCallback = add a comment about what each parameter is https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: size_t code_prefix_size = code_prefix.size(); nit: code_prefix_size = region_code.size() + 6; // 6 is the size of "data/" and the trailing "/" https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:75: if (r.first.size() <= code_prefix_size) Could we get in a situation where the key is data/CA/QC--fr (e.g., https://chromium-i18n-canary.appspot.com/ssl-aggregate-address/data/CA/QC--fr) or this will never happen here
parastoog@google.com changed reviewers: - dtrainor@chromium.org
mathp@chromium.org changed reviewers: + dtrainor@chromium.org
David, PTAL
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...
Description was changed from ========== In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. Exp: Show Quebec and not QC. Change the unit-tests accordingly. BUG=691643 ========== to ========== - In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. - If available, show the names in the device's language, otherwise, show in the default language for that country. Exp: Show Quebec and not QC, and show Québec if the device's language is set to French. - If the name of the admin area is not available, show the admin area code. - Change the unit-tests accordingly. BUG=691643 ==========
Description was changed from ========== - In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. - If available, show the names in the device's language, otherwise, show in the default language for that country. Exp: Show Quebec and not QC, and show Québec if the device's language is set to French. - If the name of the admin area is not available, show the admin area code. - Change the unit-tests accordingly. BUG=691643 ========== to ========== - In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. - If available, show the names in the device's language, otherwise, show in the default language for that country. Exp: Show Quebec and not QC, and show Québec if the device's language is set to French. - If the name of the admin area for a country is not available, show the admin area code. - Change the unit-tests accordingly. BUG=691643 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by parastoog@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2966103002/#ps120001 (title: "Show the name in the appropriate language.")
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...)
mathp@chromium.org changed reviewers: - parastoog@chromium.org
Would love to see more detailed test cases around language. +sebsg https://codereview.chromium.org/2966103002/diff/100001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2966103002/diff/100001/components/payments/co... components/payments/core/subkey_requester.h:13: using SubKeyReceiverCallback = On 2017/07/05 16:07:15, Mathieu wrote: > add a comment about what each parameter is this is still not resolved? https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... components/payments/core/subkey_requester.h:38: // |region_code|. The received subkeys will be returned to the |requester|. If add a comment about the format of |language| (or an example) https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:65: "{\"data/OZ\": " can you have different languages in your data and test that your code works? https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: // this should never happen nit: Please add capital letters to sentences and end punctuation. https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:73: if (rule_iterator == rules.end() || !rule_iterator->second) Can this happen if the rules on the server are bad? https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:82: lang_suffix = "--" + language; // exp: --fr exp -> ex: https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.h:101: // This function should be called when the rules are loaded. mention |language|
mathp@chromium.org changed reviewers: + sebsg@chromium.org
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/2966103002/diff/100001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2966103002/diff/100001/components/payments/co... components/payments/core/subkey_requester.h:13: using SubKeyReceiverCallback = On 2017/07/07 13:10:01, Mathieu wrote: > On 2017/07/05 16:07:15, Mathieu wrote: > > add a comment about what each parameter is > > this is still not resolved? Done. https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: size_t code_prefix_size = code_prefix.size(); On 2017/07/05 16:07:15, Mathieu wrote: > nit: code_prefix_size = region_code.size() + 6; // 6 is the size of "data/" > and the trailing "/" Done. https://codereview.chromium.org/2966103002/diff/100001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:75: if (r.first.size() <= code_prefix_size) On 2017/07/05 16:07:15, Mathieu wrote: > Could we get in a situation where the key is data/CA/QC--fr (e.g., > https://chromium-i18n-canary.appspot.com/ssl-aggregate-address/data/CA/QC--fr) > or this will never happen here Done. https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... components/payments/core/subkey_requester.h:38: // |region_code|. The received subkeys will be returned to the |requester|. If On 2017/07/07 13:10:01, Mathieu wrote: > add a comment about the format of |language| (or an example) Done. https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2966103002/diff/120001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:65: "{\"data/OZ\": " On 2017/07/07 13:10:02, Mathieu wrote: > can you have different languages in your data and test that your code works? This is testing the subkey-requester, I'll add the tests for language to ChromeAddressValidator unit-tests. https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:72: // this should never happen On 2017/07/07 13:10:02, Mathieu wrote: > nit: Please add capital letters to sentences and end punctuation. Done. https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:73: if (rule_iterator == rules.end() || !rule_iterator->second) On 2017/07/07 13:10:02, Mathieu wrote: > Can this happen if the rules on the server are bad? Yes, this can happen if the rules are bad, or not parsed correctly. Do you think the comment is redundant? https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:82: lang_suffix = "--" + language; // exp: --fr On 2017/07/07 13:10:02, Mathieu wrote: > exp -> ex: Done. https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2966103002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.h:101: // This function should be called when the rules are loaded. On 2017/07/07 13:10:02, Mathieu wrote: > mention |language| Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
parastoog@google.com changed reviewers: - dtrainor@chromium.org
parastoog@google.com changed reviewers: + dtrainor@chromium.org
dtrainor@chromium.org: Please review changes in
Great Job! I just have a couple of comments :) https://codereview.chromium.org/2966103002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2966103002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:320: contains(adminAreaCodes, mProfile.getRegion()) Since you now have the names too, could you check both? That would be a nice improvement :) It would probably need to be a case insensitive check. https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:73: "\"lang\":\"en\"}}")); Could you add a second language? That way we can check that we get the name from the right one in the test? https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:73: if (rule_iterator == rules.end() || !rule_iterator->second) Instead of saying "this should never happen" I would add a NOT_REACHED(); in the if before the return. It gives the same message to the reader but actually makes a crash in debug builds :) https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:87: // This should never happen. ditto
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/2966103002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2966103002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:320: contains(adminAreaCodes, mProfile.getRegion()) On 2017/07/07 18:05:04, sebsg wrote: > Since you now have the names too, could you check both? That would be a nice > improvement :) > > It would probably need to be a case insensitive check. Done. https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:73: "\"lang\":\"en\"}}")); On 2017/07/07 18:05:04, sebsg wrote: > Could you add a second language? That way we can check that we get the name from > the right one in the test? It's not the subkey_requester, but the ChromeAddressValidator that process data and language, adding a language test here would be irrelevant, and would make the "unit-tests" too complicated. I have added couple of different tests on language in ChromeAddressValidator unit test. Let me know if you think otherwise. https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:73: if (rule_iterator == rules.end() || !rule_iterator->second) On 2017/07/07 18:05:04, sebsg wrote: > Instead of saying "this should never happen" I would add a NOT_REACHED(); in the > if before the return. > > It gives the same message to the reader but actually makes a crash in debug > builds :) As discussed: this can happen if the rules are bad. In this case, we proceed, and will show a textfield at the end. Therefore, I'll change the comment to avoid further confusion. https://codereview.chromium.org/2966103002/diff/140001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:87: // This should never happen. On 2017/07/07 18:05:04, sebsg wrote: > ditto Done.
LGTM https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2966103002/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:73: "\"lang\":\"en\"}}")); On 2017/07/10 15:40:19, Parastoo wrote: > On 2017/07/07 18:05:04, sebsg wrote: > > Could you add a second language? That way we can check that we get the name > from > > the right one in the test? > > It's not the subkey_requester, but the ChromeAddressValidator that process data > and language, adding a language test here would be irrelevant, and would make > the "unit-tests" too complicated. I have added couple of different tests on > language in ChromeAddressValidator unit test. Let me know if you think > otherwise. SG!
AutofillProfileBridge.java lgtm!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.chromium.org/2966103002/#ps160001 (title: "Small Changes.")
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": 160001, "attempt_start_ts": 1499713592669270,
"parent_rev": "ee095bcb2656c72ca3411b9a2c98cb19e789bdd5", "commit_rev":
"51456a9609172d3f8a803c056c1789a55b52fd60"}
Message was sent while issue was closed.
Description was changed from ========== - In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. - If available, show the names in the device's language, otherwise, show in the default language for that country. Exp: Show Quebec and not QC, and show Québec if the device's language is set to French. - If the name of the admin area for a country is not available, show the admin area code. - Change the unit-tests accordingly. BUG=691643 ========== to ========== - In payment request form, for the admin area drop-down field, show admin areas' complete names rather than codes. - If available, show the names in the device's language, otherwise, show in the default language for that country. Exp: Show Quebec and not QC, and show Québec if the device's language is set to French. - If the name of the admin area for a country is not available, show the admin area code. - Change the unit-tests accordingly. BUG=691643 Review-Url: https://codereview.chromium.org/2966103002 Cr-Commit-Position: refs/heads/master@{#485338} Committed: https://chromium.googlesource.com/chromium/src/+/51456a9609172d3f8a803c056c17... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/51456a9609172d3f8a803c056c17... |
