1480872 - paymentDetails attributes should default to an empty arrays

Status: RESOLVED FIXED

Description

[Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me)]

Follow-up from https://bugzilla.mozilla.org/show_bug.cgi?id=1477699#c10.

::: browser/components/payments/res/containers/payment-dialog.js:318
(Diff revision 2)
> +        (!request.paymentDetails.shippingOptions ||
> +         !request.paymentDetails.shippingOptions.length)) {

Could you file a DOM follow-up to have `request.paymentDetails.shippingOptions` default to an empty array (I think the spec implies they default to an empty sequence on the web-facing side anyways) so that we don't have to keep handling the `null` vs. `[]` case in the front-end.

Comment 1

[Marcos Caceres [:marcosc]]

Noting that this applies also to displayItems and modifiers, as payment-dialog having to deal with those.

Comment 2

[Marcos Caceres [:marcosc]]

Created attachment 8999538 [details] [diff] [review]
0001-Bug-1480872-paymentDetails-attributes-should-default.patch

Eden, is there more to it than this? Would appreciate your feedback.

Comment 3

[Marcos Caceres [:marcosc]]

Ignore the test file... fixing it.

Comment 4

[Marcos Caceres [:marcosc]]

Created attachment 8999558 [details] [diff] [review]
Fixed tests

Ok, tests are passing... but I was unable to figure out if I should unwrap the details object's properties to actually test for an array? 

Ideally, I would have wanted to check if, for instance, details.displayItems is actually an JS Array. Right now, I guess it's array-like because it has a .length. 

Is that ok, or is there some way to do that? I see other tests also just check length, but not type. 

for example, in checkDupShippingOptionsPayment(), I see:
```
is(aPayment.paymentMethods.length, 1, "paymentMethods' length should be 1.");
```

Comment 5

[Eden Chuang[:edenchuang]]

Comment on attachment 8999538 [details] [diff] [review]
0001-Bug-1480872-paymentDetails-attributes-should-default.patch

Review of attachment 8999538 [details] [diff] [review]:
-----------------------------------------------------------------

It's perfect and it works.

But if we don't really care about the difference between null and empty array, I would like to remove following non-necessary codes.

https://searchfox.org/mozilla-central/source/dom/payments/ipc/PPaymentRequest.ipdl#59-61

and codes related to these attributes.

Comment 6

[Marcos Caceres [:marcosc]]

Makes sense. Let’s get rid of them. I can send an updated patch tomorrow.

Comment 7

[Eden Chuang[:edenchuang]]

Comment on attachment 8999558 [details] [diff] [review]
Fixed tests

Review of attachment 8999558 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, nsIPaymentRequest.details.displayItems is not a JS array, it is a nsIArray, a kind of XPCOM array. So you will get fail if you check its type with JS array.

The reason why it is not a JS array is when I implemented PaymentRequest API, we did not support transforming a CPP array into a JS array directly.
But it was supported one or two months ago, and there are some bugs to ask to drop the nsIArray implementation. 

However, nsIArray is for our internal using to communicate with the UI component, it would affect the merchant side behavior.
For now, I would suggest keeping the current implementation, and we will drop nsIArray and use JS array in the other bug.

Comment 8

[Marcos Caceres [:marcosc]]

Filed bug for the array conversion - not high priority https://bugzilla.mozilla.org/show_bug.cgi?id=1483083

Comment 9

[Marcos Caceres [:marcosc]]

Created attachment 8999788 [details] [diff] [review]
0001-Bug-1480872-paymentDetails-attributes-should-default.patch

Removed redundant members in the ipdl file and fixed related code. Sending to try. Hopefully good to go :)

Comment 10

[Eden Chuang[:edenchuang]]

Marcos, you might put the try result link here. :)

Comment 11

[Marcos Caceres [:marcosc]]

Still waiting for it to push to try :( hg is sooooo slooooooooow.

Comment 12

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0b7b358de58ec7448c2282a9a0b0e4a92b353c

Comment 13

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d785070b0a6edaad505f6be4704e559ef60ee967

Comment 14

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef2aae9cf2dc918801bb428296d8e7bf60f670e

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a77e82d273e
paymentDetails attributes should default to an empty arrays. r=edenchuang

Comment 16

[Dorel Luca [:dluca]]

Backed out changeset 5a77e82d273e (bug 1480872) for browser-chorme failure on dom/payments/test/browser_payment_in_different_tabs.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=194250085&repo=mozilla-inbound&lineNumber=2965

 TEST-PASS | dom/payments/test/browser_payment_in_different_tabs.js | total item's value should be '55.00'. - 
[task 2018-08-16T08:28:32.646Z] 08:28:32     INFO - Buffered messages finished
[task 2018-08-16T08:28:32.654Z] 08:28:32     INFO - TEST-UNEXPECTED-FAIL | dom/payments/test/browser_payment_in_different_tabs.js | 0 - details.displayItems should be a zero length array.
[task 2018-08-16T08:28:32.655Z] 08:28:32     INFO - Stack trace:
[task 2018-08-16T08:28:32.656Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/head.js:checkSimplePayment:20
[task 2018-08-16T08:28:32.657Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:21
[task 2018-08-16T08:28:32.658Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.659Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:10
[task 2018-08-16T08:28:32.662Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.663Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:8
[task 2018-08-16T08:28:32.665Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
[task 2018-08-16T08:28:32.666Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
[task 2018-08-16T08:28:32.667Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999
[task 2018-08-16T08:28:32.668Z] 08:28:32     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-08-16T08:28:32.678Z] 08:28:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-08-16T08:28:32.679Z] 08:28:32     INFO - TEST-UNEXPECTED-FAIL | dom/payments/test/browser_payment_in_different_tabs.js | 0 - details.modifiers should be a zero length array.
[task 2018-08-16T08:28:32.680Z] 08:28:32     INFO - Stack trace:
[task 2018-08-16T08:28:32.681Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/head.js:checkSimplePayment:21
[task 2018-08-16T08:28:32.682Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:21
[task 2018-08-16T08:28:32.683Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.684Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:10
[task 2018-08-16T08:28:32.685Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.686Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:8
[task 2018-08-16T08:28:32.687Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
[task 2018-08-16T08:28:32.688Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
[task 2018-08-16T08:28:32.689Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999
[task 2018-08-16T08:28:32.690Z] 08:28:32     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-08-16T08:28:32.691Z] 08:28:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-08-16T08:28:32.692Z] 08:28:32     INFO - TEST-UNEXPECTED-FAIL | dom/payments/test/browser_payment_in_different_tabs.js | 0 - details.shippingOptions should be a zero length array.
[task 2018-08-16T08:28:32.693Z] 08:28:32     INFO - Stack trace:
[task 2018-08-16T08:28:32.694Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/head.js:checkSimplePayment:22
[task 2018-08-16T08:28:32.695Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:21
[task 2018-08-16T08:28:32.696Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.697Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:10
[task 2018-08-16T08:28:32.697Z] 08:28:32     INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:108
[task 2018-08-16T08:28:32.698Z] 08:28:32     INFO - chrome://mochitests/content/browser/dom/payments/test/browser_payment_in_different_tabs.js:null:8
[task 2018-08-16T08:28:32.699Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
[task 2018-08-16T08:28:32.700Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
[task 2018-08-16T08:28:32.701Z] 08:28:32     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999
[task 2018-08-16T08:28:32.702Z] 08:28:32     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
[task 2018-08-16T08:28:32.704Z] 08:28:32     INFO - TEST-PASS | dom/payments/test/browser_payment_in_different_tabs.js | payerName option should be false - 

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5a77e82d273ef9dec5752614f0bb4fd35bca45d2

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9806cf666e4219fa6fe3ab95d99a360f7ad59e

Comment 17

[Marcos Caceres [:marcosc]]

Sent https://bugzilla.mozilla.org/show_bug.cgi?id=1483788 for review. 

Will try again after that merges.

Comment 18

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Marcos, since comment 16 is a backout that means this bug won't get marked as resolved fixed so you should merge the fixes of bug 1483788 into this original commit and reland from here.

Comment 19

[Marcos Caceres [:marcosc]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #18)
> Marcos, since comment 16 is a backout that means this bug won't get marked
> as resolved fixed so you should merge the fixes of bug 1483788 into this
> original commit and reland from here.

Sure. Does it matter that the two are unrelated? The backout was unfortunate, because it hit an intermittent.

Comment 20

[Marcos Caceres [:marcosc]]

Created attachment 9001856 [details] [diff] [review]
Merged in fix for intermittent test failure

Comment 21

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9399ffa5625b79ef532a985e13e241e78ef8761c

Comment 22

[Marcos Caceres [:marcosc]]

Ok, did as :MattN suggested: Merged in fix from bug 1483788. Let's see what try says :)

Comment 23

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Are you sure it's intermittent? You normally wouldn't get backed out for an intermittent failure. 

The failure seemed related to your change so I thought that was the case. If it was an intermittent then landing the intermittent fix first in its own bug is fine.

> INFO - TEST-UNEXPECTED-FAIL | dom/payments/test/browser_payment_in_different_tabs.js | 0 - details.shippingOptions should be a zero length array.

Comment 24

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee42788da76d95382cfa6d73dd6d9720c878fe4

Comment 26

[Eden Chuang[:edenchuang]]

Comment on attachment 9001856 [details] [diff] [review]
Merged in fix for intermittent test failure

Review of attachment 9001856 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment applied patch.

::: dom/payments/test/head.js
@@ +18,5 @@
>    is(details.totalItem.amount.value, "55.00", "total item's value should be '55.00'.");
>  
> +  ok(details.displayItems.length, 0, "details.displayItems should be a zero length array.");
> +  ok(details.modifiers.length, 0, "details.modifiers should be a zero length array.");
> +  ok(details.shippingOptions.length, 0, "details.shippingOptions should be a zero length array.");

ok method is used for the boolean checking, sometimes we use it for checking the existence.

For the case, you should use is method not ok method.

is(details.displayItems.length, 0, ...);

Also for above two checking.

Comment 27

[Eden Chuang[:edenchuang]]

Marcos, since your Try didn't run with any test cases, you need to submit a Try with test cases.
You can use the following try command

try: -b do -p all -u mochitests -t none

Before you submit a Try, you also can run payment mochitest in your local first under your gecko root directory

for all payment mochitest
./mach mochitest dom/payments/test

for specified payment mochitest
./mach mochitest dom/payments/test/<test_case_file_name>

Comment 28

[Marcos Caceres [:marcosc]]

(In reply to Eden Chuang[:edenchuang] from comment #27)
> Marcos, since your Try didn't run with any test cases, you need to submit a
> Try with test cases.
> You can use the following try command
> 
> try: -b do -p all -u mochitests -t none

Yeah, still had old config set up it seems. 

> Before you submit a Try, you also can run payment mochitest in your local
> first under your gecko root directory
> 
> for all payment mochitest
> ./mach mochitest dom/payments/test
> 
> for specified payment mochitest
> ./mach mochitest dom/payments/test/<test_case_file_name>

Yep, all good locally :)

Comment 29

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db783b9d4c518945609452aaf5d07b3b9cdd67a

Comment 30

[Marcos Caceres [:marcosc]]

(In reply to Eden Chuang[:edenchuang] from comment #26)
> Comment on attachment 9001856 [details] [diff] [review]
> Merged in fix for intermittent test failure
> 
> Review of attachment 9001856 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comment applied patch.
> 
> ::: dom/payments/test/head.js
> @@ +18,5 @@
> >    is(details.totalItem.amount.value, "55.00", "total item's value should be '55.00'.");
> >  
> > +  ok(details.displayItems.length, 0, "details.displayItems should be a zero length array.");
> > +  ok(details.modifiers.length, 0, "details.modifiers should be a zero length array.");
> > +  ok(details.shippingOptions.length, 0, "details.shippingOptions should be a zero length array.");
> 
> ok method is used for the boolean checking, sometimes we use it for checking
> the existence.

Oh whoops, that was copy pasta. Will fix. 

> For the case, you should use is method not ok method.
> 
> is(details.displayItems.length, 0, ...);
> 
> Also for above two checking.

On it.

Comment 31

[Marcos Caceres [:marcosc]]

Created attachment 9002347 [details] [diff] [review]
update head.js

Carrying over r+

Comment 32

[Marcos Caceres [:marcosc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b357a06fc543c3c341e032bde8c58921bc5f07d

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3378c3864897
paymentDetails attributes should default to an empty arrays. r=edenchuang

Comment 34

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3378c3864897