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

VERIFIED FIXED in Firefox 64

Status

()

enhancement
P1
normal
VERIFIED FIXED
9 months ago
4 months ago

People

(Reporter: MattN, Assigned: dpino)

Tracking

({dev-doc-needed})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified)

Details

(Whiteboard: [webpayments-reserve])

User Story

The feature should be limited to US/CA by geolocation (browser.search.region pref) and to en-US builds.

Attachments

(2 attachments, 7 obsolete attachments)

Comment hidden (empty)
Flags: qe-verify+
QA Contact: hani.yacoub
(Assignee)

Comment 1

8 months ago
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).
Attachment #8999927 - Flags: feedback?
Sorry, I meant the country check using the pref in the user story.
(Assignee)

Comment 4

8 months ago
Thanks for the pointer :) I amended the previous patch to additionally enable WebPayments if region is US or CA.
Attachment #9000201 - Flags: review?(MattN+bmo)
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
Attachment #9000201 - Flags: review?(MattN+bmo)
(Assignee)

Comment 6

8 months ago
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?
Attachment #8999927 - Attachment is obsolete: true
Attachment #9000201 - Attachment is obsolete: true
Attachment #8999927 - Flags: feedback?
Attachment #9001554 - Flags: feedback?(MattN+bmo)
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
Attachment #9001554 - Flags: feedback?(MattN+bmo) → feedback+
Whiteboard: [webpayments-reserve] → [webpayments-reserve][webpayments][triage]
Priority: P2 → --
Whiteboard: [webpayments-reserve][webpayments][triage] → [webpayments] [triage]
Priority: -- → P2
Whiteboard: [webpayments] [triage] → [webpayments]
(Assignee)

Comment 8

8 months ago
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.
Attachment #9001554 - Attachment is obsolete: true
Attachment #9003060 - Flags: review?(MattN+bmo)
Assignee: nobody → dpino
Status: NEW → ASSIGNED
Priority: P2 → P1
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 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.
Attachment #9003060 - Flags: review?(gandalf)
Attachment #9003060 - Flags: review?(amarchesini)
Attachment #9003060 - Flags: review?(MattN+bmo)
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
Attachment #9003060 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 12

8 months ago
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.
Attachment #9003060 - Attachment is obsolete: true
Attachment #9003060 - Flags: review?(gandalf)
Attachment #9008386 - Flags: review?(gandalf)
Attachment #9008386 - Flags: review?(amarchesini)
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
Attachment #9008386 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 14

7 months ago
Updated patch with feedback from :baku:
Attachment #9008386 - Attachment is obsolete: true
Attachment #9008386 - Flags: review?(gandalf)
Attachment #9008641 - Flags: review?(gandalf)
Attachment #9008641 - Flags: review?(amarchesini)
Attachment #9008641 - Flags: review?(amarchesini) → review+
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`.
Attachment #9008641 - Flags: review?(gandalf) → review+
(Assignee)

Comment 16

7 months ago
Updated patch with suggested changes by :gandalf, and added reviewers to commit message.
Attachment #9008641 - Attachment is obsolete: true
Whiteboard: [webpayments] → [webpayments-reserve]
Comment hidden (obsolete)

Updated

7 months ago
Depends on: 1494054
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.

Updated

7 months ago
Priority: P1 → P2
For assigned bugs being actively worked on we put the priority at P1.
Priority: P2 → P1
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.
Flags: needinfo?(dpino)
(Assignee)

Comment 23

7 months ago
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.
Flags: needinfo?(dpino)
(Assignee)

Comment 24

7 months ago
Make `dom/tests/mochitest/general/test_interfaces_secureContext.html` pass.
Attachment #9009617 - Attachment is obsolete: true
Attachment #9013233 - Flags: review?(amarchesini)
Attachment #9013233 - Flags: review?(amarchesini) → review+
Bug 1481295 missed running payment-handler, payment-method-basic-card, & payment-method-id tests with the pref.
Attachment #9017744 - Flags: review?(mcaceres)
Attachment #9017744 - Flags: review?(echuang)
Attachment #9017744 - Flags: review?(mcaceres) → review+
Attachment #9017744 - Flags: review?(echuang)

Comment 27

6 months ago
Pushed by mozilla@noorenberghe.ca:
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
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!
Summary: Enable PaymentRequest by default on Nightly only for US/CA users on en-US builds → Enable PaymentRequest by default on Windows/macOS Nightlies for US/CA users on en-US builds

Comment 29

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73912f1633a1
https://hg.mozilla.org/mozilla-central/rev/025fdcb0c798
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 30

6 months ago
"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.
Flags: needinfo?(dpino)

Comment 31

6 months ago
My bad :( I didn't read the user story.
Flags: needinfo?(dpino)

Comment 32

6 months ago
Verified as fixed on the latest Firefox Nightly 64.0a1 on Windows 10/7 x64 and on Mac OS X 10.13.
Flags: qe-verify+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.