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

RESOLVED FIXED in Firefox 58

Status

()

Firefox
WebPayments UI
P1
normal
RESOLVED FIXED
10 months ago
24 days ago

People

(Reporter: jonathanGB, Assigned: jonathanGB, Mentored)

Tracking

Trunk
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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)
(Assignee)

Updated

10 months ago
Attachment #8888085 - Attachment is obsolete: true
Attachment #8888085 - Flags: review?(MattN+bmo)
Duplicate of this bug: 1389519

Comment 36

10 months 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)

Updated

8 months ago
status-firefox57: --- → wontfix
Looks like I finally got the test to pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f7ca27fe056822b25e2547db595a55a7ce3c1ba

I just need to polish it up in shape to land.
Comment hidden (mozreview-request)
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

8 months 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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd2f45d07990
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox58: --- → fixed
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.