paymentDetails and paymentMethods serialization

RESOLVED FIXED in Firefox 60

Status

()

Toolkit
WebPayments UI
P1
normal
RESOLVED FIXED
23 days ago
13 days ago

People

(Reporter: sfoster, Assigned: sfoster)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

23 days ago
PaymentDialog initializeFrame method serializes the payment request in order to pass it over to content. The current method doesn't handle the nsIArrays correctly and we are losing some paymentDetails and paymentMethods data in-transit.
(Assignee)

Comment 1

22 days ago
After some discussion with :jaws and :mattn, the strategy here is to fix this in the bridge that is payments/content/paymentDialog.js, where we pass the payment request as data to the content process. We know what that data structure should look like so serialization needn't be complicated.
Assignee: nobody → sfoster
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8948579 [details]
Bug 1434508 - better serialization of paymentrequest data.

https://reviewboard.mozilla.org/r/217998/#review224626

Thanks

::: toolkit/components/payments/content/paymentDialogFrameScript.js:20
(Diff revision 3)
>  /* eslint-env mozilla/frame-script */
> +/* global XPCOMUtils */

You shouldn't need to make these `global` changes. Perhaps you're on an old version of m-c without the eslint fixes? In that case a rebase should remove the need for these

::: toolkit/components/payments/content/paymentDialogWrapper.js:208
(Diff revision 3)
> -  initializeFrame() {
> -    let requestSerialized = JSON.parse(JSON.stringify(this.request));
> +  _serialize(name, value) {
> +    // Primitives: String, Number, Boolean, null

It seems like a JSDoc comment would be valuable since the argument names are quite generic. It would also be useful to note that it's a recursive function.

::: toolkit/components/payments/content/paymentDialogWrapper.js:220
(Diff revision 3)
> +    // Structures: Array/nsIArray
> +    if (value.hasOwnProperty("length")) {

This feels a bit fragile. I'd rather we do `instanceof Ci.nsIArray` and `Array.isArray` instead.  That will also simplify the readability since they can be in two different adjacent `if` blocks since we handle them differently anyways.

::: toolkit/components/payments/content/paymentDialogWrapper.js:224
(Diff revision 3)
> +    }
> +    // Structures: Array/nsIArray
> +    if (value.hasOwnProperty("length")) {
> +      let iface;
> +      if (value instanceof Ci.nsIArray) {
> +        switch (name) {

You're missing `modifiers` too I think

::: toolkit/components/payments/content/paymentDialogWrapper.js:225
(Diff revision 3)
> +          case "displayItems":
> +            iface = Ci.nsIPaymentItem;

I think you're missing `additionalDisplayItems`

::: toolkit/components/payments/content/paymentDialogWrapper.js:258
(Diff revision 3)
> +    for (let key of Object.keys(value)) {
> +      let result = this._serialize(key, value[key]);

Nit: use can use Object.entries to avoid the `value[key]` lookup

::: toolkit/components/payments/content/paymentDialogWrapper.js:267
(Diff revision 3)
> +  serializeRequest(req) {
> +    let serialized = this._serialize(null, req);
> +    return serialized;
> +  },

Did you consider change the order of the arguments of `_serialize` and making `name` optional in order to avoid this wrapper?

::: toolkit/components/payments/content/paymentDialogWrapper.js:274
(Diff revision 3)
>      // Manually serialize the nsIPrincipal.
>      let displayHost = this.request.topLevelPrincipal.URI.displayHost;
>      requestSerialized.topLevelPrincipal = {
>        URI: {
>          displayHost,
>        },
>      };

It seems like this should be handled in the `_serialize` method. You can do it in a separate commit in this bug if you think it's clearer to review that way.
Attachment #8948579 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

14 days ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a524900f4f4
better serialization of paymentrequest data. r=MattN

Comment 10

14 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a524900f4f4
Status: NEW → RESOLVED
Last Resolved: 14 days ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.