|
|
Created:
3 years, 7 months ago by wuandy1 Modified:
3 years, 7 months ago CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, agrieve+watch_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionenable modifiers for native apps
1. set web-payments-modifiers to enabled by default
2. implement the serialization of modifiers to native apps through intent
3. integration test for app modifiers
BUG=719666
Review-Url: https://codereview.chromium.org/2885443003
Cr-Commit-Position: refs/heads/master@{#474022}
Committed: https://chromium.googlesource.com/chromium/src/+/ffe8d3c802c270b644c4ebc0ce32d03ba0a7882f
Patch Set 1 #Patch Set 2 : more formatting #Patch Set 3 : add missing piece lost in git rebase.. #Patch Set 4 : changes required by recent refactoring #
Total comments: 4
Patch Set 5 : string array -> string | filtering #Patch Set 6 : fixing compilation error and formatting #
Total comments: 40
Patch Set 7 : addressing reviewer comments and using junut4 tests #Patch Set 8 : more beautify #
Total comments: 20
Patch Set 9 : more changes to address reviewer comments #Patch Set 10 : more formatting #Patch Set 11 : fixing trybot failure #Patch Set 12 : remove other modifer test #
Total comments: 6
Patch Set 13 : addressing comments #
Total comments: 10
Patch Set 14 : addressing comments #Patch Set 15 : fix compilation error #Messages
Total messages: 69 (53 generated)
Description was changed from ========== enable modifiers for native apps 1. set web-payments-modifiers to enabled by default 2. implement the serialzation of modifiers to native apps through intent BUG=719666 ========== to ========== enable modifiers for native apps 1. set web-payments-modifiers to enabled by default 2. implement the serialization of modifiers to native apps through intent 3. integration test for app modifiers BUG=719666 ==========
wuandy@chromium.org changed reviewers: + gogerald@chromium.org, rouslan@chromium.org
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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: 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.
One quick response before continue reviewing this CL, https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private static ArrayList<String> buildModifierList( We are supposed to serialize all modifiers in to a single string instead of ArrayList according to the spec. You can refer the example here to know how to do that https://developer.android.com/reference/android/util/JsonWriter.html
Sorry for the drive-by! https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private static ArrayList<String> buildModifierList( Also, please filter out modifiers whose "supportedMethods" don't contain one of the mMethodNames.
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...
https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private static ArrayList<String> buildModifierList( On 2017/05/16 20:36:59, gogerald1 wrote: > We are supposed to serialize all modifiers in to a single string instead of > ArrayList according to the spec. > > You can refer the example here to know how to do that > https://developer.android.com/reference/android/util/JsonWriter.html Done. https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private static ArrayList<String> buildModifierList( On 2017/05/17 13:02:32, ಠ_ಠ wrote: > Also, please filter out modifiers whose "supportedMethods" don't contain one of > the mMethodNames. Done.
rouslan@chromium.org changed reviewers: - rouslan@chromium.org
Please loop me back in when Ganggui's review is done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:83: private static final String EXTRA_MODIFIERS = "modifiers"; You can move it under EXTRA_CERTIFICATE to list in alphabetic order. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:375: extras.putString(EXTRA_MODIFIERS, buildModifierListString(modifiers.values())); You can name it "serializeModifiers" or "serializeModifiersCollection" to be more specific https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:375: extras.putString(EXTRA_MODIFIERS, buildModifierListString(modifiers.values())); Do not put 'null' value to EXTRA_MODIFIERS https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private String buildModifierListString(Collection<PaymentDetailsModifier> modifiers) { annotate @Nullable https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:506: if (mMethodNames.contains(supported)) { You no need to do this again here since we already filtered out unsupported payment methods' modifiers here https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I think after removing this logic, you can make this method and the above buildExtras static https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:526: serializePaymentItem(modifier.total, json); total could be null in modifier https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:1363: public TextView getOrderSummaryTotalTextViewForTest() { You can use "getOrderSummarySectionForTest" to return mOrderSummarySection to be consistent with above interface and leave flexibility for test https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:20: * modifiers at the same time. "A payment integration test for a merchant that requests payment via Bob Pay or cards with modifiers" might be better https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:23: public PaymentRequestPaymentAppAndCardsWithModifiersTest() { We are moving from using jUnit3 to jUnit4, you can refer PaymentRequestPaymentAppTest for example. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:45: * If Bob Pay does not have any instruments, show [visa, mastercard]. Here the payment app You used 'HAVE_INSTRUMENTS' below, but here says 'does not have any instruments'? https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:50: public void testModifierUpdateTotalAndInstrumentLabel() suggest name: testUpdateTotalAndInstrumentLabelWithModifiers https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:58: clickOnPaymentMethodSuggestionOptionAndWait(1, getReadyForInput()); Comments on what you are doing here https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:65: public void testPaymentAppAbleToPayAfterApplyingModifiers() Add comments to explain this test https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:65: public void testPaymentAppAbleToPayAfterApplyingModifiers() suggest name: testPaymentAppCanPayWithModifiers https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:75: expectResultContains(new String[] {"https://bobpay.com", "\"transaction\"", "1337"}); Can you check whether the received modifiers are correct? You might could return the received information through call back. More over, can you test when total is null in modifier? https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:233: /** Returns the label of the payment method that is currently selected. */ could be simplified to: "Returns the label of the selected payment instrument." https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:238: protected String getOrderSummaryTotal() throws ExecutionException { add comments https://codereview.chromium.org/2885443003/diff/100001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_cards_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_cards_with_modifiers.js:16: [{supportedMethods: ['https://bobpay.com', 'visa', 'mastercard']}], nit: it's better to use basic-card since we will remove support of 'visa', 'mastercard' and so on
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...
https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:83: private static final String EXTRA_MODIFIERS = "modifiers"; On 2017/05/17 19:11:46, gogerald1 wrote: > You can move it under EXTRA_CERTIFICATE to list in alphabetic order. Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:375: extras.putString(EXTRA_MODIFIERS, buildModifierListString(modifiers.values())); On 2017/05/17 19:11:47, gogerald1 wrote: > Do not put 'null' value to EXTRA_MODIFIERS Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:375: extras.putString(EXTRA_MODIFIERS, buildModifierListString(modifiers.values())); On 2017/05/17 19:11:47, gogerald1 wrote: > Do not put 'null' value to EXTRA_MODIFIERS Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private String buildModifierListString(Collection<PaymentDetailsModifier> modifiers) { On 2017/05/17 19:11:47, gogerald1 wrote: > annotate @Nullable changed to return empty json instead. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:506: if (mMethodNames.contains(supported)) { On 2017/05/17 19:11:47, gogerald1 wrote: > You no need to do this again here since we already filtered out unsupported > payment methods' modifiers here > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > I think after removing this logic, you can make this method and the above > buildExtras static Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:526: serializePaymentItem(modifier.total, json); On 2017/05/17 19:11:47, gogerald1 wrote: > total could be null in modifier Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:1363: public TextView getOrderSummaryTotalTextViewForTest() { On 2017/05/17 19:11:47, gogerald1 wrote: > You can use "getOrderSummarySectionForTest" to return mOrderSummarySection to be > consistent with above interface and leave flexibility for test getSummaryRightTextView is protected..so i choosed to keep it protected. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:20: * modifiers at the same time. On 2017/05/17 19:11:47, gogerald1 wrote: > "A payment integration test for a merchant that requests payment via Bob Pay or > cards with modifiers" might be better Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:23: public PaymentRequestPaymentAppAndCardsWithModifiersTest() { On 2017/05/17 19:11:47, gogerald1 wrote: > We are moving from using jUnit3 to jUnit4, you can refer > PaymentRequestPaymentAppTest for example. Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:45: * If Bob Pay does not have any instruments, show [visa, mastercard]. Here the payment app On 2017/05/17 19:11:47, gogerald1 wrote: > You used 'HAVE_INSTRUMENTS' below, but here says 'does not have any > instruments'? fixed. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:50: public void testModifierUpdateTotalAndInstrumentLabel() On 2017/05/17 19:11:47, gogerald1 wrote: > suggest name: testUpdateTotalAndInstrumentLabelWithModifiers Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:58: clickOnPaymentMethodSuggestionOptionAndWait(1, getReadyForInput()); On 2017/05/17 19:11:47, gogerald1 wrote: > Comments on what you are doing here Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:58: clickOnPaymentMethodSuggestionOptionAndWait(1, getReadyForInput()); On 2017/05/17 19:11:47, gogerald1 wrote: > Comments on what you are doing here Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:65: public void testPaymentAppAbleToPayAfterApplyingModifiers() On 2017/05/17 19:11:47, gogerald1 wrote: > Add comments to explain this test Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:65: public void testPaymentAppAbleToPayAfterApplyingModifiers() On 2017/05/17 19:11:47, gogerald1 wrote: > suggest name: testPaymentAppCanPayWithModifiers Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:75: expectResultContains(new String[] {"https://bobpay.com", "\"transaction\"", "1337"}); On 2017/05/17 19:11:47, gogerald1 wrote: > Can you check whether the received modifiers are correct? You might could return > the received information through call back. > > More over, can you test when total is null in modifier? Current test-supporting code does not provide a way to verify what the app sees. Maybe i can add it, but not for this cl maybe. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:233: /** Returns the label of the payment method that is currently selected. */ On 2017/05/17 19:11:47, gogerald1 wrote: > could be simplified to: "Returns the label of the selected payment instrument." Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:238: protected String getOrderSummaryTotal() throws ExecutionException { On 2017/05/17 19:11:47, gogerald1 wrote: > add comments Done. https://codereview.chromium.org/2885443003/diff/100001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_cards_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_cards_with_modifiers.js:16: [{supportedMethods: ['https://bobpay.com', 'visa', 'mastercard']}], On 2017/05/17 19:11:47, gogerald1 wrote: > nit: it's better to use basic-card since we will remove support of 'visa', > 'mastercard' and so on Done.
thanks, few more comments https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private String buildModifierListString(Collection<PaymentDetailsModifier> modifiers) { On 2017/05/18 20:12:28, wuandy1 wrote: > On 2017/05/17 19:11:47, gogerald1 wrote: > > annotate @Nullable > > changed to return empty json instead. even better, https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:1363: public TextView getOrderSummaryTotalTextViewForTest() { On 2017/05/18 20:12:29, wuandy1 wrote: > On 2017/05/17 19:11:47, gogerald1 wrote: > > You can use "getOrderSummarySectionForTest" to return mOrderSummarySection to > be > > consistent with above interface and leave flexibility for test > > getSummaryRightTextView is protected..so i choosed to keep it protected. Acknowledged. https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:75: expectResultContains(new String[] {"https://bobpay.com", "\"transaction\"", "1337"}); On 2017/05/18 20:12:29, wuandy1 wrote: > On 2017/05/17 19:11:47, gogerald1 wrote: > > Can you check whether the received modifiers are correct? You might could > return > > the received information through call back. > > > > More over, can you test when total is null in modifier? > > Current test-supporting code does not provide a way to verify what the app sees. > Maybe i can add it, but not for this cl maybe. Acknowledged. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:342: private Bundle buildExtras(@Nullable String id, @Nullable String merchantName, String origin, you may want put static back here https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:519: if (modifier.total != null) { I guess you would do "json.name("total").nullValue()" when modifier.total != null https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:9: import static junit.framework.Assert.assertTrue; nit: you may consider use "import junit.framework.Assert;" to save two lines. They are not used frequently below and Assert.* is not a long name. up to you though. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:35: * A payment integration test for a merchant that requests payment via Bob Pay or cards with basic-card https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:43: public class PaymentRequestPaymentAppAndCardsWithModifiersTest { You may want to rename it to "PaymentRequestPaymentAppAndBasicCardWithModifiersTest" and other places (like file names and so on) to be specific https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:47: new PaymentRequestTestRule.MainActivityStartCallback() { you can implement MainActivityStartCallback in PaymentRequestPaymentAppAndCardsWithModifiersTest for better readability like here https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:97: public void testPaymentAppCanPayWithModifiers() please help remove this test file PaymentRequestModifierTest.java and its related files since this CL supersedes that test https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_cards_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_cards_with_modifiers.js:10: * Launches the PaymentRequest UI with Bob Pay and credit cards as payment *basic-card* https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html:15: <button onclick="buy()" id="buy">Bob Pay and Cards with modifiers Test</button><br> *basic-card* https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html:18: <script src="bobpay_and_cards_with_modifiers.js"></script> *basic-card*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:342: private Bundle buildExtras(@Nullable String id, @Nullable String merchantName, String origin, On 2017/05/18 21:57:33, gogerald1 wrote: > you may want put static back here Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:519: if (modifier.total != null) { On 2017/05/18 21:57:33, gogerald1 wrote: > I guess you would do "json.name("total").nullValue()" when modifier.total != > null Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:9: import static junit.framework.Assert.assertTrue; On 2017/05/18 21:57:33, gogerald1 wrote: > nit: you may consider use "import junit.framework.Assert;" to save two lines. > They are not used frequently below and Assert.* is not a long name. up to you > though. I personally like to keep the code shorter and add extra imports, just feel like it's easier to read. Will keep it here if you do not mind. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:35: * A payment integration test for a merchant that requests payment via Bob Pay or cards with On 2017/05/18 21:57:33, gogerald1 wrote: > basic-card Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:43: public class PaymentRequestPaymentAppAndCardsWithModifiersTest { On 2017/05/18 21:57:33, gogerald1 wrote: > You may want to rename it to > "PaymentRequestPaymentAppAndBasicCardWithModifiersTest" and other places (like > file names and so on) to be specific Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:47: new PaymentRequestTestRule.MainActivityStartCallback() { On 2017/05/18 21:57:33, gogerald1 wrote: > you can implement MainActivityStartCallback in > PaymentRequestPaymentAppAndCardsWithModifiersTest for better readability like > here > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsWithModifiersTest.java:97: public void testPaymentAppCanPayWithModifiers() On 2017/05/18 21:57:33, gogerald1 wrote: > please help remove this test file PaymentRequestModifierTest.java and its > related files since this CL supersedes that test My repository does not have this file while code search has it. guess someone just deleted it. https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_cards_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_cards_with_modifiers.js:10: * Launches the PaymentRequest UI with Bob Pay and credit cards as payment On 2017/05/18 21:57:33, gogerald1 wrote: > *basic-card* Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html:15: <button onclick="buy()" id="buy">Bob Pay and Cards with modifiers Test</button><br> On 2017/05/18 21:57:33, gogerald1 wrote: > *basic-card* Done. https://codereview.chromium.org/2885443003/diff/140001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_cards_with_modifiers_test.html:18: <script src="bobpay_and_cards_with_modifiers.js"></script> On 2017/05/18 21:57:33, gogerald1 wrote: > *basic-card* Done.
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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 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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by wuandy@chromium.org
updated with latest.
nice work, lgtm % comments https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:67: * If Bob Pay has instruments, verify modifier for bobpay is applied when it is selected, Might clear this like "Verify modifier for Bobpay is applied when it is selected and not applied to other payment methods" or "Verify modifier for Bobpay is only applied for Bobpay" https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_basic_card_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_basic_card_with_modifiers.js:2: * Copyright 2016 The Chromium Authors. All rights reserved. *2017* https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_bobpay_and_basic_card_with_modifiers_test.html (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_basic_card_with_modifiers_test.html:3: Copyright 2016 The Chromium Authors. All rights reserved. *2017*
https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:67: * If Bob Pay has instruments, verify modifier for bobpay is applied when it is selected, On 2017/05/19 19:22:48, gogerald1 wrote: > Might clear this like "Verify modifier for Bobpay is applied when it is selected > and not applied to other payment methods" or "Verify modifier for Bobpay is only > applied for Bobpay" Done. https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... File chrome/test/data/payments/bobpay_and_basic_card_with_modifiers.js (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... chrome/test/data/payments/bobpay_and_basic_card_with_modifiers.js:2: * Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/19 19:22:48, gogerald1 wrote: > *2017* Done. https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_bobpay_and_basic_card_with_modifiers_test.html (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/test/data/payme... chrome/test/data/payments/payment_request_bobpay_and_basic_card_with_modifiers_test.html:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/19 19:22:48, gogerald1 wrote: > *2017* Done.
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...
wuandy@chromium.org changed reviewers: + rouslan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Great job! Please post some screenshots and/or videos of the feature in action. https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:530: for (String method : modifier.methodData.supportedMethods) { |modifier.methodData.stringified_data| also needs to be sent to the payment app. It can contain such information as card type filter, for example. new PaymentRequest([{ supportedMethods: ['basic-card'] }], { total: { label: 'Regular price', amount: { currency: 'USD', value: '1.00' }, modifiers: [{ supportedMethods: ['basic-card'], data: { supportedTypes: ['debit'] }, total: { label: 'Debit price', amount: { currency: 'USD', value: '0.50' } } }] }.show(); https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestModifierTest.java (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestModifierTest.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. Let's try to avoid deleting existing test cases. https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... File chrome/test/data/payments/modifier.js (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... chrome/test/data/payments/modifier.js:1: /* Let's not delete existing test cases. https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_modifier_test.html (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... chrome/test/data/payments/payment_request_modifier_test.html:1: <!DOCTYPE html> Let's not delete.
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 checked by wuandy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:530: for (String method : modifier.methodData.supportedMethods) { On 2017/05/22 13:17:10, ಠ_ಠ wrote: > |modifier.methodData.stringified_data| also needs to be sent to the payment app. > It can contain such information as card type filter, for example. > > new PaymentRequest([{ > supportedMethods: ['basic-card'] > }], { > total: { > label: 'Regular price', > amount: { > currency: 'USD', > value: '1.00' > }, > modifiers: [{ > supportedMethods: ['basic-card'], > data: { > supportedTypes: ['debit'] > }, > total: { > label: 'Debit price', > amount: { > currency: 'USD', > value: '0.50' > } > } > }] > }.show(); Done. https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestModifierTest.java (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestModifierTest.java:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/22 13:17:10, ಠ_ಠ wrote: > Let's try to avoid deleting existing test cases. Done. https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/22 13:17:10, ಠ_ಠ wrote: > 2017 Done. https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... File chrome/test/data/payments/modifier.js (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... chrome/test/data/payments/modifier.js:1: /* On 2017/05/22 13:17:10, ಠ_ಠ wrote: > Let's not delete existing test cases. Done. https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... File chrome/test/data/payments/payment_request_modifier_test.html (left): https://codereview.chromium.org/2885443003/diff/240001/chrome/test/data/payme... chrome/test/data/payments/payment_request_modifier_test.html:1: <!DOCTYPE html> On 2017/05/22 13:17:10, ಠ_ಠ wrote: > Let's not delete. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2885443003/#ps280001 (title: "fix 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": 280001, "attempt_start_ts": 1495561938309150, "parent_rev": "8402d3c5bc13e018fa75eba650ed881755e0223b", "commit_rev": "ffe8d3c802c270b644c4ebc0ce32d03ba0a7882f"}
Message was sent while issue was closed.
Description was changed from ========== enable modifiers for native apps 1. set web-payments-modifiers to enabled by default 2. implement the serialization of modifiers to native apps through intent 3. integration test for app modifiers BUG=719666 ========== to ========== enable modifiers for native apps 1. set web-payments-modifiers to enabled by default 2. implement the serialization of modifiers to native apps through intent 3. integration test for app modifiers BUG=719666 Review-Url: https://codereview.chromium.org/2885443003 Cr-Commit-Position: refs/heads/master@{#474022} Committed: https://chromium.googlesource.com/chromium/src/+/ffe8d3c802c270b644c4ebc0ce32... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/ffe8d3c802c270b644c4ebc0ce32... |