Closed Bug 898499 Opened 6 years ago Closed 6 years ago

Support mozPay API on desktop webapprt

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: marco, Assigned: marco)

References

Details

(Whiteboard: DesktopWebRT2)

Attachments

(1 file, 2 obsolete files)

No description provided.
Whiteboard: DesktopWebRT2
Attached patch mozpay_desktop (obsolete) — Splinter Review
I'd just like some feedback on the overall approach, please don't mind the dust left behind for testing.
Attachment #782815 - Flags: feedback?(ferjmoreno)
Priority: -- → P1
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
Attachment #782815 - Flags: feedback?(ferjmoreno) → feedback+
(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.
Attached patch Patch (obsolete) — Splinter Review
Myk, I think we can reuse the webapp.xul window until we have a better UI, is it ok for you?
Attachment #782815 - Attachment is obsolete: true
Attachment #799759 - Flags: review?(ferjmoreno)
Attachment #799759 - Flags: feedback?(myk)
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.
Attachment #799759 - Flags: feedback?(myk) → feedback+
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.
Attachment #799759 - Flags: review?(ferjmoreno) → review+
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)
Flags: needinfo?(myk)
Flags: needinfo?(ferjmoreno)
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.
Flags: needinfo?(ferjmoreno) → needinfo?(jonas)
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).
Flags: needinfo?(myk)
Attached patch PatchSplinter Review
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.
Attachment #799759 - Attachment is obsolete: true
Attachment #811012 - Flags: review?(ferjmoreno)
Flags: needinfo?(jonas)
Attachment #811012 - Flags: review?(ferjmoreno) → review+
Keywords: checkin-needed
Depends on: 921690
Filed bug 921690 to add a simple test for this.
https://hg.mozilla.org/mozilla-central/rev/44558c02904e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
The bug for the test landed, should we mark this |in-testsuite+| or |in-testsuite-|?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Flags: in-testsuite?
Flags: in-testsuite+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.