Closed Bug 1382388 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jonathanGB, Assigned: jonathanGB, Mentored)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1382388 - add test for manual abort.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1382388 - Make the Payments Dialog unprivileged & add "abort" support.
59 bytes, text/x-review-board-request
MattN
: review+
Details
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.
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 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 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 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 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)
Attachment #8888085 - Attachment is obsolete: true
Attachment #8888085 - Flags: review?(MattN+bmo)
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)
Priority: -- → P1
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 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+
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/bd2f45d07990 Make the Payments Dialog unprivileged & add "abort" support. r=MattN
https://hg.mozilla.org/mozilla-central/rev/bd2f45d07990
Status: ASSIGNED → RESOLVED
Closed: 7 years 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
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.

Attachment

General

Creator:
Created:
Updated:
Size: