Closed Bug 1480872 Opened 3 years ago Closed 3 years ago

paymentDetails attributes should default to an empty arrays

Categories

(Core :: DOM: Web Payments, enhancement, P2)

Product:
Core
Component:
DOM: Web Payments
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: jaws, Assigned: marcosc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

update head.js
14.46 KB, patch
Details | Diff | Splinter Review 
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: nobody → mcaceres
Blocks: paymentrequest
Priority: -- → P2
Noting that this applies also to displayItems and modifiers, as payment-dialog having to deal with those.
Summary: paymentDetails.shippingOptions should default to an empty array → paymentDetails attributes should default to an empty arrays
Eden, is there more to it than this? Would appreciate your feedback.
Attachment #8999538 - Flags: feedback?(echuang)
Ignore the test file... fixing it.
Attached 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+
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+
Filed bug for the array conversion - not high priority https://bugzilla.mozilla.org/show_bug.cgi?id=1483083
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)
Still waiting for it to push to try :( hg is sooooo slooooooooow.
Flags: needinfo?(mcaceres)
Keywords: checkin-needed
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)
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.
(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.
Attachment #8999788 - Attachment is obsolete: true
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+
(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)
(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.
Attached patch update head.js — — Splinter Review 
Carrying over r+
Attachment #9001856 - Attachment is obsolete: true
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3378c3864897
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.