Make the PaymentRequest dialog unprivileged and remote plus add an abort button

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jonathanGB, Assigned: jonathanGB, Mentored)

Tracking

Trunk
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Current WebPayments prototype has a button to cancel, but it is not properly hooked to the DOM API.

The goal of this bug is therefore to add this functionality.
Comment hidden (mozreview-request)
Component: General → WebPayments UI
Comment on attachment 8888085 [details]
Bug 1382388 - add "abort" listener & move function declarations w/ wrappedJSObject.

https://reviewboard.mozilla.org/r/158988/#review165390

::: toolkit/components/payments/content/paymentRequest.js:19
(Diff revision 1)
> +    let requestId = paymentUISrv.requestIdForWindow(window);
> +    if (!requestId) {
> +      return;
> +    }
> +
> +    paymentUISrv.abortPayment(requestId);

Instead of calling the UI service's method which doesn't tell the DOM that the UI was aborted, I think you need to call the respondPayment method on the paymentservice (not UI one) and return an nsIPaymentActionResponse with ABORT_ACTION.

I believe that the DOM will then end up calling abortPayment on the ui service.
Attachment #8888085 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8890732 [details]
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.

https://reviewboard.mozilla.org/r/161924/#review171552

::: commit-message-cc818:1
(Diff revision 2)
> +Bug 1382388 - Unpriv dialog content. r=mattn

Please improve this commit message to mention payments and use better grammar e.g.:
Make the Web Payments Dialog unprileged.

::: toolkit/components/payments/content/paymentDialog.js:11
(Diff revision 2)
> +
> +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +
> +let frame = document.getElementById("paymentRequestFrame");
> +
> +dump("args: " + JSON.stringify(window.arguments[0]) + "\n\n\n");

Remove the dump statements

::: toolkit/components/payments/content/paymentDialog.js:26
(Diff revision 2)
> +// frame.addEventListener("mozPayment...", () => {
> +// });

Remove this

::: toolkit/components/payments/res/paymentRequest.js:8
(Diff revision 2)
> +  //  document.body.innerText = JSON.stringify(window.arguments[0]);
> +  //  debugger;
> +}, {once: true});
> +
> +window.addEventListener("mozPaymentResponse", onMozPaymentResponse);
> +function onMozPaymentResponse(event) {
> +  // console.log(event.detail);
> +  // console.log(event);

Please remove the commented out code

::: toolkit/components/payments/res/paymentRequest.js:13
(Diff revision 2)
> +function onMozPaymentResponse(event) {
> +  // console.log(event.detail);

We should probably have a single global with methods/properties instead of seprate global functions.
Attachment #8890732 - Flags: review?(MattN+bmo)
Comment on attachment 8890732 [details]
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.

https://reviewboard.mozilla.org/r/161924/#review171554

::: toolkit/components/payments/content/paymentDialog.js:13
(Diff revision 2)
> +
> +let frame = document.getElementById("paymentRequestFrame");
> +
> +dump("args: " + JSON.stringify(window.arguments[0]) + "\n\n\n");
> +frame.addEventListener("DOMContentLoaded", function DCL() {
> +  sendPageEvent(window.arguments[0]);

I don't think the inner page should have to know about the ordering of arguments and the arguments should be extracted into named variables at this point.
Comment on attachment 8888085 [details]
Bug 1382388 - add "abort" listener & move function declarations w/ wrappedJSObject.

https://reviewboard.mozilla.org/r/158988/#review171556

We can discuss this more in-person.

::: toolkit/components/payments/content/paymentDialog.js:9
(Diff revision 5)
> +const paymentSrv     = Cc["@mozilla.org/dom/payments/payment-request-service;1"]
> +                         .getService(Ci.nsIPaymentRequestService);

Remove extra spaces

::: toolkit/components/payments/content/paymentDialog.js:21
(Diff revision 5)
>  frame.addEventListener("DOMContentLoaded", function DCL() {
> -  sendPageEvent(window.arguments[0]);
> +  sendEventToFrame("showPaymentRequest", window.arguments[0]);
>  }, {once: true});
>  
> -function sendPageEvent(detail) {
> -  let doc = frame.contentDocument;
> +function sendEventToFrame(eventName, detail = {}) {
> +  let event = new frameDoc.defaultView.CustomEvent(eventName, {

It's easier to use a consistent event name and therefore a single listener in the inner page but vary the message details with different "types".

::: toolkit/components/payments/content/paymentDialog.js:36
(Diff revision 5)
> +function getComponent(componentName) {
> +  let component = componentsLoaded.get(componentName);

I thought the helper was also going to handle the createInstance call. Any reason it shouldn't?

::: toolkit/components/payments/paymentUIService.js:62
(Diff revision 5)
>      this.log.debug(`abortPayment: ${requestId}`);
>      let abortResponse = Cc["@mozilla.org/dom/payments/payment-abort-action-response;1"]
>                            .createInstance(Ci.nsIPaymentAbortActionResponse);
> -    abortResponse.init(requestId, Ci.nsIPaymentActionResponse.ABORT_SUCCEEDED);
>  
> +    // XXX: may be better to have a map of {[requestId] => window, ...} rather than having to loop?

Remove this please as we shouldn't land XXX without a bug number

::: toolkit/components/payments/res/paymentRequest.js:8
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +// keep this globally available, will be assigned by `onShowPaymentRequest`
> +let requestId;

Move this to an object property
Attachment #8888085 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8890732 [details]
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.

https://reviewboard.mozilla.org/r/161924/#review172602

Clearing this review since I know a new version is coming based on in-person discussions. That way I will also see the new version easier.
Attachment #8890732 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8890732 [details]
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.

https://reviewboard.mozilla.org/r/161924/#review172622

This commit would lead to a very broken feature by itself (each commit should normally stand alone and not lead to brokenness) but I don't want you to have to all your other commits on fixing that.
Attachment #8890732 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8892238 [details]
Bug 1382388 - add test for manual abort.

https://reviewboard.mozilla.org/r/163188/#review172900
Attachment #8892238 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8888085 [details]
Bug 1382388 - add "abort" listener & move function declarations w/ wrappedJSObject.

https://reviewboard.mozilla.org/r/158988/#review172628

::: toolkit/components/payments/content/paymentDialog.js:21
(Diff revision 8)
> +    showResponse.init(requestId, acceptStatus, methodName, methodData, payerName,
> +                      payerEmail,
> +                      payerPhone);

The wrapping here is inconsistent

::: toolkit/components/payments/content/paymentDialogFrameScript.js:7
(Diff revision 9)
> + * 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/. */
> +
> +"use strict";
> +
> + /* eslint-env mozilla/frame-script */

Nit: You have a space before this line

::: toolkit/components/payments/content/paymentDialogFrameScript.js:37
(Diff revision 9)
> +  init() {
> +    this.defineLazyLogGetter(this, "frameScript");
> +

`init` should generally be the first method (possibly after private variables)

::: toolkit/components/payments/content/paymentDialogFrameScript.js:41
(Diff revision 9)
> +    addEventListener("DOMContentLoaded", function DCL() {
> +      this.sendMessageToChrome("DOMContentLoaded");
> +    }.bind(this), {once: true});
> +
> +    // listen to res
> +    addEventListener("paymentResToContent", function resToContent({detail}) {

We should add the event listener to `PaymentFrameScript` aka `this` instead of having inline functions.

At a minimum these should be methods on `PaymentFrameScript`

::: toolkit/components/payments/res/paymentRequest.js:10
(Diff revision 9)
>  "use strict";
>  
> -document.addEventListener("DOMContentLoaded", function DCL() {
> -}, {once: true});
> +let PaymentRequest = {
> +  requestId: null,
> +
> +  sendMessageToContent(message, detail = {}) {

It looks like message is really just a message name?

::: toolkit/components/payments/res/paymentRequest.js:21
(Diff revision 9)
> +      }, detail),
> +    });
> +    document.dispatchEvent(event);
> +  },
>  
> -window.addEventListener("mozPaymentResponse", onMozPaymentResponse);
> +  onContentToRes({detail}) {

This should be a private method

::: toolkit/components/payments/res/paymentRequest.js:32
(Diff revision 9)
> +  init() {
> +    let onContentToRes = this.onContentToRes.bind(this);
> +
> +    // listen to content

Make this the first method too

::: toolkit/components/payments/res/paymentRequest.js:36
(Diff revision 9)
> +
> +  init() {
> +    let onContentToRes = this.onContentToRes.bind(this);
> +
> +    // listen to content
> +    window.addEventListener("paymentContentToRes", onContentToRes);

I think we should also call the event paymentContentToChrome
Attachment #8888085 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8888085 - Attachment is obsolete: true
Attachment #8888085 - Flags: review?(MattN+bmo)

Comment 36

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/3d10672e7e2b
Make the Payments Dialog unprivileged & add "abort" support. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5dae0072182f
add test for manual abort. r=MattN
Backed out for failing touched browser-chrome test browser_show_dialog.js:

https://hg.mozilla.org/integration/autoland/rev/1ec28be2c6f2ec8cb7ba7ce60ac0b78cfe0e3134
https://hg.mozilla.org/integration/autoland/rev/f4c629f0e4fc25f03c4a1142b9d4fff97fb858e7

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5dae0072182fdd0e8687a1c19e78b6993595489a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122735750&repo=autoland

[task 2017-08-12T10:59:05.763120Z] 10:59:05     INFO - TEST-START | toolkit/components/payments/test/browser/browser_show_dialog.js
[task 2017-08-12T10:59:07.662374Z] 10:59:07     INFO - TEST-INFO | started process screentopng
[task 2017-08-12T10:59:08.369276Z] 10:59:08     INFO - TEST-INFO | screentopng: exit 0
[task 2017-08-12T10:59:08.373243Z] 10:59:08     INFO - Buffered messages logged at 10:59:05
[task 2017-08-12T10:59:08.374413Z] 10:59:08     INFO - Entering test bound setup_head
[task 2017-08-12T10:59:08.376440Z] 10:59:08     INFO - Leaving test bound setup_head
[task 2017-08-12T10:59:08.378566Z] 10:59:08     INFO - Entering test bound test_show_abort_dialog
[task 2017-08-12T10:59:08.380676Z] 10:59:08     INFO - Buffered messages logged at 10:59:06
[task 2017-08-12T10:59:08.383363Z] 10:59:08     INFO - TEST-PASS | toolkit/components/payments/test/browser/browser_show_dialog.js | requestId should be defined - 
[task 2017-08-12T10:59:08.388218Z] 10:59:08     INFO - TEST-PASS | toolkit/components/payments/test/browser/browser_show_dialog.js | dialog should not be closed - 
[task 2017-08-12T10:59:08.390368Z] 10:59:08     INFO - TEST-PASS | toolkit/components/payments/test/browser/browser_show_dialog.js | dialog should be closed - 
[task 2017-08-12T10:59:08.393378Z] 10:59:08     INFO - Buffered messages logged at 10:59:07
[task 2017-08-12T10:59:08.397047Z] 10:59:08     INFO - Leaving test bound test_show_abort_dialog
[task 2017-08-12T10:59:08.400622Z] 10:59:08     INFO - Entering test bound test_show_manualAbort_dialog
[task 2017-08-12T10:59:08.404951Z] 10:59:08     INFO - TEST-PASS | toolkit/components/payments/test/browser/browser_show_dialog.js | requestId should be defined - 
[task 2017-08-12T10:59:08.408033Z] 10:59:08     INFO - TEST-PASS | toolkit/components/payments/test/browser/browser_show_dialog.js | dialog should not be closed - 
[task 2017-08-12T10:59:08.409806Z] 10:59:08     INFO - Buffered messages finished
[task 2017-08-12T10:59:08.411732Z] 10:59:08     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/payments/test/browser/browser_show_dialog.js | Uncaught exception - TypeError: content.document.getElementById(...) is null
[task 2017-08-12T10:59:08.414894Z] 10:59:08     INFO - Leaving test bound test_show_manualAbort_dialog
[task 2017-08-12T11:04:38.421220Z] 11:04:38     INFO - Buffered messages finished
[task 2017-08-12T11:04:38.423772Z] 11:04:38    ERROR - TEST-UNEXPECTED-TIMEOUT | toolkit/components/payments/test/browser/browser_show_dialog.js | application timed out after 330 seconds with no output
Flags: needinfo?(jonathan.guillotte.blouin)
I believe the problem is that the test wasn't waiting for the frame to be loaded before starting the content task to get an element reference.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec0032beb8f0184e842706e9eba015fc84858ff6
Flags: needinfo?(jonathan.guillotte.blouin) → needinfo?(MattN+bmo)
Comment on attachment 8910937 [details]
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.

https://reviewboard.mozilla.org/r/160038/#review187704
Attachment #8910937 - Flags: review?(MattN+bmo) → review+

Comment 42

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/bd2f45d07990
Make the Payments Dialog unprivileged & add "abort" support. r=MattN

Comment 43

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd2f45d07990
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(MattN+bmo)
Summary: Add "abort" button in WebPayments dialog → Make the PaymentRequest dialog unprivileged and remote plus add an abort button
Component: WebPayments UI → WebPayments UI
Product: Toolkit → Firefox
Target Milestone: mozilla58 → Firefox 58
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.