Closed
Bug 1385196
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Enable the feature based on locale and geoip
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: lchang, Assigned: lchang)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M3])
Attachments
(2 files, 1 obsolete file)
According to MVP scope, Form Autofill feature should default to enabled when a user is using en-US build and located in U.S.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Hi Matt, I didn't call `defineLazyPreferenceGetter` in my patch because changing `IS_AUTOFILL_AVAILABLE` seems not helpful as we won't re-trigger `startup()` at runtime.
Assignee | ||
Comment 3•7 years ago
|
||
Hi Mike, I checked "browser.search.region" instead of "countryCode" in my patch so it's supposed to align with search according to the description [1]. Do you think it makes sense for us to check whether a user is located in U.S.? [1] http://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#378
Flags: needinfo?(mozilla)
Comment 4•7 years ago
|
||
This function is what we use to verify if a user is in the US: http://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#428 It relies primarily on en-US as the locale and US as the browser.search.region, so you should be good. You could add the additional timezone check, but that situation should be very rare. That's really the only additional way to check.
Flags: needinfo?(mozilla)
Assignee | ||
Comment 5•7 years ago
|
||
Got it. Thanks, Mike.
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8892309 [details] Bug 1385196 - [Form Autofill] Enable the feature based on locale and geoip. https://reviewboard.mozilla.org/r/163262/#review171012 ::: browser/extensions/formautofill/FormAutofillUtils.jsm:17 (Diff revision 1) > get AUTOFILL_FIELDS_THRESHOLD() { return 3; }, > > + get IS_AUTOFILL_AVAILABLE() { > + if (this._isFeatureAvailable === undefined) { Nit: Define the the `_isFeatureAvailable` property and initialize it to `null` instead (and change the comparision to check for `null`) as it's more clear where the variable is from. Add a JSDoc comment on it explaining it's to cache whether the feature is available. ::: browser/extensions/formautofill/FormAutofillUtils.jsm:19 (Diff revision 1) > + get IS_AUTOFILL_AVAILABLE() { > + if (this._isFeatureAvailable === undefined) { > + let available = Services.prefs.getCharPref("extensions.formautofill.available"); > + if (available == "on") { IMO this would be a bit cleaner with defineLazyGetter as you wouldn't have to manually assign to a private variable and could just return when you have an answer and it will be a bit more performant with no branching. It would also reduce nesting. ::: browser/extensions/formautofill/FormAutofillUtils.jsm:24 (Diff revision 1) > + } else if (available == "detect") { > + let locale = Services.locale.getRequestedLocale(); > + let region = Services.prefs.getCharPref("browser.search.region"); > + this._isFeatureAvailable = (locale == "en-US" && region == "US"); Can you add some basic unit tests for this API? I think you should be able to set prefs and use Cu.unload+Cu.import to have the cached value recomputed but I've never tried that. ::: browser/extensions/formautofill/FormAutofillUtils.jsm:26 (Diff revision 1) > + let available = Services.prefs.getCharPref("extensions.formautofill.available"); > + if (available == "on") { > + this._isFeatureAvailable = true; > + } else if (available == "detect") { > + let locale = Services.locale.getRequestedLocale(); > + let region = Services.prefs.getCharPref("browser.search.region"); This will throw an exception in a new profile since there is no default defined for the pref. You should add a 2nd argument to provide a default of "" or something not "en-US" to avoid this. ::: browser/extensions/formautofill/bootstrap.js:15 (Diff revision 1) > const STYLESHEET_URI = "chrome://formautofill/content/formautofill.css"; > const CACHED_STYLESHEETS = new WeakMap(); > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://formautofill/FormAutofillUtils.jsm"); Nit: I think you should use defineLazyModuleGetter to delay this from parse time to `startup` time (even if it's a minimal difference). ::: browser/extensions/formautofill/bootstrap.js:44 (Diff revision 1) > function startup() { > - if (Services.prefs.getStringPref("extensions.formautofill.available") != "on") { > + if (!FormAutofillUtils.IS_AUTOFILL_AVAILABLE) { > return; > } > This will need to be rebased on bug 1387611, sorry.
Attachment #8892309 -
Flags: review?(MattN+bmo)
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Patch has been updated according to our offline discussion. It now checks status in bootstrap directly so we can avoid loading another JSM at the very beginning. I haven't added unit tests because I'm not sure if it's feasible to test "startup()" function directly.
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8892309 [details] Bug 1385196 - [Form Autofill] Enable the feature based on locale and geoip. https://reviewboard.mozilla.org/r/163262/#review172530 ::: browser/extensions/formautofill/bootstrap.js:52 (Diff revision 2) > + if (prefAvailable == "on") { > + featureAvailable = true; > + } else if (prefAvailable == "detect") { > + let locale = Services.locale.getRequestedLocale(); > + let region = Services.prefs.getCharPref("browser.search.region", ""); > + featureAvailable = (locale == "en-US" && region == "US"); Nit: the braces aren't necessary here (unless eslint prefers otherwise)
Attachment #8892309 -
Flags: review?(MattN+bmo) → review+
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8892309 [details] Bug 1385196 - [Form Autofill] Enable the feature based on locale and geoip. https://reviewboard.mozilla.org/r/163262/#review171012 > Can you add some basic unit tests for this API? I think you should be able to set prefs and use Cu.unload+Cu.import to have the cached value recomputed but I've never tried that. I added a test for you in https://hg.mozilla.org/try/diff/b83368ecec1e/browser/extensions/formautofill/test/unit/test_isAvailable.js and I'll land it when try is green.
Assignee | ||
Comment 11•7 years ago
|
||
Thanks for your help, Matt.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8896106 [details] Bug 1385196 - [Form Autofill] Enable the feature based on locale and geoip. https://reviewboard.mozilla.org/r/167398/#review172600
Attachment #8896106 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8892309 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8896107 [details] Bug 1385196 - [Form Autofill] Test: Enable the feature based on locale and geoip. https://reviewboard.mozilla.org/r/167400/#review172610 Looks good!
Attachment #8896107 -
Flags: review?(lchang) → review+
Comment 17•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 45934e2c1cb5 -d 77865d6f19c1: rebasing 412899:45934e2c1cb5 "Bug 1385196 - [Form Autofill] Enable the feature based on locale and geoip. r=MattN" merging browser/extensions/formautofill/bootstrap.js warning: conflicts while merging browser/extensions/formautofill/bootstrap.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/e0975a1eefb2 [Form Autofill] Enable the feature based on locale and geoip. r=MattN https://hg.mozilla.org/integration/autoland/rev/5f8a0a826ad1 [Form Autofill] Test: Enable the feature based on locale and geoip. r=lchang
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0975a1eefb2 https://hg.mozilla.org/mozilla-central/rev/5f8a0a826ad1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/03cb4db8182b https://hg.mozilla.org/releases/mozilla-beta/rev/df42573c68fd
status-firefox56:
--- → fixed
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•