Last Comment Bug 767818 - Implement navigator.mozPay
: Implement navigator.mozPay
Status: VERIFIED FIXED
[LOE:M]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla18
Assigned To: Fernando Jiménez Moreno [:ferjm] (PTO until August)
: Jason Smith [:jsmith]
Mentors:
https://wiki.mozilla.org/WebAPI/WebPa...
Depends on: 764966 768943 776417 779888 793329 793811 795854 797300 804080 804143 809219
Blocks: Apps-Dev-Doc-Needed 766199 basecamp-payments
  Show dependency treegraph
 
Reported: 2012-06-24 11:46 PDT by Fernando Jiménez Moreno [:ferjm] (PTO until August)
Modified: 2014-10-11 08:18 PDT (History)
36 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
WIP (12.26 KB, patch)
2012-06-24 11:46 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP (16.00 KB, patch)
2012-06-25 10:57 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP (23.48 KB, patch)
2012-06-30 02:25 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Gaia-WIP (1.59 KB, patch)
2012-06-30 02:37 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP (28.94 KB, patch)
2012-07-04 11:05 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 1: IDL v1 (1.04 KB, patch)
2012-07-06 09:53 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' v1 (7.80 KB, patch)
2012-07-06 09:54 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v1 (20.80 KB, patch)
2012-07-06 09:55 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v2 (22.28 KB, patch)
2012-07-10 13:21 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Gaia-WIP (6.83 KB, patch)
2012-07-10 13:23 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v3 (22.49 KB, patch)
2012-07-11 11:03 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
BlueVia user stories related to payments (1.23 KB, text/plain)
2012-07-13 12:34 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details
Part 1: IDL v1 (1.06 KB, patch)
2012-07-21 15:17 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' v2 (10.69 KB, patch)
2012-07-21 15:19 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v4 (26.10 KB, patch)
2012-07-21 15:21 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v5 (26.44 KB, patch)
2012-07-23 05:42 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' v3 (10.21 KB, patch)
2012-07-24 11:47 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 3: B2G implementation v5 (26.80 KB, patch)
2012-07-24 11:48 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: B2G implementation v5 (26.75 KB, patch)
2012-07-24 11:55 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Test page (4.66 KB, text/html)
2012-07-26 08:56 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details
Part 3: B2G implementation v5 (26.91 KB, patch)
2012-07-26 09:00 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' v4 (9.93 KB, patch)
2012-07-30 05:01 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
jst: review-
Details | Diff | Splinter Review
Part 3: B2G implementation v6 (26.20 KB, patch)
2012-07-30 05:05 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' v5 (3.50 KB, patch)
2012-07-31 04:07 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
jst: review+
Details | Diff | Splinter Review
Part 3: B2G implementation v6 (25.92 KB, patch)
2012-07-31 04:08 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 1: IDLs v3 (2.64 KB, patch)
2012-08-02 05:48 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (16.21 KB, patch)
2012-08-02 05:49 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review+
Details | Diff | Splinter Review
Part 4: B2G implementation (14.86 KB, patch)
2012-08-02 05:50 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 1: IDLs v3 (2.84 KB, patch)
2012-08-03 04:03 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (15.62 KB, patch)
2012-08-03 04:05 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (15.12 KB, patch)
2012-08-06 03:53 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.61 KB, patch)
2012-08-06 03:55 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Payment providers data (670 bytes, application/json)
2012-08-06 03:59 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details
Part 4: B2G implementation (13.61 KB, patch)
2012-08-06 04:05 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (13.69 KB, patch)
2012-08-07 10:14 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 4: B2G implementation (13.69 KB, patch)
2012-08-07 10:15 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Payment providers data (993 bytes, text/plain)
2012-08-07 10:26 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details
Part 4: B2G implementation (14.08 KB, patch)
2012-08-09 04:38 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (15.62 KB, patch)
2012-08-09 08:08 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 2: Expose 'pay' function to 'navigator' (2.90 KB, patch)
2012-08-10 05:40 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
jst: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (15.62 KB, patch)
2012-08-10 05:43 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 4: B2G implementation (13.42 KB, patch)
2012-08-10 05:44 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 4: B2G implementation (13.46 KB, patch)
2012-08-10 05:55 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (15.62 KB, patch)
2012-08-12 09:55 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
ferjmoreno: review+
Details | Diff | Splinter Review
Part 1: IDLs (4.48 KB, patch)
2012-08-17 21:44 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (19.63 KB, patch)
2012-08-17 21:45 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.51 KB, patch)
2012-08-17 21:47 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 1: IDLs (4.42 KB, patch)
2012-08-18 07:56 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (19.65 KB, patch)
2012-08-18 07:58 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 1: IDLs (4.79 KB, patch)
2012-08-20 15:40 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (21.44 KB, patch)
2012-08-20 15:41 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.52 KB, patch)
2012-08-20 15:41 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 3: DOM implementation (21.70 KB, patch)
2012-08-22 12:28 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 4: B2G implementation (13.14 KB, patch)
2012-08-23 03:01 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.97 KB, patch)
2012-08-23 08:54 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review-
Details | Diff | Splinter Review
Part 1: IDLs (5.07 KB, patch)
2012-08-28 11:07 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
jonas: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (24.12 KB, patch)
2012-08-28 11:10 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.67 KB, patch)
2012-08-28 11:11 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.73 KB, patch)
2012-08-28 14:33 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
Part 4: B2G implementation (13.72 KB, patch)
2012-08-28 18:35 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review+
Details | Diff | Splinter Review
Part 3: DOM implementation (23.53 KB, patch)
2012-08-29 05:50 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
fabrice: review+
Details | Diff | Splinter Review

Description Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-06-24 11:46:33 PDT
Created attachment 636175 [details] [diff] [review]
WIP

Implement navigator.pay based on https://docs.google.com/document/d/1NLKbHVPQXa9uvDBC3cfgOD7sIrtIxi0qDoXMQrxcCsI/edit
Comment 1 Andreas Gal :gal 2012-06-24 12:27:18 PDT
Please track as P1 blocker
Comment 2 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-06-25 10:57:34 PDT
Created attachment 636390 [details] [diff] [review]
WIP
Comment 3 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-06-30 02:25:15 PDT
Created attachment 638087 [details] [diff] [review]
WIP

WIP updated with most of the DOMRequest functionality done. Tested with a fake payment processor. It requires some modifications in Gaia.
Comment 4 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-06-30 02:37:30 PDT
Created attachment 638089 [details] [diff] [review]
Gaia-WIP

This patch allows Gaia to create a system iframe on chrome demand
Comment 5 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-04 11:05:43 PDT
Created attachment 639142 [details] [diff] [review]
WIP

WIP updated with payment processors registered as preferences and endpoint selection according to jwt type.
Comment 6 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-06 09:53:28 PDT
Created attachment 639705 [details] [diff] [review]
Part 1: IDL v1

This IDL exposes the 'navigator.pay' function. I would appreciate a lot if Jonas could give me feedback about this :)

Despite we are focusing on one payment provider on each call of navigator.pay for v1 (a single JWT instead of an array of JWTs), I opted for using a jsval instead of a single DOMString for the JWT parameter (I'll remove the TODO comment before landing this in m-c), so we won't need to modify the interface once we support multiple payment providers for one payment call. Is that ok for you Jonas? Should I change it for a single DOMString?

Also I am not sure if it would be better to call this function 'mozPay()' instead of 'pay()'.
Comment 7 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-06 09:54:28 PDT
Created attachment 639706 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v1
Comment 8 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-06 09:55:24 PDT
Created attachment 639707 [details] [diff] [review]
Part 3: B2G implementation v1
Comment 9 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-10 13:21:02 PDT
Created attachment 640743 [details] [diff] [review]
Part 3: B2G implementation v2

B2G implementation updated to support Gaia system dialogs as proposed in Bug 768943.
Comment 10 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-10 13:23:21 PDT
Created attachment 640744 [details] [diff] [review]
Gaia-WIP

Gaia bits to support system dialogs as proposed in Bug 768943. A pull request to Gaia will be sent with this content.
Comment 11 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-11 11:03:09 PDT
Created attachment 641119 [details] [diff] [review]
Part 3: B2G implementation v3

B2G implementation updated with "system dialog" renamed to "trusted dialog" as per Gaia PR feedback and some bug fixing and security checks done.
Comment 12 Kumar McMillan [:kumar] (needinfo all the things) 2012-07-11 11:04:22 PDT
Is it possible to bring navigator.pay() in sync with mozmarket.buy() ? The latest API looks like this:

var request = mozmarket.buy(onPaySuccess, onPayFailure);  
request.sign(signedRequest);

More info: https://developer.mozilla.org/en/Apps/In-app_payments

This change was made for two reasons:
1) it allows our shim implementation to work around pop-up blockers since window.open can happen synchronously. This is probably not a concern for B2G right now (but maybe it is?).
2) it makes for a smoother user experience because the app has time to hit its own server to sign the JWT while the first request to the payment provider is fulfilled.  

It would make sense for navigator.pay() to go this route for #2 and also so that developers did not have to put an awkward if branch in their code when supporting both nav.pay and mozmarket.buy.

Maybe this API change is not possible for nav.pay() since it reads the typ field in the JWT but I don't know.
Comment 13 Kumar McMillan [:kumar] (needinfo all the things) 2012-07-12 09:22:10 PDT
over IRC we decided that it doesn't make much sense for nav.pay() to implement the async API because there's not much it can do in the waiting period before it receives a JWT.
Comment 14 Gregor Wagner [:gwagner] 2012-07-12 10:05:57 PDT
Is there a reason why you exposed the pay function in c++ and not via a manifest like https://hg.mozilla.org/mozilla-central/file/f9499238bd4b/dom/contacts/ContactManager.manifest#l22 ?
Comment 15 [:fabrice] Fabrice Desré 2012-07-12 10:17:00 PDT
(In reply to Gregor Wagner [:gwagner] from comment #14)
> Is there a reason why you exposed the pay function in c++ and not via a
> manifest like
> https://hg.mozilla.org/mozilla-central/file/f9499238bd4b/dom/contacts/
> ContactManager.manifest#l22 ?

Because we can only expose properties with JavaScript-navigator-property, not functions on navigator.
Comment 16 Gregor Wagner [:gwagner] 2012-07-12 10:21:22 PDT
(In reply to Fabrice Desré [:fabrice] from comment #15)
> (In reply to Gregor Wagner [:gwagner] from comment #14)
> > Is there a reason why you exposed the pay function in c++ and not via a
> > manifest like
> > https://hg.mozilla.org/mozilla-central/file/f9499238bd4b/dom/contacts/
> > ContactManager.manifest#l22 ?
> 
> Because we can only expose properties with JavaScript-navigator-property,
> not functions on navigator.

Ah right. I keep forgetting :)
Comment 17 Dietrich Ayala (:dietrich) 2012-07-12 10:41:04 PDT
(In reply to Kumar McMillan [:kumar] from comment #13)
> over IRC we decided that it doesn't make much sense for nav.pay() to
> implement the async API because there's not much it can do in the waiting
> period before it receives a JWT.

Is this API *only* called on background content processes? If there's any chance it could ever get called in the main system process, then it needs to be async.
Comment 18 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-12 11:34:16 PDT
As discussed via IR(In reply to Dietrich Ayala (:dietrich) from comment #17)
> (In reply to Kumar McMillan [:kumar] from comment #13)
> > over IRC we decided that it doesn't make much sense for nav.pay() to
> > implement the async API because there's not much it can do in the waiting
> > period before it receives a JWT.
> 
> Is this API *only* called on background content processes? If there's any
> chance it could ever get called in the main system process, then it needs to
> be async.

as discussed via IRC, this API is async as it is returning a DOMRequest object before starting the payment flow, so it is not blocking the main process.
Comment 19 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 05:57:42 PDT
I've made some modifications to the navigator.pay draft mentioned in comment #1 including: 
- Some BlueVia Product user stories related to navigator.pay, as Jonas requested.(BlueVia is Telefónica's payment processor).
- Some clarifications regarding the dinamic modification of the postback and chargeback URLs.
Comment 20 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 05:58:39 PDT
Sorry, s/comment #1/Description/
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-13 11:37:04 PDT
Fernando: Did you have that list of feature requirements? I can't see it attached.
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-13 11:40:58 PDT
I don't understand how the jwt format can be defined by the payment processor. The caller of the API doesn't seem to have any way of finding out which payment processor will be used and so has no way of encoding the payment information.
Comment 23 Andreas Gal :gal 2012-07-13 11:45:49 PDT
Jonas, the caller knows that because payments can only be processed by the payment provider the caller is registered with. You send all the tokens you know that the payment processor can pay you, and the device will pick one it supports and initiate the flow with that. Makes sense?
Comment 24 Kumar McMillan [:kumar] (needinfo all the things) 2012-07-13 11:54:27 PDT
In other words: if you pay two providers, you sign two JWTs: blueViaJWT is signed with the secret created by BlueVia, mozJWT with the secret created by Mozilla Marketplace.
Comment 25 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 12:34:20 PDT
Created attachment 642002 [details]
BlueVia user stories related to payments
Comment 26 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 12:38:46 PDT
(In reply to Jonas Sicking (:sicking) from comment #21)
> Fernando: Did you have that list of feature requirements? I can't see it
> attached.
As I mentioned on comment #19, I added the US that the PM allowed me to published to the navigator.pay draft (https://docs.google.com/document/d/1NLKbHVPQXa9uvDBC3cfgOD7sIrtIxi0qDoXMQrxcCsI/edit). I am also adding it as attachment.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-13 13:10:39 PDT
If the caller has to have a pre-setup arrangement with the payment provider, then at the very least we should have an additional argument which specifies which provider the caller wants to request payment through.

It's unfortunate that the developer has to set up an agreement with the payment provider, but that's a product decision, so I'll leave that to you.

However it seems like we need to pass enough data in cleartext that the API implementation can display UI to the user describing how much money is getting payed, and for what service.

I also don't quite understand how refunds work. There are a bunch of "should"s in the requirements doc, but I don't see any mechanisms for enabling them.
Comment 28 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 13:19:58 PDT
(In reply to Jonas Sicking (:sicking) from comment #27)
> If the caller has to have a pre-setup arrangement with the payment provider,
> then at the very least we should have an additional argument which specifies
> which provider the caller wants to request payment through.
That information goes within the JWT (the 'typ' parameter). The current implementation stores the information about allowed payment processors as a preference. 

> However it seems like we need to pass enough data in cleartext that the API
> implementation can display UI to the user describing how much money is
> getting payed, and for what service.
All that information goes within the JWT, which goes base64 encoded and signed with the application secret.

(let me answer about refunds on my next comment, I am on a train with a crappy connection :\)
Comment 29 Andreas Gal :gal 2012-07-13 13:23:22 PDT
How do you get paid if you aren't setup with the provider? And the caller does specify that. The order of
The jwts matters. The client will pick the first it supports (user must be able to pay that way).
Comment 30 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 13:44:39 PDT
(In reply to Jonas Sicking (:sicking) from comment #27)
> I also don't quite understand how refunds work. There are a bunch of
> "should"s in the requirements doc, but I don't see any mechanisms for
> enabling them.

For refunds, as a mandatory requirement, the payment gateway should keep a list of transactions associated with digital goods purchases accessible via user profile allowing the user to cancel them. This is already mostly implemented for BlueVia. The UA should direct users to that refund page. In B2G the plan is to make this available as a setting. The refund page will be loaded in a trusted dialog, the same way as the buy flow currently does.

Also, the navigator.pay API allows refunds the same way as it allow purchases, so the developer has the choice to also expose a way to request refunds from his app. The difference between a purchase and a refund is within the JWT request parameters.
Comment 31 Gregor Wagner [:gwagner] 2012-07-13 14:12:08 PDT
What's the story on desktop? Should it be enabled? The current patch tries to load the payment system via navigator.pay on desktop as well right?
Comment 32 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-13 15:56:16 PDT
(In reply to Gregor Wagner [:gwagner] from comment #31)
> What's the story on desktop? Should it be enabled? The current patch tries
> to load the payment system via navigator.pay on desktop as well right?

The current priority for v1 is B2G and the current patches only work for B2G. After B2G the idea is to provide support for desktop. I should ifdef the current implementation for only exposing navigator.pay to b2g until desktop support is done. I'll do that if you agree.
Comment 33 Gregor Wagner [:gwagner] 2012-07-13 16:05:49 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #32)
> (In reply to Gregor Wagner [:gwagner] from comment #31)
> > What's the story on desktop? Should it be enabled? The current patch tries
> > to load the payment system via navigator.pay on desktop as well right?
> 
> The current priority for v1 is B2G and the current patches only work for
> B2G. After B2G the idea is to provide support for desktop. I should ifdef
> the current implementation for only exposing navigator.pay to b2g until
> desktop support is done. I'll do that if you agree.

Yes, only add it for B2G builds and don't load anything for desktop builds.
BTW, do you have any tests?
Comment 34 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-13 18:16:24 PDT
So a few comments here:

I think it should be a requirement from Mozilla's side *not* to send *any* requests to the payment provider before the user has chosen to pay using that payment provider.

The document at https://docs.google.com/document/d/1NLKbHVPQXa9uvDBC3cfgOD7sIrtIxi0qDoXMQrxcCsI/edit *needs* to be made available to the public. Or at least a document which contains the technical requirements enumerated in that document. We *must* be open with the requirements that are backing our APIs, that is what makes us an open organization. I'm very reluctant to sr any APIs until this has been done.

I don't understand why we are encoding the request JSON object? Why not provide a JSON string and a signature for each payment provider. So something like:
[{ request: '{ ... JSON encoded object ... }',
   signature: "EaBbb9atv3ad..." },
 { request: '{ ... JSON encoded object for second payment provider ... }',
   signature: "riIOP14-Qzfe..." }]

This way it's easy for the API implementation to display the necessary data itself and just display buttons for the individual providers if we want to do that (which I strongly think we should).

The order of the requests should *not* matter. It we should choose whichever provider as default that the user has chosen as preferred provider and which we received a request for.
Comment 35 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-14 02:50:30 PDT
> > (In reply to Gregor Wagner [:gwagner] from comment #31)
> Yes, only add it for B2G builds and don't load anything for desktop builds.
Ok, I'll do that.

> BTW, do you have any tests?
I have only my own local tests. I'll be writing mochitests for this on Monday.
Comment 36 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-14 04:09:36 PDT
(In reply to Jonas Sicking (:sicking) from comment #34)
> So a few comments here:
> 
> I think it should be a requirement from Mozilla's side *not* to send *any*
> requests to the payment provider before the user has chosen to pay using
> that payment provider.
Agreed and it will be done that way.

As I mentioned on comment #6, the proposed idl allows multiple jwt in each call, but the proposed implementation doesn't (my reasons and doubts for this differences between interface and implementation are on comment #6). The original requirement *was* to only allow one jwt in each call for v1, so the developer should present the different payment options that he supports so he lets the user choose the more appropriate for him. Once the user makes that choice, the developer must encode and sign the jwt according to the chosen payment processor. After that, the developer must call navigator.pay with the encoded and signed jwt. So the UA would not send any request to the payment processor before the user would make his choice.
Anway, while reviewing the BlueVia product requirements with the PM, we realize that some of this steps may need to be changed. The reason is this requirement:
"As a developer, when setting up a new payment processor in the market, I don't want to have to update my application".
That means that we shouldn't give the developer the responsability of presenting the different payment choices to the user. The UA should do that with the help of the Marketplace. I was recently (yesterday) told that during the Barcelona work week there were some meetings about this. If I am not wrong (Andreas can correct me) the idea is that the Marketplace provides a helper for encoding the payment requests. So the developer could use this feature to encode the same payment request in a different jwt for each payment processor that he has set up on the marketplace. That way, he could call navigator.pay with multiple jwt and the UA would show the different payment choices that the developer supports so the user can choose the more appropriate for him. Andreas, is that correct? Kumar, is that ok for the Marketplace? In that case, is there already any work done for this on the marketplace side?

> The document at
> https://docs.google.com/document/d/
> 1NLKbHVPQXa9uvDBC3cfgOD7sIrtIxi0qDoXMQrxcCsI/edit *needs* to be made
> available to the public. Or at least a document which contains the technical
> requirements enumerated in that document. We *must* be open with the
> requirements that are backing our APIs, that is what makes us an open
> organization. I'm very reluctant to sr any APIs until this has been done.
That Google doc is and has been public since day 1. I am really sorry if it made a different impression. I agree that Google Docs is not the best place for this. I'll write its content in MDN or any other place that you consider that is more appropriate.
Regarding the product requirements from BlueVia, there are more requirements for the payment processor that don't affect the navigator.pay implementation. I've asked to publish them but they are still under revision. I'll make them public as soon as they allow me to do it. But, once again, are more related to the payment processor itself.


> I don't understand why we are encoding the request JSON object? Why not
> provide a JSON string and a signature for each payment provider. So
> something like:
> [{ request: '{ ... JSON encoded object ... }',
>    signature: "EaBbb9atv3ad..." },
>  { request: '{ ... JSON encoded object for second payment provider ... }',
>    signature: "riIOP14-Qzfe..." }]
> 
> This way it's easy for the API implementation to display the necessary data
> itself and just display buttons for the individual providers if we want to
> do that (which I strongly think we should).
We are following the JSON Web Token specification (http://openid.net/specs/draft-jones-json-web-token-07.html).
I am not sure that I am understanding your concerns, but the fact that is encoded does not avoid the API implementation to display the necessary data about the payment. In fact, the current implementation decodes and select the appropriate payment processor according to the jwt 'typ' parameter.
 
> The order of the requests should *not* matter. It we should choose whichever
> provider as default that the user has chosen as preferred provider and which
> we received a request for.
Agreed. The order doesn't matter. I think choosing the first jwt it was only a suggestion for v1, but the current implementation doesn't allow that, as it only allows one jwt. With the (new for me) requirement that I previosly mentioned, multiple choices will be shown by the UA so the user can choose the one he considers more appropriate. The same way, I agree, a default one should be settable.

If you agree with this, I'll try to update the implementation with this new requirement asap.

Thanks for your feedback Jonas! :)
Comment 37 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-14 04:12:48 PDT
Comment on attachment 639706 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v1

I am clearing the r? flags until I make the suggested changes.
Comment 38 Mounir Lamouri (:mounir) 2012-07-15 20:11:39 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> That Google doc is and has been public since day 1. I am really sorry if it
> made a different impression. I agree that Google Docs is not the best place
> for this. I'll write its content in MDN or any other place that you consider
> that is more appropriate.

Adding this to https://wiki.mozilla.org/WebAPI would do. Maybe write down the API in https://wiki.mozilla.org/WebAPI/{MozPay,Payment,MicroPayment,Whatever}.
Comment 39 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-16 01:42:49 PDT
Ok, I guess I don't understand the intended flow here. It's getting more complicated because we don't have all the requirements in one place. I'm receiving part of the information here, and seemingly contradictory information from Andreas.

Some questions that we need to straighten out:

* Where is the jwt encoding intended to take place? In the code of the app running on the B2G device? On the server? In the API implementation, i.e. in Gecko? The design doc says that it needs to be server-side, is that still accurate? Why do we want the encoding to happen there?

* What is the purpose of the jwt encoding? As I understand it it's to produce a signature of some sort, but why do we need things to be signed here? What types of attacks are we trying to protect against, or what type of legal requirements are we trying to fulfill? Or are we using jwt encoding since that's what various payment providers are using? Details here would be very helpful.

* Is the intent that no requests are going to go to any payment provider until the user has made an explicit choice as to which payment provider is chosen? (You and Andreas give different answers, that's why I'm asking again).

* Is the intent that the app author should need to change *no* client side code in order to add support for new payment providers? What about server side code?

* Do we need a way to use the API to detect that the app and Gecko can agree on a payment provider? It seems suboptimal if the user ends up going through a whole UI process only to detect at the very end that there are no common payment providers between what the app supports and what Gecko supports. As a user I would be annoyed is that means that I end up wasting time.

* Who will be responsible for showing UI describing what purchase is being handled? The app, B2G or the payment provider? I imagine it couldn't be the app since we want the user to get that information from a trusted source before approving the payment.

> That Google doc is and has been public since day 1. I am really sorry if it
> made a different impression.

Ugh, I'm really sorry. The UI in Google Docs made it look like the document was shared with only selected people. Absolutely my mistake!

> I agree that Google Docs is not the best place
> for this. I'll write its content in MDN or any other place that you consider
> that is more appropriate.

Ideal would be to have it attached to this bug or sent to the newsgroup. That way there's an archived copy somewhere. But the most important part is that it's in a publicly accessible place, which is already the case.

> We are following the JSON Web Token specification
> (http://openid.net/specs/draft-jones-json-web-token-07.html).
> I am not sure that I am understanding your concerns, but the fact that is
> encoded does not avoid the API implementation to display the necessary data
> about the payment. In fact, the current implementation decodes and select
> the appropriate payment processor according to the jwt 'typ' parameter.

I guess what confuses me is the need for the JWT encoding. It seems like we're asking the server to encode data in a fairly complicated format (jwt), and then we have to go through the effort of decoding said complex format.

/ Jonas
Comment 40 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-16 03:50:43 PDT
(In reply to Mounir Lamouri (:mounir) from comment #38)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> > That Google doc is and has been public since day 1. I am really sorry if it
> > made a different impression. I agree that Google Docs is not the best place
> > for this. I'll write its content in MDN or any other place that you consider
> > that is more appropriate.
> 
> Adding this to https://wiki.mozilla.org/WebAPI would do. Maybe write down
> the API in
> https://wiki.mozilla.org/WebAPI/{MozPay,Payment,MicroPayment,Whatever}.

Ok, thanks Mounir. I'll add the doc to MDN and send a mail to dev-webapi.

(In reply to Jonas Sicking (:sicking) from comment #39)
> Ok, I guess I don't understand the intended flow here. It's getting more
> complicated because we don't have all the requirements in one place. I'm
> receiving part of the information here, and seemingly contradictory
> information from Andreas.
> 

That's probably my fault as I have not been asking Andreas for feedback about the proposed implementation as much as I should have. Sorry about that.

> Some questions that we need to straighten out:
> 
> * Where is the jwt encoding intended to take place? In the code of the app
> running on the B2G device? On the server? In the API implementation, i.e. in
> Gecko? The design doc says that it needs to be server-side, is that still
> accurate? Why do we want the encoding to happen there?

Yes, the encoding and signing needs to take place on a server-side. The main reason is because currently there is not a secure way to store application credentials (app secret to sign the jwt) on the client side.
Note that the postback and chargeback notifications also need to be received and validated (jwt decode and sign verification) on a server-side.

> 
> * What is the purpose of the jwt encoding? As I understand it it's to
> produce a signature of some sort, but why do we need things to be signed
> here? What types of attacks are we trying to protect against, or what type
> of legal requirements are we trying to fulfill? Or are we using jwt encoding
> since that's what various payment providers are using? Details here would be
> very helpful.
> 

The main purpose of signing the intention-to-pay is to avoid alteration of data, either by the user or by a mallicious application. One possible attack scenario would be for the user to modify the payment-due quantity (to decrease it). He then proceeds to make the payment, the payment provider would generate a correct receipt (albeit for the wrong quantity). That modification could be detected on the developer's side (note that this can be use for in app payments and thus Mozilla Store would not need to be in the loop at all) but it would require the developer to keep track not only of pending transactions but also of the details of every transaction. Details that can change for reasons such as taxes, currency changes,... And even taking into account all that, assuming the developer identifies the fraud, he would have to revoke the payment, inform the user, revoke the authorization to the application...
It's much easier just to sign the intention-to-pay information and only allow payments with correct signatures to proceed.

> * Is the intent that no requests are going to go to any payment provider
> until the user has made an explicit choice as to which payment provider is
> chosen? (You and Andreas give different answers, that's why I'm asking
> again).
>
 
Sorry, I can't see any different answers between Andreas and me on this matter. 

We had different answers about accepting multiple jwt on the navigator.pay call (Andreas suggested taking the first one supported while I proposed showing a selection screen to the user, because of the new (for me) requirement that I mention in comment #36), but I can see none of us suggesting that a request is going to go to any payment provider *before* the user has made a explicit choice.

In the proposed implementation, the developer would be responsible of showing the payment method selection screen, so he would be able to generate the appropriate jwt to pass to navigator.pay call based on user's selection. That would mean that navigator.pay would always receive *one* jwt, so accepting multiple jwt and taking the first one, as Andreas suggested, does not make much sense to me. But I am open to implement it that way if there is any good reasons :). Despite that, correct me if I am wrong, no request would be done to a payment processor before user's selection. Allowing or not multiple jwts on the same navigator.pay call (which is, sorry for repeating myself, where Andreas and me disagree) IMHO does not affect here.

Anyway, regarding the multiple jwt thing, I am not sure if Andreas is familiar with the new requirement that I mention in comment #36. I received that requirement from TEF side.

> * Is the intent that the app author should need to change *no* client side
> code in order to add support for new payment providers? What about server
> side code?

Yes, that is what the new requirement that I mention in comment #36 is about.

With the current implementation, the developer would need to:
1. Register himself on the payment provider side and get the app credentials linked to that payment provider.
2. Register those new app credentials on the marketplace (in the case of affecting the app purchase, not for the in-app billing case).
3. Update his client side code to let the user know about the new payment option.
4. Update his server side code to generate the appropriate jwts for the new payment processor.

I agree that modifying the current implementation to address the new requirement is probably a must. It would reduce the process to 1. and 2. Even if the marketplace does finally not expose the helper for generating the jwts, supporting multiple jwt on each navigator.pay call would at least avoid the need of step 3. So I guess I better start working on that :)

> 
> * Do we need a way to use the API to detect that the app and Gecko can agree
> on a payment provider? It seems suboptimal if the user ends up going through
> a whole UI process only to detect at the very end that there are no common
> payment providers between what the app supports and what Gecko supports. As
> a user I would be annoyed is that means that I end up wasting time.

The current implementation already does that. If the developer generates a jwt of a type not registered in Gecko (Note that the jwt type is matched with the payment processor) and calls navigator.pay with that jwt, the current implementation will fire an error over the DOMRequest object. The only UI that the user would see is the app UI and the way it handles this kind of errors.

> 
> * Who will be responsible for showing UI describing what purchase is being
> handled? The app, B2G or the payment provider? I imagine it couldn't be the
> app since we want the user to get that information from a trusted source
> before approving the payment.

The payment provider is responsible for showing the buy flow UI within a trusted dialog as suggested in Bug 768943 (already merged in Gaia).

> 
> > That Google doc is and has been public since day 1. I am really sorry if it
> > made a different impression.
> 
> Ugh, I'm really sorry. The UI in Google Docs made it look like the document
> was shared with only selected people. Absolutely my mistake!
> > I agree that Google Docs is not the best place
> > for this. I'll write its content in MDN or any other place that you consider
> > that is more appropriate.
> 
> Ideal would be to have it attached to this bug or sent to the newsgroup.
> That way there's an archived copy somewhere. But the most important part is
> that it's in a publicly accessible place, which is already the case.
> 

No problem :) As I mentioned before I'll move that info to MDN and let you all know in dev-webapi when it is ready, so we can involve more people on the discussion. Sorry, I should have done that long time ago.

> > We are following the JSON Web Token specification
> > (http://openid.net/specs/draft-jones-json-web-token-07.html).
> > I am not sure that I am understanding your concerns, but the fact that is
> > encoded does not avoid the API implementation to display the necessary data
> > about the payment. In fact, the current implementation decodes and select
> > the appropriate payment processor according to the jwt 'typ' parameter.
> 
> I guess what confuses me is the need for the JWT encoding. It seems like
> we're asking the server to encode data in a fairly complicated format (jwt),
> and then we have to go through the effort of decoding said complex format.
> 

Does my previous reply about the signing need answer your question?

Regarding the encoding need. Our main reason to base64 encode the data is because JWT RFC, as defined on http://tools.ietf.org/html/draft-ietf-oauth-json-web-token-00#page-11 specify it has to be encoded, and in turn the JWS RFC as defined on http://tools.ietf.org/html/draft-jones-json-web-signature-03#section-5 also specify the TBS data has to be encoded. If I had to guess why they ask for encoding before signing I would guess it's to minimize the risk of in transit encoding breaking the signature, but that's just a guess. 
We're using JWT in turn because we need a format to transmit some signed data, and this one is publicly available and defined, and it was a format used already on the Mozilla Store

Maybe Kumar, who also used this jwt format on his in-app payment solution, have a better answer for this.
Comment 42 Kumar McMillan [:kumar] (needinfo all the things) 2012-07-16 08:56:59 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #40)
> > Adding this to https://wiki.mozilla.org/WebAPI would do. Maybe write down
> > the API in
> > https://wiki.mozilla.org/WebAPI/{MozPay,Payment,MicroPayment,Whatever}.

FWIW, we're still deciding what the *developer* facing API will be. So far the proposal is to let developers use the mozmarket.js shim -- https://developer.mozilla.org/en/Apps/In-app_payments -- which will call navigator.pay() under the hood. This would yield the most compatible apps since they would work on both B2G and non-B2G devices (such as desktop, Android)

> > * What is the purpose of the jwt encoding? As I understand it it's to
> > produce a signature of some sort, but why do we need things to be signed
> > here? What types of attacks are we trying to protect against, or what type
> > of legal requirements are we trying to fulfill? Or are we using jwt encoding
> > since that's what various payment providers are using? Details here would be
> > very helpful.

The JWT spec (linked earlier) calls for encoding and all the common JWT libraries adhere to this. I believe it's encoded because there are many ways you can serialize a JSON object (whitespace, alphabetizing, etc) so it's less error prone to sign/verify a base 64 encoded blob.

Here is the original threat model we planned for: https://github.com/mozilla/apps-payment-server/blob/master/DESIGN.md#threat-model

Here is the official security review, with some additional threats: https://wiki.mozilla.org/Security/Reviews/AppStore#Threat_Model

These threat models apply to the Marketplace payments API. A security review of navigator.pay() is forthcoming, as noted in this bug, and might have some different threats.
Comment 43 Andreas Gal :gal 2012-07-17 01:34:14 PDT
Kumar, the developer facing interface is navigator.pay(). Including a JS file from a third party sever (Mozilla) into millions of web sites/apps creates a central point of attack which when successfully broken into allows executing rogue code in the context of millions of domains. This additional attack surface is not part of our security design and security review for v1. If this needs any additional clarification, please find me on irc or via phonebook.

As Fernando explained, we want to support multiple payment providers (and jwt tokens) via a selection box. Again, if you have any questions here or need help implementing support in the marketplace, please flag me down. Thats probably more effective than lengthy bug debates.
Comment 44 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-17 02:19:02 PDT
> Yes, the encoding and signing needs to take place on a server-side. The main
> reason is because currently there is not a secure way to store application
> credentials (app secret to sign the jwt) on the client side.
> Note that the postback and chargeback notifications also need to be received
> and validated (jwt decode and sign verification) on a server-side.

Makes sense.

> The main purpose of signing the intention-to-pay is to avoid alteration of
> data, either by the user or by a mallicious application. One possible attack
> scenario would be for the user to modify the payment-due quantity (to
> decrease it). He then proceeds to make the payment, the payment provider
> would generate a correct receipt (albeit for the wrong quantity). That
> modification could be detected on the developer's side (note that this can
> be use for in app payments and thus Mozilla Store would not need to be in
> the loop at all) but it would require the developer to keep track not only
> of pending transactions but also of the details of every transaction.
> Details that can change for reasons such as taxes, currency changes,... And
> even taking into account all that, assuming the developer identifies the
> fraud, he would have to revoke the payment, inform the user, revoke the
> authorization to the application...
> It's much easier just to sign the intention-to-pay information and only
> allow payments with correct signatures to proceed.

Makes sense. We should definitely encourage developers to verify the receipt that they get back, but signing things properly on the server seems like a good thing to do.

> > * Is the intent that no requests are going to go to any payment provider
> > until the user has made an explicit choice as to which payment provider is
> > chosen? (You and Andreas give different answers, that's why I'm asking
> > again).
> >
>  
> Sorry, I can't see any different answers between Andreas and me on this
> matter. 

Sorry, I wasn't clear. The contradictory information I had received wasn't in this bug, but through other channels.

> In the proposed implementation, the developer would be responsible of
> showing the payment method selection screen, so he would be able to generate
> the appropriate jwt to pass to navigator.pay call based on user's selection.
> That would mean that navigator.pay would always receive *one* jwt, so
> accepting multiple jwt and taking the first one, as Andreas suggested, does
> not make much sense to me. But I am open to implement it that way if there
> is any good reasons :). Despite that, correct me if I am wrong, no request
> would be done to a payment processor before user's selection. Allowing or
> not multiple jwts on the same navigator.pay call (which is, sorry for
> repeating myself, where Andreas and me disagree) IMHO does not affect here.

One problem with this flow is that it means that the website has to sprinkle all sorts of logo's on their site to allow the user to choose which one to pay with. Even if the user doesn't support a bunch of them, or has no interest in using others since their favorite payment provider is available. This is similar to the NASCAR problem [1] for login pages.

[1] http://factoryjoe.com/blog/2009/04/06/does-openid-need-to-be-hard/

This is one reason why it's attractive to send a whole list of payment providers to B2G, and then B2G can choose to only display the options that the user might be interested in, based on which payment providers the user has configured and preferences about which ones are preferred.

> > * Is the intent that the app author should need to change *no* client side
> > code in order to add support for new payment providers? What about server
> > side code?
> 
> Yes, that is what the new requirement that I mention in comment #36 is about.
> 
> With the current implementation, the developer would need to:
> 1. Register himself on the payment provider side and get the app credentials
> linked to that payment provider.
> 2. Register those new app credentials on the marketplace (in the case of
> affecting the app purchase, not for the in-app billing case).
> 3. Update his client side code to let the user know about the new payment
> option.
> 4. Update his server side code to generate the appropriate jwts for the new
> payment processor.
> 
> I agree that modifying the current implementation to address the new
> requirement is probably a must. It would reduce the process to 1. and 2.

It would still require 4 as well, no? Since you'd have to generate jwt tokens for all payment providers.

But I agree that getting rid of 3 is attractive.

> Even if the marketplace does finally not expose the helper for generating
> the jwts, supporting multiple jwt on each navigator.pay call would at least
> avoid the need of step 3. So I guess I better start working on that :)

I think it's fine to require 3 for now and improve the API later. And it's fine to do something more complex as well. Someone just needs to make a decision (not me :) ).

> > * Do we need a way to use the API to detect that the app and Gecko can agree
> > on a payment provider? It seems suboptimal if the user ends up going through
> > a whole UI process only to detect at the very end that there are no common
> > payment providers between what the app supports and what Gecko supports. As
> > a user I would be annoyed is that means that I end up wasting time.
> 
> The current implementation already does that. If the developer generates a
> jwt of a type not registered in Gecko (Note that the jwt type is matched
> with the payment processor) and calls navigator.pay with that jwt, the
> current implementation will fire an error over the DOMRequest object. The
> only UI that the user would see is the app UI and the way it handles this
> kind of errors.

Sure, but that's only possible once the user has decided that he/she wants to actually purchase something.

If the store really won't be able to bill the user, they might want to find that out much earlier. But we can wait with providing that ability until after v1.

> > * Who will be responsible for showing UI describing what purchase is being
> > handled? The app, B2G or the payment provider? I imagine it couldn't be the
> > app since we want the user to get that information from a trusted source
> > before approving the payment.
> 
> The payment provider is responsible for showing the buy flow UI within a
> trusted dialog as suggested in Bug 768943 (already merged in Gaia).

Unless we decide to address the new requirement in comment 36, right?

If I understand correctly, if we decide to address that new requirement then the store would simply send a list of jwt tokens for all the payment providers that it supports to B2G, and then B2G would display UI allowing the user to choose which provider to use, right?

> > > We are following the JSON Web Token specification
> > > (http://openid.net/specs/draft-jones-json-web-token-07.html).
> > > I am not sure that I am understanding your concerns, but the fact that is
> > > encoded does not avoid the API implementation to display the necessary data
> > > about the payment. In fact, the current implementation decodes and select
> > > the appropriate payment processor according to the jwt 'typ' parameter.
> > 
> > I guess what confuses me is the need for the JWT encoding. It seems like
> > we're asking the server to encode data in a fairly complicated format (jwt),
> > and then we have to go through the effort of decoding said complex format.
> > 
> 
> Does my previous reply about the signing need answer your question?

Yes. Thanks!

However weather we decide that we want to address comment 36 or not affects the API pretty significantly, so it would be great to know if that's something we want to do.
Comment 45 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-17 13:15:45 PDT
(In reply to Jonas Sicking (:sicking) from comment #44)
> > In the proposed implementation, the developer would be responsible of
> > showing the payment method selection screen, so he would be able to generate
> > the appropriate jwt to pass to navigator.pay call based on user's selection.
> > That would mean that navigator.pay would always receive *one* jwt, so
> > accepting multiple jwt and taking the first one, as Andreas suggested, does
> > not make much sense to me. But I am open to implement it that way if there
> > is any good reasons :). Despite that, correct me if I am wrong, no request
> > would be done to a payment processor before user's selection. Allowing or
> > not multiple jwts on the same navigator.pay call (which is, sorry for
> > repeating myself, where Andreas and me disagree) IMHO does not affect here.
> 
> One problem with this flow is that it means that the website has to sprinkle
> all sorts of logo's on their site to allow the user to choose which one to
> pay with. Even if the user doesn't support a bunch of them, or has no
> interest in using others since their favorite payment provider is available.
> This is similar to the NASCAR problem [1] for login pages.
> 
> [1] http://factoryjoe.com/blog/2009/04/06/does-openid-need-to-be-hard/
> 
> This is one reason why it's attractive to send a whole list of payment
> providers to B2G, and then B2G can choose to only display the options that
> the user might be interested in, based on which payment providers the user
> has configured and preferences about which ones are preferred.
>

I agree. I've been talking with Andreas, and as he mentioned before, we will be supporting multiple JWT in the same navigator.pay call. I am already working on that.

Apart from the multiple JWT support, we can also add a setting to let the user set up his prefered payment providers from a list of supported providers and also set a default one to avoid showing the selection screen in every payment. Does it work for you?

> > With the current implementation, the developer would need to:
> > 1. Register himself on the payment provider side and get the app credentials
> > linked to that payment provider.
> > 2. Register those new app credentials on the marketplace (in the case of
> > affecting the app purchase, not for the in-app billing case).
> > 3. Update his client side code to let the user know about the new payment
> > option.
> > 4. Update his server side code to generate the appropriate jwts for the new
> > payment processor.
> > 
> > I agree that modifying the current implementation to address the new
> > requirement is probably a must. It would reduce the process to 1. and 2.
> 
> It would still require 4 as well, no? Since you'd have to generate jwt
> tokens for all payment providers.

If the Marketplace provides the helper for generating the JWTs, then step 4 would not be required.

> > Even if the marketplace does finally not expose the helper for generating
> > the jwts, supporting multiple jwt on each navigator.pay call would at least
> > avoid the need of step 3. So I guess I better start working on that :)
> 
> I think it's fine to require 3 for now and improve the API later. And it's
> fine to do something more complex as well. Someone just needs to make a
> decision (not me :) ).
>

As I mentioned before, I've been talking to Andreas about this and he agrees about supporting multiple JWTs. TEF PM also agrees. So I am already working on getting rid of step 3 :)

> > The current implementation already does that. If the developer generates a
> > jwt of a type not registered in Gecko (Note that the jwt type is matched
> > with the payment processor) and calls navigator.pay with that jwt, the
> > current implementation will fire an error over the DOMRequest object. The
> > only UI that the user would see is the app UI and the way it handles this
> > kind of errors.
> 
> Sure, but that's only possible once the user has decided that he/she wants
> to actually purchase something.
> 
> If the store really won't be able to bill the user, they might want to find
> that out much earlier. But we can wait with providing that ability until
> after v1.
>

Yes, that's true and I agree that it would be a nice to have for next versions.
 
> > > * Who will be responsible for showing UI describing what purchase is being
> > > handled? The app, B2G or the payment provider? I imagine it couldn't be the
> > > app since we want the user to get that information from a trusted source
> > > before approving the payment.
> > 
> > The payment provider is responsible for showing the buy flow UI within a
> > trusted dialog as suggested in Bug 768943 (already merged in Gaia).
> 
> Unless we decide to address the new requirement in comment 36, right?
> 
> If I understand correctly, if we decide to address that new requirement then
> the store would simply send a list of jwt tokens for all the payment
> providers that it supports to B2G, and then B2G would display UI allowing
> the user to choose which provider to use, right?
> 

Sorry, I didn't explain myself properly. I was refering only to the provider buy flow UI.
You are right, as we are going to support multiple JWTs on navigator.pay() calls, after verifying the JWTs, B2G would display a payment provider selection UI based on the received JWTs. After that, the selected payment provider buy flow would be shown.
Comment 46 Ben Adida [:benadida] 2012-07-17 20:33:49 PDT
(In reply to Andreas Gal :gal from comment #43)
> Including a JS
> file from a third party sever (Mozilla) into millions of web sites/apps
> creates a central point of attack which when successfully broken into allows
> executing rogue code in the context of millions of domains.

FYI, we have solved this for Identity: we have world-wide monitoring of our include.js file that triggers and escalates notifications if it changes. We could easily extend this monitoring to an additional include file.

I mention this because the idea that a developer would need to develop against a particular user-agent, which has to be B2G since I'm not sure nav.pay is going to be ready on desktop anytime soon, is not very realistic.
Comment 47 Andreas Gal :gal 2012-07-17 20:40:10 PDT
I think we are arguing past each other here. For v1 we are using navigator.pay(). It is not clear to me whether if and when any shims in this area will be launched. So we are putting the cart before the horse requesting changes to navigator.pay() to accommodate some shim solution. This bug is about implementing navigator.pay(). Lets please keep it focused on that.
Comment 48 Justin Lebar (not reading bugmail) 2012-07-18 06:50:28 PDT
I was a bit confused by the Gaia pull request [1].  What's problematic is that that the event has a "trusted frame" parameter, but doesn't actually pass a frame element.

> +        let paymentFrame = {
> +          jwt: msg.json.jwt,
> +          url: paymentProcessorUri + msg.json.jwt
> +        };
> +
> +        let id = kOpenTrustedDialogEvent + "-" + Math.random();
> +        let event = content.document.createEvent("CustomEvent");
> +        event.initCustomEvent("mozChromeEvent", true, true,
> +                              {type: kOpenTrustedDialogEvent,
> +                               id: id,
> +                               trustedFrame: paymentFrame});


IMO this custom event should have detail

    { type, id, jwt, url }

although I don't really know what the ID is doing there, and if it really needs to be unique, Math.random() may not be sufficient.
Comment 49 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-18 06:58:41 PDT
(In reply to Justin Lebar [:jlebar] from comment #48)
> I was a bit confused by the Gaia pull request [1].  What's problematic is
> that that the event has a "trusted frame" parameter, but doesn't actually
> pass a frame element.
> 
> > +        let paymentFrame = {
> > +          jwt: msg.json.jwt,
> > +          url: paymentProcessorUri + msg.json.jwt
> > +        };
> > +
> > +        let id = kOpenTrustedDialogEvent + "-" + Math.random();
> > +        let event = content.document.createEvent("CustomEvent");
> > +        event.initCustomEvent("mozChromeEvent", true, true,
> > +                              {type: kOpenTrustedDialogEvent,
> > +                               id: id,
> > +                               trustedFrame: paymentFrame});
> 
> 
> IMO this custom event should have detail
> 
>     { type, id, jwt, url }
> 
> although I don't really know what the ID is doing there, and if it really
> needs to be unique, Math.random() may not be sufficient.

Thanks Justin! That is changing on the next patches.

The ID is needed for the mozChromeEvent <-> mozContentEvent dialog. I need to make sure that the chrome is receiving the appropriate replies for it requests. Make sense?
Comment 50 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-19 09:41:14 PDT
I've just moved most part of the doc to https://wiki.mozilla.org/WebAPI/WebPayment . Any feedback is very much appreciated.
Comment 51 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-21 15:17:08 PDT
Created attachment 644688 [details] [diff] [review]
Part 1: IDL v1
Comment 52 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-21 15:19:39 PDT
Created attachment 644689 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v2
Comment 53 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-21 15:21:51 PDT
Created attachment 644690 [details] [diff] [review]
Part 3: B2G implementation v4
Comment 54 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-21 15:27:36 PDT
Comment on attachment 640744 [details] [diff] [review]
Gaia-WIP

>commit 61e23eb59cec54107b5ea83c22971ce1c900a294
>Author: Fernando Jiménez <ferjmoreno@gmail.com>
>Date:   Tue Jul 10 21:28:58 2012 +0200
>
>    Provide functionality to create and close 'system dialogs' on chrome demand. Based on trustworthy UI (https://bugzilla.mozilla.org/show_bug.cgi?id=768943) proposal.
>
>diff --git a/apps/system/index.html b/apps/system/index.html
>index 17c80b0..56f3a2f 100644
>--- a/apps/system/index.html
>+++ b/apps/system/index.html
>@@ -107,6 +107,10 @@
>     <!-- Theme and localization -->
>     <link rel="stylesheet" type="text/css" href="style/themes/default/system.css">
>     <link rel="resource" type="application/l10n" href="locale/l10n.ini">
>+
>+    <!-- System dialog -->
>+    <link rel="stylesheet" type="text/css" href="style/system_dialog/system_dialog.css"> 
>+    <script defer src="js/system_dialog.js"></script>
>   </head>
> 
>   <body onload="startup()">
>@@ -307,5 +311,8 @@
>       <div></div>
>     </div>
> 
>+    <div id="systemDialog">
>+    </div>
>+
>   </body>
> </html>
>diff --git a/apps/system/js/system_dialog.js b/apps/system/js/system_dialog.js
>new file mode 100644
>index 0000000..d1cc9e0
>--- /dev/null
>+++ b/apps/system/js/system_dialog.js
>@@ -0,0 +1,65 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- /
>+/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>+
>+'use strict';
>+
>+var SystemDialog = (function() {
>+  var lastDisplayedApp = null;
>+  var systemDialogElement = document.getElementById('systemDialog');
>+
>+  function open(systemFrame) {
>+    if (!systemFrame)
>+      return;
>+
>+    // We only allow one system dialog at a time.
>+    if (systemDialogIsShown())
>+      return;
>+
>+    lastDisplayedApp = WindowManager.getDisplayedApp();
>+    console.log("Displayed app: " + lastDisplayedApp);
>+
>+    // Show the homescreen.
>+    WindowManager.setDisplayedApp(null);
>+
>+    // Create the iframe to be shown as a system dialog.
>+    var frame = document.createElement('iframe');
>+    frame.dataset.frameType = 'window';
>+    frame.dataset.frameOrigin = systemFrame.url;
>+    frame.setAttribute('mozbrowser', 'true');
>+    frame.classList.add('frame');
>+    frame.src = systemFrame.url;
>+    systemDialogElement.appendChild(frame);
>+
>+    // Make the system dialog overlay active.
>+    systemDialogElement.classList.add('active');
>+
>+    // Make sure we're in portrait mode.
>+    screen.mozLockOrientation('portrait');
>+
>+    return frame;
>+  };
>+
>+  function close(callback) {
>+    if (!systemDialogIsShown())
>+      return;
>+
>+    // Make the system dialog overlay inactive.
>+    systemDialogElement.classList.remove('active');
>+    // Shows the previously displayed app.
>+    WindowManager.setDisplayedApp(lastDisplayedApp);
>+    // Switch back to apps orientation.
>+    WindowManager.setOrientationForApp(lastDisplayedApp);
>+
>+    callback();
>+  };
>+
>+  function systemDialogIsShown() {
>+    console.log("systemDialogIsShown");
>+    return systemDialogElement.classList.contains('active');
>+  };
>+
>+  return {
>+    open: open,
>+    close: close
>+  };
>+})();
>diff --git a/apps/system/js/windows/window_manager.js b/apps/system/js/windows/window_manager.js
>index 07266b8..6baf0e4 100644
>--- a/apps/system/js/windows/window_manager.js
>+++ b/apps/system/js/windows/window_manager.js
>@@ -484,24 +484,28 @@ var WindowManager = (function() {
>   // There are two types of mozChromeEvent we need to handle
>   // in order to launch the app for Gecko
>   window.addEventListener('mozChromeEvent', function(e) {
>+    console.log("mozChromeEvent received: " + e.detail.type);
>+
>     var origin = e.detail.origin;
>-    var app = Applications.getByOrigin(origin);
>-    var name = app.manifest.name;
>-
>-    /*
>-    * Check if it's a virtual app from a entry point.
>-    * If so, change the app name and origin to the
>-    * entry point.
>-    */
>-    var entryPoints = app.manifest.entry_points;
>-    if (entryPoints) {
>-      for (var ep in entryPoints) {
>-        //Remove the origin and / to find if if the url is the entry point
>-        var path = e.detail.url.substr(e.detail.origin.length + 1);
>-        if (path.indexOf(ep) == 0 &&
>-            (ep + entryPoints[ep].launch_path) == path) {
>-          origin = origin + '/' + ep;
>-          name = entryPoints[ep].name;
>+    if (origin) {
>+      var app = Applications.getByOrigin(origin);
>+      var name = app.manifest.name;
>+
>+      /*
>+      * Check if it's a virtual app from a entry point.
>+      * If so, change the app name and origin to the
>+      * entry point.
>+      */
>+      var entryPoints = app.manifest.entry_points;
>+      if (entryPoints) {
>+        for (var ep in entryPoints) {
>+          //Remove the origin and / to find if if the url is the entry point
>+          var path = e.detail.url.substr(e.detail.origin.length + 1);
>+          if (path.indexOf(ep) == 0 &&
>+              (ep + entryPoints[ep].launch_path) == path) {
>+            origin = origin + '/' + ep;
>+            name = entryPoints[ep].name;
>+          }
>         }
>       }
>     }
>@@ -551,6 +555,30 @@ var WindowManager = (function() {
>                     app.manifest.name, app.manifest, app.manifestURL, true);
> 
>         break;
>+
>+      // Chrome asks Gaia to create a system iframe. Once it is created,
>+      // Gaia sends the iframe back to chrome so frame scripts can be loaded
>+      // as part of the iframe content.
>+      case 'open-system-dialog':
>+        if (!e.detail.systemFrame)
>+          return;
>+        var frame = SystemDialog.open(e.detail.systemFrame);
>+        var event = document.createEvent('CustomEvent');
>+        event.initCustomEvent('mozContentEvent', true, true,
>+                              {id: e.detail.id, frame: frame});
>+        window.dispatchEvent(event);
>+        break;
>+
>+      // Chrome is asking Gaia to close the current system dialog. Once it is
>+      // closed, Gaia notifies back to the chrome.
>+      case 'close-system-dialog':
>+        SystemDialog.close(function closeSystemDialog() {
>+          var event = document.createEvent('CustomEvent');
>+          event.initCustomEvent('mozContentEvent', true, true,
>+                                {id: e.detail.id});
>+          window.dispatchEvent(event);
>+        });
>+        break;
>     }
>   });
> 
>diff --git a/apps/system/style/system_dialog/system_dialog.css b/apps/system/style/system_dialog/system_dialog.css
>new file mode 100644
>index 0000000..dbbfb54
>--- /dev/null
>+++ b/apps/system/style/system_dialog/system_dialog.css
>@@ -0,0 +1,28 @@
>+#systemDialog {
>+  opacity: 0;
>+  position: absolute;
>+  top: 0;
>+  left: 0;
>+  width: 100%;
>+  height: 100%;
>+  -moz-transform: scale(0);
>+  -moz-transition: all 0.4s ease;
>+  -moz-user-select: none;
>+  overflow: scroll;
>+  background-color: rgba(0, 0, 0, 0.8);
>+  z-index: 10002;
>+}
>+
>+#systemDialog.active {
>+  opacity: 1;
>+  -moz-transform: scale(1);
>+}
>+
>+#systemDialog .frame {
>+  display: inline-block;
>+  width: 100%;
>+  height: 100%;
>+  margin: 0;
>+  position: relative;
>+  -moz-transform: scale(0.8);
>+}
Comment 55 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-23 05:42:16 PDT
Created attachment 644907 [details] [diff] [review]
Part 3: B2G implementation v5

Some code clean up and bugfixing
Comment 56 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-24 11:47:12 PDT
Created attachment 645407 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v3

Conditional build fixed for B2G
Comment 57 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-24 11:48:55 PDT
Created attachment 645409 [details] [diff] [review]
Part 3: B2G implementation v5

Conditional build for B2G fixed.
Comment 58 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-24 11:55:11 PDT
Created attachment 645412 [details] [diff] [review]
Part 3: B2G implementation v5

Sorry for the spam, I forgot to remove a couple of extra ifdef.
Comment 59 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-26 08:56:41 PDT
Created attachment 646160 [details]
Test page

These are the tests which I am currently using.

The tests are based on 3 different payment providers:

- BlueVia: TEF payment gateway. Not public yet, and only available inside TEF network.
- Mozilla Marketplace: It currently doesn't support the navigator.pay flow.
- Mock payment provider (https://mockpayprovider.phpfogapp.com): I've build this fake payment provider and make it public, so you can test the navigator.pay flow with it. It just decodes the JWT request and expose the 'Pay' and 'Cancel' buttons, which call paymentSuccess() and paymentFailed() functions injected by the navigator.pay implementation.

So, as you can see, currently you can only test with the mock payment provider.

The Gaia code needed to test navigator.pay lives at https://github.com/ferjm/gaia/tree/mock-payment-provider
Comment 60 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-26 09:00:48 PDT
Created attachment 646162 [details] [diff] [review]
Part 3: B2G implementation v5

I've added the mock payment provider so you can test the patches.

As Fabrice is back and he is following this implementation since the begining I am clearing the r? flag for Gregor.
Comment 61 [:fabrice] Fabrice Desré 2012-07-26 09:26:27 PDT
Comment on attachment 645407 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v3

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

We'll need a DOM peer to review that also.

::: configure.in
@@ +7345,5 @@
> +if test -n "$MOZ_PAY"; then
> +   AC_DEFINE(MOZ_PAY)
> +fi
> +AC_SUBST(MOZ_PAY)
> +

You'll need a build peer to r+ that. For system messages we have a flag but we don't expose an --enable option since it's not really an option supported everywhere (we set it in b2g/confvars.sh). I think you should also do that for this API.

::: dom/base/Makefile.in
@@ +119,5 @@
>  
> +ifdef MOZ_PAY
> +DEFINES += -DMOZ_PAY
> +endif
> +

Why do you need that? look at https://mxr.mozilla.org/mozilla-central/search?string=MOZ_SYS_MSG for a way to not need it.

::: dom/base/Navigator.cpp
@@ +1368,5 @@
> +// nsNavigator::nsDOMNavigatorPayment
> +//****************************************************************************
> +NS_IMETHODIMP
> +Navigator::GetPaymentContentHelper()
> +{

Do an early return with:
if (mPaymentContentHelper) {
  return NS_OK;
}

@@ +1387,5 @@
> +      jsval prop_val = JSVAL_VOID;
> +      rv = gpi->Init(window, &prop_val);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }
> +  }

or:

  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);

  nsresult rv;
  nsCOMPtr<nsIDOMNavigatorPayment> paymentHelper =
    do_CreateInstance("@mozilla.org/payment/content-helper;1", &rv);
  
  nsCOMPtr<nsIDOMGlobalPropertyInitializer> gpi =
    do_QueryInterface(messageManager);
  NS_ENSURE_TRUE(gpi, NS_ERROR_FAILURE);

  // We don't do anything with the return value.
  jsval prop_val = JSVAL_VOID;
  rv = gpi->Init(window, &prop_val);
  NS_ENSURE_SUCCESS(rv, rv);

  mPaymentContentHelper = paymentHelper.forget();

@@ +1402,5 @@
> +  NS_ENSURE_TRUE(sgo, NS_ERROR_FAILURE);
> +
> +  nsIScriptContext* scriptContext = sgo->GetScriptContext();
> +  NS_ENSURE_TRUE(scriptContext, NS_ERROR_FAILURE);
> +

If you declare Pay() to be [implicit_jscontext] in the idl you can get the javascript context directly (this is what navigator.vibrate does: https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMNavigator.idl#81)

::: dom/base/Navigator.h
@@ +146,5 @@
>  #endif
>  
> +#ifdef MOZ_PAY  
> +  // Helper to set mPaymentContentHelper. 
> +  nsresult GetPaymentContentHelper();

You're not returning the helper, so rename this method to EnsurePaymentContentHelper()
Comment 62 Kumar McMillan [:kumar] (needinfo all the things) 2012-07-26 09:35:03 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #59)
> - Mozilla Marketplace: It currently doesn't support the navigator.pay flow.

We're working on it this week, tracked in bug 776638

> - Mock payment provider (https://mockpayprovider.phpfogapp.com): I've build
> this fake payment provider and make it public, so you can test the
> navigator.pay flow with it. 

cool, thanks!
Comment 63 [:fabrice] Fabrice Desré 2012-07-26 10:44:37 PDT
Comment on attachment 646162 [details] [diff] [review]
Part 3: B2G implementation v5

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

This is mostly good! I haven't tested the patches yet, so I will do another pass anyway when I'll have my test environment set up.

::: dom/payment/Payment.js
@@ +14,5 @@
> +
> +const PAYMENTCONTENTHELPER_CID =
> +  Components.ID("{a920adc0-c36e-4fd0-8de0-aac1ac6ebbd0}");
> +
> +const PAYMENT_IPC_MSG_NAMES = ["Payment:Pay",

This message is unused.

@@ +82,5 @@
> +    debug("Fire request error, id: " + requestId +
> +          ", result: " + JSON.stringify(error));
> +    Services.DOMRequest.fireError(request, error);
> +  },
> +

Remove fireRequestSuccess and fireRequestError. They only have one line of useful code ;)

@@ +85,5 @@
> +  },
> +
> +  receiveMessage: function receiveMessage(msg) {
> +    debug("Received message '" + msg.name + "': " + JSON.stringify(msg.json));
> +    let requestId = msg.json.requestId;

let request = this.takeRequest(requestId);
if (!request) {
 return;
}

@@ +88,5 @@
> +    debug("Received message '" + msg.name + "': " + JSON.stringify(msg.json));
> +    let requestId = msg.json.requestId;
> +    switch (msg.name) {
> +      case "Payment:Success":
> +        this.fireRequestSuccess(requestId);

Services.DOMRequest.fireSuccess(request, ???); (you forgot the argument to fireRequestSuccess)

@@ +91,5 @@
> +      case "Payment:Success":
> +        this.fireRequestSuccess(requestId);
> +        break;
> +      case "Payment:Failed":
> +        this.fireRequestError(requestId, msg.json.errorMsg);

Services.DOMRequest.fireError(request, msg.json.erroMsg);

@@ +106,5 @@
> +    dump("-*- PaymentContentHelper: " + s + "\n");
> +  };
> +} else {
> +  debug = function (s) {};
> +}

Just do :
function debug(s) {
  dump("-*- PaymentContentHelper: " + s + "\n");
}

and comment the dump line when needed (also, put that at the beginning of the file),

::: dom/payment/Payment.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const DEBUG = true;
> +
> +let EXPORTED_SYMBOLS = ["PaymentManager"];

No need to export anything since the object is not used outside of the module.

@@ +42,5 @@
> +
> +  // Payment providers data are stored as a preference.
> +  this.registeredProviders = {};
> +  this.registerPaymentProviders();
> +};

Nit: add a blank line

Also, you can just have a :
let PaymentManager = {
  init: function() {
... // do all the init here
  }
}

@@ +57,5 @@
> +   */
> +  receiveMessage: function receiveMessage(msg) {
> +    debug("Received '" + msg.name + "' message from content process");
> +
> +    let self = this;

I don't think you need self anywhere

@@ +74,5 @@
> +        // registered payment providers to get the correct url.
> +        let paymentProviders = [];
> +        let jwtTypes = [];
> +        for (let i in msg.json.jwts) {
> +          let pp = self.getPaymentProvider(msg.json.jwts[i]);

you don't need |self| here

@@ +82,5 @@
> +          // We consider jwt type repetition an error.
> +          if (jwtTypes[pp.typ]) {
> +            let errorMsg = "Only one JWT for each specific Payment Provider" +
> +                           " is allowed";
> +            ppmm.sendAsyncMessage("Payment:Failed", {requestId: self.requestId,

this.requestId

@@ +83,5 @@
> +          if (jwtTypes[pp.typ]) {
> +            let errorMsg = "Only one JWT for each specific Payment Provider" +
> +                           " is allowed";
> +            ppmm.sendAsyncMessage("Payment:Failed", {requestId: self.requestId,
> +                                                     errorMsg: errorMsg});

DOMError messages are not expected to be human readable, but rather enum-like. Maybe "DUPLICATE_JWT_TYPE" for this error?

@@ +90,5 @@
> +          jwtTypes[pp.typ] = true;
> +          paymentProviders.push(pp);
> +        }
> +
> +        if (!paymentProviders[0]) {

Nit: if (!paymentProviders.length)

@@ +92,5 @@
> +        }
> +
> +        if (!paymentProviders[0]) {
> +          let errorMsg = "No valid payment providers found matching the " +
> +                         " introduced JWTs types";

"NO_PAYMENT_PROVIDER" ?

@@ +113,5 @@
> +          this.showPaymentFlow(paymentProviders[0]);
> +          return;
> +        }
> +        self.requestPaymentSelectionId = kOpenPaymentSelectionEvent + "-" +
> +                                         Math.random();

Math.random() is not safe to create a unique id. Use the UUID generator instead.

@@ +118,5 @@
> +        let event = content.document.createEvent("CustomEvent");
> +        event.initCustomEvent("mozChromeEvent", true, true,
> +                              {type: kOpenPaymentSelectionEvent,
> +                               id: self.requestPaymentSelectionId,
> +                               paymentProviders: paymentProviders});

Nit: spaces in JS literals: { type: ....,
                              paymentProviders: paymentProviders }

@@ +132,5 @@
> +      case "Payment:Failed": {
> +        // After receiving the payment provider confirmation about the
> +        // successful or failed payment flow, we notify the UI to close the
> +        // trusted dialog and return to the caller application.
> +        let id = kCloseTrustedDialogEvent + "-" + Math.random();

Same thing here with random()

@@ +135,5 @@
> +        // trusted dialog and return to the caller application.
> +        let id = kCloseTrustedDialogEvent + "-" + Math.random();
> +        let event = content.document.createEvent("CustomEvent");
> +        event.initCustomEvent("mozChromeEvent", true, true,
> +                              {type: kCloseTrustedDialogEvent, id: id});

Nit: js spaces

@@ +147,5 @@
> +          if (evt.detail.id != id) {
> +            debug("mozContentEvent. evt.detail.id != " + id);
> +            return;
> +          }
> +          ppmm.sendAsyncMessage(msg.name, {requestId: self.requestId});

Here you should not use the ppmm to send the message back, since it broadcast to all child processes and we want to avoid that as much as possible. You need to save the |msg.target| when you receive a "Payment:Pay" message and use it there. This means it must be tied to the requestId to not get mismatch in case of simultaneous Pay() calls.

@@ +150,5 @@
> +          }
> +          ppmm.sendAsyncMessage(msg.name, {requestId: self.requestId});
> +          content.removeEventListener("mozContentEvent",
> +                                      closeTrustedDialogReturn);
> +        });

you can bind(this) closeTrustedDialogReturn to not have to use |self|

@@ +163,5 @@
> +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +    let content = browserWin.shell.contentBrowser.contentWindow;
> +    if (!content) {
> +      debug("Could not retrieved most recent contentWindow");
> +      return;

return null;

@@ +207,5 @@
> +        let requestMethod = branch.getCharPref("requestMethod");
> +        this.registeredProviders[type] = {name: name,
> +                                          uri: uri,
> +                                          description: description,
> +                                          requestMethod: requestMethod};

Nit:

this.registeredProviders[type] = {
  name:  branch.getCharPref("name"),
  ...
};

@@ +249,5 @@
> +      }
> +      if (payload.charAt(payload.length - 1) === '"') {
> +        payload = payload.slice(0, -1);
> +      }
> +      payload = payload.replace(/\\/g, '');

ouch... I guess we have no chance to change the payload format to not have this escaping?

@@ +276,5 @@
> +    // We only allow https for payment providers urls.
> +    if (!/^https/.exec(provider.uri)) {
> +      debug("Payment provider urls must be https: " + provider.uri);
> +      return;
> +    }

this fails for HTTPS://MYCAPSLOCKPROVIDER.COM/
you can either use toLowerCase() before testing the uri, or create a nsIURI with Services.io.newURI() and check the scheme.

@@ +284,5 @@
> +    provider.typ = payloadObject.typ;
> +    provider.jwt = jwt;
> +
> +    debug("Payment provider URI: " + provider.uri);
> +    return provider;

Here you return the provider, so in any error case please return an explicit |null|

@@ +318,5 @@
> +    this.showPaymentFlow(selectedProvider);
> +
> +    let content = this.getContent();
> +    if (!content) {
> +      return;

return null;

@@ +346,5 @@
> +                          {type: kOpenPaymentFlowEvent,
> +                           id: this.requestPaymentFlowId,
> +                           url: paymentProvider.uri,
> +                           method: paymentProvider.requestMethod,
> +                           jwt: paymentProvider.jwt});

Nit: spaces

@@ +377,5 @@
> +
> +    // Try to load the payment shim file containing the payment callbacks
> +    // in the content script.
> +    let frameLoader = evt.detail.frame.QueryInterface(
> +                      Ci.nsIFrameLoaderOwner).frameLoader;

nit:
let frameLoader = evt.detail.frame.QueryInterface(Ci.nsIFrameLoaderOwner)
                  .frameLoader;

@@ +390,5 @@
> +    let content = this.getContent();
> +    if (!content) {
> +      return;
> +    }
> +    content.removeEventListener("mozContentEvent", this.loadPaymentShim);

if (content) {
  content.removeEventListener(...)
}

@@ +401,5 @@
> +      for each (let msgname in PAYMENT_IPC_MSG_NAMES) {
> +        ppmm.removeMessageListener(msgname, this);
> +      }
> +      Services.obs.removeObserver(this, "xpcom-shutdown");
> +      ppmm = null;

remove that, this causes issues with other jsm that also use it.

@@ +414,5 @@
> +    dump("-*- PaymentManager: " + s + "\n");
> +  };
> +} else {
> +  debug = function (s) {};
> +}

Same change as in Payment.js

@@ +417,5 @@
> +  debug = function (s) {};
> +}
> +
> +let paymentManager = new PaymentManager();
> +paymentManager.init();

Just do PaymentManager.init();
Comment 64 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-30 05:01:32 PDT
Created attachment 647130 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v4

Thanks a lot for your review Fabrice! :)

I think this addresses all your review comments, except for the one regarding the implicit_jscontext usage for the reasons that we commented via IRC.
Comment 65 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-30 05:05:35 PDT
Created attachment 647133 [details] [diff] [review]
Part 3: B2G implementation v6

This patch includes the modifications required to address most of your review comments.

I've also added the lazy initialization of the payment providers list so it doesn't affect the boot process.
Comment 66 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-30 06:32:05 PDT
Comment on attachment 647130 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v4

I am asking Johnny Stenback r? for the DOM part as suggested on the previous comments.
Comment 67 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-30 10:33:49 PDT
Comment on attachment 647130 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v4

We have a mechanism in place where you can simply register a component with the category manager and have it automatically become available on the navigator object for all windows. Any reason not to use that here, and write the hookup code by hand instead? The approach I'm talking about also uses nsIDOMGlobalPropertyInitializer, but only implements it, it's called by existing code if registered appropriately. See patch in bug 753239 for an example.

r- assuming that is an acceptable approach here.
Comment 68 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-30 10:38:56 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #67)
> Comment on attachment 647130 [details] [diff] [review]
> Part 2: Expose 'pay' function to 'navigator' v4
> 
> We have a mechanism in place where you can simply register a component with
> the category manager and have it automatically become available on the
> navigator object for all windows. Any reason not to use that here, and write
> the hookup code by hand instead? The approach I'm talking about also uses
> nsIDOMGlobalPropertyInitializer, but only implements it, it's called by
> existing code if registered appropriately. See patch in bug 753239 for an
> example.
> 
> r- assuming that is an acceptable approach here.
Thanks for the review Johnny!

If you are referring to 'JavaScript-navigator-property', I tried using it, but then I find out that it is only for registering properties, not functions.
Comment 69 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-30 14:31:36 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #68)
> If you are referring to 'JavaScript-navigator-property', I tried using it,
> but then I find out that it is only for registering properties, not
> functions.

That's what I'm talking about, and it *should* work for functions as well, *as long as* you return a function from the init() call on your implementation of nsIDOMGlobalPropertyInitializer then that function will be the value of navigator.pay, and navigator.pay() should work.

Let me know if that does not work, and we'll figure out where to go from there.
Comment 70 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-31 04:07:47 PDT
Created attachment 647490 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator' v5

You were absolutely right. It works returning the pay() function from 'nsIDOMGlobalPropertyInitializer.init()'. Thanks!
Comment 71 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-31 04:08:41 PDT
Created attachment 647492 [details] [diff] [review]
Part 3: B2G implementation v6
Comment 72 [:fabrice] Fabrice Desré 2012-07-31 08:34:37 PDT
Comment on attachment 647492 [details] [diff] [review]
Part 3: B2G implementation v6

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

::: b2g/app/b2g.js
@@ +490,5 @@
> +pref("dom.payment.provider.2.name", "mockpayprovider");
> +pref("dom.payment.provider.2.description", "Mock Payment Provider");
> +pref("dom.payment.provider.2.type", "mock/payments/inapp/v1");
> +pref("dom.payment.provider.2.uri", "https://mockpayprovider.phpfogapp.com/?req=");
> +pref("dom.payment.provider.2.requestMethod", "GET");

We can't hardcode all these providers here. Custom ones can be added to user.js on the gaia side.

::: dom/payment/Payment.js
@@ +20,5 @@
> +                                   "@mozilla.org/childprocessmessagemanager;1",
> +                                   "nsIFrameMessageManager");
> +
> +function debug (s) {
> +  dump("-*- PaymentContentHelper: " + s + "\n");

Nit: turn this off before landing

@@ +64,5 @@
> +  },
> +
> +  // nsIFrameMessageListener
> +
> +  receiveMessage: function receiveMessage(msg) {

Nit: receiveMessage(aMessage) {
 let msg = aMessage.json; // and use that after

::: dom/payment/Payment.jsm
@@ +58,5 @@
> +
> +  /**
> +   * Process a message from the content process.
> +   */
> +  receiveMessage: function receiveMessage(msg) {

Nit : s/msg/aMessage

@@ +139,5 @@
> +        // Once the user makes his choice, we can ask Gaia to create a trusted
> +        // dialog containing the selected payment provider buy flow.
> +        content.addEventListener("mozContentEvent",
> +                                 this.onPaymentProviderSelection.bind(this));
> +        content.dispatchEvent(event);

So, I should have caught that sooner but this is very b2g specific, and will not work at all on desktop or android. We need to abstract out the b2g specific parts into some glue interface, and implement it in b2g/components for b2g.

@@ +188,5 @@
> +      debug("Could not retrieved most recent contentWindow");
> +      return null;
> +    }
> +    return content;
> +  },

This is b2g specific, so should not be there.

@@ +240,5 @@
> +  /**
> +   * Helper function to get the payment provider info according to the jwt
> +   * type. Payment provider's data is stored as a preference.
> +   */
> +  getPaymentProvider: function getPaymentProvider(jwt) {

Nit: aJwt

@@ +311,5 @@
> +  /**
> +   * Helper function to handle the user's selection about the payment
> +   * provider via UI.
> +   */
> +  onPaymentProviderSelection: function onPaymentProviderSelection(evt) {

Nit: aEvent - but that will probably disappear like getContent()

@@ +349,5 @@
> +  /**
> +   * Helper to send a chrome event asking gaia to show the payment provider
> +   * buy flow.
> +   */
> +  showPaymentFlow: function showPaymentFlow(paymentProvider) {

nit: aPaymentProvider
Comment 73 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-07-31 09:44:19 PDT
(In reply to Fabrice Desré [:fabrice] from comment #72)
> @@ +139,5 @@
> > +        // Once the user makes his choice, we can ask Gaia to create a trusted
> > +        // dialog containing the selected payment provider buy flow.
> > +        content.addEventListener("mozContentEvent",
> > +                                 this.onPaymentProviderSelection.bind(this));
> > +        content.dispatchEvent(event);
> 
> So, I should have caught that sooner but this is very b2g specific, and will
> not work at all on desktop or android. We need to abstract out the b2g
> specific parts into some glue interface, and implement it in b2g/components
> for b2g.
>

Ouch, that is unfortunate :\. As I told you via IRC, I thought about landing this and then create the glue interface to allow other implementations, as there's only going to be a b2g implementation for v1.

Anyway, it's ok :) I'll work on the glue interface right away.

I am clearing the r? flag for the idl, as it needs to be changed.
Comment 74 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-02 05:48:18 PDT
Created attachment 648323 [details] [diff] [review]
Part 1: IDLs v3

New IDLs with the glue interface addition.
Comment 75 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-02 05:49:33 PDT
Created attachment 648325 [details] [diff] [review]
Part 3: DOM implementation

DOM implementation with the glue interface addition
Comment 76 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-02 05:50:20 PDT
Created attachment 648326 [details] [diff] [review]
Part 4: B2G implementation

B2G specific implementation
Comment 77 [:fabrice] Fabrice Desré 2012-08-02 15:52:59 PDT
Comment on attachment 648325 [details] [diff] [review]
Part 3: DOM implementation

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

r=me with the comments addressed.

::: dom/payment/Payment.jsm
@@ +57,5 @@
> +      this.requestId = msg.requestId;
> +    }
> +
> +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +    let content = browserWin.shell.contentBrowser.contentWindow;

This is b2g specific, and you don't use it anymore.

@@ +119,5 @@
> +          this.showPaymentFlow(paymentProviders[0]);
> +          return;
> +        }
> +        let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> +                   .createInstance(Ci.nsIPaymentUIGlue);

Check if this succeeds before using it.

@@ +270,5 @@
> +    paymentFlowInfo.url = aPaymentProvider.uri;
> +    paymentFlowInfo.requestMethod = aPaymentProvider.requestMethod;
> +    paymentFlowInfo.jwt = aPaymentProvider.jwt;
> +    let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> +               .createInstance(Ci.nsIPaymentUIGlue);

Here also, check that creating the glue succeeds.

::: dom/payment/PaymentFlowInfo.js
@@ +16,5 @@
> +
> +PaymentFlowInfo.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPaymentFlowInfo]),
> +  classID:        PAYMENTFLOWINFO_CID,
> +  classInfo:      XPCOMUtils.generateCI({

No need for classInfo for objects that are not exposed to web content.

@@ +45,5 @@
> +  },
> +
> +  set requestMethod(aValue) {
> +    this._requestMethod = aValue;
> +  }

If these are just simple properties, you don't even need specific getters and setters.

just use PaymentFlowInfo.prototype = {
  QueryInterface: ...,
  classId: ...,
  url: null,
  jwt: null,
  requestMethod: null
}
Comment 78 [:fabrice] Fabrice Desré 2012-08-02 16:23:45 PDT
Comment on attachment 648326 [details] [diff] [review]
Part 4: B2G implementation

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

Only minor stuff, apart from the issue of storing the list of providers as prefs in b2g.js

::: b2g/app/b2g.js
@@ +486,5 @@
> +pref("dom.payment.provider.1.description", "Telefonica BlueVia");
> +pref("dom.payment.provider.1.type", "tu.com/payments/inapp/v1");
> +pref("dom.payment.provider.1.uri", "https://m.connect.qa-opentel-04.hi.inet/pt-br/inapp-pay/pay_start?req=");
> +pref("dom.payment.provider.1.requestMethod", "GET");
> +

we need a decision on what to include here, if any.

::: b2g/chrome/content/payment.js
@@ +17,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> +                                   "@mozilla.org/childprocessmessagemanager;1",
> +                                   "nsIFrameMessageManager");
> +
> +const kCloseTrustedDialogEvent = 'close-trusted-dialog';

The name is a bit generic. Is gaia reusing the same dialog for payments and something else? If it's a payment only thing, I would prefer "close-payment-dialog" (and use double quotes).

@@ +64,5 @@
> +      return;
> +    }
> +    aCallback();
> +    content.removeEventListener("mozContentEvent",
> +                                closeTrustedDialogReturn);

remove the event listener before calling the callback

::: b2g/components/PaymentGlue.js
@@ +15,5 @@
> +
> +// Type of MozChromEvents to handle Gaia trusted dialogs.
> +const kOpenPaymentSelectionEvent = 'open-payment-selection-dialog';
> +const kOpenPaymentFlowEvent = 'open-payment-flow-dialog';
> +

Nit: double quotes on strings

@@ +45,5 @@
> +      paymentProviders: aProviders
> +    });
> +
> +    // Once the user makes his choice, we can ask Gaia to create a trusted
> +    // dialog containing the selected payment provider buy flow.

Nit: we're not creating the dialog in this function, so the comment is a bit misleading.

@@ +58,5 @@
> +              "any payment flow!");
> +        return;
> +      }
> +      aCallback.handleUserSelection(userSelection);
> +      content.removeEventListener("mozContentEvent", handleSelection);

remove the event listener before calling the callback.

@@ +116,5 @@
> +  },
> +
> +  getContent: function getContent() {
> +    let browserWin = Services.wm.getMostRecentWindow("navigator:browser");
> +    let content = browserWin.shell.contentBrowser.contentWindow;

use let content = browserWin.getContentWindow(); (and in other places also)
Comment 79 [:fabrice] Fabrice Desré 2012-08-02 16:24:15 PDT
Comment on attachment 648323 [details] [diff] [review]
Part 1: IDLs v3

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

Also, we should probably prefix the API to be mozPay

::: dom/payment/interfaces/nsIPaymentFlowInfo.idl
@@ +8,5 @@
> +interface nsIPaymentFlowInfo : nsISupports
> +{
> +    attribute DOMString url;
> +    attribute DOMString jwt;
> +    attribute DOMString requestMethod;

Can you add comments on what are these properties?
Comment 80 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-03 02:02:31 PDT
(In reply to Fabrice Desré [:fabrice] from comment #78)
> Comment on attachment 648326 [details] [diff] [review]
> Part 4: B2G implementation
> 
> Review of attachment 648326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/payment.js
> @@ +17,5 @@
> > +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> > +                                   "@mozilla.org/childprocessmessagemanager;1",
> > +                                   "nsIFrameMessageManager");
> > +
> > +const kCloseTrustedDialogEvent = 'close-trusted-dialog';
> 
> The name is a bit generic. Is gaia reusing the same dialog for payments and
> something else? If it's a payment only thing, I would prefer
> "close-payment-dialog" (and use double quotes).
>

The trusted dialog should be used for other cases not related to payment. Currently the only implemented case in Gaia is the payment one, but that should change soon.
Comment 81 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-03 02:34:30 PDT
(In reply to Fabrice Desré [:fabrice] from comment #79)
> Comment on attachment 648323 [details] [diff] [review]
> Part 1: IDLs v3
> 
> Review of attachment 648323 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, we should probably prefix the API to be mozPay
> 

I am ok with that. I asked about it in IRC to Mounir but he redirected me to Jonas.
Jonas, do you agree with prefixing the API to be mozPay?
Comment 82 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-03 04:03:54 PDT
Created attachment 648659 [details] [diff] [review]
Part 1: IDLs v3

Comments added to IDL
Comment 83 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-03 04:05:41 PDT
Created attachment 648660 [details] [diff] [review]
Part 3: DOM implementation

r=fabrice

Fabrice's comments addressed
Comment 84 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-06 03:53:47 PDT
Created attachment 649231 [details] [diff] [review]
Part 3: DOM implementation

With the latest changes the payment provider list is get from an json located on an URL stored as a preference (dom.payment.providersURL). Currently, I am adding this preference and the json containing the payment providers info to Gaia, but this could change in the future.
Comment 85 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-06 03:55:01 PDT
Created attachment 649232 [details] [diff] [review]
Part 4: B2G implementation

No more payment providers info as preferences for now.
Comment 86 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-06 03:59:00 PDT
Created attachment 649235 [details]
Payment providers data

This is the list of payment providers that I am using for testing.
To use it, you need to locate it remotely or localy in Gaia and provide the location URL as the value of the dom.payment.providersURL preference.
Comment 87 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-06 04:05:16 PDT
Created attachment 649237 [details] [diff] [review]
Part 4: B2G implementation

Sorry, for the spam. I only fixed a nit.
Comment 88 Axel Nennker 2012-08-06 07:58:11 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #85)
> Created attachment 649232 [details] [diff] [review]
> Part 4: B2G implementation
> 
> No more payment providers info as preferences for now.

I suggest to give the payment providers an API to register themselves.

A payment provider would then write an addon, the user would download
and install it, and the addon would register with with the
navigator.pay implementation on some event.
Maybe this is too much Firefox-like thinking?
This way you don't have to store any information about the payment
providers not even an url to the list because each provider would register itself. When the user
deinstalls the provider is "gone" automatically (or through an
unregister event/call)

Axel
Comment 89 Kumar McMillan [:kumar] (needinfo all the things) 2012-08-06 09:03:00 PDT
(In reply to Axel Nennker from comment #88)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #85)
> > Created attachment 649232 [details] [diff] [review]
> > Part 4: B2G implementation
> > 
> > No more payment providers info as preferences for now.
> 
> I suggest to give the payment providers an API to register themselves.

This is out of scope for V1 but, yeah, it has always been our ultimate vision. The challenge is in establishing a trusted relationship between the merchant (the app) and the payment provider. If we open that up to anyone on the web we need more APIs that engage the merchant and consumer in an agreement before processing the payment.
Comment 90 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-06 09:04:28 PDT
(In reply to Axel Nennker from comment #88)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #85)
> > Created attachment 649232 [details] [diff] [review]
> > Part 4: B2G implementation
> > 
> > No more payment providers info as preferences for now.
> 
> I suggest to give the payment providers an API to register themselves.
> 

I started a thread to discuss about where and how to store the payment providers info at https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.b2g/HKnUHcbsAm0

As I comment there, I don't think that exposing an API to register payment providers would be an option for security reasons. Gecko need to have a trusted relationship with the payment providers. The users may be required to enter personal information (let's say his PayPal user and password, for example), and letting for example evil.paypal.com register themselve without any control or revision process IMHO would be pretty bad.

> A payment provider would then write an addon, the user would download
> and install it, and the addon would register with with the
> navigator.pay implementation on some event.
> Maybe this is too much Firefox-like thinking?
> This way you don't have to store any information about the payment
> providers not even an url to the list because each provider would register
> itself. When the user
> deinstalls the provider is "gone" automatically (or through an
> unregister event/call)
> 

AFAIK B2G v1 is not allowing add-ons instalation. But I might be wrong.

Anyway, storing the payment providers list URL as a preference still doesn't convince me, again for security reasons. If I am not wrong, add-ons may be able to modify the preference, even if it is a locked preference http://kb.mozillazine.org/Locking_preferences ...

> Axel

Thanks Axel!
Comment 91 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-07 10:14:07 PDT
Created attachment 649682 [details] [diff] [review]
Part 3: DOM implementation

As per https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.b2g/HKnUHcbsAm0 I am reverting this patch to the previous state. Payment providers information is read from preferences.

This patch was already reviewed and approved by Fabrice, so I am adding the r+ flag. But feel free to review it again :)
Comment 92 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-07 10:15:54 PDT
Created attachment 649683 [details] [diff] [review]
Part 4: B2G implementation
Comment 93 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-07 10:26:41 PDT
Created attachment 649685 [details]
Payment providers data

An example of payment providers as preferences
Comment 94 [:fabrice] Fabrice Desré 2012-08-08 17:46:53 PDT
Comment on attachment 649683 [details] [diff] [review]
Part 4: B2G implementation

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

Nothing major, but I'd like to see the final version.

::: b2g/components/PaymentGlue.js
@@ +57,5 @@
> +      if (!userSelection) {
> +        debug("No user selection was returned from content. Cannot load " +
> +              "any payment flow!");
> +        return;
> +      } 

nit: whitespace

@@ +58,5 @@
> +        debug("No user selection was returned from content. Cannot load " +
> +              "any payment flow!");
> +        return;
> +      } 
> +      content.removeEventListener("mozContentEvent", handleSelection);

you also need to remove it when returning early.

@@ +66,5 @@
> +  },
> +
> +  showPaymentFlow: function showPaymentFlow(aPaymentFlowInfo) {
> +    debug("showPaymentFlow. url " + aPaymentFlowInfo.url);
> +    // We ask Gaia to browse to the selected payment flow.

nit: Remove all mentions of Gaia. This could be another frontend.

@@ +89,5 @@
> +    // content.
> +    content.addEventListener("mozContentEvent", function loadPaymentShim(evt) {
> +      if (evt.detail.id != id) {
> +        debug("mozContentEvent. evt.detail.id != " + id);
> +        return;

remove the event listener before returning here.

@@ +108,5 @@
> +      try {
> +        mm.loadFrameScript(kPaymentShimFile, true);
> +      } catch (e) {
> +        debug("Error loading " + kPaymentShimFile + " as a frame script: " + e);
> +        return;

you don't remove the event listener here.

@@ +110,5 @@
> +      } catch (e) {
> +        debug("Error loading " + kPaymentShimFile + " as a frame script: " + e);
> +        return;
> +      }
> +      content.removeEventListener("mozContentEvent", loadPaymentShim);

put that in a finally { }
Comment 95 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-09 04:38:56 PDT
Created attachment 650508 [details] [diff] [review]
Part 4: B2G implementation
Comment 96 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-09 08:08:41 PDT
Created attachment 650561 [details] [diff] [review]
Part 3: DOM implementation

Wrong patch
Comment 97 [:fabrice] Fabrice Desré 2012-08-09 15:18:20 PDT
Comment on attachment 650508 [details] [diff] [review]
Part 4: B2G implementation

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

r=me with nits addressed.

::: b2g/chrome/content/payment.js
@@ +65,5 @@
> +                                  closeTrustedDialogReturn);
> +      return;
> +    }
> +    content.removeEventListener("mozContentEvent",
> +                                closeTrustedDialogReturn);

what about doing:
if (evt.detail.id == id && aCallback) {
  aCallback();
}

content.removeEventListener(...);

::: b2g/components/PaymentGlue.js
@@ +62,5 @@
> +        return;
> +      }
> +
> +      content.removeEventListener("mozContentEvent", handleSelection);
> +      aCallback.handleUserSelection(userSelection);

same as in payment.js: we can save a removeEventListener, and check for callback being null

@@ +91,5 @@
> +    // callbacks for firing DOMRequest events can be loaded on its
> +    // content.
> +    content.addEventListener("mozContentEvent", function loadPaymentShim(evt) {
> +      if (evt.detail.id != id) {
> +        debug("mozContentEvent. evt.detail.id != " + id);        

nit: whitespace at the end of the line.

@@ +103,5 @@
> +              "callbacks!");
> +        content.removeEventListener("mozContentEvent", loadPaymentShim);
> +        return;
> +      }
> +

you could group booth of these in:
if (evt.detail.id != id || !evt.detail.frame) {
  content.removeEventListener("mozContentEvent", loadPaymentShim);
  return;
}
Comment 98 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-10 05:40:53 PDT
Created attachment 650853 [details] [diff] [review]
Part 2: Expose 'pay' function to 'navigator'

Moving from navigator.pay to navigator.mozPay

Also, after Bug 742795 landed, I had to remove the autoconf.mk.in changes. I am asking jst r? again because of that.
Comment 99 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-10 05:43:33 PDT
Created attachment 650854 [details] [diff] [review]
Part 3: DOM implementation

r=fabrice

I had to rebase the patch after prefixing mozPay
Comment 100 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-10 05:44:43 PDT
Created attachment 650855 [details] [diff] [review]
Part 4: B2G implementation

r=fabrice

Review comments addressed.
Comment 101 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-10 05:55:24 PDT
Created attachment 650858 [details] [diff] [review]
Part 4: B2G implementation

r=fabrice

Sorry, the try push message changed the patch header
Comment 102 David Chan [:dchan] 2012-08-10 08:58:14 PDT
Comment on attachment 650854 [details] [diff] [review]
Part 3: DOM implementation

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

::: dom/payment/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEPTH     = ../..

I believe we're moving to use @DEPTH@ now. See bug 774032

::: dom/payment/Payment.jsm
@@ +173,5 @@
> +      }
> +      try {
> +        let type = branch.getCharPref("type");
> +        if (type in this.registeredProviders) {
> +          return;

Should we skip over duplicates rather than return?

::: dom/payment/interfaces/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEPTH     = ../../..

@DEPTH@
Comment 103 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-12 09:55:36 PDT
Created attachment 651205 [details] [diff] [review]
Part 3: DOM implementation

Thanks David!
Comment 104 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-16 10:08:05 PDT
It should be a [LOE:S], but I am marking this as [LOE:M] cause it depends on the suggestions that could appear from the revision process.
Comment 105 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-17 21:44:27 PDT
Created attachment 653016 [details] [diff] [review]
Part 1: IDLs

As requested in https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.b2g/YGITHnnjh0M[1-25], now there's always a user confirmation step on Gecko side.
Comment 106 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-17 21:45:48 PDT
Created attachment 653017 [details] [diff] [review]
Part 3: DOM implementation
Comment 107 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-17 21:47:31 PDT
Created attachment 653018 [details] [diff] [review]
Part 4: B2G implementation
Comment 108 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-17 21:51:04 PDT
The current Gaia implementation needs to be updated to support the new confirmation screen. I'll be sending a PR for this tomorrow (pending on UX and visual design input).
Comment 109 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-18 07:56:52 PDT
Created attachment 653061 [details] [diff] [review]
Part 1: IDLs

nsIPaymentRequestInfo -> nsIDOMPaymentRequestInfo
nsIPaymentProductPrice -> nsIDOMPaymentProductPrice
Comment 110 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-18 07:58:58 PDT
Created attachment 653062 [details] [diff] [review]
Part 3: DOM implementation

nsIPaymentRequestInfo -> nsIDOMPaymentRequestInfo
nsIPaymentProductPrice -> nsIDOMPaymentProductPrice
Comment 111 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-20 09:17:39 PDT
Comment on attachment 653018 [details] [diff] [review]
Part 4: B2G implementation

I am including a few changes during the next few hours, so I am clearing the r? flags.
Comment 112 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-20 15:40:11 PDT
Created attachment 653554 [details] [diff] [review]
Part 1: IDLs
Comment 113 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-20 15:41:00 PDT
Created attachment 653555 [details] [diff] [review]
Part 3: DOM implementation
Comment 114 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-20 15:41:42 PDT
Created attachment 653557 [details] [diff] [review]
Part 4: B2G implementation
Comment 115 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-22 12:28:12 PDT
Created attachment 654315 [details] [diff] [review]
Part 3: DOM implementation

I've only modified an error check and message.
Comment 116 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-23 03:01:52 PDT
Created attachment 654557 [details] [diff] [review]
Part 4: B2G implementation

Send mozChromeEvents via |shell.sendChromeEvent|
Comment 117 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-23 08:54:53 PDT
Created attachment 654644 [details] [diff] [review]
Part 4: B2G implementation
Comment 118 [:fabrice] Fabrice Desré 2012-08-24 13:18:18 PDT
Comment on attachment 654315 [details] [diff] [review]
Part 3: DOM implementation

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

::: dom/payment/Payment.js
@@ +28,5 @@
> +};
> +
> +PaymentContentHelper.prototype = {
> +  __proto__: DOMRequestIpcHelper.prototype,
> +

Don't we also need :
__exposedProps__: {
                    pay: "r"
                  }

::: dom/payment/Payment.jsm
@@ +106,5 @@
> +        // that he prefers.
> +        let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> +                   .createInstance(Ci.nsIPaymentUIGlue);
> +        if (!glue) {
> +          debug("Could not create nsIPaymentUIGlue instance");

We should also fire a Payment:Failed here, no?

@@ +306,5 @@
> +    request.type = payloadObject.typ;
> +
> +    // We finally add the jwt content to the provider information for later
> +    // ussage while opening the payment flow.
> +    provider.jwt = aJwt;

that means that we can't have two concurrent payment flows running with the same provider, right?

@@ +308,5 @@
> +    // We finally add the jwt content to the provider information for later
> +    // ussage while opening the payment flow.
> +    provider.jwt = aJwt;
> +
> +    return request;

This function returns null for lots of different reasons, but there's no way for the caller to distinguish them. I wonder if this is good enough.

@@ +314,5 @@
> +
> +  showPaymentFlow: function showPaymentFlow(aPaymentProvider) {
> +    let paymentFlowInfo = Cc["@mozilla.org/payment/flow-info;1"]
> +                          .createInstance(Ci.nsIPaymentFlowInfo);
> +    paymentFlowInfo.url = aPaymentProvider.uri;

Please use uri everywhere.

@@ +321,5 @@
> +    let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> +               .createInstance(Ci.nsIPaymentUIGlue);
> +    if (!glue) {
> +      debug("Could not create nsIPaymentUIGlue instance");
> +      return;

return false;

@@ +323,5 @@
> +    if (!glue) {
> +      debug("Could not create nsIPaymentUIGlue instance");
> +      return;
> +    }
> +    glue.showPaymentFlow(paymentFlowInfo);

is this a blocking call? the caller should be able to know when the glue failed and fire a Payment:Fail, but this is not possible currently.

::: dom/payment/PaymentRequestInfo.js
@@ +97,5 @@
> +    interfaces: [Ci.nsIDOMPaymentRequestRefundInfo]
> +  }),
> +  reason: null
> +};
> +

We need __exposedProps__ for all xpcom objects exposed to content.
Comment 119 [:fabrice] Fabrice Desré 2012-08-24 13:35:02 PDT
Comment on attachment 654644 [details] [diff] [review]
Part 4: B2G implementation

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

r- just because it'd really like to not use cpmm there.

::: b2g/chrome/content/payment.js
@@ +21,5 @@
> +const kClosePaymentFlowEvent = "close-payment-flow-dialog";
> +
> +function paymentSuccess(aResult) {
> +  closePaymentFlowDialog(function notifySuccess() {
> +    cpmm.sendAsyncMessage("Payment:Success", { result: aResult });

Can you check if you need cpmm at all? the message manager shoud be the global scope in a frame script.

::: b2g/chrome/jar.mn
@@ +21,5 @@
>    content/content.css                   (content/content.css)
>    content/touchcontrols.css             (content/touchcontrols.css)
>  
> +  content/payment.js                    (content/payment.js)
> +  

Nit: whitespace

::: dom/payment/Payment.jsm
@@ +25,5 @@
>                                     "@mozilla.org/preferences-service;1",
>                                     "nsIPrefService");
>  
>  function debug (s) {
> +  dump("-*- PaymentManager: " + s + "\n");

oops! :)
Comment 120 Philipp von Weitershausen [:philikon] 2012-08-27 13:59:04 PDT
Comment on attachment 654644 [details] [diff] [review]
Part 4: B2G implementation

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

Drive-by

::: b2g/chrome/content/payment.js
@@ +15,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> +                                   "@mozilla.org/childprocessmessagemanager;1",
> +                                   "nsIFrameMessageManager");

Use nsIMessageSender now, please.

@@ +36,5 @@
> +  // After receiving the payment provider confirmation about the
> +  // successful or failed payment flow, we notify the UI to close the
> +  // payment flow dialog and return to the caller application.
> +  let randomId = Cc["@mozilla.org/uuid-generator;1"]
> +                 .getService(Ci.nsIUUIDGenerator)

Lazy getter for this service please.
Comment 121 Philipp von Weitershausen [:philikon] 2012-08-27 14:01:08 PDT
Comment on attachment 654315 [details] [diff] [review]
Part 3: DOM implementation

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

::: dom/payment/Payment.jsm
@@ +18,5 @@
> +const PREF_PAYMENTPROVIDERS_BRANCH = "dom.payment.provider.";
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "ppmm",
> +                                   "@mozilla.org/parentprocessmessagemanager;1",
> +                                   "nsIFrameMessageManager");

Use nsIMessageListenerManager now, please.

@@ +67,5 @@
> +
> +        // We save the message target message manager so we can later dispatch
> +        // back messages without broadcasting to all child processes.
> +        let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);
> +        this.messageManagers[this.requestId] = mm;

The QueryInterface() is now no longer necessary, you can directly save aMessage.target.
Comment 122 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-27 21:29:34 PDT
Comment on attachment 653554 [details] [diff] [review]
Part 1: IDLs

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

I'm not sure I understand where all of these APIs are going to be exposed? Is all but the nsIDOMNavigatorPayment interface only exposed to the system app?
Comment 123 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 11:07:19 PDT
Created attachment 656108 [details] [diff] [review]
Part 1: IDLs
Comment 124 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 11:10:16 PDT
Created attachment 656110 [details] [diff] [review]
Part 3: DOM implementation
Comment 125 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 11:11:20 PDT
Created attachment 656111 [details] [diff] [review]
Part 4: B2G implementation
Comment 126 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 13:11:26 PDT
(In reply to Fabrice Desré [:fabrice] from comment #118)
> Comment on attachment 654315 [details] [diff] [review]
> Part 3: DOM implementation
> 
> Review of attachment 654315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payment/Payment.js
> @@ +28,5 @@
> > +};
> > +
> > +PaymentContentHelper.prototype = {
> > +  __proto__: DOMRequestIpcHelper.prototype,
> > +
> 
> Don't we also need :
> __exposedProps__: {
>                     pay: "r"
>                   }
> 

Done. I've added the __exposedProps__ to all XPCOM objects exposed to content.

> ::: dom/payment/Payment.jsm
> @@ +106,5 @@
> > +        // that he prefers.
> > +        let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> > +                   .createInstance(Ci.nsIPaymentUIGlue);
> > +        if (!glue) {
> > +          debug("Could not create nsIPaymentUIGlue instance");
> 
> We should also fire a Payment:Failed here, no?

Done. I've also added the |paymentFailed| helper to avoid code repetition.

> 
> @@ +306,5 @@
> > +    request.type = payloadObject.typ;
> > +
> > +    // We finally add the jwt content to the provider information for later
> > +    // ussage while opening the payment flow.
> > +    provider.jwt = aJwt;
> 
> that means that we can't have two concurrent payment flows running with the
> same provider, right?
> 

Now the JWT goes within the payment request, so it can be recovered once the user confirm the requests and makes his choice.

> @@ +308,5 @@
> > +    // We finally add the jwt content to the provider information for later
> > +    // ussage while opening the payment flow.
> > +    provider.jwt = aJwt;
> > +
> > +    return request;
> 
> This function returns null for lots of different reasons, but there's no way
> for the caller to distinguish them. I wonder if this is good enough.
> 

We already discuss this and agreed that it is not necessary to provide any other error information.

> @@ +314,5 @@
> > +
> > +  showPaymentFlow: function showPaymentFlow(aPaymentProvider) {
> > +    let paymentFlowInfo = Cc["@mozilla.org/payment/flow-info;1"]
> > +                          .createInstance(Ci.nsIPaymentFlowInfo);
> > +    paymentFlowInfo.url = aPaymentProvider.uri;
> 
> Please use uri everywhere.
> 

Done.

> @@ +321,5 @@
> > +    let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> > +               .createInstance(Ci.nsIPaymentUIGlue);
> > +    if (!glue) {
> > +      debug("Could not create nsIPaymentUIGlue instance");
> > +      return;
> 
> return false;
> 
Done.

> @@ +323,5 @@
> > +    if (!glue) {
> > +      debug("Could not create nsIPaymentUIGlue instance");
> > +      return;
> > +    }
> > +    glue.showPaymentFlow(paymentFlowInfo);
> 
> is this a blocking call? the caller should be able to know when the glue
> failed and fire a Payment:Fail, but this is not possible currently.
> 

I've added an error callback function. And also added success and error callbacks to the nsIPaymentUIGlue.confirmPaymentRequest function.

> ::: dom/payment/PaymentRequestInfo.js
> @@ +97,5 @@
> > +    interfaces: [Ci.nsIDOMPaymentRequestRefundInfo]
> > +  }),
> > +  reason: null
> > +};
> > +
> 
> We need __exposedProps__ for all xpcom objects exposed to content.

Done.
Comment 127 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 13:13:15 PDT
I also addressed philikon's comments.
Comment 128 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 14:33:45 PDT
Created attachment 656214 [details] [diff] [review]
Part 4: B2G implementation

I only added information for an error case.
Comment 129 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-28 18:35:55 PDT
Created attachment 656296 [details] [diff] [review]
Part 4: B2G implementation

Nits
Comment 130 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-08-29 05:50:15 PDT
Created attachment 656409 [details] [diff] [review]
Part 3: DOM implementation

Removed all the __exposedProps__
Comment 131 [:fabrice] Fabrice Desré 2012-08-29 11:39:29 PDT
Comment on attachment 656296 [details] [diff] [review]
Part 4: B2G implementation

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

r=me with nit addressed.

::: b2g/components/PaymentGlue.js
@@ +120,5 @@
> +
> +  getRandomId: function getRandomId() {
> +    return Cc["@mozilla.org/uuid-generator;1"]
> +           .getService(Ci.nsIUUIDGenerator)
> +           .generateUUID().toString();

nit: use a lazy getter here also.
Comment 133 Andreas Gal :gal 2012-08-29 15:31:55 PDT
\o/
Comment 135 Jason Smith [:jsmith] 2012-09-04 16:13:13 PDT
Testing is blocked on this until the following issues are resolved:

- https://bugzilla.mozilla.org/show_bug.cgi?id=787542
- https://github.com/mozilla-b2g/gaia/issues/4322
- Marketplace support landed for paid apps application identifiers and APP_SECRET
Comment 136 Jason Smith [:jsmith] 2012-09-14 17:58:04 PDT
I'm blocked on testing - need a response from Fernando on why the test case I tried (sent over email) isn't working.
Comment 137 Antonio Manuel Amaya Calvo (:amac) 2012-09-14 19:51:29 PDT
Sent you a modified preferences file so the testing provider can work. Hope that unblocks the issue.
Comment 138 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-09-15 11:44:38 PDT
(In reply to Jason Smith [:jsmith] from comment #136)
> I'm blocked on testing - need a response from Fernando on why the test case
> I tried (sent over email) isn't working.

I've sent you via email the reason of why the test case wasn't working.

(In reply to Antonio Manuel Amaya Calvo from comment #137)
> Sent you a modified preferences file so the testing provider can work. Hope
> that unblocks the issue.

Thanks Antonio! :)
No need to change the preferences file though, just the test case :)
Comment 139 Jason Smith [:jsmith] 2012-09-18 00:15:50 PDT
Initial testing showed that I was able to get the trusted UI to come up, but I ended up getting hit by https://github.com/mozilla-b2g/gaia/issues/4843 in the process (which is likely multiple bugs in one, feel free to break this up).
Comment 140 Jason Smith [:jsmith] 2012-09-21 15:33:28 PDT
I realized what went wrong - it's on my end.

Anyways, at the API level this is okay, although there's a problem at the trusted UI level. Marking as verified to say that I've tested this at a proof of concept level. I'll be sure to do some more digging and will file bugs as appropriate.
Comment 141 Jason Smith [:jsmith] 2012-09-30 08:31:45 PDT
*** Bug 776678 has been marked as a duplicate of this bug. ***
Comment 142 David Bruant 2013-02-20 03:49:50 PST
As far as documentation is concerned, we have:
* https://wiki.mozilla.org/WebAPI/WebPayment (protocol and complete view)
** http://openid.net/specs/draft-jones-json-web-token-07.html (not on our side, but important to keep around)
* https://developer.mozilla.org/en-US/docs/Apps/Publishing/In-app_payments (more app developer-oriented than the wikimo page)
* https://developer.mozilla.org/en-US/docs/DOM/navigator.pay (more a placeholder than anything else)

If it hasn't happened recently, can someone with excellent knowledge of the payment workflow and API review these materials and say if something is obviously missing or wrong.

Also, for now, it is not entirely clear to me how developers will be able to test the API without making actual payments. What's the testing story?

Thanks.
Comment 143 Jason Smith [:jsmith] 2013-02-20 08:04:11 PST
needinfo on Kumar for comment 142
Comment 144 Jeremie Patonnier :Jeremie 2013-04-11 03:45:55 PDT
Hi,

navigator.mozPay is documented here : 

 - https://developer.mozilla.org/en-US/docs/DOM/window.navigator.mozPay

About the payment testing process, it's all about having a payment key for testing as stated here: https://developer.mozilla.org/en-US/docs/Apps/Publishing/In-app_payments#Obtain_a_payment_key_for_testing
Comment 145 Kumar McMillan [:kumar] (needinfo all the things) 2013-04-11 09:59:55 PDT
Hi! Sorry, I missed comment #142 entirely. David: testing is done with simulations: https://developer.mozilla.org/en-US/docs/Apps/Publishing/In-app_payments#Simulating_payments

Jeremie, nice start on the docs. I made some edits to correct a few technical details. I expanded the example code to check the server for a signed payment result (this is mandatory). I know the code required to make a payment isn't pretty but it's what we got right now.
Comment 146 Anders Rundgren 2014-10-11 08:18:29 PDT
A problem with mozPay is that it is hampered by the short-comings of the 20 year old NSS architecture.

Apple wouldn't have been able "Pushing the Envelope" without fundamental changes in the core including the addition of some kind of security element.

Here is an example of what you can do if you only keep NSS as a legacy interface and create a new security core:
http://webpki.org/papers/PKI/EMV-Tokenization-SET-3DSecure-WebCryptoPlusPlus-combo.pdf#page=4

Cheers,
Anders

Note You need to log in before you can comment on or make changes to this bug.