813756 - Payments frontend for Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Wesley Johnston (:wesj)]

I was told it wasn't interoperable at first, but fabrice tells me we just need a small shim to use the b2g payments backend. If so we should hook it up!

cc'ing fabrice to point us to the code we can use.

Comment 1

[Caitlin Galimidi [:c8o]]

hey there wes j - is this a dupe of 811866?

Comment 2

[Wesley Johnston (:wesj)]

This is about building that native api (not the shim). Fabrice mentioned that this shouldn't require much in a meeting today. Hoping he can point us to some code here.

Comment 3

[Jason Smith [:jsmith]]

putting the original track flag back on that we were originally using - if this changes, then I'll flip the flag

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Not blocking since we have the shim approach

Comment 5

[Wesley Johnston (:wesj)]

Fernando provided me with a whole boatload of info on how to implement this. Dumping it here in case someone else gets to it before me:

I was planning to write something about this, but in the meantime a high level overview of the flow would be like:

- An application requests a payment via navigator.mozPay([JWT1, ..., JWTn]), where JWTx contains the base64-encoded and digitally signed information for a specific payment provider request, including the 'typ' parameter that needs to match with one of the allowed (via preferences) payment providers. For Firefox OS v1 the only allowed payment provider is the Firefox Marketplace [1], so far.

- The code for the mozPay initial call lives at [2] and it is executed in the child process. There we mostly create a request ID, check if the caller application is in the foreground and send a message to the parent to start doing the real thing.

- The message from the child is handled at [3]. That code has enough comments to understand what is happening there, but we mainly process the JWTs to get the list of valid providers so we can ask the user which one she prefers for this specific payment. To do that we use the nsIPaymentUIGlue.confirmPaymentRequest implementation [5 for the B2G case] that gets as parameters:
       1. the request ID corresponding to the current DOMRequest
       2. the list of valid providers
       3. Two callbacks (success and failure cases, I'll be explaining the success one)

- The following part is specific to B2G, but what we do is that we request [5] the Gaia side to open a trusted UI with the list of providers, so the user can confirm and choose one. Let me know if you need more details on this part.

- Once we get confirmation from the user, we execute the successful callback [6], which takes the flow to [7].

- After that, we again use the nsIPaymentUIGlue implementation, in this case the showPaymentFlow() [8] function, that gets as parameters:
       1. the request ID.
       2. an instance of nsIPaymentFlowInfo [9]
       3. a callback for the failure case.

  this function basically requests to the Gaia side to open an iframe and load the payment flow with the provided URL and JWT.

- Once Gaia opens the iframe it sends it back to the chrome side [10] so we can load [11] a content script [12] in the payment flow.

- The content script contains two functions that are injected [13] in the payment flow and allow it to complete the payment process by sending a message to the child process [14] after asking Gaia to close the UI [15]. Note that these functions are called within the payment flow by the payment provider.

- The flow is completed by triggering the DOMRequest.onsuccess [16] callback from the child.

I also added some information to [17] some time ago. It seems that it is not completely updated, but it contains the basic idea. I'll make sure to update it as soon as I can.

[1] http://mxr.mozilla.org/gaia/source/build/payment-prefs.js#4
[2] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.js#46
[3] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#67
[4] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#112
[5] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#79
[6] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#71
[7] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#124
[8] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#335
[9] http://mxr.mozilla.org/mozilla-central/source/dom/payment/interfaces/nsIPaymentFlowInfo.idl
[10] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#124
[11] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#129
[12] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js
[13] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js#95
[14] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js#34
[15] http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js#30
[16] http://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.js#98
[17] https://wiki.mozilla.org/WebAPI/WebPayment#Current_implementation

Comment 6

[Jason Smith [:jsmith]]

Bumping up to P1 given that the original reason we made this P2 was that the hosted shim was an option that made this non-blocking, but we just made that a WONTFIX. That now bumps this up blocking.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

With a set of fake prefs this seems to work fine on Fernando's test page here: http://ferjm.github.io/simplepayprovider/page1.html

pref("dom.payment.provider.1.name", "fakeprovider");
pref("dom.payment.provider.1.description", "fakeprovider");
pref("dom.payment.provider.1.uri", "http://ferjm.github.io/simplepayprovider/page1.html?req=");
pref("dom.payment.provider.1.type", "ferjm/payment/tests");
pref("dom.payment.provider.1.requestMethod", "GET");
pref("dom.payment.skipHTTPSCheck", true);

Much simpler than B2G's implementation since we can remove all the e10s stuff. But this method injecting seems fragile and dangerous Also, I hate that we have to drag Payment.jsm into browser.js, but the b2g implementation seems to depend on it in their shell.js: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#17 :( I'd like to look more and see if we can remove both of these things... later. Seems like a big project.

Fernando, adding you for feedback, but I think it would be good to get at least a high level review of this from you or kumar? and mark can review the fennec specific code.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 740020 [details] [diff] [review]
Patch

Whoops. Forgot I need to localize this. Will finish up soon. Feedback still appreciated.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch

Localized the string.

Comment 10

[Jason Smith [:jsmith]]

Hmm...I don't know if we should land the payment provider preferences by default on in this patch.

mozpay's payment provider currently operates in the context of B2G, including the underling payment provider being used here - Bango. The payment provider isn't expecting to take payments from FxAndroid. I can go into more details here, but it would have to be done over email.

But my general suggestion would be to not land with those preferences in the build just yet.

Comment 11

[Jason Smith [:jsmith]]

A try build would also be very useful. I could take a look and quickly do a behavior comparison on FxAndroid vs. B2G to give you some feedback.

Comment 12

[Wesley Johnston (:wesj)]

I talked to David on Friday and we have permission to use the same payment provider on Android that we're using for B2G. But yes, it would be good to have some more confirmation before those land.

Comment 13

[Wesley Johnston (:wesj)]

Builds are here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9ba442e9ca28/try-android/

or

http://people.mozilla.com/~wjohnston/payments.apk

Comment 14

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #12)
> I talked to David on Friday and we have permission to use the same payment
> provider on Android that we're using for B2G. But yes, it would be good to
> have some more confirmation before those land.

Right. From a quality perspective, I'd have differing view here though - I'd lean away from landing those preferences until we know the marketplace payment provider is ready for basic consumption on FxAndroid. Given that Krupa owns testing of the marketplace payment provider, let's get her opinion here.

Comment 15

[krupa raj[:krupa]]

David Bialer spoke to Martin from Bango and we think it should work seamlessly with Android as well (at least in theory)

We can land it and check if it works. If not we can just revert it.

Comment 16

[Jason Smith [:jsmith]]

(In reply to krupa raj 82[:krupa] from comment #15)
> David Bialer spoke to Martin from Bango and we think it should work
> seamlessly with Android as well (at least in theory)
> 
> We can land it and check if it works. If not we can just revert it.

Fair enough, that sounds like an okay plan then. Keep the prefs and back out if we discover if something is severely not working.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 740053 [details] [diff] [review]
Patch

>diff --git a/mobile/android/components/PaymentsUIGlue.js b/mobile/android/components/PaymentsUIGlue.js

Let's call this PaymentsUI.js

>+  confirmPaymentRequest: function confirmPaymentRequest(aRequestId,

>+    var listitems = [];

var -> let

>+    for (var i = 0; i < aRequests.length; i++) {
>+      var request = aRequests[i].wrappedJSObject;
>+      var requestText = request.providerName;

All: var -> let

>+      if (request.productPrice) {
>+        requestText += ' (' + request.productPrice[0].amount + ' ' +
>+                              request.productPrice[0].currency + ')';

nit: Use " instead of '
nit: Will this be good for l10n ?

>+    var result = this.sendMessageToJava({

var -> let

>diff --git a/mobile/android/installer/package-manifest.in b/mobile/android/installer/package-manifest.in
>-@BINPATH@/components/SiteSpecificUserAgent.js

On purpose?

>+
>+

Only one is needed

>diff --git a/mobile/android/locales/en-US/chrome/payments.properties b/mobile/android/locales/en-US/chrome/payments.properties

>+payments.providerdialog.title="Pay with"

Not sure if this is the best label. Maybe "Pay using" ?  Get Ian to take a look?

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 740053 [details] [diff] [review]
Patch

Review of attachment 740053 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff! Thanks Wesley!

r=me with the comments addressed, please.

I would love to see what kind of UI are we creating here. Is this UI trustworthy? I am not sure if in Android we have the same issues that we had for B2G (check Bug 768943 for more info about it).

It would also be great if we could have input from UX.

::: mobile/android/app/mobile.js
@@ +708,5 @@
>  // Enable Web Audio for Firefox for Android in Nightly and Aurora
>  pref("media.webaudio.enabled", true);
>  #endif
>  
> +pref("dom.payment.provider.0.name", "firefoxmarket");

This name is going to be shown to the user, so it needs to be something more like "Firefox Marketplace".

::: mobile/android/components/PaymentsUIGlue.js
@@ +10,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> +                                    "@mozilla.org/childprocessmessagemanager;1",
> +                                    "nsIMessageSender");

nit: indentation

@@ +14,5 @@
> +                                    "nsIMessageSender");
> +
> +function debug (s) {
> +  //dump("-*- PaymentGlue: " + s + "\n");
> +};

It seems that you are not using this function.

@@ +35,5 @@
> +}
> +
> +let paymentTabs = {};
> +
> +function closePaymentTab(id, callback) {

aId, aCallback

@@ +37,5 @@
> +let paymentTabs = {};
> +
> +function closePaymentTab(id, callback) {
> +  if (paymentTabs[id]) {
> +    // We ask the UI to browse to the selected payment flow.

s/browse/close ?

@@ +39,5 @@
> +function closePaymentTab(id, callback) {
> +  if (paymentTabs[id]) {
> +    // We ask the UI to browse to the selected payment flow.
> +    let content = Services.wm.getMostRecentWindow("navigator:browser");
> +    if (content)

nit: use {}, please.

@@ +54,5 @@
> +
> +PaymentUI.prototype = {
> +  get bundle() {
> +    delete this.bundle;
> +    return this.bundle = Services.strings.createBundle("chrome://browser/locale/payments.properties");

nit: 80 chars lines, please.

@@ +58,5 @@
> +    return this.bundle = Services.strings.createBundle("chrome://browser/locale/payments.properties");
> +  },
> +
> +  sendMessageToJava: function(aMsg) {
> +    let data = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge).handleGeckoMessage(JSON.stringify(aMsg));

ditto

@@ +68,5 @@
> +                                                        aSuccessCb,
> +                                                        aErrorCb) {
> +    let _error = this._error(aErrorCb);
> +
> +    var listitems = [];

nit: listItems

@@ +70,5 @@
> +    let _error = this._error(aErrorCb);
> +
> +    var listitems = [];
> +
> +    // If there's only one payment provider that will work, just move on without prompting the user

nit: 80 chars lines, please.

@@ +77,5 @@
> +      return;
> +    }
> +
> +    // Otherwise, let the user select a payment provider from a list
> +    for (var i = 0; i < aRequests.length; i++) {

s/var/let/ in the whole patch, please.

@@ +82,5 @@
> +      var request = aRequests[i].wrappedJSObject;
> +      var requestText = request.providerName;
> +      if (request.productPrice) {
> +        requestText += ' (' + request.productPrice[0].amount + ' ' +
> +                              request.productPrice[0].currency + ')';

In the future we would need to show the user's currency, probably based on her locale, mcc or other info that we can get. Please, file a bug about it and add a TODO including the bug #. We didn't do this for B2G v1, since we are not showing the payment confirmation screen, but we will need to do it soon.

@@ +108,5 @@
> +      _error(aRequestId, "USER_CANCELED");
> +    }
> +  },
> +
> +  _error: function(callback) {

nit: aCallback

@@ +131,5 @@
> +
> +    // TODO: For now, known payment providers (BlueVia and Mozilla Market)
> +    // only accepts the JWT by GET, so we just add it to the URI.
> +    // https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/payment.js
> +    let tab = content.BrowserApp.addTab(aPaymentFlowInfo.uri + "?" + aPaymentFlowInfo.jwt);

I think that you don't need the "?"

@@ +133,5 @@
> +    // only accepts the JWT by GET, so we just add it to the URI.
> +    // https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/payment.js
> +    let tab = content.BrowserApp.addTab(aPaymentFlowInfo.uri + "?" + aPaymentFlowInfo.jwt);
> +
> +    // inject paymentSuccess and paymentFailed methods into the document after its loaded

nit: In general, comments starting with capital letter and "." at the end, please.

@@ +151,5 @@
> +    tab.browser.addEventListener("TabClose", function paymentCanceled() {
> +      paymentFailed(aRequestId)();
> +    });
> +
> +    // store a reference to the tab so that we can close it when the payment succeeds or

suceeds or ...?

@@ +159,5 @@
> +  cleanup: function cleanup() {
> +    // nothing to do here
> +  },
> +
> +  classID: Components.ID("{3c6c9575-f57e-427b-a8aa-57bc3cbff48f}"), 

nit: whitespace

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Fernando Jiménez Moreno [:ferjm] (use needinfo instead of CC, please) from comment #18)

> nit: use {}, please.
> nit: 80 chars lines, please.

Just an FYI. These are two style guidelines that JS code in mobile does not strictly follow. Especially the 80 char limit.

Comment 20

[Wesley Johnston (:wesj)]

to try: https://tbpl.mozilla.org/?tree=Try&rev=10067d759ac3

Comment 21

[Wesley Johnston (:wesj)]

Try failed a test because there are some new interfaces defined. Trying again:
https://tbpl.mozilla.org/?tree=Try&rev=f88c9402eb64

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Test fix

This is a very odd test. It only runs on android AFAICT? Smaug wrote it, so asking to make sure this is ok (a try run looks ok so far...)

https://tbpl.mozilla.org/?tree=Try&rev=f88c9402eb64

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 742108 [details] [diff] [review]
Test fix

test_interfaces.html runs on desktop too and is there to make sure we don't
accidentally add new stuff to the global scope. Nothing odd there.

And it catches a bug here. There certainly shouldn't be
NavigatorPayment thing in the global scope. That interface is just for
adding a new getter to navigator object.
nsIDOMNavigatorPayment interface should be renamed to nsINavigatorPayment.

(Though, looks like NavigatorPayment is implemented in some odd JS way which I'm not familiar with)

Comment 24

[Wesley Johnston (:wesj)]

talking to imelven. cc'ing some security people as well. I'd like to make sure the use of wrappedJSObject to inject methods into content is right? I think there are probably better solutions to this, but it seems like at this point we're kinda stuck with this one (for b2g and forwards compat).

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Test fix

moved interface to nsINavigatorPayment

Comment 26

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cba2c26c95c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/43cccb89e04a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43cccb89e04a
https://hg.mozilla.org/mozilla-central/rev/cba2c26c95c4

Comment 28

[Francesco Lodolo [:flod]]

I stumbled upon this file while checking the entire repo for my locale.

http://hg.mozilla.org/mozilla-central/file/c0f85061c7d3/mobile/android/locales/en-US/chrome/payments.properties

This file is certainly strange: no license header, string is wrapped in quotes while I guess it shouldn't really be (that's a properties file, I don't think the label is meant to have quotes). 

Are both of these errors that need a fix?

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Francesco Lodolo [:flod] from comment #28)

> Are both of these errors that need a fix?

Yeah, you are probably right.

Wesj - Want to clean this up?