Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775403002:
[Payments] Preview available payment options when no option is selected (Closed)
|
Created:
3 years, 9 months ago by gogerald1 Modified:
3 years, 8 months ago CC:
chromium-reviews, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, srahim+watch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreview available payment options when no option is selected
The behavior when there is a selected option in a payment section is the same as before.
When there is no selected option in a payment section:
a) Display a summary of the options in the section.
b) The summary is displayed in a single line in grey color with font 14sp.
c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more".
N+1 is the total number of options in the section. "* and N more" should never be elided.
<Summary of the first option> can be shorten to make the entire summary fit the width of the view.
Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA".
d) If there is only one option in the section, the string is "<Summary of the first option>".
Elides at the end of the string if it does not fit the width of the view.
e) If there is no option in the section, do not show anything.
BUG=703806
Review-Url: https://codereview.chromium.org/2775403002
Cr-Commit-Position: refs/heads/master@{#463488}
Committed: https://chromium.googlesource.com/chromium/src/+/67cd4a8011cebc5e1f860aaa05dc772c784e9e3a
Patch Set 1 #
Total comments: 33
Patch Set 2 : fix compile error and comments #
Total comments: 6
Patch Set 3 : address comments #Patch Set 4 : use plurals and update strings description #
Total comments: 8
Patch Set 5 : update string descriptions #
Total comments: 6
Patch Set 6 : update comments #Messages
Total messages: 63 (44 generated)
Description was changed from ========== Preview payment options when no option is selected BUG= ========== to ========== Preview payment options when no option is selected BUG= ==========
Patchset #1 (id:1) 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...
Description was changed from ========== Preview payment options when no option is selected BUG= ========== to ========== Preview payment options when no option is selected BUG=703806 ==========
Description was changed from ========== Preview payment options when no option is selected BUG=703806 ========== to ========== Preview available payment options when no option is selected BUG=703806 ==========
gogerald@chromium.org changed reviewers: + dfalcantara@chromium.org, rouslan@chromium.org
Hi rouslan@ and dfalcantara@, ptal,
Thank you for working on this! It's quite complex. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1576: if (option.isPaymentCard()) { Instead of switching on option.isPaymentCard(), define a getSummary(Context,int,..) method in PaymentOption.java or PaymentInstrument.java with the default summary that you have on lines 1587-1591. Then override getSummary() in AutofillPaymentInstrument.java using the code that you have on lines 1577-1584 from this file. Then please remove isPaymentCard(). https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1598: if (option.isPaymentCard()) return previewString; Define default getPreviewString(Context) in PaymentOption.java or PaymentInstrument.java. Override it with custom implementation in AutofillPaymentInstrument.java. Then please remove isPaymentCard(). https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1602: if (previewString.length() > 0) previewString += labelSeparator; Does this work in RTL? https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:187: int resId = 0; No need for a temp variable. It's simpler to return from within each of the cases. switch (mDataType) { case PaymentRequestUI.TYPE_SHIPPING_ADDRESSES: return R.string.payment_request_shipping_addresses_preview; case ... https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:201: } default: assert false; return 0; https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. Either upload with --similarity=10 (which tries hard to detect the origin of this file) or set the year to 2017. https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... File components/payments_strings.grdp (right): https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... components/payments_strings.grdp:189: <ph name="PAYMENT_METHOD_PREVIEW">%1$s<ex>VISA ....1234</ex></ph>... and <ph name="NUMBER_OF_AVAILABLE_PAYMENT_METHODS">%2$s<ex>1</ex></ph> more Please use the unicode ellipsis instead of three periods. http://www.fileformat.info/info/unicode/char/2026/index.htm
https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... File components/payments_strings.grdp (right): https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... components/payments_strings.grdp:188: <message name="IDS_PAYMENT_REQUEST_PAYMENT_METHODS_PREVIEW" desc="The summary of the available payment methods in the payment method section on the Payment Request dialog." formatter_data="android_java"> Let's put these messages inside of <if expr="is_android"> for now. When/if desktop UI uses these, someone from the desktop UI team can remove <if>. For now let's ship the resources only to the platforms that will use it.
Can you fill out the description to something more useful for reviewing? It's a big change and one sentence isn't really telling me much.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #2 (id:40001) 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...
Description was changed from ========== Preview available payment options when no option is selected BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: Display a summary of the available options in the section. The summary is displayed in a single line in grey color with font 14sp. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: Display a summary of the available options in the section. The summary is displayed in a single line in grey color with font 14sp. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: Display a summary of the available options in the section. The summary is displayed in a single line in grey color with font 14sp. If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: Display a summary of the available options in the section. The summary is displayed in a single line in grey color with font 14sp. If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the available options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it can not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. Do not elide card summary in the middle of card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. Do not elide card summary in the middle of card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. <Summary of the first option> can be shorten to make the entire summary fit the width of the view. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ==========
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. <Summary of the first option> can be shorten to make the entire summary fit the width of the view. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show any thing. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. <Summary of the first option> can be shorten to make the entire summary fit the width of the view. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show anything. BUG=703806 ==========
please take another look, add more CL description, you can also check the screenshots in the bug, https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1576: if (option.isPaymentCard()) { On 2017/03/27 20:28:31, ಠ_ಠwrote: > Instead of switching on option.isPaymentCard(), define a > getSummary(Context,int,..) method in PaymentOption.java or > PaymentInstrument.java with the default summary that you have on lines > 1587-1591. Then override getSummary() in AutofillPaymentInstrument.java using > the code that you have on lines 1577-1584 from this file. Then please remove > isPaymentCard(). Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1598: if (option.isPaymentCard()) return previewString; On 2017/03/27 20:28:30, ಠ_ಠwrote: > Define default getPreviewString(Context) in PaymentOption.java or > PaymentInstrument.java. Override it with custom implementation in > AutofillPaymentInstrument.java. Then please remove isPaymentCard(). Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1602: if (previewString.length() > 0) previewString += labelSeparator; On 2017/03/27 20:28:30, ಠ_ಠwrote: > Does this work in RTL? Yes, words order in a string doesn't change in RTL, https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:187: int resId = 0; On 2017/03/27 20:28:31, ಠ_ಠwrote: > No need for a temp variable. It's simpler to return from within each of the > cases. > > > switch (mDataType) { > case PaymentRequestUI.TYPE_SHIPPING_ADDRESSES: > return R.string.payment_request_shipping_addresses_preview; > case ... Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:201: } On 2017/03/27 20:28:31, ಠ_ಠwrote: > default: > assert false; > return 0; Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/27 20:28:31, ಠ_ಠwrote: > Either upload with --similarity=10 (which tries hard to detect the origin of > this file) or set the year to 2017. Done. https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... File components/payments_strings.grdp (right): https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... components/payments_strings.grdp:188: <message name="IDS_PAYMENT_REQUEST_PAYMENT_METHODS_PREVIEW" desc="The summary of the available payment methods in the payment method section on the Payment Request dialog." formatter_data="android_java"> On 2017/03/27 20:34:03, ಠ_ಠwrote: > Let's put these messages inside of <if expr="is_android"> for now. When/if > desktop UI uses these, someone from the desktop UI team can remove <if>. For now > let's ship the resources only to the platforms that will use it. Done. https://codereview.chromium.org/2775403002/diff/20001/components/payments_str... components/payments_strings.grdp:189: <ph name="PAYMENT_METHOD_PREVIEW">%1$s<ex>VISA ....1234</ex></ph>... and <ph name="NUMBER_OF_AVAILABLE_PAYMENT_METHODS">%2$s<ex>1</ex></ph> more On 2017/03/27 20:28:31, ಠ_ಠwrote: > Please use the unicode ellipsis instead of three periods. > > http://www.fileformat.info/info/unicode/char/2026/index.htm Done.
https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1341: null /* rightTruncate */, false /* rightIsSingleLine */); this is set repeatedly in the same way, but in different places; should you just have some sort of helper function that does this and explains in better detail what you're doing? https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1514: private static class SectionUiUtils { Should pull this out into its own file. It's standalone and the file is already 1500 lines long. That'd let you test the eliding algorithm directly, too. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1534: // manually ellide the string in the middle if necessary. elide https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1540: view.removeOnLayoutChangeListener(this); what happens if the screen is rotated or the screen is split in half (android N+)? should you keep listening in that case? https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1558: private static String getSectionSummaryForPreviewInASingleLine(Context context, Explain your shrinking algorithm here. It's a mysterious block of code otherwise. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1568: // Append string to indicate how many more options in the given section. Did you talk with srahim@ like I asked? You can't always just concatenate two strings and expect it to be grammatically correct. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:184: * @return The resource Id. use "ID"; we only use "Id" in the Java function names because of weirdo camelcasing conventions. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java:23: */ @Nullable here and everywhere appropriate below
Screenshots are great (thanks!) but you can't expect going through git logs to dig up the bug number and then look at screenshots to try and understand a 500 line change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % comments https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:339: String obfuscatedCardNumber = getLabel(); Can we use StringBuilder? https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:343: obfuscatedCardNumber.substring(0, obfuscatedCardNumber.indexOf(" ")); .substring() will throw an IndexOutOfBounds exception if the |obfuscacatedCardNumber| does not contain a space for whatever reason. (This can happen if, for example, anyone changes from the plain old space to a non-breaking space.) Therefore, please check the return value of indexOf() for -1 and handle that case. https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:194: String previewString = mLabels[0]; Let's use a StringBuilder to avoid concatenating String objects. Every String concatenation creates a new String object, which is expensive on Android.
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...
Thanks, addressed all comments, ptal, https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1341: null /* rightTruncate */, false /* rightIsSingleLine */); On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > this is set repeatedly in the same way, but in different places; should you just > have some sort of helper function that does this and explains in better detail > what you're doing? Added more explains, not sure what helper function would make it better https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1514: private static class SectionUiUtils { On 2017/03/28 00:20:39, dfalcantara (load balance plz) wrote: > Should pull this out into its own file. It's standalone and the file is already > 1500 lines long. That'd let you test the eliding algorithm directly, too. Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1534: // manually ellide the string in the middle if necessary. On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > elide Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1540: view.removeOnLayoutChangeListener(this); On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > what happens if the screen is rotated or the screen is split in half (android > N+)? should you keep listening in that case? Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1558: private static String getSectionSummaryForPreviewInASingleLine(Context context, On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > Explain your shrinking algorithm here. It's a mysterious block of code > otherwise. Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1568: // Append string to indicate how many more options in the given section. On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > Did you talk with srahim@ like I asked? You can't always just concatenate two > strings and expect it to be grammatically correct. I've talked to bbergher@ before who is in the same team as srahim@. Didn't see any problem. Mentioned your concern again in the team meeting. bbergher@ will double confirm it. I've put a resource string in payments_strings.grdp for each payment section, should be much easier for translation, https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:184: * @return The resource Id. On 2017/03/28 00:20:39, dfalcantara (load balance plz) wrote: > use "ID"; we only use "Id" in the Java function names because of weirdo > camelcasing conventions. Done. https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ShippingSummaryInformation.java:23: */ On 2017/03/28 00:20:39, dfalcantara (load balance plz) wrote: > @Nullable here and everywhere appropriate below Done. https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:339: String obfuscatedCardNumber = getLabel(); On 2017/03/28 14:42:49, ಠ_ಠwrote: > Can we use StringBuilder? Done. https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:343: obfuscatedCardNumber.substring(0, obfuscatedCardNumber.indexOf(" ")); On 2017/03/28 14:42:49, ಠ_ಠwrote: > .substring() will throw an IndexOutOfBounds exception if the > |obfuscacatedCardNumber| does not contain a space for whatever reason. (This can > happen if, for example, anyone changes from the plain old space to a > non-breaking space.) Therefore, please check the return value of indexOf() for > -1 and handle that case. Done. https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2775403002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:194: String previewString = mLabels[0]; On 2017/03/28 14:42:49, ಠ_ಠwrote: > Let's use a StringBuilder to avoid concatenating String objects. Every String > concatenation creates a new String object, which is expensive on Android. Done.
https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2775403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1568: // Append string to indicate how many more options in the given section. On 2017/03/28 18:46:24, gogerald1 wrote: > On 2017/03/28 00:20:38, dfalcantara (load balance plz) wrote: > > Did you talk with srahim@ like I asked? You can't always just concatenate two > > strings and expect it to be grammatically correct. > > I've talked to bbergher@ before who is in the same team as srahim@. Didn't see > any problem. > Mentioned your concern again in the team meeting. bbergher@ will double confirm > it. > I've put a resource string in payments_strings.grdp for each payment section, > should be much easier for translation, srahim@ was specifically hired for strings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) 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...
Patchset #4 (id:140001) has been deleted
gogerald@chromium.org changed reviewers: + srahim@chromium.org
Hi srahim@, please review strings in chrome/android/java/strings/android_chrome_strings.grd as discussed offline. You can find the screenshots on the bug.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping srahim@, could you review strings in android_chrome_strings.grd. M59 branch point (Thursday) is very close, we would like to land this change before that date.
Recommendation to update a message desc for additional clarity for translators. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:201: Choose Is this the string for the CHOOSE button? If so, please update message desc: "Label on a button. Tapping the button allows the user to choose an option for a payment method, shipping address, or contact from Contacts on their device."
Some additional nits in the message descriptions. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:348: <message name="IDS_PAYMENT_REQUEST_PAYMENT_METHODS_PREVIEW" desc="This is a snippet of a payment method a user has saved to Chrome, plus an indication of the number of additional payment methods the user has saved. Its function is to show the user has payment methods can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> Minor nit: missing the word "that" "Its function is to show the user has payment methods that can be used to complete a payment, and thus doesn't have to type the entire payment method." https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:353: <message name="IDS_PAYMENT_REQUEST_SHIPPING_ADDRESSES_PREVIEW" desc="This is a snippet of a shipping address a user has saved to Chrome, plus an indication of the number of additional shipping addresses the user has saved. Its function is to show the user has shipping addresses can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> Update message desc: This is a snippet of a shipping address a user has saved to Chrome, plus an indication of the number of additional shipping addresses the user has saved. Its function is to show the user has shipping addresses that can be used to complete a purchase, and thus doesn't have to type the entire address. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:363: <message name="IDS_PAYMENT_REQUEST_CONTACTS_PREVIEW" desc="This is a snippet of a contact a user has saved to Chrome, plus an indication of the number of additional contacts the user has saved. Its function is to show the user has contacts can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> Update message desc: This is a snippet of a contact a user has saved to Chrome, plus an indication of the number of additional contacts the user has saved. Its function is to show the user has contacts that can be used to complete a purchase, and thus doesn't have to type the entire contact name.
Thanks srahim@. Hi dfalcantara@, I think we are unlocked from the format of the new strings after talking to translators and our writing expert, please help continue review this CL. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:201: Choose On 2017/04/10 15:53:35, srahim wrote: > Is this the string for the CHOOSE button? If so, please update message desc: > "Label on a button. Tapping the button allows the user to choose an option for a > payment method, shipping address, or contact from Contacts on their device." Yes, but we may could make this button a little more generic as the other similar buttons above https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:348: <message name="IDS_PAYMENT_REQUEST_PAYMENT_METHODS_PREVIEW" desc="This is a snippet of a payment method a user has saved to Chrome, plus an indication of the number of additional payment methods the user has saved. Its function is to show the user has payment methods can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> On 2017/04/10 15:58:51, srahim wrote: > Minor nit: missing the word "that" > "Its function is to show the user has payment methods that can be used to > complete a payment, and thus doesn't have to type the entire payment method." Done. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:353: <message name="IDS_PAYMENT_REQUEST_SHIPPING_ADDRESSES_PREVIEW" desc="This is a snippet of a shipping address a user has saved to Chrome, plus an indication of the number of additional shipping addresses the user has saved. Its function is to show the user has shipping addresses can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> On 2017/04/10 15:58:51, srahim wrote: > Update message desc: > This is a snippet of a shipping address a user has saved to Chrome, plus an > indication of the number of additional shipping addresses the user has saved. > Its function is to show the user has shipping addresses that can be used to > complete a purchase, and thus doesn't have to type the entire address. Done. https://codereview.chromium.org/2775403002/diff/160001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:363: <message name="IDS_PAYMENT_REQUEST_CONTACTS_PREVIEW" desc="This is a snippet of a contact a user has saved to Chrome, plus an indication of the number of additional contacts the user has saved. Its function is to show the user has contacts can be used to complete payment request, and thus doesn't have to type it all from beginning. [ICU Syntax]"> On 2017/04/10 15:58:51, srahim wrote: > Update message desc: > This is a snippet of a contact a user has saved to Chrome, plus an indication of > the number of additional contacts the user has saved. Its function is to show > the user has contacts that can be used to complete a purchase, and thus doesn't > have to type the entire contact name. Done. Contact not only include name, might also include email and phone number, so I changed it to "*type the entire contact info"
lgtm https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java (right): https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:75: // If there are more than one options in the section, then the returned string pattern is Move lines 75-77 to line 85. Closer to what it actually describes. https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:77: // section. If layout or paint is null, return the summary string without shrink. Otherwise, nit: without shrinking. or return the full summary string. https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:96: while (Layout.getDesiredWidth(summary, paint) > ellipsizedWidth) { Explain why this while loop terminates. There's nothing obvious here without digging into the calls it makes.
Thanks, sending to CQ https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java (right): https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:75: // If there are more than one options in the section, then the returned string pattern is On 2017/04/10 23:19:03, dfalcantara (load balance plz) wrote: > Move lines 75-77 to line 85. Closer to what it actually describes. Done. https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:77: // section. If layout or paint is null, return the summary string without shrink. Otherwise, On 2017/04/10 23:19:03, dfalcantara (load balance plz) wrote: > nit: > without shrinking. > or > return the full summary string. Done. https://codereview.chromium.org/2775403002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionUiUtils.java:96: while (Layout.getDesiredWidth(summary, paint) > ellipsizedWidth) { On 2017/04/10 23:19:03, dfalcantara (load balance plz) wrote: > Explain why this while loop terminates. There's nothing obvious here without > digging into the calls it makes. Done.
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, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2775403002/#ps200001 (title: "update comments")
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": 200001, "attempt_start_ts": 1491870041492380,
"parent_rev": "19edf6e0b7f80c82a2691a041fdbd6cce13564ae", "commit_rev":
"67cd4a8011cebc5e1f860aaa05dc772c784e9e3a"}
Message was sent while issue was closed.
Description was changed from ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. <Summary of the first option> can be shorten to make the entire summary fit the width of the view. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show anything. BUG=703806 ========== to ========== Preview available payment options when no option is selected The behavior when there is a selected option in a payment section is the same as before. When there is no selected option in a payment section: a) Display a summary of the options in the section. b) The summary is displayed in a single line in grey color with font 14sp. c) If there are more than one options in the section, the string pattern is "<Summary of the first option>... and N more". N+1 is the total number of options in the section. "* and N more" should never be elided. <Summary of the first option> can be shorten to make the entire summary fit the width of the view. Do not elide card summary in the middle of the card number, e.g either display "VISA ....1234" or "VISA". d) If there is only one option in the section, the string is "<Summary of the first option>". Elides at the end of the string if it does not fit the width of the view. e) If there is no option in the section, do not show anything. BUG=703806 Review-Url: https://codereview.chromium.org/2775403002 Cr-Commit-Position: refs/heads/master@{#463488} Committed: https://chromium.googlesource.com/chromium/src/+/67cd4a8011cebc5e1f860aaa05dc... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/67cd4a8011cebc5e1f860aaa05dc... |
