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

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

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.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 1. r?MattN

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.

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 2. r?baku

Support default payment UI service in DOM code.

If testing UI service is registered, running testing UI service, otherwise, running default UI service.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Mochitest for support default payment UI in DOM code. r?baku

Update the existing mochitests for default payment UI support.

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Eden Chuang[:edenchuang]]

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

Comment 6

[Matthew N. [:MattN]]

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.

Comment 7

[Eden Chuang[:edenchuang]]

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

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 1. r?MattN

According to review in comment 6, using XPCOMUtils.defineLazyServiceGetter to get nsIPaymentRequestService instance.

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 2. r=baku

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Mochitest for support default payment UI in DOM code. r=baku

Comment 11

[Matthew N. [:MattN]]

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

Comment 12

[Eden Chuang[:edenchuang]]

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

Comment 13

[Eden Chuang[:edenchuang]]

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

Comment 14

[Eden Chuang[:edenchuang]]

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

Comment 15

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 1. r=MattN

Comment 16

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 2. r=baku

Comment 17

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Mochitest for support default payment UI in DOM code. r=baku

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser_all_files_referenced.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b32e958d9923f6db21962fcced570db1a06fe84
https://hg.mozilla.org/integration/mozilla-inbound/rev/869cb94f4f556e568c8d880ac5ac9573846bad7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b152562e5da9af54d8157b5515811a6a51f33958

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b532acf3b5ea597b7e70140fd71aad9177bd11f1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=121275831&repo=mozilla-inbound

[task 2017-08-06T13:19:33.895156Z] 13:19:33     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unused whitelist entry: chrome://payments/content/paymentRequest.xhtml -

Comment 20

[Eden Chuang[:edenchuang]]

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

Comment 21

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 1. r=MattN

Comment 22

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part 1. r=MattN

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84aac02c78a9
https://hg.mozilla.org/mozilla-central/rev/39c022e64d98
https://hg.mozilla.org/mozilla-central/rev/1eaa794d2533

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 26

[Eden Chuang[:edenchuang]]

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

Comment 27

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part3. r=MattN

Fix the problem mentioned in comment 24.

Ryan, could you help me to check in the part 3 patch to resolve the problem? Thanks.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Will do, thanks!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4991cb4ac8
Remove the NIGHTLY_BUILD requirement for building the payments UI service. r=RyanVM

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Android bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e67a1b834af03666a879c3445bb9e1538848f428

The patch also removed the condition to only include it if Firefox for Desktop gets built:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4991cb4ac88b3da30ca18db2b21042c17e1302#l1.31

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4f4991cb4ac88b3da30ca18db2b21042c17e1302&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=122757635&repo=mozilla-inbound

Comment 31

[Eden Chuang[:edenchuang]]

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

Comment 32

[Eden Chuang[:edenchuang]]

Attachment: Bug 1382092 - Support default payment UI service in DOM code part3. r=MattN

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.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/113580d267ea
Support default payment UI service in DOM code part 3. r=MattN.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

In the future, please try to use more descriptive commit messages, BTW.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/113580d267ea

Comment 36

[Eden Chuang[:edenchuang]]

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?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

This doesn't backout cleanly at this point.

Comment 38

[Eden Chuang[:edenchuang]]

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.

Comment 39

[Ryan VanderMeulen [: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