Open and close a stub dialog when PaymentUIService's showPayment and abortPayment are called (respectively)

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: MattN, Assigned: jonathanGB, Mentored)

Tracking

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments)

When our nsIPaymentUIService implementation's `showPayment` method is called, open paymentRequest.xhtml as a dialog.

When `abortPayment` is called while a PaymentRequest is in progress, close the appropriate dialog (using the requestId).
Comment hidden (mozreview-request)
Do not run this snippet with `request` and `request2` at the same time.
This snippet might not work if you run it with the new API specs; notably, `supportedMethods` is now simply a string.
Here's a preview of the current behaviour: http://g.recordit.co/VI8f1wv8AI.gif

We're now able to hook onto the `showPayment` and `abortPayment` methods from the js. The gif presents a first PaymentRequest shown then aborted, and a 2nd PaymentRequest shown then aborted (both aborted inside `setTimeout`s.

A next step would be to be able to abort the PaymentRequest from the UI itself - for example, a button "abort" in the UI triggers the abort.
Attachment #8887298 - Flags: review?(MattN+bmo)
(In reply to Jonathan Guillotte-Blouin [:jonathanGB] from comment #3)
> Here's a preview of the current behaviour:
> http://g.recordit.co/VI8f1wv8AI.gif
> 
> We're now able to hook onto the `showPayment` and `abortPayment` methods
> from the js. The gif presents a first PaymentRequest shown then aborted, and
> a 2nd PaymentRequest shown then aborted (both aborted inside `setTimeout`s.

Nice!

> A next step would be to be able to abort the PaymentRequest from the UI
> itself - for example, a button "abort" in the UI triggers the abort.

I think that can be a separate bug though.

For this bug I think we should add a mochitest-browser-chrome test to test the two methods and that's all.

In a separate commit you could add console.jsm usage with the maxLogLevelPref if you think it will be useful to leave some debug logging in for development.
Comment hidden (mozreview-request)
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8887745 [details]
Bug 1381186 - add show/abort chrome test.

https://reviewboard.mozilla.org/r/158660/#review163966

::: toolkit/components/payments/test/browser/blank_page.html:4
(Diff revision 1)
> +    <title>Blank page</title>
> +    <meta charset="utf8">

<meta charset> has to come before <title> in order for it to apply to the title. I believe you would still get a console as you have it.

::: toolkit/components/payments/test/browser/browser_show_dialog.js:3
(Diff revision 1)
> +const BLANK_PAGE_URL = "https://example.com/browser/toolkit/components/" +
> +                       "payments/test/browser/blank_page.html";

Move this to head.html

::: toolkit/components/payments/test/browser/browser_show_dialog.js:8
(Diff revision 1)
> +  data: {
> +    supportedNetworks: ["visa", "mastercard"],
> +    supportedTypes: ["debit"],
> +  },

Remove the `data` property since it's not required[1] and may cause this test to fail in the future when we actually check if storage has supported cards.


[1] https://www.w3.org/TR/payment-method-basic-card/#basiccardrequest

::: toolkit/components/payments/test/browser/browser_show_dialog.js:14
(Diff revision 1)
> +    supportedNetworks: ["visa", "mastercard"],
> +    supportedTypes: ["debit"],
> +  },
> +}];
> +const details = {
> +  id: "super-store-order-123-12312",

Remove the optional id property

::: toolkit/components/payments/test/browser/browser_show_dialog.js:17
(Diff revision 1)
> +}];
> +const details = {
> +  id: "super-store-order-123-12312",
> +  total: {
> +    label: "Total due",
> +    amount: { currency: "USD", value: "60.00" }, // US$60.00

Remove the comment please as it's not useful

::: toolkit/components/payments/test/browser/browser_show_dialog.js:20
(Diff revision 1)
> +const options = {
> +  requestShipping: true,
> +};

Since the third argument is optional and requestShipping isn't related to this test we should leave it out to keep it as minimal/focused as possible. You can leave it available on the helper for later though.

::: toolkit/components/payments/test/browser/browser_show_dialog.js:25
(Diff revision 1)
> +const options = {
> +  requestShipping: true,
> +};
> +
> +add_task(async function test_show_abort_dialog() {
> +  await SpecialPowers.pushPrefEnv({set: [[PREF_PAYMENT_ENABLED, true]]});

This should go in a separate setup task which is in head.js:

add_task(async function setup_head() {
  …
});

::: toolkit/components/payments/test/browser/browser_show_dialog.js:34
(Diff revision 1)
> +                            async({methodData, details, options}) => {
> +                              let rq = new content.PaymentRequest(methodData, details, options);
> +                              content.rq = rq; // assign it so we can retrieve it later
> +                              rq.show();
> +                            });

Since I think many tests will need this pattern and the indenation is a bit hard to read with this inline, I think we should make this async method a helper in head.js on an object that makes it clear it's for content helpers, not running in browser-chrome:

```js
/**
 * Common content tasks functions to be used with ContentTask.spawn.
 */
let ContentTasks = {
  async showRequest({methodData, details, options}) => {
    …
  }
};
```

::: toolkit/components/payments/test/browser/browser_show_dialog.js:46
(Diff revision 1)
> +    if (win.closed || !requestId) {
> +      return;
> +    }

Do bother with nicer paths in test failure scenarios. i.e. remove this

::: toolkit/components/payments/test/browser/head.js:9
(Diff revision 1)
> +    args: "none",
> +    varsIgnorePattern: "^(Cc|Ci|Cr|Cu|EXPORTED_SYMBOLS)$",
> +  }],
> +*/
> +
> +const REQUEST_ID_REGEX = /^paymentRequest-(.*)$/;

I don't think you need a regex here, only to define the prefix, that way it can be used both for matching and constructing the name.

::: toolkit/components/payments/test/browser/head.js:12
(Diff revision 1)
> +*/
> +
> +const REQUEST_ID_REGEX = /^paymentRequest-(.*)$/;
> +
> +async function getDialogWindow() {
> +  return Services.wm.getMostRecentWindow(null);

I think you should check that the window is a payment  window otherwise `throw new Error("…")`

::: toolkit/components/payments/test/browser/head.js:15
(Diff revision 1)
> +function getDialogRequestId(windowName) {
> +  let result = windowName.match(REQUEST_ID_REGEX);

This function signature won't apply to the proper future widget so we should change it to be forward compatible. We should write the tests so that we can simply replace the widget and update these helpers (without touching the callers) and everything will pass still but in this case I think passing win.name is too much of an implementation detail.

Maybe call this requestIdForWindow(window) and extract the name from the window inside it.
Attachment #8887745 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Component: General → WebPayments UI
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8887745 [details]
Bug 1381186 - add show/abort chrome test.

https://reviewboard.mozilla.org/r/158660/#review165354

::: toolkit/components/payments/test/browser/browser.ini:8
(Diff revision 2)
> +
>  support-files =
> +  blank_page.html
>  
>  [browser_request_summary.js]
> +[browser_show_dialog.js]

Add `skip-if = !e10s` due to bug 1365964 and add a comment to fix the failing test on try:
# Bug 1365964 - Payment Request isn't implemented for non-e10s

::: toolkit/components/payments/test/browser/head.js:16
(Diff revision 2)
> +  let win = Services.wm.getMostRecentWindow(null);
> +  if (!win.name.startsWith(REQUEST_ID_PREFIX)) {
> +    throw new Error("most recent window is not a PaymentRequest dialog");
> +  }

Since the dialog window may not be opened yet and we don't have a generic way to know about that yet, we can change this to use BrowserTestUtils.waitForCondition where the condition is updating `win = Services.wm.getMostRecentWindow(null);` and returning `win.name.startsWith(REQUEST_ID_PREFIX)`. Then the `getDialogWindow` method can still `return win;`

This should fix the other test failure
Attachment #8887745 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8887298 [details]
Bug 1381186 - open/close stub dialog on (show/abort)Payment.

https://reviewboard.mozilla.org/r/158100/#review165368

There are some eslint issues to fix
Attachment #8887298 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8887298 [details]
Bug 1381186 - open/close stub dialog on (show/abort)Payment.

https://reviewboard.mozilla.org/r/158100/#review166980

::: npm-shrinkwrap.json:1
(Diff revision 3)
>  {
>    "name": "mozillaeslintsetup",
> -  "requires": true,
>    "lockfileVersion": 1,

Revert this file accidentally touched.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8887298 [details]
Bug 1381186 - open/close stub dialog on (show/abort)Payment.

https://reviewboard.mozilla.org/r/158100/#review167042

::: toolkit/components/payments/paymentUIService.js:50
(Diff revision 4)
> +    chromeWindow.openDialog(DIALOG_URL,
> +                            `${this.REQUEST_ID_PREFIX}${requestId}`, "modal,dialog,centerscreen");

Nit: This is wrapped inconsistently

::: toolkit/components/payments/paymentUIService.js:62
(Diff revision 4)
>                            .createInstance(Ci.nsIPaymentAbortActionResponse);
>      abortResponse.init(requestId, Ci.nsIPaymentActionResponse.ABORT_SUCCEEDED);
> +
> +    let enu = Services.wm.getEnumerator(null);
> +    let win;
> +    this.log.debug(`requestId: ${requestId}`);

Can you move this to the abortPayment log message so that we know what the requestId is for:

`abortPayment: ${requestId}`
(Reporter)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8887745 [details]
Bug 1381186 - add show/abort chrome test.

https://reviewboard.mozilla.org/r/158660/#review167046

::: browser/installer/package-manifest.in:400
(Diff revision 4)
> +#ifdef NIGHTLY_BUILD
>  @RESPATH@/components/payments.manifest
> +@RESPATH@/components/paymentUIService.js
> +#endif

This needs to be rebased on m-c

::: toolkit/components/payments/test/browser/head.js:1
(Diff revision 4)
> +/* eslint
> +  "no-unused-vars": ["error", {

Can you add "use strict"; to this file please and make sure tests still pass locally.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8887745 [details]
Bug 1381186 - add show/abort chrome test.

https://reviewboard.mozilla.org/r/158660/#review167046

> This needs to be rebased on m-c

After rebase an instruction was added to `package-manifest.in`, see interdiff 4-5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8887745 [details]
Bug 1381186 - add show/abort chrome test.

https://reviewboard.mozilla.org/r/158660/#review171560

::: browser/installer/package-manifest.in:403
(Diff revision 6)
> -#if defined(NIGHTLY_BUILD) && defined(MOZ_BUILD_APP_IS_BROWSER)
> +#ifdef NIGHTLY_BUILD
>  @RESPATH@/components/payments.manifest
> +@RESPATH@/components/paymentUIService.js
>  #endif

Shouldn't the package and browser_all_files_referenced.js changes be in the other commit? Can you also rebase this on recent m-c so it can autoland?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/1df19277c9c1
open/close stub dialog on (show/abort)Payment. r=MattN
https://hg.mozilla.org/integration/autoland/rev/4bdfc48d9639
add show/abort chrome test. r=MattN

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1df19277c9c1
https://hg.mozilla.org/mozilla-central/rev/4bdfc48d9639
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1389519
Depends on: 1390009
Component: WebPayments UI → WebPayments UI
Product: Toolkit → Firefox
Target Milestone: mozilla57 → Firefox 57
You need to log in before you can comment on or make changes to this bug.