paymentDetails attributes should default to an empty arrays

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: jaws, Assigned: marcosc)

Tracking

(Blocks 1 bug)

57 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Assignee

Updated

10 months ago
Assignee: nobody → mcaceres
Assignee

Updated

10 months ago
Priority: -- → P2
Assignee

Comment 1

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

Updated

9 months ago
Summary: paymentDetails.shippingOptions should default to an empty array → paymentDetails attributes should default to an empty arrays
Assignee

Comment 2

9 months ago
Eden, is there more to it than this? Would appreciate your feedback.
Attachment #8999538 - Flags: feedback?(echuang)
Assignee

Comment 3

9 months ago
Ignore the test file... fixing it.
Assignee

Comment 4

9 months ago
Posted patch Fixed tests (obsolete) — Splinter Review
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.");
```
Attachment #8999558 - Flags: feedback?(echuang)
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.
Attachment #8999538 - Flags: feedback?(echuang) → feedback+
Assignee

Comment 6

9 months ago
Makes sense. Let’s get rid of them. I can send an updated patch tomorrow.
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.
Attachment #8999558 - Flags: feedback?(echuang) → feedback+
Assignee

Comment 8

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

Comment 9

9 months ago
Removed redundant members in the ipdl file and fixed related code. Sending to try. Hopefully good to go :)
Attachment #8999538 - Attachment is obsolete: true
Attachment #8999558 - Attachment is obsolete: true
Attachment #8999788 - Flags: review?(echuang)
Attachment #8999788 - Flags: review?(echuang) → review+
Marcos, you might put the try result link here. :)
Flags: needinfo?(mcaceres)
Assignee

Comment 11

9 months ago
Still waiting for it to push to try :( hg is sooooo slooooooooow.
Flags: needinfo?(mcaceres)
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 15

9 months ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a77e82d273e
paymentDetails attributes should default to an empty arrays. r=edenchuang
Keywords: checkin-needed
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
Flags: needinfo?(mcaceres)
Assignee

Comment 17

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

Will try again after that merges.
Flags: needinfo?(mcaceres)
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.
Assignee

Comment 19

9 months ago
(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.
Assignee

Comment 20

9 months ago
Attachment #8999788 - Attachment is obsolete: true
Assignee

Comment 22

9 months ago
Ok, did as :MattN suggested: Merged in fix from bug 1483788. Let's see what try says :)
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.
Duplicate of this bug: 1483788
Attachment #9001856 - Flags: review?(echuang)
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.
Attachment #9001856 - Flags: review+
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>
Flags: needinfo?(mcaceres)
Attachment #9001856 - Flags: review?(echuang) → review+
Assignee

Comment 28

9 months ago
(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 :)
Flags: needinfo?(mcaceres)
Assignee

Comment 30

9 months ago
(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.
Assignee

Comment 31

9 months ago
Carrying over r+
Attachment #9001856 - Attachment is obsolete: true
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 33

9 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3378c3864897
paymentDetails attributes should default to an empty arrays. r=edenchuang
Keywords: checkin-needed

Comment 34

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3378c3864897
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.