Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2852363002:
[payment] display mixed currency in item list and shipping optiopns. When there is more than one cu… (Closed)
|
Created:
3 years, 7 months ago by wuandy1 Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMixed currency support on Android
This change will display 3-digit currency code before any amount on PR
dialog, as long as these are more than one currency involved at any
time, so user can be sure what currencies they are paying.
This change only include android support. Other platforms will be in
future CLs.
BUG=709296
Review-Url: https://codereview.chromium.org/2852363002
Cr-Commit-Position: refs/heads/master@{#469978}
Committed: https://chromium.googlesource.com/chromium/src/+/da7c77096c29ccd804917018dc74bebd925e164a
Patch Set 1 #
Total comments: 23
Patch Set 2 : addressing code review comments #
Total comments: 12
Patch Set 3 : more minor changes requested by code reviewing #
Total comments: 2
Patch Set 4 : remove android platform check #Patch Set 5 : fixing dumb compilation error! #
Messages
Total messages: 35 (21 generated)
Description was changed from ========== [payment] display mixed currency in item list and shipping optiopns. When there is more than one currency involved, this change will display 3-digit currency code for each of the amount showing in the PR dialog. BUG=709296 ========== to ========== [payment] display mixed currency in item list and shipping optiopns. When there is more than one currency involved, this change will display 3-digit currency code for each of the amount showing in the PR dialog. BUG=709296 ==========
wuandy@google.com changed reviewers: + gogerald@chromium.org, rouslan@chromium.org
Description was changed from ========== [payment] display mixed currency in item list and shipping optiopns. When there is more than one currency involved, this change will display 3-digit currency code for each of the amount showing in the PR dialog. BUG=709296 ========== to ========== display mixed currency in item list and shipping optiopns. When there is more than one currency involved, this change will display 3-digit currency code for each of the amount showing in the PR dialog. BUG=709296 ==========
rouslan/gangui, please take a look at my change for mixed-currency support in clank. If you are wondering where the tests are: i tried to add functional tests, but found it hard. The existing functional tests all look for certain string patterns in response, which seemed to include no currency information. Happy to learn how I can write tests in this case. Thanks
Ganggui, please take the lead reviewing this. I will take a pass after you're done.
good job, thanks! https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:402: if (formatter != null) { 'formatter' should never be null, instead of 'if' statement, please do 'assert formatter != null' https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:788: preloadRequiredCurrencyFormatters(details); 'preload' usually means you no need it right now and this function is also called by updateWith(). So may be just 'LoadCurrencyFormatterForPaymentDetails'? https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:916: } one space line https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:918: for (PaymentItem item : details.displayItems) { use 'for(int i = 0; i <....)' loop if possible since it is more efficient on Android than iterator https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:921: } one space line https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:923: for (PaymentShippingOption option : details.shippingOptions) { use 'for(int i = 0; i <....)' loop if possible since it is more efficient on Android than iterator https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:926: } one space line https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:928: for (PaymentDetailsModifier modifier : details.modifiers) { use 'for(int i = 0; i <....)' loop if possible since it is more efficient on Android than iterator https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:930: for (PaymentItem item : modifier.additionalDisplayItems) { use 'for(int i = 0; i <....)' loop if possible since it is more efficient on Android than iterator https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:946: Pair<String, String> key = new Pair<>(amount.currency, amount.currencySystem); We only need an unique identifier here, so "amount.currency + amount.currencySystem" should be good enough, https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:948: mCurrencyFormatterMap.put(key, when this could happen? PaymentDetails only change during init and updateWith, https://codereview.chromium.org/2852363002/diff/1/components/payments/content... File components/payments/content/payment_details_validation.cc (left): https://codereview.chromium.org/2852363002/diff/1/components/payments/content... components/payments/content/payment_details_validation.cc:43: if (total && item->amount->currency != total->amount->currency) { This is cross platform code, might '#if !defined(OS_ANDROID)' before other platforms support mixed currency.
wuandy@google.com changed reviewers: + wuandy@google.com
https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:402: if (formatter != null) { On 2017/05/02 18:57:32, gogerald1 wrote: > 'formatter' should never be null, instead of 'if' statement, please do 'assert > formatter != null' Done. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:788: preloadRequiredCurrencyFormatters(details); On 2017/05/02 18:57:32, gogerald1 wrote: > 'preload' usually means you no need it right now and this function is also > called by updateWith(). So may be just 'LoadCurrencyFormatterForPaymentDetails'? Done. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:916: } On 2017/05/02 18:57:32, gogerald1 wrote: > one space line Done. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:918: for (PaymentItem item : details.displayItems) { On 2017/05/02 18:57:32, gogerald1 wrote: > use 'for(int i = 0; i <....)' loop if possible since it is more efficient on > Android than iterator Keeping the change as discussed offline. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:921: } On 2017/05/02 18:57:32, gogerald1 wrote: > one space line Done. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:923: for (PaymentShippingOption option : details.shippingOptions) { On 2017/05/02 18:57:32, gogerald1 wrote: > use 'for(int i = 0; i <....)' loop if possible since it is more efficient on > Android than iterator Keeping the change as discussed offline. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:926: } On 2017/05/02 18:57:32, gogerald1 wrote: > one space line Done. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:928: for (PaymentDetailsModifier modifier : details.modifiers) { On 2017/05/02 18:57:32, gogerald1 wrote: > use 'for(int i = 0; i <....)' loop if possible since it is more efficient on > Android than iterator Keeping the change as discussed offline. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:930: for (PaymentItem item : modifier.additionalDisplayItems) { On 2017/05/02 18:57:32, gogerald1 wrote: > use 'for(int i = 0; i <....)' loop if possible since it is more efficient on > Android than iterator Keeping the change as discussed offline. https://codereview.chromium.org/2852363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:946: Pair<String, String> key = new Pair<>(amount.currency, amount.currencySystem); On 2017/05/02 18:57:32, gogerald1 wrote: > We only need an unique identifier here, so "amount.currency + > amount.currencySystem" should be good enough, Done. https://codereview.chromium.org/2852363002/diff/1/components/payments/content... File components/payments/content/payment_details_validation.cc (left): https://codereview.chromium.org/2852363002/diff/1/components/payments/content... components/payments/content/payment_details_validation.cc:43: if (total && item->amount->currency != total->amount->currency) { On 2017/05/02 18:57:33, gogerald1 wrote: > This is cross platform code, might '#if !defined(OS_ANDROID)' before other > platforms support mixed currency. Done.
The CQ bit was checked by wuandy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, few more minor comments, https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:788: loadCurrencyFormattersForPaymentDetails(details); one space line above and below https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:899: isMixedCurrency() ? formatter.getFormattedCurrencyCode() + " " : ""; is this space needed? if true, unicode space is better (\u0020). https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:919: private void loadCurrencyFormattersForPaymentDetails(PaymentDetails details) { The comments might could be simplified like below /** * Loads required currency formatter for a given payment details. * * Note that the cache (mCurrencyFormatterMap) is not cleared for * updated payment details so as to indicate the currency is changed. * * @param details The given payment details. */ https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:946: private boolean isMixedCurrency() { rename to isMixedOrChangedCurrency() to be more specific? https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:951: * Look for a formatter for a given PaymentCurrencyAmount, create one if no existing instance nit: recommend /** * Gets currency formatter for a given PaymentCurrencyAmount, * creates one if no existing instance is found. * * @amount The given payment amount. */ https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:956: if (!mCurrencyFormatterMap.containsKey(key)) { nit: recommend below similar code to save a lookup. CurrencyFormatter formatter = mCurrencyFormatterMap.get(key); if(formatter == null) { formatter = new CurrencyFormatter(amount......); mCurrentFormatterMap.put(formatter); } return formatter;
https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:788: loadCurrencyFormattersForPaymentDetails(details); On 2017/05/05 17:40:42, gogerald1 wrote: > one space line above and below Done. https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:899: isMixedCurrency() ? formatter.getFormattedCurrencyCode() + " " : ""; On 2017/05/05 17:40:42, gogerald1 wrote: > is this space needed? if true, unicode space is better (\u0020). Done. https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:919: private void loadCurrencyFormattersForPaymentDetails(PaymentDetails details) { On 2017/05/05 17:40:42, gogerald1 wrote: > The comments might could be simplified like below > > /** > * Loads required currency formatter for a given payment details. > * > * Note that the cache (mCurrencyFormatterMap) is not cleared for > * updated payment details so as to indicate the currency is changed. > * > * @param details The given payment details. > */ Done. https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:946: private boolean isMixedCurrency() { On 2017/05/05 17:40:42, gogerald1 wrote: > rename to isMixedOrChangedCurrency() to be more specific? Done. https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:951: * Look for a formatter for a given PaymentCurrencyAmount, create one if no existing instance On 2017/05/05 17:40:42, gogerald1 wrote: > nit: recommend > > /** > * Gets currency formatter for a given PaymentCurrencyAmount, > * creates one if no existing instance is found. > * > * @amount The given payment amount. > */ Done. https://codereview.chromium.org/2852363002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:956: if (!mCurrencyFormatterMap.containsKey(key)) { On 2017/05/05 17:40:42, gogerald1 wrote: > nit: recommend below similar code to save a lookup. > > CurrencyFormatter formatter = mCurrencyFormatterMap.get(key); > if(formatter == null) { > formatter = new CurrencyFormatter(amount......); > mCurrentFormatterMap.put(formatter); > } > return formatter; Done.
lgtm
wuandy@chromium.org changed reviewers: - wuandy@google.com
Please format the CL description according to these guidelines: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2852363002/diff/40001/components/payments/con... File components/payments/content/payment_details_validation.cc (right): https://codereview.chromium.org/2852363002/diff/40001/components/payments/con... components/payments/content/payment_details_validation.cc:38: #if !defined(OS_ANDROID) Desktop should behave the same as Android. Please remove lines 38-43. You can add the relevant desktop UI in follow up patch (or ask a desktop engineer to help).
Description was changed from ========== display mixed currency in item list and shipping optiopns. When there is more than one currency involved, this change will display 3-digit currency code for each of the amount showing in the PR dialog. BUG=709296 ========== to ========== Mixed currency support on Android This change will display 3-digit currency code before any amount on PR dialog, as long as these are more than one currency involved at any time, so user can be sure what currencies they are paying. This change only include android support. Other platforms will be in future CLs. BUG=709296 ==========
https://codereview.chromium.org/2852363002/diff/40001/components/payments/con... File components/payments/content/payment_details_validation.cc (right): https://codereview.chromium.org/2852363002/diff/40001/components/payments/con... components/payments/content/payment_details_validation.cc:38: #if !defined(OS_ANDROID) On 2017/05/05 19:13:55, ಠ_ಠ wrote: > Desktop should behave the same as Android. Please remove lines 38-43. You can > add the relevant desktop UI in follow up patch (or ask a desktop engineer to > help). Done.
lgtm
The CQ bit was checked by wuandy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2852363002/#ps60001 (title: "remove android platform check")
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by wuandy@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.
The CQ bit was checked by wuandy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2852363002/#ps80001 (title: "fixing dumb compilation error!")
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": 80001, "attempt_start_ts": 1494252592807310,
"parent_rev": "321a171abcca381e3ef72d01996e4ae4dd860212", "commit_rev":
"da7c77096c29ccd804917018dc74bebd925e164a"}
Message was sent while issue was closed.
Description was changed from ========== Mixed currency support on Android This change will display 3-digit currency code before any amount on PR dialog, as long as these are more than one currency involved at any time, so user can be sure what currencies they are paying. This change only include android support. Other platforms will be in future CLs. BUG=709296 ========== to ========== Mixed currency support on Android This change will display 3-digit currency code before any amount on PR dialog, as long as these are more than one currency involved at any time, so user can be sure what currencies they are paying. This change only include android support. Other platforms will be in future CLs. BUG=709296 Review-Url: https://codereview.chromium.org/2852363002 Cr-Commit-Position: refs/heads/master@{#469978} Committed: https://chromium.googlesource.com/chromium/src/+/da7c77096c29ccd804917018dc74... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/da7c77096c29ccd804917018dc74... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
