Closed
Bug 1480872
Opened 5 years ago
Closed 5 years ago
paymentDetails attributes should default to an empty arrays
Categories
(Core :: DOM: Web Payments, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jaws, Assigned: marcosc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
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 | ||
Updated•5 years ago
|
Assignee: nobody → mcaceres
Assignee | ||
Updated•5 years ago
|
Blocks: paymentrequest
Priority: -- → P2
Assignee | ||
Comment 1•5 years ago
|
||
Noting that this applies also to displayItems and modifiers, as payment-dialog having to deal with those.
Assignee | ||
Updated•5 years ago
|
Summary: paymentDetails.shippingOptions should default to an empty array → paymentDetails attributes should default to an empty arrays
Assignee | ||
Comment 2•5 years ago
|
||
Eden, is there more to it than this? Would appreciate your feedback.
Attachment #8999538 -
Flags: feedback?(echuang)
Assignee | ||
Comment 3•5 years ago
|
||
Ignore the test file... fixing it.
Assignee | ||
Comment 4•5 years ago
|
||
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 5•5 years ago
|
||
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•5 years ago
|
||
Makes sense. Let’s get rid of them. I can send an updated patch tomorrow.
Comment 7•5 years ago
|
||
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•5 years ago
|
||
Filed bug for the array conversion - not high priority https://bugzilla.mozilla.org/show_bug.cgi?id=1483083
Assignee | ||
Comment 9•5 years 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)
Updated•5 years ago
|
Attachment #8999788 -
Flags: review?(echuang) → review+
Comment 10•5 years ago
|
||
Marcos, you might put the try result link here. :)
Flags: needinfo?(mcaceres)
Assignee | ||
Comment 11•5 years ago
|
||
Still waiting for it to push to try :( hg is sooooo slooooooooow.
Flags: needinfo?(mcaceres)
Assignee | ||
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d0b7b358de58ec7448c2282a9a0b0e4a92b353c
Assignee | ||
Comment 13•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d785070b0a6edaad505f6be4704e559ef60ee967
Assignee | ||
Comment 14•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef2aae9cf2dc918801bb428296d8e7bf60f670e
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Comment 15•5 years 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
Comment 16•5 years ago
|
||
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•5 years ago
|
||
Sent https://bugzilla.mozilla.org/show_bug.cgi?id=1483788 for review. Will try again after that merges.
Flags: needinfo?(mcaceres)
Comment 18•5 years ago
|
||
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•5 years 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•5 years ago
|
||
Attachment #8999788 -
Attachment is obsolete: true
Assignee | ||
Comment 21•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9399ffa5625b79ef532a985e13e241e78ef8761c
Assignee | ||
Comment 22•5 years ago
|
||
Ok, did as :MattN suggested: Merged in fix from bug 1483788. Let's see what try says :)
Comment 23•5 years ago
|
||
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.
Assignee | ||
Comment 24•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee42788da76d95382cfa6d73dd6d9720c878fe4
Updated•5 years ago
|
Attachment #9001856 -
Flags: review?(echuang)
Comment 26•5 years ago
|
||
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+
Comment 27•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #9001856 -
Flags: review?(echuang) → review+
Assignee | ||
Comment 28•5 years 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 29•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6db783b9d4c518945609452aaf5d07b3b9cdd67a
Assignee | ||
Comment 30•5 years 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•5 years ago
|
||
Carrying over r+
Attachment #9001856 -
Attachment is obsolete: true
Assignee | ||
Comment 32•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b357a06fc543c3c341e032bde8c58921bc5f07d
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Comment 33•5 years 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3378c3864897
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•4 years ago
|
Keywords: dev-doc-needed
Updated•4 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•