1481971 - Enable PaymentRequest by default on Windows/macOS Nightlies for US/CA users on en-US builds

Status: VERIFIED FIXED

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

Comment 1

[Diego Pino]

Attachment: 0001-Enable-WebPayments-by-default-in-en-US-builds.patch

The attached patch enables web payments in en-US builds. I'm less sure about the other half of the bug (enable web payments if location is US or CA).

Comment 2

[Matthew N. [:MattN]]

You can probably implement the locale check in https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/dom/payments/PaymentRequest.cpp#56-64 instead of using configure.

Comment 3

[Matthew N. [:MattN]]

Sorry, I meant the country check using the pref in the user story.

Comment 4

[Diego Pino]

Attachment: Enable PaymentRequest by default on Nightly only for US/CA users on en-US builds

Thanks for the pointer :) I amended the previous patch to additionally enable WebPayments if region is US or CA.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9000201 [details] [diff] [review]
Enable PaymentRequest by default on Nightly only for US/CA users on en-US builds

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

Note that we're not ready to enable on Nightly yet anyways so this wouldn't be able to land yet even if it gets r+ soon.

::: dom/payments/PaymentRequest.cpp
@@ +59,5 @@
> +  nsAutoString region;
> +  Preferences::GetString("browser.search.region", region);
> +  if (region.EqualsLiteral("US") || region.EqualsLiteral("CA")) {
> +    Preferences::SetBool("dom.payments.request.enabled", true);
> +  }

I was thinking the locale/region logic would be:

* If the pref dom.payments.request.enabled isn't enabled, return false
* If you're not in an en-US build, return false;
* If you're not in one of the valid countries, return false;

Otherwise return XRE_IsContentProcess()

Which means if you're not in a supported region then you need to change your region pref in order to test the feature. Ideally the region list would be a comma-separated list but I'm not sure if that's overkill.

::: moz.configure
@@ +317,5 @@
> +def enable_web_payments(locale):
> +    return locale[0] == 'en-US'
> +
> +# set_config('MOZ_WEBPAYMENTS', enable_web_payments)
> +set_define('MOZ_WEBPAYMENTS', enable_web_payments)

I think we can keep all of the logic in PaymentRequest::PrefEnabled to keep it together and because I'm not sure --enable-ui-locale will work with l10n repacks.

I don't think we need a define to control the feature in this case, we can use #ifdef NIGHTLY_BUILD

Comment 6

[Diego Pino]

Attachment: Enable-PaymentRequest-by-default-on-Nightly-only-for.patch

I addressed the changes requested.

OTOH, I tried to figure out how to fetch the language used in the build. I looked into about:config, searching by "en-US" but I couldn't find anything meaningful. Any hint?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 9001554 [details] [diff] [review]
Enable-PaymentRequest-by-default-on-Nightly-only-for.patch

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

> OTOH, I tried to figure out how to fetch the language used in the build. 

Related form autofill JS code uses `Services.locale.getRequestedLocale()`[1] so I guess seeing how that's implemented is a hint.

[1] https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/browser/extensions/formautofill/bootstrap.js#65,73

Comment 8

[Diego Pino]

Attachment: Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

Thanks for the hint. I amended the patch to add the language check. I wrote similar code to https://dxr.mozilla.org/mozilla-central/source/dom/encoding/FallbackEncoding.cpp?q=FallbackEncoding.cpp&redirect_type=direct#92.

Comment 9

[Matthew N. [:MattN]]

Sorry for the delay, I've been putting off the review since we're not ready to land this yet. I'll look at this when I come back on the 10th.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 9003060 [details] [diff] [review]
Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

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

Redirecting the reviews but please don't land this even if it gets r+. We will land it when we're done milestone 3.

::: dom/payments/PaymentRequest.cpp
@@ -60,5 @@
> -  return XRE_IsContentProcess() &&
> -         Preferences::GetBool("dom.payments.request.enabled");
> -#else
> -  return false;
> -#endif

Can you leave the ifdef so that users cannot enable the feature by flipping the pref in non-Nightlies. We got bug reports for this in the past and don't want to have to support that for now.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9003060 [details] [diff] [review]
Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

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

I don't know enough about regions and locales. I just removed the DOM part of the patch.

::: dom/payments/PaymentRequest.cpp
@@ +59,5 @@
>  bool
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
> +  nsAutoString supportedRegions[] = { "US", "CA" };
> +

I agree with MattN. What you want here is:

#ifndef NIGHTLY_BUILD
  return false;
#endif

Why removing XRE_IsContentProcess()? payment API doesn't behave correctly in non-e10s mode. Keep that check.

@@ +60,5 @@
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
> +  nsAutoString supportedRegions[] = { "US", "CA" };
> +
> +  if (!Preferences::GetBool("dom.payments.request.enabled")) {

Can we have a StaticPref here?

::: modules/libpref/init/all.js
@@ +5856,5 @@
>  pref("dom.timeout.max_consecutive_callbacks_ms", 4);
>  
>  // Use this preference to house "Payment Request API" during development
> +#ifdef NIGHTLY_BUILD
> +pref("dom.payments.request.enabled", true);

Move this to StaticPrefs

Comment 12

[Diego Pino]

Attachment: Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

Updated patch addressing feedback. With regard to the XRE_IsContentProcess() check, I didn't remove it in the previous patch, the check was at the bottom. Now I moved that check to the front.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 9008386 [details] [diff] [review]
Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

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

::: modules/libpref/init/StaticPrefList.h
@@ +189,5 @@
> +VARCACHE_PREF(
> +  "dom.payments.request.enabled",
> +  dom_payments_request_enabled,
> +  bool, false
> +)

If you want to enable it in nightly, you want to do:

#ifdef NIGHTLY_BUILD
# define PREF_VALUE  true
#else
# define PREF_VALUE  false
#endif
VARCACHE_PREF(
  "dom.payments.request.enabled",
  dom_payments_request_enabled,
  bool, PREF_VALUE
)
#undef PREF_VALUE

Comment 14

[Diego Pino]

Attachment: Bug 1481971 - Enable PaymentRequest by default on Nightly only for US/CA users on en-US builds

Updated patch with feedback from :baku:

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 9008641 [details] [diff] [review]
Bug 1481971 - Enable PaymentRequest by default on Nightly only for US/CA users on en-US builds

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

Minor suggestion to avoid matching against hardcoded strings.

::: dom/payments/PaymentRequest.cpp
@@ +82,5 @@
> +    return false;
> +  }
> +  nsAutoCString locale;
> +  LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);
> +  if (!locale.EqualsLiteral("en-US")) {

I would prefer if you created a MozLocale out of the returned string and verified if `getLanguage` returns `en` and `getRegion` returns `US`.

Comment 16

[Diego Pino]

Attachment: Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

Updated patch with suggested changes by :gandalf, and added reviewers to commit message.

Comment 17

[Matthew N. [:MattN]]

Thank Diego. We have 8 bugs to fix before we can enable on Nightly.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=058085abcfbc22cc92a5a1280f6bbae8d026d4e6

Comment 18

[Matthew N. [:MattN]]

That try push had an infra. bug. Here's a new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a804ebba3d5d101479edef4d9e4b16dda5f94f77

Comment 19

[Matthew N. [:MattN]]

And I messed up the syntax on that one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfb98ab608c9fb78de05cf329b2cd774f7bf569f

Comment 20

[Matthew N. [:MattN]]

This depends on bug 1481295 to update the WPT test expectations but test_interfaces_secureContext.html will need to be addressed separately. I think we just need to change all of the Payment* interfaces to only be exposed to secure contexts so I filed bug 1494054. Not sure if Diego or Eden want to work on that.

Comment 21

[Marco Mucci [:MarcoM]]

For assigned bugs being actively worked on we put the priority at P1.

Comment 22

[Matthew N. [:MattN]]

Please update dom/tests/mochitest/general/test_interfaces_secureContext.html so that it passes and re-request review from :baku. Run `./mach test dom/tests/mochitest/general/test_interfaces_secureContext.html` to test.

We should be able to land this next week so getting this done ASAP would be great.

Comment 23

[Diego Pino]

Hi, I was away last week (conference). I'm catching up on email at the moment. I will try to move the ticket forward as soon as I can.

Comment 24

[Diego Pino]

Attachment: Bug-1481971-Enable-PaymentRequest-by-default-on-Nigh.patch

Make `dom/tests/mochitest/general/test_interfaces_secureContext.html` pass.

Comment 25

[Matthew N. [:MattN]]

Rebased try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=319dbe4e3256ae5c224e1323cbd5a74eed985697
Rebased beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea3aa06257ea2e77c467aee5465a7fb01d57b10d

Comment 26

[Matthew N. [:MattN]]

Attachment: Run payment-handler, payment-method-basic-card, & payment-method-id WPT with the pref

Bug 1481295 missed running payment-handler, payment-method-basic-card, & payment-method-id tests with the pref.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73912f1633a1
Enable PaymentRequest on Nightly for Windows/macOS for US/CA users on en-US builds r=MattN,baku,gandalf
https://hg.mozilla.org/integration/mozilla-inbound/rev/025fdcb0c798
Update payment-(handler|method-basic-card|method-id) wpt expectations. r=marcosc

Comment 28

[Matthew N. [:MattN]]

This bug is only enabling for Windows and macOS as we're figuring out the re-authentication plan for Linux in bug 1494478 and dependencies. See bug 1494478 comment 8.

Thanks for your help Diego!

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/73912f1633a1
https://hg.mozilla.org/mozilla-central/rev/025fdcb0c798

Comment 30

[Hani Yacoub, Desktop QA]

"dom.payments.request.enabled" is now pref'd ON by default on the latest Nightly but web payments dialog isn't displayed after clicking on "Buy" button, "PaymentRequest API is not supported." error appears.

We also tried accessing test pages via VPN connection (USA) and we confronted with the same issue.

Comment 31

[Hani Yacoub, Desktop QA]

My bad :( I didn't read the user story.

Comment 32

[Hani Yacoub, Desktop QA]

Verified as fixed on the latest Firefox Nightly 64.0a1 on Windows 10/7 x64 and on Mac OS X 10.13.