Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(43)

Issue 2885443003: enable modifiers for native apps (Closed)

Created:
6 months ago by wuandy1
Modified:
5 months, 4 weeks 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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -2 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +51 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +1 line, -1 line 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +110 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestCommon.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +25 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java View 1 2 3 4 5 6 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_feature_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
A chrome/test/data/payments/bobpay_and_basic_card_with_modifiers.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +50 lines, -0 lines 0 comments Download
A chrome/test/data/payments/payment_request_bobpay_and_basic_card_with_modifiers_test.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 69 (53 generated)
gogerald1
One quick response before continue reviewing this CL, https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode499 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private ...
6 months ago (2017-05-16 20:36:59 UTC) #15
please use gerrit instead
Sorry for the drive-by! https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode499 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private static ArrayList<String> buildModifierList( Also, ...
6 months ago (2017-05-17 13:02:32 UTC) #16
wuandy1
https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode499 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: ...
6 months ago (2017-05-17 16:24:27 UTC) #19
please use gerrit instead
Please loop me back in when Ganggui's review is done.
6 months ago (2017-05-17 16:31:07 UTC) #21
gogerald1
https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode83 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:83: private static final String EXTRA_MODIFIERS = "modifiers"; You can ...
6 months ago (2017-05-17 19:11:47 UTC) #28
wuandy1
https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode83 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:83: private static final String EXTRA_MODIFIERS = "modifiers"; On 2017/05/17 ...
6 months ago (2017-05-18 20:12:30 UTC) #31
gogerald1
thanks, few more comments https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/100001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode499 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:499: private String buildModifierListString(Collection<PaymentDetailsModifier> modifiers) { ...
6 months ago (2017-05-18 21:57:34 UTC) #32
wuandy1
https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode342 chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java:342: private Bundle buildExtras(@Nullable String id, @Nullable String merchantName, String ...
6 months ago (2017-05-19 01:59:58 UTC) #35
wuandy1
updated with latest.
6 months ago (2017-05-19 18:59:20 UTC) #47
gogerald1
nice work, lgtm % comments https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java#newcode67 chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:67: * If Bob Pay ...
6 months ago (2017-05-19 19:22:49 UTC) #48
wuandy1
https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java (right): https://codereview.chromium.org/2885443003/diff/220001/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java#newcode67 chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndBasicCardWithModifiersTest.java:67: * If Bob Pay has instruments, verify modifier for ...
6 months ago (2017-05-19 20:03:50 UTC) #49
please use gerrit instead
Great job! Please post some screenshots and/or videos of the feature in action. https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File ...
5 months, 4 weeks ago (2017-05-22 13:17:10 UTC) #55
wuandy1
https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java (right): https://codereview.chromium.org/2885443003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java#newcode530 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, ...
5 months, 4 weeks ago (2017-05-23 15:46:58 UTC) #59
please use gerrit instead
lgtm
5 months, 4 weeks ago (2017-05-23 15:51:35 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2885443003/280001
5 months, 4 weeks ago (2017-05-23 17:52:55 UTC) #66
commit-bot: I haz the power
5 months, 4 weeks ago (2017-05-23 20:12:05 UTC) #69
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/ffe8d3c802c270b644c4ebc0ce32...

Powered by Google App Engine
This is Rietveld efc10ee0f