Closed Bug 1382092 Opened 3 years ago Closed 3 years ago

[Payment Request API] Support default payment UI service in DOM code

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: edenchuang, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 9 obsolete files)

11.11 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
17.23 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
4.73 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
818 bytes, patch
edenchuang
: review+
Details | Diff | Splinter Review
Default Payment UI service is implemented 
https://hg.mozilla.org/integration/autoland/rev/4f577bce4a5d5dae7bdd0eb3db769c571c555260#l10.3 

DOM code need to use this UI service in default.
Assignee: nobody → echuang
Blocks: 1383597
nsIPaymentUIService is going to be modified with following changes
1. Remove unnecessary method canMakePayment(). Because this response should be answered in DOM code.
2. Remove return value for all methods. Because UI needn't return the response immediately.

The part 1 patch modifies the paymentUIService.js to fit above changes.
Attachment #8889450 - Flags: review?(MattN+bmo)
Support default payment UI service in DOM code.

If testing UI service is registered, running testing UI service, otherwise, running default UI service.
Attachment #8889454 - Flags: review?(amarchesini)
Update the existing mochitests for default payment UI support.
Attachment #8889455 - Flags: review?(amarchesini)
Comment on attachment 8889454 [details] [diff] [review]
Bug 1382092 - Support default payment UI service in DOM code part 2. r?baku

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

::: dom/payments/PaymentRequestService.cpp
@@ +156,2 @@
>    } else {
> +    uiService = do_GetService(NS_PAYMENT_UI_SERVICE_CONTRACT_ID);

This can fail.
Attachment #8889454 - Flags: review?(amarchesini) → review+
Attachment #8889455 - Flags: review?(amarchesini) → review+
Comment on attachment 8889450 [details] [diff] [review]
Bug 1382092 - Support default payment UI service in DOM code part 1. r?MattN

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

> 1. Remove unnecessary method canMakePayment(). Because this response should be answered in DOM code.

I thought that what is available in storage (accessed via the UI service) would affect the response. Is this no longer true?

::: toolkit/components/payments/paymentUIService.js
@@ +10,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +const paymentSrv = Cc["@mozilla.org/dom/payments/payment-request-service;1"]
> +                     .getService(Ci.nsIPaymentRequestService);

This should use XPCOMUtils.defineLazyServiceGetter so that if the ui service is loaded that it won't unnecessarily create the payment request service.
Attachment #8889450 - Flags: review?(MattN+bmo)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #6)
> Comment on attachment 8889450 [details] [diff] [review]
> Bug 1382092 - Support default payment UI service in DOM code part 1. r?MattN
> 
> Review of attachment 8889450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > 1. Remove unnecessary method canMakePayment(). Because this response should be answered in DOM code.
> 
> I thought that what is available in storage (accessed via the UI service)
> would affect the response. Is this no longer true?

I think canMakePayment response should not be related to the storage.
If merchant specifies the supportedMethods with "basic-card" and its data has valid supportedNetworks and supportedTypes, we should return true with canMakePayment().
Might be I miss something that how the storage data affect the response result.
Could you give me an example how the storage affect the response result?

> ::: toolkit/components/payments/paymentUIService.js
> @@ +10,5 @@
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  
> > +const paymentSrv = Cc["@mozilla.org/dom/payments/payment-request-service;1"]
> > +                     .getService(Ci.nsIPaymentRequestService);
> 
> This should use XPCOMUtils.defineLazyServiceGetter so that if the ui service
> is loaded that it won't unnecessarily create the payment request service.

will using XPCOMUtils.defineLazyServiceGetter.
Flags: needinfo?(MattN+bmo)
According to review in comment 6, using XPCOMUtils.defineLazyServiceGetter to get nsIPaymentRequestService instance.
Attachment #8889450 - Attachment is obsolete: true
Attachment #8892402 - Flags: review?(MattN+bmo)
(In reply to Eden Chuang[:edenchuang] from comment #7)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #6)
> > Comment on attachment 8889450 [details] [diff] [review]
> > Bug 1382092 - Support default payment UI service in DOM code part 1. r?MattN
> > 
> > Review of attachment 8889450 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > > 1. Remove unnecessary method canMakePayment(). Because this response should be answered in DOM code.
> > 
> > I thought that what is available in storage (accessed via the UI service)
> > would affect the response. Is this no longer true?
> 
> I think canMakePayment response should not be related to the storage.
> If merchant specifies the supportedMethods with "basic-card" and its data
> has valid supportedNetworks and supportedTypes, we should return true with
> canMakePayment().
> Might be I miss something that how the storage data affect the response
> result.
> Could you give me an example how the storage affect the response result?

I thought there was an example in the spec that involved taking storage into account but if you say there isn't then this is fine.
Flags: needinfo?(MattN+bmo)
Attachment #8892402 - Flags: review?(MattN+bmo) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/869c1393d5e2
Support default payment UI service in DOM code part 1. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54efba3a04f
Support default payment UI service in DOM code part 2. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b532acf3b5ea
Mochitest for supporting default payment UI in DOM code. r=baku
Keywords: checkin-needed
Flags: needinfo?(echuang)
Attachment #8894446 - Flags: review+
Attachment #8894151 - Attachment is obsolete: true
Attachment #8894446 - Attachment is obsolete: true
Attachment #8894447 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84aac02c78a9
Support default payment UI in DOM code part 1. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/39c022e64d98
Support default payment UI service in DOM code part 2. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eaa794d2533
Mochitest for supporting default payment UI in DOM code. r=baku
Keywords: checkin-needed
(In reply to Wes Kocher (:KWierso) from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/84aac02c78a9

This is going to break packaging on Beta because toolkit/components/moz.build still only includes the payments directory conditional on NIGHTLY_BUILD. Please fix ASAP.
https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/moz.build#l117
Flags: needinfo?(echuang)
Fix the problem mentioned in comment 24.

Ryan, could you help me to check in the part 3 patch to resolve the problem? Thanks.
Flags: needinfo?(echuang) → needinfo?(ryanvm)
Attachment #8896562 - Flags: review+
Will do, thanks!
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4991cb4ac8
Remove the NIGHTLY_BUILD requirement for building the payments UI service. r=RyanVM
Hello Ryan

Sorry to bother you again. Could you help me to check in the part 3 patch again to fix the problem in comment 30?

Thanks.
Attachment #8896562 - Attachment is obsolete: true
Flags: needinfo?(echuang) → needinfo?(ryanvm)
Attachment #8896622 - Flags: review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/113580d267ea
Support default payment UI service in DOM code part 3. r=MattN.
Depends on: 1390018
Hi Ryan

To resolve the root cause of bug 1390009 and bug 1380018, could you help me to back out all the patches of this bug from mozilla-central and Beta?
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
This doesn't backout cleanly at this point.
Flags: needinfo?(ryanvm) → needinfo?(echuang)
After discussing with Matt, I will write a patch to keep toolkit/components/payments being included in nightly only.
If "if CONFIG['NIGHTLY_BUILD']" is restored back in toolkit/components/moz.build, will this block packaging Gecko57 to Beta?
Because if we want to keep toolkit/components/payments being included in nightly, we need this "if" judgment in the file.
Flags: needinfo?(echuang) → needinfo?(ryanvm)
Yes, reverting the moz.build patch will re-introduce the packaging bustage I originally pinged about. You'll also need to update the packaging manifest to only include those files if they're on Nightly, i.e. reverting this hunk:
https://hg.mozilla.org/integration/mozilla-inbound/diff/84aac02c78a9/browser/installer/package-manifest.in
Flags: needinfo?(ryanvm)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.