1408250 - Don't expose PaymentRequest Constructor in non-e10s.

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

PaymentRequest constructor should not be able to use under non-e10s.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r?baku

Return false in PaymentRequest::PrefEnabled() while in non-e10s.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8920084 [details] [diff] [review]
Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r?baku

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

::: dom/payments/PaymentRequest.cpp
@@ +51,5 @@
>  
>  bool
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
> +  return mozilla::BrowserTabsRemoteAutostart() &&

You want to check if this method is called in the content process.
Probably you want to use XRE_IsContentProcess() ?

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r?baku

Update the patch according to comment 2.

Using XRE_IsContentProcess instead of BrowserTabsRemoteAutostart().

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8920252 [details] [diff] [review]
Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r?baku

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

Can you add a test where you disable e10s and check if the payment API is disabled.

Comment 5

[Andrew Overholt [:overholt]]

Fennec is non-e10s, though.

Comment 6

[Eden Chuang[:edenchuang]]

(In reply to Andrea Marchesini [:baku] from comment #4)
> Comment on attachment 8920252 [details] [diff] [review]
> Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r?baku
> 
> Review of attachment 8920252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you add a test where you disable e10s and check if the payment API is
> disabled.

The patch includes a test, test_block_none10s.html, which is specified to run with non-e10s in mochitest.ini.

Do you mean that we need another test that disabling e10s in manual and then testing Payment API?

If yes, there is one problem with disabling e10s in manual. We can set e10s prefs, such as browser.tabs.remote.autostart.2 and others, to disable e10s, but we need to restart the browser to apply the prefs changes, however, I don't know how to restart the browser in the mochitest environment. One thing we probably can do is opening a non-e10s tab for testing Payment API, but it seems to be different with disabling e10s in manual.(In reply to Andrew Overholt [:overholt] from comment #5)

> Fennec is non-e10s, though.

Yes, according to Product's plan, currently we only support WebPayment on desktop. Maybe they a plan for mobile, I will check with them.

Comment 7

[Andrea Marchesini [:baku]]

You can create a normal mochitest and mark it as 'skip-if = e10s' in mochitest.ini

Comment 8

[Eden Chuang[:edenchuang]]

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

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1408250 - Don't expose PaymentRequest API in non-e10s. r=baku

modify the configuration in mochitest.ini from "run-if = !e10s" to "skip-if = e10s".

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2dad936121
Don't expose PaymentRequest API in non-e10s. r=baku

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/3d2dad936121