Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530793002:
PaymentApp: Allow multiple payment method names for one instrument. (Closed)
DescriptionPaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
Committed: https://crrev.com/0611804b7881c52ae3637bcaaaf4831f30363adb
Cr-Commit-Position: refs/heads/master@{#435587}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address Rouslan's comments #
Total comments: 1
Patch Set 3 : Remove a superfluous word from a comment #
Dependent Patchsets: Messages
Total messages: 33 (20 generated)
The CQ bit was checked by tommyt@opera.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...
Description was changed from
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=661608
==========
to
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=661608
==========
tommyt@opera.com changed reviewers: + jinho.bang@samsung.com, rouslan@chromium.org
Here's a small change in preparation for handling third party apps in the Payment Request UI. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2016/11/24 07:19:51, tommyt wrote: > Here's a small change in preparation for handling third party apps in the > Payment Request UI. > > PTAL I'm not expert but this looks good to me.
Tommy, our goal is to use only Android native apps on Android. I've been circulating a design doc in W3C for a while and finally sent it out to chromium-dev@ today: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/fsslHD1Gf88/K2Kpi... The Android design is simplified in the first iteration to increase our velocity. For example, there're no payment options and only one payment method per app. This can be expanded in the future, but let's implement this first iteration first. I've demoed the integration at Lisbon TPAC based on this code: http://crrev.com/2341203002 Samsung folks have picked it up and ran with it as well, e.g.: http://crrev.com/2507223002 If you are interested in web-based payment apps more, we plan to use that spec on desktop only. Desktop work has begun. You can see the progress after enabling this flag: chrome://flags/#enable-experimental-web-platform-features And opening an example page: https://googlechrome.github.io/samples/paymentrequest/credit-cards/ Let's not land this patch. To avoid wasted effort in the future, let's coordinate via either email or http://crbug.com.
On 2016/11/28 17:51:32, rouslan wrote: > If you are interested in web-based payment apps more, we plan to use that spec > on desktop only. Desktop work has begun. You can see the progress after enabling > this flag: > Hi rouslan@, I have a question. You said "you plan to use that spec on desktop only". Does it mean that the priority of web-based approach is just lower than native approach? or there is no plan to support it forever on Android? If Chrome doesn't support the web-based approach on Android, the spec's worth might be low sadly. Thanks.
Hi Rouslan! I was aware of the Android native apps design, and your intention to focus all your effort on the native apps implementation. I was not, however, aware that you were not intending to support the service worker based payment apps at all on Android. Like Jinho said, that would be a bit of a setback to the Payment Apps API specification, and I think a lot of people (myself included) would be disappointed by this decision. Would you be open to support the Payment Apps API on Chrome for Android if it was done less disruptively? An initial implementation could certainly be aligned with your goals to provide a simplified design. I will definitely check out the progress on desktop. For the sake of initial testing of the API together with app implementers, this might be a good approach. However, in my opinion, Web Payments is more relevant for mobile platforms. Part of the motivation behind Web Payments is that online payments are too inconvenient on mobile devices, and that's the main problem I'm trying to solve. Desktop, for me, is a lower priority.
Sorry to have cut off your efforts so rashly. The priority for us right now is native payment apps, but we'll be looking into web-based in the future. So, please, if you have time, try to look into supporting native Android payment apps first. As for this patch, we should land it after addressing the review comments. Overall comment: I'm not convinced that PaymentInstrument.invokePayment() is better than PaymentIntrument.getInstrumentDetails(). If you insist on changing the name, do you think "invokePaymentApp()" is a more clear name for what you're trying to convey? https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:26: * Called by the payment instrument to let the UI that there will be a delay in Called by the payment instrument to let Chrome know that the payment app's UI is now hidden, but the payment instrument has not been returned yet. This is a good time to show a "loading" progress indicator UI. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:29: void onInstrumentDetailsLoading(); onInstrumentdetailsLoadingWithoutUI(); https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:59: * Invoke a payment app using this payment instrument. Invoke the payment app to retrieve this payment instrument. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:72: public abstract void invokePayment(String merchantName, String origin, PaymentItem total, invokePaymentApp(). https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:942: methodData, this); Wrap methodData in Collections.unmodifiableMap(). https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1076: // instrument is in mMethodData, add this instrument. Your approach is 9 lines long. Here's a shorter way to write the same thing: Set<String> instrumentMethodNames = instrument.getInstrumentMethodNames(); instrumentMethodNames.retainAll(mMethodData.keySet()); if (!instrumentMethodNames.isEmpty()) { addPendingInstrument(instrument); } else { instrument.dismissInstrument(); } This takes up 7 lines, saving you 2 lines of code. The additional comments seem to be not necessary either.
Hey Rouslan! I'm glad to hear that there's an opening for service worker based payment apps in Chrome after all :) It's nearing the end of my work day here, but I'll address your review comments first thing tomorrow morning.
On 2016/11/29 14:17:18, rouslan wrote:
> Overall comment: I'm not convinced that PaymentInstrument.invokePayment() is
> better than PaymentIntrument.getInstrumentDetails(). If you insist on changing
> the name, do you think "invokePaymentApp()" is a more clear name for what
you're
> trying to convey?
Well my rationale was that now that we're introducing payment apps (both
service-worker based and native), this method can potentially trigger lot more
than just fetching some data, so I wanted to give it a name that didn't make it
look like a regular getter function. I wanted to use the word "invoke", since
that's the term used in section 10 of the payment apps specification:
https://w3c.github.io/webpayments-payment-apps-api/#invocation
"invokePaymentApp()" is probably a better name, and if that's alright with you,
I say we run with that :)
The CQ bit was checked by tommyt@opera.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...
Thanks for your feedback Rouslan. I've tried to address your comments in the latest patchset. PTAL https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:26: * Called by the payment instrument to let the UI that there will be a delay in On 2016/11/29 14:17:18, rouslan wrote: > Called by the payment instrument to let Chrome know that the payment app's UI is > now hidden, but the payment instrument has not been returned yet. This is a good > time to show a "loading" progress indicator UI. Done. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:29: void onInstrumentDetailsLoading(); On 2016/11/29 14:17:18, rouslan wrote: > onInstrumentdetailsLoadingWithoutUI(); Done. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:59: * Invoke a payment app using this payment instrument. On 2016/11/29 14:17:18, rouslan wrote: > Invoke the payment app to retrieve this payment instrument. How about if I take a step back and use something closer to the original comment?: "Invoke the payment app to retrieve the instrument details." https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:72: public abstract void invokePayment(String merchantName, String origin, PaymentItem total, On 2016/11/29 14:17:18, rouslan wrote: > invokePaymentApp(). Done. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:942: methodData, this); On 2016/11/29 14:17:18, rouslan wrote: > Wrap methodData in Collections.unmodifiableMap(). Done. https://codereview.chromium.org/2530793002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1076: // instrument is in mMethodData, add this instrument. On 2016/11/29 14:17:18, rouslan wrote: > Your approach is 9 lines long. Here's a shorter way to write the same thing: > > Set<String> instrumentMethodNames = instrument.getInstrumentMethodNames(); > instrumentMethodNames.retainAll(mMethodData.keySet()); > if (!instrumentMethodNames.isEmpty()) { > addPendingInstrument(instrument); > } else { > instrument.dismissInstrument(); > } > > This takes up 7 lines, saving you 2 lines of code. > > The additional comments seem to be not necessary either. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=661608
==========
to
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
==========
lgtm % nit https://codereview.chromium.org/2530793002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2530793002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:69: * e.g., whether the app should be invoked in test or production key, a nit: remove the word "key" here.
The CQ bit was checked by tommyt@opera.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.
The CQ bit was checked by tommyt@opera.com
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/2530793002/#ps40001 (title: "Remove a superfluous word from a comment")
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": 40001, "attempt_start_ts": 1480582254005930,
"parent_rev": "58d46952844cb5f9f2f18c5a97d785aa9a480413", "commit_rev":
"ebc4d8e11148f43ee09aef3876977895491813b2"}
Message was sent while issue was closed.
Description was changed from
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
==========
to
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
==========
to
==========
PaymentApp: Allow multiple payment method names for one instrument.
This changes the name and signature of the
PaymentInstrument.getInstrumentMethodName method to:
Set<String> getInstrumentMethodNames()
This is to match the "enabledMethods" field in the PaymentAppOption
dictionary in the Payment Apps specification, which is defined to be a
sequence of strings.
See: https://w3c.github.io/webpayments-payment-apps-api/#payment-app-options
I also change the name of PaymentInstrument.getInstrumentDetails to
"invokePayment" in order to convey better that this is where the payment
method specific stuff happens. For a Web Based Payment App, this method
is an appropriate point to launch the payment request event into the
service worker.
BUG=669876
Committed: https://crrev.com/0611804b7881c52ae3637bcaaaf4831f30363adb
Cr-Commit-Position: refs/heads/master@{#435587}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0611804b7881c52ae3637bcaaaf4831f30363adb Cr-Commit-Position: refs/heads/master@{#435587} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
