Closed Bug 1330567 Opened 7 years ago Closed 7 years ago

Fallback to form history if form autofill pref is disabled

Categories

(Toolkit :: Form Manager, defect)

Product:
Toolkit
Component:
Form Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:M1])

Attachments

(2 files)

Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1330567 - Part 2: Add xpcshell test for the pref fallback,
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Based on bug 1325724 comment 1, there should be another fallback that check the "browser.formautofill.enabled" pref is set or not, and add an observer to reg/unreg the factory when flag is changed.
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

Hi Matt,
It's a quick patch about adding the checking the pref at parent and content. I also remove the event and message listener while pref off, although I think they should be harmless.
Attachment #8826541 - Flags: feedback?(MattN+bmo)
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

https://reviewboard.mozilla.org/r/104498/#review105138

Sorry to jump in.

::: browser/extensions/formautofill/content/FormAutofillContent.js:296
(Diff revision 1)
> -    ProfileAutocomplete.ensureRegistered();
> +        ProfileAutocomplete.ensureRegistered();
> -
> -    addEventListener("DOMContentLoaded", this);
> +        addEventListener("DOMContentLoaded", this);
> +      } else {
> +        ProfileAutocomplete.ensureUnregistered();
> +        removeEventListener("DOMContentLoaded", this);

We should still need to handle "DOMContentLoaded" when pref is off as it won't be triggered again.

::: browser/extensions/formautofill/content/FormAutofillContent.js:300
(Diff revision 1)
> +        ProfileAutocomplete.ensureUnregistered();
> +        removeEventListener("DOMContentLoaded", this);
> +      }
> +    }
> +
> +    Services.prefs.addObserver(ENABLED_PREF, toggleListener, false);

Just a thought. Since we'll also need to toggle ProfileAutocomplete's registration by listening to "ProfileChanged" message from Parent (or something like that), is it possible to determine whether to register or unregister in parent and send one single message to content to avoid duplicate conditions?
See Also: → 1325724
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

https://reviewboard.mozilla.org/r/104498/#review106202

This should have tests.

::: browser/extensions/formautofill/FormAutofillParent.jsm:68
(Diff revision 1)
> +      } else {
> +        mm.removeMessageListener("FormAutofill:PopulateFieldValues", this);
> +      }
> +    }
> +
> +    Services.prefs.addObserver(ENABLED_PREF, toggleListener, false);

It's usually preferred to pass `this` instead of a method so that we don't have a nested observer function. You then move the logic to an `observe` method with the following signature:
https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/xpcom/ds/nsIObserver.idl#33

You will need to add the following property to the object:
`QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver])`

Eventually we would then change the the addObserver to observe browser.formautofill.*

::: browser/extensions/formautofill/FormAutofillParent.jsm:80
(Diff revision 1)
>     *
>     * @param   {string} message.name The name of the message.
>     * @param   {object} message.data The data of the message.
>     * @param   {nsIFrameMessageManager} message.target Caller's message manager.
>     */
>    receiveMessage: function({name, data, target}) {

Note that due to my slow reviews you will need to rebase on eslint changes. Sorry :(

::: browser/extensions/formautofill/content/FormAutofillContent.js:300
(Diff revision 1)
> +        ProfileAutocomplete.ensureUnregistered();
> +        removeEventListener("DOMContentLoaded", this);
> +      }
> +    }
> +
> +    Services.prefs.addObserver(ENABLED_PREF, toggleListener, false);

I would avoid the nesting of the function inside the method here too by using `this`. observer => `this.toggleListener()`
Attachment #8826541 - Flags: review+
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

https://reviewboard.mozilla.org/r/104498/#review105138

> We should still need to handle "DOMContentLoaded" when pref is off as it won't be triggered again.

Hmm… I think at least for now it would be okay to require a reload after changing the pref.

> Just a thought. Since we'll also need to toggle ProfileAutocomplete's registration by listening to "ProfileChanged" message from Parent (or something like that), is it possible to determine whether to register or unregister in parent and send one single message to content to avoid duplicate conditions?

Good idea. Using a message from the parent makes sense to me.
Attachment #8826541 - Flags: review+
Attachment #8826541 - Flags: feedback?(MattN+bmo)
Attachment #8826541 - Flags: feedback+
Attachment #8826541 - Flags: review?(MattN+bmo)
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

https://reviewboard.mozilla.org/r/104498/#review111888

::: browser/extensions/formautofill/FormAutofillParent.jsm:53
(Diff revision 3)
> +    let mm = Cc["@mozilla.org/globalmessagemanager;1"]
> +           .getService(Ci.nsIMessageListenerManager);

I think this is available as Services.…

::: browser/extensions/formautofill/FormAutofillParent.jsm:74
(Diff revision 3)
> +        target.messageManager.sendAsyncMessage("FormAutofill:enabledStatus",
> +                                               this.isEnabledStatus());

I think it would be good to cache the enabled state in whatever object has these observers for prefs and profile storage so we can always just respond with that cached state instead of doing work to check the prefs/storage to reply.

This would also make it easy to unit test because you can set a pref and check the current state. You can also test that the broadcast works.

::: browser/extensions/formautofill/FormAutofillParent.jsm:109
(Diff revision 3)
> +   * Add/remove message listener and broadcast the status to frames while the
> +   * form autofill status changed.
> +   *
> +   * @param   {boolean} enabled The status of the form autofill.
> +   */
> +  statusHandler: function(enabled) {

Nit: I think `onStatusChanged` may be a clearer name.

I would also just fold the StatusObserver into FormAutofillParent.

::: browser/extensions/formautofill/content/FormAutofillContent.js:352
(Diff revision 3)
> +        ProfileAutocomplete.ensureRegistered();
> +      } else {
> +        ProfileAutocomplete.ensureUnregistered();
> +      }
> +    });
> +    sendAsyncMessage("FormAutofill:getEnabledStatus");

I'm worried about this being async because a field may become focused before the response is received and then that field won't get profile autocomplete. IIRC there is an API to send initialization data to new frame scripts.
Attachment #8826541 - Flags: review?(MattN+bmo)
Comment on attachment 8835439 [details]
Bug 1330567 - Part 2: Add xpcshell test for the pref fallback,

https://reviewboard.mozilla.org/r/111180/#review112714

::: browser/extensions/formautofill/test/unit/test_enabledStatus.js:5
(Diff revision 1)
> +/*
> + * Test for status handling in Form Autofill Parent.
> + */
> +
> +/* global FormAutofillParent, sinon */

I think you can move this definition of the sinon global to head.js and the mozilla/import-headjs-globals.js rule will find it in the test scope.

I also think the FormAutofillParent global should automatically be found from the import.

::: browser/extensions/formautofill/test/unit/test_enabledStatus.js:52
(Diff revision 1)
> +  Services.prefs.setBoolPref("browser.formautofill.enabled", false);
> +  do_check_eq(formAutofillParent._getStatus(), false);
> +
> +  Services.prefs.clearUserPref("browser.formautofill.enabled");

If code above this throws then the pref will have the wrong value for subsequent tests.
If you instead use SpecialPowers.pushPrefEnv(…) then it will cleanup for you at the end (in mochitest). In xpcshell you can run `SpecialPowers.flushPrefEnv()` in head.js so it cleans up for all xpcshell tests.
Attachment #8835439 - Flags: review?(MattN+bmo) → review+
Blocks: 1338485
Comment on attachment 8826541 [details]
Bug 1330567 - Part 1: Fallback to form history if form autofill pref is disabled,

https://reviewboard.mozilla.org/r/104498/#review113072

::: browser/extensions/formautofill/content/FormAutofillContent.js:352
(Diff revision 4)
> +        ProfileAutocomplete.ensureRegistered();
> +      } else {
> +        ProfileAutocomplete.ensureUnregistered();
> +      }
> +    });
> +    sendAsyncMessage("FormAutofill:getEnabledStatus");

We can change this to use `Services.cpmm.initialProcessData` after the planned refactoring of process vs. frame lifetime.
Attachment #8826541 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72ecf19a8ec0
Part 1: Fallback to form history if form autofill pref is disabled, r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d52a2b47ce6
Part 2: Add xpcshell test for the autofill pref fallback, r=MattN
Comment on attachment 8835439 [details]
Bug 1330567 - Part 2: Add xpcshell test for the pref fallback,

https://reviewboard.mozilla.org/r/111180/#review112714

> If code above this throws then the pref will have the wrong value for subsequent tests.
> If you instead use SpecialPowers.pushPrefEnv(…) then it will cleanup for you at the end (in mochitest). In xpcshell you can run `SpecialPowers.flushPrefEnv()` in head.js so it cleans up for all xpcshell tests.

I guess SpecialPowers isn't generally available in xpcshell but some indexedDB tests copied some stuff so do_register_cleanup is fine.
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
https://hg.mozilla.org/mozilla-central/rev/72ecf19a8ec0
https://hg.mozilla.org/mozilla-central/rev/8d52a2b47ce6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1339007
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: