Closed
Bug 1481971
Opened 6 years ago
Closed 6 years ago
Enable PaymentRequest by default on Windows/macOS Nightlies for US/CA users on en-US builds
Categories
(Core :: DOM: Web Payments, enhancement, P1)
Core
DOM: Web Payments
Tracking
()
VERIFIED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | verified |
People
(Reporter: MattN, Assigned: dpino)
References
Details
(Keywords: dev-doc-needed, 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 files, 7 obsolete files)
8.55 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.69 KB,
patch
|
marcosc
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•6 years ago
|
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee | ||
Comment 1•6 years 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?
Reporter | ||
Comment 2•6 years ago
|
||
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.
Reporter | ||
Comment 3•6 years ago
|
||
Sorry, I meant the country check using the pref in the user story.
Assignee | ||
Comment 4•6 years 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)
Reporter | ||
Comment 5•6 years ago
|
||
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•6 years 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)
Reporter | ||
Comment 7•6 years ago
|
||
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+
Updated•6 years ago
|
Whiteboard: [webpayments-reserve] → [webpayments-reserve][webpayments][triage]
Updated•6 years ago
|
Priority: P2 → --
Whiteboard: [webpayments-reserve][webpayments][triage] → [webpayments] [triage]
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [webpayments] [triage] → [webpayments]
Assignee | ||
Comment 8•6 years 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)
Updated•6 years ago
|
Assignee: nobody → dpino
Status: NEW → ASSIGNED
Priority: P2 → P1
Reporter | ||
Comment 9•6 years ago
|
||
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.
Updated•6 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 10•6 years ago
|
||
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 11•6 years ago
|
||
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•6 years 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)
Reporter | ||
Updated•6 years ago
|
Attachment #9008386 -
Flags: review?(gandalf)
Attachment #9008386 -
Flags: review?(amarchesini)
Comment 13•6 years ago
|
||
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•6 years 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)
Updated•6 years ago
|
Attachment #9008641 -
Flags: review?(amarchesini) → review+
Comment 15•6 years ago
|
||
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•6 years ago
|
||
Updated patch with suggested changes by :gandalf, and added reviewers to commit message.
Attachment #9008641 -
Attachment is obsolete: true
Updated•6 years ago
|
Whiteboard: [webpayments] → [webpayments-reserve]
Reporter | ||
Comment 17•6 years ago
|
||
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 hidden (obsolete) |
Reporter | ||
Comment 19•6 years ago
|
||
And I messed up the syntax on that one. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfb98ab608c9fb78de05cf329b2cd774f7bf569f
Reporter | ||
Comment 20•6 years ago
|
||
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•6 years ago
|
Priority: P1 → P2
Comment 21•6 years ago
|
||
For assigned bugs being actively worked on we put the priority at P1.
Priority: P2 → P1
Reporter | ||
Comment 22•6 years ago
|
||
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•6 years 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•6 years ago
|
||
Make `dom/tests/mochitest/general/test_interfaces_secureContext.html` pass.
Attachment #9009617 -
Attachment is obsolete: true
Attachment #9013233 -
Flags: review?(amarchesini)
Updated•6 years ago
|
Attachment #9013233 -
Flags: review?(amarchesini) → review+
Reporter | ||
Comment 25•6 years ago
|
||
Reporter | ||
Comment 26•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #9017744 -
Flags: review?(mcaceres) → review+
Reporter | ||
Updated•6 years ago
|
Attachment #9017744 -
Flags: review?(echuang)
Comment 27•6 years 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
Reporter | ||
Comment 28•6 years ago
|
||
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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73912f1633a1
https://hg.mozilla.org/mozilla-central/rev/025fdcb0c798
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 30•6 years 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 32•6 years 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+
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•