Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482533002:
[Payments] Display warning messages for incomplete shipping address and update editor title (Closed)
DescriptionDisplay warning messages for incomplete shipping address and update editor title
BUG=663041
Committed: https://crrev.com/281e2fc3d9454e6f11fa0b4c1cf2bff114ab18a9
Cr-Commit-Position: refs/heads/master@{#430987}
Patch Set 1 #
Total comments: 12
Patch Set 2 : address comments and add test #
Total comments: 14
Patch Set 3 : fix android_chrome_strings #Patch Set 4 : refactor function #
Total comments: 2
Messages
Total messages: 59 (43 generated)
Description was changed from ========== format format format shipping address BUG= ========== to ========== Update shipping address UIs BUG= ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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 #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 ========== Update shipping address UIs BUG= ========== to ========== Display warning strings for incomplete shipping address and update editor title BUG=663041 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
gogerald@chromium.org changed reviewers: + rouslan@chromium.org, tedchoc@chromium.org
Hi all, PTAL,
Description was changed from ========== Display warning strings for incomplete shipping address and update editor title BUG=663041 ========== to ========== Display warning messages for incomplete shipping address and update editor title BUG=663041 ==========
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...)
https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:298: mCardEditor.updateBillingAddressIfComplete(addresses.get(i)); Can't re-use the same address for billing and shipping addresses, because billing address need to include name in label. See line 285 here: false /* includenameInLabel */ vs. line 148 in CardEditor.java: true /* includeNameInLabel */ Also note that billing addresses must be local. That's noted in the comments on lines 152-153 of CardEditor.java. That property seems to have been dropped from code, but it's important to keep it. (Billing addresses must be local because credit cards identify their billing addresses using GUIDs, which are unchanging only for local addresses.) https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:72: AutofillAddress mSelectedShippingAddress = (AutofillAddress) selectedShippingAddress; That's a redeclaration of the member variable, so the member variable is never used. You probably will get a NullPointerException in onAddressNormalized(). https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2589: <message name="IDS_PAYMENTS_PHONE_NUMBER_REQUIRED" desc="The label to indicate phone number is required for shipping."> Translators might need more information about the meaning of this label to provide an accurate translation. Try this: "The label to indicate phone number is required in the shipping address. This phone number can be used, for example, if there's a problem with shipping a package to its destination." https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2592: <message name="IDS_PAYMENTS_ADD_PHONE_NUMBER" desc="The title of the dialog for user to add phone number for shipping."> Ditto https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2595: <message name="IDS_PAYMENTS_RECIPIENT_REQUIRED" desc="The label to indicate recipient is required for shipping."> Add some more information about what is "recipient". Even if you just say "name of recipient", that's better. Also say "shipping address" instead of "shipping". It's more clear to translators, who may not have much background information about this UI. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2598: <message name="IDS_PAYMENTS_ADD_RECIPIENT" desc="The title of the dialog for user to add recipient for shipping."> Ditto
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:298: mCardEditor.updateBillingAddressIfComplete(addresses.get(i)); On 2016/11/07 22:41:28, rouslan wrote: > Can't re-use the same address for billing and shipping addresses, because > billing address need to include name in label. See line 285 here: > > false /* includenameInLabel */ > > vs. line 148 in CardEditor.java: > > true /* includeNameInLabel */ > > Also note that billing addresses must be local. That's noted in the comments on > lines 152-153 of CardEditor.java. That property seems to have been dropped from > code, but it's important to keep it. (Billing addresses must be local because > credit cards identify their billing addresses using GUIDs, which are unchanging > only for local addresses.) Done. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:72: AutofillAddress mSelectedShippingAddress = (AutofillAddress) selectedShippingAddress; On 2016/11/07 22:41:28, rouslan wrote: > That's a redeclaration of the member variable, so the member variable is never > used. You probably will get a NullPointerException in onAddressNormalized(). Done. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2589: <message name="IDS_PAYMENTS_PHONE_NUMBER_REQUIRED" desc="The label to indicate phone number is required for shipping."> On 2016/11/07 22:41:28, rouslan wrote: > Translators might need more information about the meaning of this label to > provide an accurate translation. Try this: > > "The label to indicate phone number is required in the shipping address. This > phone number can be used, for example, if there's a problem with shipping a > package to its destination." Done. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2592: <message name="IDS_PAYMENTS_ADD_PHONE_NUMBER" desc="The title of the dialog for user to add phone number for shipping."> On 2016/11/07 22:41:28, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2595: <message name="IDS_PAYMENTS_RECIPIENT_REQUIRED" desc="The label to indicate recipient is required for shipping."> On 2016/11/07 22:41:28, rouslan wrote: > Add some more information about what is "recipient". Even if you just say "name > of recipient", that's better. Also say "shipping address" instead of "shipping". > It's more clear to translators, who may not have much background information > about this UI. Done. https://codereview.chromium.org/2482533002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2598: <message name="IDS_PAYMENTS_ADD_RECIPIENT" desc="The title of the dialog for user to add recipient for shipping."> On 2016/11/07 22:41:28, rouslan wrote: > Ditto Done.
Good job on the tests. LGTM after the translator-related comments are addressed. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2592: <message name="IDS_PAYMENTS_ADD_PHONE_NUMBER" desc="The title of the dialog for user to add phone number to the shipping address. Refer IDS_PAYMENTS_PHONE_NUMBER_REQUIRED for the usage of the phone number"> Translators don't have access to message identifiers. Please copy-paste the description from IDS_PAYMENTS_PHONE_NUMBER_REQUIRED instead. This comment does not affect the size of Chrome binary. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2598: <message name="IDS_PAYMENTS_ADD_RECIPIENT" desc="The title of the dialog for user to add recipient in the shipping address. Refer IDS_PAYMENTS_RECIPIENT_REQUIRED for further explanation of recipient"> Ditto https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2604: <message name="IDS_PAYMENTS_ADD_VALID_ADDRESS" desc="The title of the dialog for user to add valid shipping address. Refer IDS_PAYMENTS_INVALID_ADDRESS for more information"> Ditto
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2592: <message name="IDS_PAYMENTS_ADD_PHONE_NUMBER" desc="The title of the dialog for user to add phone number to the shipping address. Refer IDS_PAYMENTS_PHONE_NUMBER_REQUIRED for the usage of the phone number"> On 2016/11/08 21:12:40, rouslan wrote: > Translators don't have access to message identifiers. Please copy-paste the > description from IDS_PAYMENTS_PHONE_NUMBER_REQUIRED instead. This comment does > not affect the size of Chrome binary. Done. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2598: <message name="IDS_PAYMENTS_ADD_RECIPIENT" desc="The title of the dialog for user to add recipient in the shipping address. Refer IDS_PAYMENTS_RECIPIENT_REQUIRED for further explanation of recipient"> On 2016/11/08 21:12:40, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2604: <message name="IDS_PAYMENTS_ADD_VALID_ADDRESS" desc="The title of the dialog for user to add valid shipping address. Refer IDS_PAYMENTS_INVALID_ADDRESS for more information"> On 2016/11/08 21:12:40, rouslan wrote: > Ditto 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Ted, please help take a look of changes in chrome/android/java/strings/android_chrome_strings.grd Thanks,
https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:83: ? mContext.getString(R.string.autofill_create_profile) : toEdit.getEditTitle()); +4 indent https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:105: public static int[] getInformationRequiredMessageResIdsForEdit(AutofillProfile profile) { don't want to derail this too much, but I find this to be an odd api (especially for one to be exposed). are the message ids only used in this class? If so, I think the public api should be something like boolean isAutofillProfileComplete(AutofillProfile profile) { return getInformationRequiredMessageResIdsForEdit(profile) == null; } Granted, the "Is Complete" might not be right for this context, but it seems like what outside users would actually want. Another option would be to not have this method deal with message ids but instead would return a state for the profile. public static final int PROFILE_COMPLETE = 0; public static final int PROFILE_MISSING_PHONE_NUMBER = 1; ... public static final int PROFILE_MISSING_MULTIPLE_FIELDS = X; then this method would simply return that state. For the external case, it would just check that it returns PROFILE_COMPLETE. Then for message calculation you would look at the other states and populate it that way. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:139: return editMessageResId == 0 ? null : new int[] {editMessageResId, editTitleResId}; not really important here, but I think return a Pair<Integer, Integer> is slightly better than an int[]. It's not as memory efficient, but this should have a relatively low call rate (just wouldn't recommend this for something in the draw path per frame).
Patchset #5 (id:360001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:83: ? mContext.getString(R.string.autofill_create_profile) : toEdit.getEditTitle()); On 2016/11/08 22:14:06, Ted C wrote: > +4 indent Done. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:105: public static int[] getInformationRequiredMessageResIdsForEdit(AutofillProfile profile) { On 2016/11/08 22:14:06, Ted C wrote: > don't want to derail this too much, but I find this to be an odd api (especially > for one to be exposed). > > are the message ids only used in this class? If so, I think the public api > should be something like > > boolean isAutofillProfileComplete(AutofillProfile profile) { > return getInformationRequiredMessageResIdsForEdit(profile) == null; > } > > Granted, the "Is Complete" might not be right for this context, but it seems > like what outside users would actually want. > > Another option would be to not have this method deal with message ids but > instead would return a state for the profile. > > public static final int PROFILE_COMPLETE = 0; > public static final int PROFILE_MISSING_PHONE_NUMBER = 1; > ... > public static final int PROFILE_MISSING_MULTIPLE_FIELDS = X; > > then this method would simply return that state. For the external case, it > would just check that it returns PROFILE_COMPLETE. Then for message calculation > you would look at the other states and populate it that way. Done. Yes, a little bit, was more worried about time efficiency. https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:139: return editMessageResId == 0 ? null : new int[] {editMessageResId, editTitleResId}; On 2016/11/08 22:14:06, Ted C wrote: > not really important here, but I think return a Pair<Integer, Integer> is > slightly better than an int[]. It's not as memory efficient, but this should > have a relatively low call rate (just wouldn't recommend this for something in > the draw path per frame). not applicable now, but why it would be more time efficient?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:139: return editMessageResId == 0 ? null : new int[] {editMessageResId, editTitleResId}; I think Ted meant that Pair is not as memory efficient as int[], not that other way around. Number of calls in your code is important for CPU cache invalidation. There's no Big-O time complexity difference.
https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:139: return editMessageResId == 0 ? null : new int[] {editMessageResId, editTitleResId}; On 2016/11/09 01:42:46, gogerald1 wrote: > On 2016/11/08 22:14:06, Ted C wrote: > > not really important here, but I think return a Pair<Integer, Integer> is > > slightly better than an int[]. It's not as memory efficient, but this should > > have a relatively low call rate (just wouldn't recommend this for something in > > the draw path per frame). > > not applicable now, but why it would be more time efficient? Ah, I think I misunderstand this "low call rate" as "more time efficient" for one call. Agreed that code clarity is preferred here.
On 2016/11/09 17:04:52, rouslan wrote: > https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java > (right): > > https://codereview.chromium.org/2482533002/diff/300001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:139: > return editMessageResId == 0 ? null : new int[] {editMessageResId, > editTitleResId}; > I think Ted meant that Pair is not as memory efficient as int[], not that other > way around. Number of calls in your code is important for CPU cache > invalidation. There's no Big-O time complexity difference. Indeed. Pair requires allocating two capital "I" integers as well as the container object where the int[] doesn't require that. On older android versions, allocating these small objects can add up, but really it is only important for very, very frequent calls. If we actually needed to call this rapidly, we would allocate a temp int[] and pass it in as a param to even avoid allocating that. Not important here though.
lgtm https://codereview.chromium.org/2482533002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:42: INVALID_MULTIPLE_FIELDS}) nice! even better with the IntDef
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2482533002/#ps340001 (title: "refactor function")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Ted for detailed comments. https://codereview.chromium.org/2482533002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2482533002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:42: INVALID_MULTIPLE_FIELDS}) On 2016/11/09 18:25:49, Ted C wrote: > nice! even better with the IntDef Acknowledged.
Message was sent while issue was closed.
Description was changed from ========== Display warning messages for incomplete shipping address and update editor title BUG=663041 ========== to ========== Display warning messages for incomplete shipping address and update editor title BUG=663041 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Display warning messages for incomplete shipping address and update editor title BUG=663041 ========== to ========== Display warning messages for incomplete shipping address and update editor title BUG=663041 Committed: https://crrev.com/281e2fc3d9454e6f11fa0b4c1cf2bff114ab18a9 Cr-Commit-Position: refs/heads/master@{#430987} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/281e2fc3d9454e6f11fa0b4c1cf2bff114ab18a9 Cr-Commit-Position: refs/heads/master@{#430987} |
