Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2281913002:
Currency code exceeding 6 chars will be ellipsized. (Closed)
DescriptionCurrency code exceeding 6 chars will be ellipsized.
BUG=636723, 640847
Committed: https://crrev.com/394b1d9222e816e1fbe03b9c8e51f8cb2fb59e8c
Cr-Commit-Position: refs/heads/master@{#417201}
Patch Set 1 : tests #Patch Set 2 : Fixed tests #Patch Set 3 : Rebased #
Total comments: 6
Patch Set 4 : formatCurrency() #
Total comments: 16
Patch Set 5 : Fixed issues #Patch Set 6 : Added comments #
Total comments: 2
Messages
Total messages: 43 (29 generated)
Description was changed from ========== Accept any string for currency code in PaymentRequest. BUG=636723, 640847 ========== to ========== Currency code exceeding 6 chars will be ellipsized. BUG=636723, 640847 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL.
sanjoy.pal@samsung.com changed reviewers: + rouslan@chromium.org
PTAL.
UI shows two things currently: [Currency code] [Formatted amount] Instead of erasing [Currency code], we need to format it for display. Let's add a method getcurrencyStringForDisplay() to CurrencyStringFormatter.java. This method should ellipsize the long strings. https://codereview.chromium.org/2281913002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:175: /** @return The currency code formatted for display. */ public String getCurrencyForDisplay() { https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:106: private final String mExpectedFormatting; private final String mExpectedCurrencyFormatting; private final String mExpectedAmountFormatting; https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:118: String expectedFormatting, ExpectedValidity expectedValidity) { public CurrencyStringFormatterTest(String amount, String currency, String languageTag, String expectedCurrencyFormatting, String expectedAmountFormatting, ExpectedValidity expectedValidity) {
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:180001) has been deleted
Made changes as suggested. Please take a look. https://codereview.chromium.org/2281913002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:175: On 2016/09/01 14:55:55, rouslan wrote: > /** @return The currency code formatted for display. */ > public String getCurrencyForDisplay() { Done. https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:106: private final String mExpectedFormatting; On 2016/09/01 14:55:55, rouslan wrote: > private final String mExpectedCurrencyFormatting; > private final String mExpectedAmountFormatting; Done. https://codereview.chromium.org/2281913002/diff/160001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:118: String expectedFormatting, ExpectedValidity expectedValidity) { On 2016/09/01 14:55:55, rouslan wrote: > public CurrencyStringFormatterTest(String amount, String currency, > String languageTag, String expectedCurrencyFormatting, > String expectedAmountFormatting, ExpectedValidity expectedValidity) { Done.
You're getting there :-) https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:37: This object is created based on a single currencyCode. I did not save it before, because there was no need to for it. Now that we're formatting the currencyCode, let's save the formatted version of it as a member variable. /** * The currency formatted for display. Currency can be any string of at most * 2048 characters.Currency code more than 6 character is formatted to first * 5 characters and ellipsis. */ public final String mFormattedCurrencyCode; https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:74: mFormattedCurrencyCode = currencyCode.length() <= MAX_CURRENCY_CHARS ? currencyCode : currencyCode.substring(0, MAX_CURRENCY_CHARS - 1) + ELLIPSIS. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:137: public String formatCurrency(String currency) { /** @return The currency code formatted for display. */ public String getFormattedCurrencyCode() { return mFormattedCurrencyCode; } https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:493: details.total.label, formatter.formatCurrency(totalCurrency), formatter.getFormattedCurrencyCode(), https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:104: assertEquals("ABCDE" + ELLIPSIS, formatter.formatCurrency("ABCDEFG")); Use use the ELLIPSIS only once here., so it's fewer lines to type "ABCDE\u2026" and add a comment above about what it is. // \u2026 is an ellipsis. assertEquals("ABCDE\u2026", formatter.formatCurrency("ABCDEFG")); https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:33: VALID_CURRENCY, This enum covers the three states of validation that we want to test. 1) Everything is valid, including both "amount currency code" and "amount value". 2) "Amount currency code" is invalid. 3) "Amount value" is invalid. Let's not add VALID_CURRENCY, because it's already covered by VALID_AMOUNT. Sorry about the confusion. The words "amount" and "value" are usually interchangeable, but PaymentRequest distinguishes them. { label: "Total", amount: { currency: "USD", value: "5.00" } } "Amount" means both "currency" and "value", so it's not the same as "value" alone. We could not come up with better names for these concepts. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:86: {"55.00", longStringOfLength(2048), "en-US", "AAAAA" + ELLIPSIS, "55.00", ELLIPSIS is used only once, so it's easier to just write "AAAAA\u2026" instead. It's a good idea to add a comment about \u2026 on the line above. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:157: mExpectedCurrencyFormatting, formatter.formatCurrency(mCurrency)); Move lines 155-157 between lines 164 and 65 below.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed all the comments. PTAL. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:37: On 2016/09/02 17:30:55, rouslan wrote: > This object is created based on a single currencyCode. I did not save it before, > because there was no need to for it. Now that we're formatting the currencyCode, > let's save the formatted version of it as a member variable. > > /** > * The currency formatted for display. Currency can be any string of at most > * 2048 characters.Currency code more than 6 character is formatted to first > * 5 characters and ellipsis. > */ > public final String mFormattedCurrencyCode; Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:74: On 2016/09/02 17:30:55, rouslan wrote: > mFormattedCurrencyCode = currencyCode.length() <= MAX_CURRENCY_CHARS > ? currencyCode > : currencyCode.substring(0, MAX_CURRENCY_CHARS - 1) + ELLIPSIS. Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:137: public String formatCurrency(String currency) { On 2016/09/02 17:30:55, rouslan wrote: > /** @return The currency code formatted for display. */ > public String getFormattedCurrencyCode() { > return mFormattedCurrencyCode; > } Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:493: details.total.label, formatter.formatCurrency(totalCurrency), On 2016/09/02 17:30:55, rouslan wrote: > formatter.getFormattedCurrencyCode(), Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:104: assertEquals("ABCDE" + ELLIPSIS, formatter.formatCurrency("ABCDEFG")); On 2016/09/02 17:30:55, rouslan wrote: > Use use the ELLIPSIS only once here., so it's fewer lines to type "ABCDE\u2026" > and add a comment above about what it is. > > // \u2026 is an ellipsis. > assertEquals("ABCDE\u2026", formatter.formatCurrency("ABCDEFG")); Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java (right): https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:33: VALID_CURRENCY, On 2016/09/02 17:30:55, rouslan wrote: > This enum covers the three states of validation that we want to test. > > 1) Everything is valid, including both "amount currency code" and "amount > value". > > 2) "Amount currency code" is invalid. > > 3) "Amount value" is invalid. > > Let's not add VALID_CURRENCY, because it's already covered by VALID_AMOUNT. > > Sorry about the confusion. The words "amount" and "value" are usually > interchangeable, but PaymentRequest distinguishes them. > > { > label: "Total", > amount: { > currency: "USD", > value: "5.00" > } > } > > "Amount" means both "currency" and "value", so it's not the same as "value" > alone. We could not come up with better names for these concepts. Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:86: {"55.00", longStringOfLength(2048), "en-US", "AAAAA" + ELLIPSIS, "55.00", On 2016/09/02 17:30:55, rouslan wrote: > ELLIPSIS is used only once, so it's easier to just write "AAAAA\u2026" instead. > It's a good idea to add a comment about \u2026 on the line above. Done. https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:157: mExpectedCurrencyFormatting, formatter.formatCurrency(mCurrency)); On 2016/09/02 17:30:55, rouslan wrote: > Move lines 155-157 between lines 164 and 65 below. Done.
sanjoy.pal@samsung.com changed reviewers: + dfalcantara@chromium.org
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/06 06:09:00, pals wrote: > Fixed all the comments. PTAL. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java > (right): > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:37: > > On 2016/09/02 17:30:55, rouslan wrote: > > This object is created based on a single currencyCode. I did not save it > before, > > because there was no need to for it. Now that we're formatting the > currencyCode, > > let's save the formatted version of it as a member variable. > > > > /** > > * The currency formatted for display. Currency can be any string of at most > > * 2048 characters.Currency code more than 6 character is formatted to first > > * 5 characters and ellipsis. > > */ > > public final String mFormattedCurrencyCode; > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:74: > > On 2016/09/02 17:30:55, rouslan wrote: > > mFormattedCurrencyCode = currencyCode.length() <= MAX_CURRENCY_CHARS > > ? currencyCode > > : currencyCode.substring(0, MAX_CURRENCY_CHARS - 1) + ELLIPSIS. > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:137: > public String formatCurrency(String currency) { > On 2016/09/02 17:30:55, rouslan wrote: > > /** @return The currency code formatted for display. */ > > public String getFormattedCurrencyCode() { > > return mFormattedCurrencyCode; > > } > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java > (right): > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:493: > details.total.label, formatter.formatCurrency(totalCurrency), > On 2016/09/02 17:30:55, rouslan wrote: > > formatter.getFormattedCurrencyCode(), > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... > File > chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java > (right): > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/javates... > chrome/android/javatests/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:104: > assertEquals("ABCDE" + ELLIPSIS, formatter.formatCurrency("ABCDEFG")); > On 2016/09/02 17:30:55, rouslan wrote: > > Use use the ELLIPSIS only once here., so it's fewer lines to type > "ABCDE\u2026" > > and add a comment above about what it is. > > > > // \u2026 is an ellipsis. > > assertEquals("ABCDE\u2026", formatter.formatCurrency("ABCDEFG")); > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... > File > chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java > (right): > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... > chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:33: > VALID_CURRENCY, > On 2016/09/02 17:30:55, rouslan wrote: > > This enum covers the three states of validation that we want to test. > > > > 1) Everything is valid, including both "amount currency code" and "amount > > value". > > > > 2) "Amount currency code" is invalid. > > > > 3) "Amount value" is invalid. > > > > Let's not add VALID_CURRENCY, because it's already covered by VALID_AMOUNT. > > > > Sorry about the confusion. The words "amount" and "value" are usually > > interchangeable, but PaymentRequest distinguishes them. > > > > { > > label: "Total", > > amount: { > > currency: "USD", > > value: "5.00" > > } > > } > > > > "Amount" means both "currency" and "value", so it's not the same as "value" > > alone. We could not come up with better names for these concepts. > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... > chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:86: > {"55.00", longStringOfLength(2048), "en-US", "AAAAA" + ELLIPSIS, "55.00", > On 2016/09/02 17:30:55, rouslan wrote: > > ELLIPSIS is used only once, so it's easier to just write "AAAAA\u2026" > instead. > > It's a good idea to add a comment about \u2026 on the line above. > > Done. > > https://codereview.chromium.org/2281913002/diff/200001/chrome/android/junit/s... > chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java:157: > mExpectedCurrencyFormatting, formatter.formatCurrency(mCurrency)); > On 2016/09/02 17:30:55, rouslan wrote: > > Move lines 155-157 between lines 164 and 65 below. > > Done. +dfalcantara@ for CurrencyStringFormatterTest.java. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2281913002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:84: : currencyCode.substring(0, MAX_CURRENCY_CHARS - 1) + ELLIPSIS; Given that you're just tacking on an ellipsis, will it show up in the correct place in RTL or BIDI languages? Is that something that you need to be concerned about?
oh, lgtm % question
I guess RTL rendering is taken care by the view. I have attached a screenshot here https://bugs.chromium.org/p/chromium/issues/detail?id=636723 https://codereview.chromium.org/2281913002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java (right): https://codereview.chromium.org/2281913002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/CurrencyStringFormatter.java:84: : currencyCode.substring(0, MAX_CURRENCY_CHARS - 1) + ELLIPSIS; On 2016/09/06 17:20:32, dfalcantara wrote: > Given that you're just tacking on an ellipsis, will it show up in the correct > place in RTL or BIDI languages? Is that something that you need to be concerned > about? I guess RTL rendering is taken care by view. I have attached a screenshot here https://bugs.chromium.org/p/chromium/issues/detail?id=636723
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Currency code exceeding 6 chars will be ellipsized. BUG=636723, 640847 ========== to ========== Currency code exceeding 6 chars will be ellipsized. BUG=636723, 640847 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Currency code exceeding 6 chars will be ellipsized. BUG=636723, 640847 ========== to ========== Currency code exceeding 6 chars will be ellipsized. BUG=636723, 640847 Committed: https://crrev.com/394b1d9222e816e1fbe03b9c8e51f8cb2fb59e8c Cr-Commit-Position: refs/heads/master@{#417201} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/394b1d9222e816e1fbe03b9c8e51f8cb2fb59e8c Cr-Commit-Position: refs/heads/master@{#417201} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
