898499 - Support mozPay API on desktop webapprt

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Comment 1

[Marco Castelluccio [:marco]]

Attachment: mozpay_desktop

I'd just like some feedback on the overall approach, please don't mind the dust left behind for testing.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 782815 [details] [diff] [review]
mozpay_desktop

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

Sorry for the huge delay on this. I've been on PTO during August.

This looks great Marco! :)

I've only added a few comments.

::: webapprt/Makefile.in
@@ +20,5 @@
>    CommandLineHandler.js \
>    ContentPermission.js \
>    ContentPolicy.js \
>    DirectoryProvider.js \
> +  PaymentsHandler.js \

I would name this "PaymentGlue", "PaymentUI" or "PaymentUIGlue"

::: webapprt/PaymentsHandler.js
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import('resource://gre/modules/Payment.jsm');
> +Cu.import('resource://gre/modules/Timer.jsm');

Is this used?

@@ +31,5 @@
> +          });
> +        };
> +}
> +
> +

nit: no need for extra line break

@@ +55,5 @@
> +      }
> +    };
> +  },
> +
> +  confirmPaymentRequest: function confirmPaymentRequest(aRequestId,

If I am not wrong, there is no need to name function properties anymore in new files. We have better logging now and don't need them for debugging :).

@@ +59,5 @@
> +  confirmPaymentRequest: function confirmPaymentRequest(aRequestId,
> +                                                        aRequests,
> +                                                        aSuccessCb,
> +                                                        aErrorCb) {
> +    let items = [];

nit: move this declaration after checking if there is only one provider, please.

@@ +71,5 @@
> +    // Otherwise, let the user select a payment provider from a list.
> +    for (let i = 0; i < aRequests.length; i++) {
> +      let request = aRequests[i].wrappedJSObject;
> +      let requestText = request.providerName;
> +      if (request.productPrice) {

if (request.productPrice && Array.isArray(request.productPrice))

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

Instead of getting the first product price, it would be great if we could guess the user's currency, probably based on her locale, mcc (probably not available on desktop :\) or other info that we can get. It's ok to do it in a follow up bug though. Add a comment here in that case, please.

@@ +80,5 @@
> +    }
> +
> +    let selected = {};
> +
> +    let result = Services.prompt.select(null, "Payment", "Which payment provider do you want to use?", items.length, items, selected);

Did you get any feedback from UX about the UI and the strings shown?

I'm not familiar about how l10n works in desktop, so maybe you can ignore me, but this doesn't seems to be a localized string.

Also, in general, lines with less than 80 chars, please :)

@@ +93,5 @@
> +                                            aPaymentFlowInfo,
> +                                            aErrorCb) {
> +    // 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

Remove this comment please. I don't think we ever do POST requests and BlueVia has no payment provider anymore.

@@ +98,5 @@
> +    let win = Services.ww.openWindow(null,
> +                                     "chrome://webapprt/content/webapp.xul",
> +                                     "_blank",
> +                                     "chrome,dialog=no,resizable,scrollbars,centerscreen",
> +                                     null);

Dumb question: is this window showing the URL bar, a padlock for https address, etc.?

I am asking this because of the need of a trustworthy UI to embed the payment flow. We have several issues with this in B2G. I'm just wondering if we have a better scenario in desktop.

@@ +105,5 @@
> +    win.addEventListener("load", function loadPaymentShim() {
> +      let browserElement = win.document.getElementById("content");
> +      browserElement.setAttribute("src", aPaymentFlowInfo.uri + aPaymentFlowInfo.jwt);
> +
> +      browserElement.addEventListener("load", function onLoad() {

Why "load"? Can we use "DOMWindowCreated" instead? We had problems because of this before. Check bug 843309

@@ +113,5 @@
> +        let pf = paymentFailed(aRequestId);
> +
> +        contentWindow.wrappedJSObject.paymentSuccess = ps;
> +        contentWindow.wrappedJSObject.paymentFailed = pf;
> +        contentWindow.wrappedJSObject.mozPaymentProvider = {

You only need to expose the mozPaymentProvider API. No need to expose paymentSuccess and paymentFailed in the global window anymore.

@@ +125,5 @@
> +      }, true);
> +    });
> +
> +    // Fail the payment if the window is closed on its own
> +    /*win.addEventListener("unload", function paymentCanceled() {

I guess this will not be commented in the future :)

@@ +130,5 @@
> +      paymentFailed(aRequestId)();
> +    });*/
> +
> +    // Store a reference to the window so that we can close it when the payment succeeds or fails.
> +    paymentWindows[aRequestId] = win;

This would probably look better before line 105

@@ +137,5 @@
> +  cleanup: function cleanup() {
> +    // Nothing to do here.
> +  },
> +
> +  classID: Components.ID("{ede1124f-72e8-4a31-9567-3270d46f21fb}"), 

nit: white space

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo, please) from comment #2)
> @@ +80,5 @@
> Did you get any feedback from UX about the UI and the strings shown?
>
> Dumb question: is this window showing the URL bar, a padlock for https
> address, etc.?
> 
> I am asking this because of the need of a trustworthy UI to embed the
> payment flow. We have several issues with this in B2G. I'm just wondering if
> we have a better scenario in desktop.

At the moment we don't have a UX contact for the webapp runtime :(
The window is just a dialog window with a <browser> element where we load the payment provider url.
(Note that I'll create a new dialog window and not reuse the webapp.xul dialog, this was just to test. But we'll still have a non-trustworthy UI until someone from UX can help us).

> @@ +125,5 @@
> > +      }, true);
> > +    });
> > +
> > +    // Fail the payment if the window is closed on its own
> > +    /*win.addEventListener("unload", function paymentCanceled() {
> 
> I guess this will not be commented in the future :)

For some reason the "unload" event was triggered when the window was opened...
I've replaced that using nsIWindowWatcher::registerNotification.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

Myk, I think we can reuse the webapp.xul window until we have a better UI, is it ok for you?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

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

(In reply to Marco Castelluccio [:marco] from comment #4)
> Myk, I think we can reuse the webapp.xul window until we have a better UI,
> is it ok for you?

Since we use webapp.xul for both app content and third-party content (like identity provider interfaces), it seems reasonable to use it for payment content too.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

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

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

Thanks Marco! Looks great.

In general, one minor nit about lines longer than 80 chars.

::: webapprt/PaymentUIGlue.js
@@ +83,5 @@
> +      aErrorCb.onresult(aRequestId, "USER_CANCELLED");
> +    }
> +  },
> +
> +  showPaymentFlow: function showPaymentFlow(aRequestId,

nit: no need to name the function.

Comment 7

[Marco Castelluccio [:marco]]

We've a small problem here, MOZ_PAY in confvars.sh enables the API for both Firefox and the Webapp Runtime, but we don't really support it in Firefox.
So the tests under dom/payments are failing for Firefox Desktop.

I think this would be the first case of an API implemented in the Webapp Runtime and not in Firefox, I don't know what's the best way to proceed.

Can we just disable those tests? (and maybe add new payments tests to our webapp runtime testing infrastructure)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Ugh, you are right.

I don't think we should just disable the tests.

I am not familiar enough with webapprt, but if there is no way to expose the API only to it, we should at least hide it under a preference.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Agreed, we should get the tests working.  And find a way to selectively enable mozPay only for webapprt.

I'm not sure how to do the former, but it looks like they're xpcshell tests, so they don't depend on browser chrome.  What specifically fails?

As for the latter, perhaps we could switch to a runtime flag to determine whether or not mozPay is enabled (like the dom.w3c_touch_events.enabled pref, which enables touch events at runtime).

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch

It was just a packaging problem, I've added the necessary files to package-manifest.in and the test failures are gone.
I've put the API behind a pref anyway.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/44558c02904e

Should this have tests?

Comment 12

[Marco Castelluccio [:marco]]

Filed bug 921690 to add a simple test for this.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44558c02904e

Comment 14

[Marco Castelluccio [:marco]]

The bug for the test landed, should we mark this |in-testsuite+| or |in-testsuite-|?