Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621033003:
[Payments] Currency formatter for order amounts. (Closed)
Description[Payments] Currency formatter for order amounts.
Uses ICU. This change also enables using it on the Desktop implementation. A followup will enable it on Android.
BUG=679797
TEST=CurrencyFormatterTest, LANGUAGE=fr_CA ./out/Default/chrome
Review-Url: https://codereview.chromium.org/2621033003
Cr-Commit-Position: refs/heads/master@{#443493}
Committed: https://chromium.googlesource.com/chromium/src/+/6758be03e71c7423fb1b43dc267049ebc4ceaf27
Patch Set 1 : Initial #
Total comments: 30
Patch Set 2 : addressed rouslan's comments; caching CurrencyFormatter #Patch Set 3 : Added tests #Patch Set 4 : fix dependency #Patch Set 5 : currency formatter takes locale_name as string #
Total comments: 14
Patch Set 6 : Addressed comments #
Total comments: 2
Patch Set 7 : deps -> public_deps #Patch Set 8 : checked_cast #Messages
Total messages: 37 (20 generated)
Description was changed from ========== [Payments] Currency formatter. Uses ICU BUG=679797 ========== to ========== [Payments] Currency formatter for order amounts. Uses ICU. This change also enables using it on the Desktop implementation. A followup will enable it on Android. BUG=679797 TEST=CurrencyFormatterTest, LANGUAGE=fr_CA ./out/Default/chrome ==========
mathp@chromium.org changed reviewers: + rouslan@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Hi Rouslan, please take a look!
Great work! https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:44: const std::string formatted_amount = FormatAmountForCurrency( nit1: no need for const, because a quick perusal of this function shows that the formatted amount is never changed. nit2: If you return base::stirng16 from FormatAmountForCurrency(), then it's more clear that the output is intended to be displayed to the user. nit3: ICU operations can be expensive. It's best to calculate this value once in the model and share it among the views. nit4: If |total->amount->currencySystem| is present and is not set to "urn:iso:std:iso:4217", then the formatter should be aware that |total->amount->currency| is not ISO code that can be interpreted by ICU. For example, the formatter should use "" instead of "$" for "USD", if the currencySystem is not ISO. https://w3c.github.io/browser-payment-api/#paymentcurrencyamount-dictionary https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter.cc (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:32: double amount_double = 0.0; Do you think it may be better to not parse the "amount" into a double, because the "amount" can have a huge number of digits, up to 30 in some cases? I wonder if an overflow can occur. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:40: icu::NumberFormat::createCurrencyInstance(locale, ignored)); Why ignore the error? https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:55: LOG(ERROR) << "Could not set currency on amount formatter: " Will print an error for non-ISO currency codes? An example of a non-ISO currency code is "BITCOIN". These are allowed by the spec, so it's not technically an error. Perhaps you could check whether currency_code matches ^[A-Z]{3}$ regex beforehand? src/third_party/re2 should be able to do that efficiently. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:63: if (!output.isEmpty()) { If the output is empty, you should probably return |amount| as well, right? https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:64: // Explicitely removes the currency code (truncated to its 3-letter and Explicitly https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:77: // Trims any unicode whitespace (including non-breaking space). base::TrimeWhitespace? https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter.h (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.h:22: const std::string FormatAmountForCurrency(const std::string& amount, The "const" keyword here does not seem to server any purpose? https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter_unittest.cc (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:14: struct CurrencyFormatterTestCase { nit: A shorter name like "TestCase" will make the rest of the file easier to read without detriment. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:25: const char* amount; const char* const amount; (First "const" says "don't change where this pointer goes" and second "const" says "don't change the data at this pointer," or vice versa.) https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:28: std::string expected_amount; const std::string expected_amount; https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:63: CurrencyFormatterTestCase("1234", "USD", "en_US", "$1,234.00"), Also: CurrencyFormatterTestCase("0.1234", "USD", "en_US", "$0.1234"), Trading for fractions of a penny is more common than you think. For example, US gas stations sell gas with prices like 3.999/gallon. Fractions of penny can also be used in micro-transactions or by equity and commodities traders. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:73: CurrencyFormatterTestCase("55.00", "CAD", "fr_FR", "55,00 $"), Also: CurrencyFormatterTestCase("1234", "USD", "fr_FR", "1 234,00 $"), https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:85: CurrencyFormatterTestCase("-55.00", "BTX", "en_US", "-55.00"), Also: CurrencyFormatterTestCase("-55.00", "BTX", "fr_FR", "-55,00"), https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:92: // happen. Can we avoid this similar to how it's done in CurrencyStringFormatter.java or at least gracefully handle overflow and imprecise parsings? Examples of what can be done: * Don't parse the string into a double. Manually replace "." with "," depending on the locale. Manually pad "0" after the decimal separator. Manually insert " " or "," for digit group separation. * Print the double back into a string in "C" locale and compare to the input string. If these don't match, don't use the double.
Thanks for your knowledge on this :) PTAL https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:44: const std::string formatted_amount = FormatAmountForCurrency( On 2017/01/11 18:29:30, rouslan wrote: > nit1: no need for const, because a quick perusal of this function shows that the > formatted amount is never changed. > > nit2: If you return base::stirng16 from FormatAmountForCurrency(), then it's > more clear that the output is intended to be displayed to the user. > > nit3: ICU operations can be expensive. It's best to calculate this value once in > the model and share it among the views. > > nit4: If |total->amount->currencySystem| is present and is not set to > "urn:iso:std:iso:4217", then the formatter should be aware that > |total->amount->currency| is not ISO code that can be interpreted by ICU. For > example, the formatter should use "" instead of "$" for "USD", if the > currencySystem is not ISO. > > https://w3c.github.io/browser-payment-api/#paymentcurrencyamount-dictionary Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter.cc (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:32: double amount_double = 0.0; On 2017/01/11 18:29:30, rouslan wrote: > Do you think it may be better to not parse the "amount" into a double, because > the "amount" can have a huge number of digits, up to 30 in some cases? I wonder > if an overflow can occur. I would think base::StringToDouble does the right thing. For example there is one test case that I copied from Java that has a lot of digits. It's parsed up to the precision that double supports. We have to have a double because that's what ICU needs... https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:40: icu::NumberFormat::createCurrencyInstance(locale, ignored)); On 2017/01/11 18:29:30, rouslan wrote: > Why ignore the error? a null result is ultimately returned when there is an error, but indeed checking for failure is more consistent with the API. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:55: LOG(ERROR) << "Could not set currency on amount formatter: " On 2017/01/11 18:29:30, rouslan wrote: > Will print an error for non-ISO currency codes? An example of a non-ISO currency > code is "BITCOIN". These are allowed by the spec, so it's not technically an > error. Perhaps you could check whether currency_code matches ^[A-Z]{3}$ regex > beforehand? src/third_party/re2 should be able to do that efficiently. I've changed currency code handling. This function/class will no longer early return for odd currency codes such as "BITCOIN". The number will be formatted after all. I think this achieves what you need. Validation of the actual code structure (^[A-Z]{3}$) is done in the PaymentsValidators::isValidCurrencyCodeFormat which this class assumes was run. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:63: if (!output.isEmpty()) { On 2017/01/11 18:29:30, rouslan wrote: > If the output is empty, you should probably return |amount| as well, right? Done. fair enough! https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:64: // Explicitely removes the currency code (truncated to its 3-letter and On 2017/01/11 18:29:30, rouslan wrote: > Explicitly Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.cc:77: // Trims any unicode whitespace (including non-breaking space). On 2017/01/11 18:29:30, rouslan wrote: > base::TrimeWhitespace? Does not handle unicode non-breaking space trimming. Ultimately this is why I'm doing all the findAndReplace and manual trimming with a UnicodeString and only converting to UTF8/UTF16 at the end. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter.h (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter.h:22: const std::string FormatAmountForCurrency(const std::string& amount, On 2017/01/11 18:29:30, rouslan wrote: > The "const" keyword here does not seem to server any purpose? Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... File components/payments/currency_formatter_unittest.cc (right): https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:14: struct CurrencyFormatterTestCase { On 2017/01/11 18:29:30, rouslan wrote: > nit: A shorter name like "TestCase" will make the rest of the file easier to > read without detriment. Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:25: const char* amount; On 2017/01/11 18:29:30, rouslan wrote: > const char* const amount; > > (First "const" says "don't change where this pointer goes" and second "const" > says "don't change the data at this pointer," or vice versa.) Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:28: std::string expected_amount; On 2017/01/11 18:29:30, rouslan wrote: > const std::string expected_amount; Thanks. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:63: CurrencyFormatterTestCase("1234", "USD", "en_US", "$1,234.00"), On 2017/01/11 18:29:30, rouslan wrote: > Also: > > CurrencyFormatterTestCase("0.1234", "USD", "en_US", "$0.1234"), > > Trading for fractions of a penny is more common than you think. For example, US > gas stations sell gas with prices like 3.999/gallon. Fractions of penny can also > be used in micro-transactions or by equity and commodities traders. ICU is not modern enough to know about micro transactions :) See the test case output. Seriously though, it is opinionated about a few things including the Japanese Yen not having "cents" either. Is there a spec that mentions fractional pennies or it's a nice to have? https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:73: CurrencyFormatterTestCase("55.00", "CAD", "fr_FR", "55,00 $"), On 2017/01/11 18:29:31, rouslan wrote: > Also: > > CurrencyFormatterTestCase("1234", "USD", "fr_FR", "1 234,00 $"), Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:85: CurrencyFormatterTestCase("-55.00", "BTX", "en_US", "-55.00"), On 2017/01/11 18:29:30, rouslan wrote: > Also: > > CurrencyFormatterTestCase("-55.00", "BTX", "fr_FR", "-55,00"), Done. https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... components/payments/currency_formatter_unittest.cc:92: // happen. On 2017/01/11 18:29:30, rouslan wrote: > Can we avoid this similar to how it's done in CurrencyStringFormatter.java or at > least gracefully handle overflow and imprecise parsings? > > Examples of what can be done: > > * Don't parse the string into a double. Manually replace "." with "," depending > on the locale. Manually pad "0" after the decimal separator. Manually insert " " > or "," for digit group separation. > > * Print the double back into a string in "C" locale and compare to the input > string. If these don't match, don't use the double. Thanks for prodding. Used a different API that accepts a string.
Patchset #2 (id:80002) has been deleted
On 2017/01/11 22:22:26, Mathieu Perreault wrote: > Thanks for your knowledge on this :) > > PTAL > > https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): > > https://codereview.chromium.org/2621033003/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/payments/order_summary_view_controller.cc:44: const > std::string formatted_amount = FormatAmountForCurrency( > On 2017/01/11 18:29:30, rouslan wrote: > > nit1: no need for const, because a quick perusal of this function shows that > the > > formatted amount is never changed. > > > > nit2: If you return base::stirng16 from FormatAmountForCurrency(), then it's > > more clear that the output is intended to be displayed to the user. > > > > nit3: ICU operations can be expensive. It's best to calculate this value once > in > > the model and share it among the views. > > > > nit4: If |total->amount->currencySystem| is present and is not set to > > "urn:iso:std:iso:4217", then the formatter should be aware that > > |total->amount->currency| is not ISO code that can be interpreted by ICU. For > > example, the formatter should use "" instead of "$" for "USD", if the > > currencySystem is not ISO. > > > > https://w3c.github.io/browser-payment-api/#paymentcurrencyamount-dictionary > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > File components/payments/currency_formatter.cc (right): > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:32: double amount_double = 0.0; > On 2017/01/11 18:29:30, rouslan wrote: > > Do you think it may be better to not parse the "amount" into a double, because > > the "amount" can have a huge number of digits, up to 30 in some cases? I > wonder > > if an overflow can occur. > > I would think base::StringToDouble does the right thing. For example there is > one test case that I copied from Java that has a lot of digits. It's parsed up > to the precision that double supports. We have to have a double because that's > what ICU needs... > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:40: > icu::NumberFormat::createCurrencyInstance(locale, ignored)); > On 2017/01/11 18:29:30, rouslan wrote: > > Why ignore the error? > > a null result is ultimately returned when there is an error, but indeed checking > for failure is more consistent with the API. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:55: LOG(ERROR) << "Could not set > currency on amount formatter: " > On 2017/01/11 18:29:30, rouslan wrote: > > Will print an error for non-ISO currency codes? An example of a non-ISO > currency > > code is "BITCOIN". These are allowed by the spec, so it's not technically an > > error. Perhaps you could check whether currency_code matches ^[A-Z]{3}$ regex > > beforehand? src/third_party/re2 should be able to do that efficiently. > > I've changed currency code handling. This function/class will no longer early > return for odd currency codes such as "BITCOIN". The number will be formatted > after all. I think this achieves what you need. Validation of the actual code > structure (^[A-Z]{3}$) is done in the > PaymentsValidators::isValidCurrencyCodeFormat which this class assumes was run. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:63: if (!output.isEmpty()) { > On 2017/01/11 18:29:30, rouslan wrote: > > If the output is empty, you should probably return |amount| as well, right? > > Done. fair enough! > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:64: // Explicitely removes the > currency code (truncated to its 3-letter and > On 2017/01/11 18:29:30, rouslan wrote: > > Explicitly > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.cc:77: // Trims any unicode whitespace > (including non-breaking space). > On 2017/01/11 18:29:30, rouslan wrote: > > base::TrimeWhitespace? > > Does not handle unicode non-breaking space trimming. Ultimately this is why I'm > doing all the findAndReplace and manual trimming with a UnicodeString and only > converting to UTF8/UTF16 at the end. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > File components/payments/currency_formatter.h (right): > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter.h:22: const std::string > FormatAmountForCurrency(const std::string& amount, > On 2017/01/11 18:29:30, rouslan wrote: > > The "const" keyword here does not seem to server any purpose? > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > File components/payments/currency_formatter_unittest.cc (right): > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:14: struct > CurrencyFormatterTestCase { > On 2017/01/11 18:29:30, rouslan wrote: > > nit: A shorter name like "TestCase" will make the rest of the file easier to > > read without detriment. > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:25: const char* amount; > On 2017/01/11 18:29:30, rouslan wrote: > > const char* const amount; > > > > (First "const" says "don't change where this pointer goes" and second "const" > > says "don't change the data at this pointer," or vice versa.) > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:28: std::string > expected_amount; > On 2017/01/11 18:29:30, rouslan wrote: > > const std::string expected_amount; > > Thanks. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:63: > CurrencyFormatterTestCase("1234", "USD", "en_US", "$1,234.00"), > On 2017/01/11 18:29:30, rouslan wrote: > > Also: > > > > CurrencyFormatterTestCase("0.1234", "USD", "en_US", "$0.1234"), > > > > Trading for fractions of a penny is more common than you think. For example, > US > > gas stations sell gas with prices like 3.999/gallon. Fractions of penny can > also > > be used in micro-transactions or by equity and commodities traders. > > ICU is not modern enough to know about micro transactions :) See the test case > output. > > Seriously though, it is opinionated about a few things including the Japanese > Yen not having "cents" either. Is there a spec that mentions fractional pennies > or it's a nice to have? > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:73: > CurrencyFormatterTestCase("55.00", "CAD", "fr_FR", "55,00 $"), > On 2017/01/11 18:29:31, rouslan wrote: > > Also: > > > > CurrencyFormatterTestCase("1234", "USD", "fr_FR", "1 234,00 $"), > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:85: > CurrencyFormatterTestCase("-55.00", "BTX", "en_US", "-55.00"), > On 2017/01/11 18:29:30, rouslan wrote: > > Also: > > > > CurrencyFormatterTestCase("-55.00", "BTX", "fr_FR", "-55,00"), > > Done. > > https://codereview.chromium.org/2621033003/diff/100001/components/payments/cu... > components/payments/currency_formatter_unittest.cc:92: // happen. > On 2017/01/11 18:29:30, rouslan wrote: > > Can we avoid this similar to how it's done in CurrencyStringFormatter.java or > at > > least gracefully handle overflow and imprecise parsings? > > > > Examples of what can be done: > > > > * Don't parse the string into a double. Manually replace "." with "," > depending > > on the locale. Manually pad "0" after the decimal separator. Manually insert " > " > > or "," for digit group separation. > > > > * Print the double back into a string in "C" locale and compare to the input > > string. If these don't match, don't use the double. > > Thanks for prodding. Used a different API that accepts a string. Please don't review, forgot some tests
OK should be good to go, PTAL!
https://codereview.chromium.org/2621033003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:43: CurrencyFormatter* formatter = request()->GetOrCreateCurrencyFormatter( perfection! https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter.cc (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:46: std::string locale_name; Nit: it's confusing that your have a locale_name variable here that hides the locale_name constructor argument. Can you use a different name? https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:79: if (!icu_formatter_ || !icu_formatter_->getCurrency()) { nit: no need for {} https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:88: if (output.isEmpty()) { ditto https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:94: // displays the currency code alongside this result. display https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter.h (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.h:47: }; DISALLOW_COPY_AND_ASSIGN for good measure. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter_unittest.cc (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter_unittest.cc:70: TestCase("0.1234", "USD", "en_US", "$0.12"), I'm still a bit concerned that users won't know the exact amount they are being charged based on the user interface. Can we configure ICU to be smart or submit a patch upstream?
PTAL! https://codereview.chromium.org/2621033003/diff/190001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/190001/chrome/browser/ui/view... chrome/browser/ui/views/payments/order_summary_view_controller.cc:43: CurrencyFormatter* formatter = request()->GetOrCreateCurrencyFormatter( On 2017/01/12 19:23:23, rouslan wrote: > perfection! Acknowledged. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter.cc (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:46: std::string locale_name; On 2017/01/12 19:23:24, rouslan wrote: > Nit: it's confusing that your have a locale_name variable here that hides the > locale_name constructor argument. Can you use a different name? Done. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:79: if (!icu_formatter_ || !icu_formatter_->getCurrency()) { On 2017/01/12 19:23:24, rouslan wrote: > nit: no need for {} Done. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:88: if (output.isEmpty()) { On 2017/01/12 19:23:24, rouslan wrote: > ditto Done. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.cc:94: // displays the currency code alongside this result. On 2017/01/12 19:23:24, rouslan wrote: > display Done. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter.h (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter.h:47: }; On 2017/01/12 19:23:24, rouslan wrote: > DISALLOW_COPY_AND_ASSIGN for good measure. Done. https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... File components/payments/currency_formatter_unittest.cc (right): https://codereview.chromium.org/2621033003/diff/190001/components/payments/cu... components/payments/currency_formatter_unittest.cc:70: TestCase("0.1234", "USD", "en_US", "$0.12"), On 2017/01/12 19:23:24, rouslan wrote: > I'm still a bit concerned that users won't know the exact amount they are being > charged based on the user interface. Can we configure ICU to be smart or submit > a patch upstream? Using a max of 10 now.
lgtm
The CQ bit was checked by mathp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mathp@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in chrome/browser/ui
https://codereview.chromium.org/2621033003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:164: request()->details()->total->amount->currencySystem, How come this isn't currency_sytem?
The CQ bit was checked by mathp@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 Scott! https://codereview.chromium.org/2621033003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right): https://codereview.chromium.org/2621033003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:164: request()->details()->total->amount->currencySystem, On 2017/01/12 23:49:02, sky wrote: > How come this isn't currency_sytem? Aha, good catch! Someone had been coding in Java the day they wrote this line in the mojom file... I've filed a bug (crbug.com/680803) to convert this (and methodData) throughout. It's likely to be my next change.
Ok, LGTM
The CQ bit was unchecked by mathp@chromium.org
The CQ bit was checked by mathp@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/2621033003/#ps230001 (title: "deps -> public_deps")
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 mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2621033003/#ps250001 (title: "checked_cast")
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": 250001, "attempt_start_ts": 1484272548052940,
"parent_rev": "c43f4b93273c33a1a3c0ea483bca8559a8fd87e9", "commit_rev":
"6758be03e71c7423fb1b43dc267049ebc4ceaf27"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Currency formatter for order amounts. Uses ICU. This change also enables using it on the Desktop implementation. A followup will enable it on Android. BUG=679797 TEST=CurrencyFormatterTest, LANGUAGE=fr_CA ./out/Default/chrome ========== to ========== [Payments] Currency formatter for order amounts. Uses ICU. This change also enables using it on the Desktop implementation. A followup will enable it on Android. BUG=679797 TEST=CurrencyFormatterTest, LANGUAGE=fr_CA ./out/Default/chrome Review-Url: https://codereview.chromium.org/2621033003 Cr-Commit-Position: refs/heads/master@{#443493} Committed: https://chromium.googlesource.com/chromium/src/+/6758be03e71c7423fb1b43dc2670... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:250001) as https://chromium.googlesource.com/chromium/src/+/6758be03e71c7423fb1b43dc2670... |
