Closed Bug 767818 Opened 12 years ago Closed 12 years ago

Implement navigator.mozPay

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: ferjm, Assigned: ferjm)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [LOE:M])

Attachments

(7 files, 54 obsolete files)

1.23 KB, text/plain
Details
4.66 KB, text/html
Details
993 bytes, text/plain
Details
2.90 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.07 KB, patch
sicking
: review+
Details | Diff | Splinter Review
13.72 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
23.53 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
Attachment #636175 - Attachment is patch: true
Please track as P1 blocker
Attached patch WIP (obsolete) — Splinter Review
Attachment #636175 - Attachment is obsolete: true
Assignee: nobody → ferjmoreno
Depends on: 768943
Attached patch WIP (obsolete) — Splinter Review
WIP updated with most of the DOMRequest functionality done. Tested with a fake payment processor. It requires some modifications in Gaia.
Attachment #636390 - Attachment is obsolete: true
blocking-basecamp: --- → ?
Attached patch Gaia-WIP (obsolete) — Splinter Review
This patch allows Gaia to create a system iframe on chrome demand
Attached patch WIP (obsolete) — Splinter Review
WIP updated with payment processors registered as preferences and endpoint selection according to jwt type.
Attachment #638087 - Attachment is obsolete: true
Attached patch Part 1: IDL v1 (obsolete) — Splinter Review
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()'.
Attachment #639142 - Attachment is obsolete: true
Attachment #639705 - Flags: feedback?(jonas)
Attached patch Part 3: B2G implementation v1 (obsolete) — Splinter Review
Attachment #639707 - Attachment description: Part 3: B2G implementation → Part 3: B2G implementation v1
blocking-basecamp: ? → +
Attached patch Part 3: B2G implementation v2 (obsolete) — Splinter Review
B2G implementation updated to support Gaia system dialogs as proposed in Bug 768943.
Attachment #639707 - Attachment is obsolete: true
Attachment #640743 - Flags: review?(fabrice)
Attachment #639706 - Flags: review?(fabrice)
Attached patch Gaia-WIP (obsolete) — Splinter Review
Gaia bits to support system dialogs as proposed in Bug 768943. A pull request to Gaia will be sent with this content.
Attachment #638089 - Attachment is obsolete: true
Attachment #640744 - Attachment is patch: true
Attached patch Part 3: B2G implementation v3 (obsolete) — Splinter Review
B2G implementation updated with "system dialog" renamed to "trusted dialog" as per Gaia PR feedback and some bug fixing and security checks done.
Attachment #640743 - Attachment is obsolete: true
Attachment #640743 - Flags: review?(fabrice)
Attachment #641119 - Flags: review?(fabrice)
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.
Attachment #639706 - Flags: review?(anygregor)
Attachment #641119 - Flags: review?(anygregor)
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 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 ?
(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.
(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 :)
(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 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.
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.
Sorry, s/comment #1/Description/
Fernando: Did you have that list of feature requirements? I can't see it attached.
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.
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?
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.
(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.
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.
(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 :\)
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).
(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.
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?
(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.
(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?
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.
> > (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.
(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 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.
Attachment #639706 - Flags: review?(fabrice)
Attachment #639706 - Flags: review?(anygregor)
Attachment #641119 - Flags: review?(fabrice)
Attachment #641119 - Flags: review?(anygregor)
(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, 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
(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.
(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.
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.
> 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.
(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.
(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.
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.
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.
(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?
I've just moved most part of the doc to https://wiki.mozilla.org/WebAPI/WebPayment . Any feedback is very much appreciated.
Attached patch Part 1: IDL v1 (obsolete) — Splinter Review
Attachment #639705 - Attachment is obsolete: true
Attachment #639705 - Flags: feedback?(jonas)
Attachment #644688 - Flags: review?(jonas)
Attachment #639706 - Attachment is obsolete: true
Attachment #644689 - Flags: review?(fabrice)
Attachment #644689 - Flags: review?(anygregor)
Attached patch Part 3: B2G implementation v4 (obsolete) — Splinter Review
Attachment #641119 - Attachment is obsolete: true
Attachment #644690 - Flags: review?(fabrice)
Attachment #644690 - Flags: review?(anygregor)
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);
>+}
Attachment #640744 - Attachment is obsolete: true
Component: DOM → DOM: Device Interfaces
Attached patch Part 3: B2G implementation v5 (obsolete) — Splinter Review
Some code clean up and bugfixing
Attachment #644690 - Attachment is obsolete: true
Attachment #644690 - Flags: review?(fabrice)
Attachment #644690 - Flags: review?(anygregor)
Attachment #644907 - Flags: review?(fabrice)
Attachment #644907 - Flags: review?(anygregor)
Conditional build fixed for B2G
Attachment #644689 - Attachment is obsolete: true
Attachment #644689 - Flags: review?(fabrice)
Attachment #644689 - Flags: review?(anygregor)
Attachment #645407 - Flags: review?(fabrice)
Attachment #645407 - Flags: review?(anygregor)
Attached patch Part 3: B2G implementation v5 (obsolete) — Splinter Review
Conditional build for B2G fixed.
Attachment #644907 - Attachment is obsolete: true
Attachment #644907 - Flags: review?(fabrice)
Attachment #644907 - Flags: review?(anygregor)
Attachment #645409 - Flags: review?(fabrice)
Attachment #645409 - Flags: review?(anygregor)
Attached patch Part 3: B2G implementation v5 (obsolete) — Splinter Review
Sorry for the spam, I forgot to remove a couple of extra ifdef.
Attachment #645409 - Attachment is obsolete: true
Attachment #645409 - Flags: review?(fabrice)
Attachment #645409 - Flags: review?(anygregor)
Attachment #645412 - Flags: review?(fabrice)
Attachment #645412 - Flags: review?(anygregor)
Depends on: 777023
Attached file 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
Attached patch Part 3: B2G implementation v5 (obsolete) — Splinter Review
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.
Attachment #645412 - Attachment is obsolete: true
Attachment #645412 - Flags: review?(fabrice)
Attachment #645412 - Flags: review?(anygregor)
Attachment #646162 - Flags: review?(fabrice)
Attachment #645407 - Flags: review?(anygregor)
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()
Attachment #645407 - Flags: review?(fabrice) → review-
(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 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();
Attachment #646162 - Flags: review?(fabrice) → review-
Blocks: 766199
No longer blocks: 766201
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.
Attachment #645407 - Attachment is obsolete: true
Attachment #647130 - Flags: review?(fabrice)
Attached patch Part 3: B2G implementation v6 (obsolete) — Splinter Review
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.
Attachment #646162 - Attachment is obsolete: true
Attachment #647133 - Flags: review?(fabrice)
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.
Attachment #647130 - Flags: superreview?
Attachment #647130 - Flags: review?(jst)
Attachment #647130 - Flags: superreview?
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.
Attachment #647130 - Flags: review?(jst) → review-
(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.
(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.
You were absolutely right. It works returning the pay() function from 'nsIDOMGlobalPropertyInitializer.init()'. Thanks!
Attachment #647130 - Attachment is obsolete: true
Attachment #647130 - Flags: review?(fabrice)
Attachment #647490 - Flags: review?(jst)
Attached patch Part 3: B2G implementation v6 (obsolete) — Splinter Review
Attachment #647133 - Attachment is obsolete: true
Attachment #647133 - Flags: review?(fabrice)
Attachment #647492 - Flags: review?(fabrice)
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
Attachment #647492 - Flags: review?(fabrice) → review-
(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.
Attachment #647492 - Attachment is obsolete: true
Attachment #644688 - Flags: review?(jonas)
Attachment #644688 - Attachment is obsolete: true
Attachment #647490 - Flags: review?(jst) → review+
Attached patch Part 1: IDLs v3 (obsolete) — Splinter Review
New IDLs with the glue interface addition.
Attachment #648323 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
DOM implementation with the glue interface addition
Attachment #648325 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
B2G specific implementation
Attachment #648326 - Flags: review?(fabrice)
Depends on: 779888
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
}
Attachment #648325 - Flags: review?(fabrice) → review+
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)
Attachment #648326 - Flags: review?(fabrice) → review-
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?
(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.
(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?
Attached patch Part 1: IDLs v3 (obsolete) — Splinter Review
Comments added to IDL
Attachment #648323 - Attachment is obsolete: true
Attachment #648323 - Flags: review?(jonas)
Attachment #648659 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
r=fabrice

Fabrice's comments addressed
Attachment #648325 - Attachment is obsolete: true
Attachment #648660 - Flags: review+
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
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.
Attachment #648660 - Attachment is obsolete: true
Attachment #649231 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
No more payment providers info as preferences for now.
Attachment #648326 - Attachment is obsolete: true
Attachment #649232 - Flags: review?(fabrice)
Attached file Payment providers data (obsolete) —
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.
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Sorry, for the spam. I only fixed a nit.
Attachment #649232 - Attachment is obsolete: true
Attachment #649232 - Flags: review?(fabrice)
Attachment #649237 - Flags: review?(fabrice)
(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
(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.
(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!
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
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 :)
Attachment #649231 - Attachment is obsolete: true
Attachment #649231 - Flags: review?(fabrice)
Attachment #649682 - Flags: review+
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #649237 - Attachment is obsolete: true
Attachment #649237 - Flags: review?(fabrice)
Attachment #649683 - Flags: review?(fabrice)
Attached file Payment providers data
An example of payment providers as preferences
Attachment #649235 - Attachment is obsolete: true
Priority: -- → P1
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 { }
Attachment #649683 - Flags: review?(fabrice) → review-
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #649683 - Attachment is obsolete: true
Attachment #650508 - Flags: review?(fabrice)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
Wrong patch
Attachment #649682 - Attachment is obsolete: true
Attachment #650561 - Flags: review+
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;
}
Attachment #650508 - Flags: review?(fabrice) → review+
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.
Attachment #647490 - Attachment is obsolete: true
Attachment #650853 - Flags: review?(jst)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
r=fabrice

I had to rebase the patch after prefixing mozPay
Attachment #650561 - Attachment is obsolete: true
Attachment #650854 - Flags: review+
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
r=fabrice

Review comments addressed.
Attachment #650508 - Attachment is obsolete: true
Attachment #650855 - Flags: review+
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
r=fabrice

Sorry, the try push message changed the patch header
Attachment #650855 - Attachment is obsolete: true
Attachment #650858 - Flags: review+
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@
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
Thanks David!
Attachment #650854 - Attachment is obsolete: true
Attachment #651205 - Flags: review+
Summary: Implement navigator.pay → Implement navigator.mozPay
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.
Whiteboard: [LOE:M]
Attachment #650853 - Flags: review?(jst) → review+
Attached patch Part 1: IDLs (obsolete) — Splinter Review
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.
Attachment #648659 - Attachment is obsolete: true
Attachment #648659 - Flags: review?(jonas)
Attachment #653016 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
Attachment #651205 - Attachment is obsolete: true
Attachment #653017 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #650858 - Attachment is obsolete: true
Attachment #653018 - Flags: review?(fabrice)
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).
Depends on: 776417
Attached patch Part 1: IDLs (obsolete) — Splinter Review
nsIPaymentRequestInfo -> nsIDOMPaymentRequestInfo
nsIPaymentProductPrice -> nsIDOMPaymentProductPrice
Attachment #653016 - Attachment is obsolete: true
Attachment #653016 - Flags: review?(jonas)
Attachment #653061 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
nsIPaymentRequestInfo -> nsIDOMPaymentRequestInfo
nsIPaymentProductPrice -> nsIDOMPaymentProductPrice
Attachment #653017 - Attachment is obsolete: true
Attachment #653017 - Flags: review?(fabrice)
Attachment #653062 - Flags: review?(fabrice)
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.
Attachment #653018 - Attachment is obsolete: true
Attachment #653018 - Flags: review?(fabrice)
Attachment #653061 - Attachment is obsolete: true
Attachment #653061 - Flags: review?(jonas)
Attachment #653062 - Attachment is obsolete: true
Attachment #653062 - Flags: review?(fabrice)
Attached patch Part 1: IDLs (obsolete) — Splinter Review
Attachment #653554 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
Attachment #653555 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #653557 - Flags: review?(fabrice)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
I've only modified an error check and message.
Attachment #653555 - Attachment is obsolete: true
Attachment #653555 - Flags: review?(fabrice)
Attachment #654315 - Flags: review?(fabrice)
Depends on: 784760
Depends on: 784769
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Send mozChromeEvents via |shell.sendChromeEvent|
Attachment #653557 - Attachment is obsolete: true
Attachment #653557 - Flags: review?(fabrice)
Attachment #654557 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #654557 - Attachment is obsolete: true
Attachment #654557 - Flags: review?(fabrice)
Attachment #654644 - Flags: review?(fabrice)
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.
Attachment #654315 - Flags: review?(fabrice) → review-
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! :)
Attachment #654644 - Flags: review?(fabrice) → review-
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 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 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?
Attached patch Part 1: IDLsSplinter Review
Attachment #653554 - Attachment is obsolete: true
Attachment #653554 - Flags: review?(jonas)
Attachment #656108 - Flags: review?(jonas)
Attached patch Part 3: DOM implementation (obsolete) — Splinter Review
Attachment #654315 - Attachment is obsolete: true
Attachment #656110 - Flags: review?(fabrice)
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
Attachment #654644 - Attachment is obsolete: true
Attachment #656111 - Flags: review?(fabrice)
(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.
I also addressed philikon's comments.
Attached patch Part 4: B2G implementation (obsolete) — Splinter Review
I only added information for an error case.
Attachment #656111 - Attachment is obsolete: true
Attachment #656111 - Flags: review?(fabrice)
Attachment #656214 - Flags: review?(fabrice)
Nits
Attachment #656214 - Attachment is obsolete: true
Attachment #656214 - Flags: review?(fabrice)
Attachment #656296 - Flags: review?(fabrice)
Removed all the __exposedProps__
Attachment #656110 - Attachment is obsolete: true
Attachment #656110 - Flags: review?(fabrice)
Attachment #656409 - Flags: review?(fabrice)
Attachment #656409 - Flags: review?(fabrice) → review+
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.
Attachment #656296 - Flags: review?(fabrice) → review+
QA Contact: jsmith
Whiteboard: [LOE:M] → [LOE:M], [qa+]
\o/
Keywords: verifyme
Whiteboard: [LOE:M], [qa+] → [LOE:M]
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
Keywords: verifyme
Whiteboard: [LOE:M] → [LOE:M], [qa verification blocked]
Whiteboard: [LOE:M], [qa verification blocked] → [LOE:M], [qa+]
Keywords: verifyme
Whiteboard: [LOE:M], [qa+] → [LOE:M]
I'm blocked on testing - need a response from Fernando on why the test case I tried (sent over email) isn't working.
Keywords: verifyme
Whiteboard: [LOE:M] → [LOE:M], [qa verification blocked]
Sent you a modified preferences file so the testing provider can work. Hope that unblocks the issue.
Keywords: verifyme
Whiteboard: [LOE:M], [qa verification blocked] → [LOE:M]
(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 :)
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).
Keywords: verifyme
Whiteboard: [LOE:M] → [LOE:M], [qa verification failed]
No longer depends on: 784760, 784769
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.
Status: RESOLVED → VERIFIED
Whiteboard: [LOE:M], [qa verification failed] → [LOE:M]
Depends on: 793329
Depends on: 793811
Depends on: 795854
Depends on: 797300
Depends on: 804080
Depends on: 804143
Blocks: basecamp-payments
No longer blocks: marketplace-payments
No longer depends on: 777023
Depends on: 809219
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.
needinfo on Kumar for comment 142
Flags: needinfo?(kumar.mcmillan)
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
Flags: needinfo?(kumar.mcmillan)
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: