1434508 - paymentDetails and paymentMethods serialization

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Sam Foster [:sfoster] (he/him)]

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.

Comment 1

[Sam Foster [:sfoster] (he/him)]

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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1434508 - better serialization of paymentrequest data.

* Add a serializeRequest method to paymentDialogWrapper to correctly serialize the nsIArray values
* Test results of serializing a request with multiple displayItems, shippingOptions and paymentMethods

Review commit: https://reviewboard.mozilla.org/r/217998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217998/

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8948579 [details]
Bug 1434508 - better serialization of paymentrequest data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217998/diff/1-2/

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8948579 [details]
Bug 1434508 - better serialization of paymentrequest data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217998/diff/2-3/

Comment 5

[Matthew N. [:MattN]]

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.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8948579 [details]
Bug 1434508 - better serialization of paymentrequest data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217998/diff/3-4/

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8948579 [details]
Bug 1434508 - better serialization of paymentrequest data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217998/diff/4-5/

Comment 8

[Sam Foster [:sfoster] (he/him)]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a573b24eacd0dea0b68a884d276773eaf9b7f3

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a524900f4f4
better serialization of paymentrequest data. r=MattN

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0a524900f4f4

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0a524900f4f4