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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

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.
Depends on: 1385201
Assignee: nobody → lchang
Status: NEW → ASSIGNED
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.
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)
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)
Blocks: 1386959
Got it. Thanks, Mike.
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]
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 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 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.
Thanks for your help, Matt.
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+
Attachment #8892309 - Attachment is obsolete: true
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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/e0975a1eefb2
https://hg.mozilla.org/mozilla-central/rev/5f8a0a826ad1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.