1421551 - Expose the availability of Form Autofill through a member method

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Luke Chang [:lchang] (inactive)]

Since the pref, "extensions.formautofill.available", can't tell if Form Autofill system add-on is currently available, we may need a mechanism to let Preferences (or other modules in the future) know Form Autofill's current availability.

As bug 1407568 comment 74 mentioned, we prefer to implement a member method within Form Autofill modules.

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

Review commit: https://reviewboard.mozilla.org/r/203838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203838/

Comment 2

[Luke Chang [:lchang] (inactive)]

Just realized that FormAutofillParent is a prototype and we are not exposing its instance anywhere. Clear review flag as I need to find another way to implement this.

Comment 3

[Luke Chang [:lchang] (inactive)]

Currently I can think of two approaches:

1. Expose it via FormAutofillUtils instead. The downside is that Utils can be loaded in the content process and the property will always be "false" if called from content. Besides, Utils is supposed to contain stateless helper functions only.

2. Make FormAutofillParent singleton by creating and exposing its instance inside its own JSM.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

(In reply to Luke Chang [:lchang] from comment #3)
> Currently I can think of two approaches:
> 
> 1. Expose it via FormAutofillUtils instead. The downside is that Utils can
> be loaded in the content process and the property will always be "false" if
> called from content. Besides, Utils is supposed to contain stateless helper
> functions only.
> 
> 2. Make FormAutofillParent singleton by creating and exposing its instance
> inside its own JSM.

I prefer the second option as well. If we would like to reserve FormAutofillParent in class form, it's a concept for the reference:


diff --git a/browser/extensions/formautofill/FormAutofillParent.jsm b/browser/extensions/formautofill/FormAutofillParent.jsm
index f112f139b067..25abf3988219 100644
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -27,7 +27,7 @@

 "use strict";

-this.EXPORTED_SYMBOLS = ["FormAutofillParent"];
+this.EXPORTED_SYMBOLS = ["FormAutofillParentSinglton", "FormAutofillParent"];

 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

@@ -54,6 +54,22 @@ const {
   CREDITCARDS_COLLECTION_NAME,
 } = FormAutofillUtils;

+let FormAutofillParentSinglton = (() => {
+  let _parent, _isInitalized = false;
+  return {
+    isInitialized() {
+      return _isInitalized;
+    },
+    init() {
+      if (!this._isInitalized) {
+        _parent = new FormAutofillParent();
+        this._parent.init();
+        _isInitalized = true;
+      }
+    },
+  };
+})();
+
 function FormAutofillParent() {
   // Lazily load the storage JSM to avoid disk I/O until absolutely needed.
   // Once storage is loaded we need to update saved field names and inform content processes.
diff --git a/browser/extensions/formautofill/bootstrap.js b/browser/extensions/formautofill/bootstrap.js
index 3a21191a5b0b..b670f6a413b6 100644
--- a/browser/extensions/formautofill/bootstrap.js
+++ b/browser/extensions/formautofill/bootstrap.js
@@ -110,8 +110,7 @@ function startup(data) {
   // Listen for the autocomplete popup message to lazily append our stylesheet related to the popup.
   Services.mm.addMessageListener("FormAutoComplete:MaybeOpenPopup", onMaybeOpenPopup);

-  let parent = new FormAutofillParent();
-  parent.init().catch(Cu.reportError);
+  FormAutofillParentSinglton.init().catch(Cu.report.Error);
   Services.ppmm.loadProcessScript("data:,new " + function() {
     Components.utils.import("resource://formautofill/FormAutofillContent.jsm");
   }, true);

Comment 5

[Luke Chang [:lchang] (inactive)]

Thanks, Sean. That would be useful. For now, however, it's probably not that complicated as I don't see any downside if we expose the instance directly. My patch is on the way.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8932777 [details]
Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203838/diff/1-2/

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8932777 [details]
Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

https://reviewboard.mozilla.org/r/203838/#review210038

I'm okay with this for now since we're not supporting updates to the extension at runtime and tests will continue to use new instances so they don't have to worry about cleaning up state.

Comment 8

[Evan Tseng [:evanxd]]

Comment on attachment 8932777 [details]
Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

https://reviewboard.mozilla.org/r/203838/#review210074

::: browser/extensions/formautofill/test/unit/test_activeStatus.js:7
(Diff revision 2)
>   * Test for status handling in Form Autofill Parent.
>   */
>  
>  "use strict";
>  
> -Cu.import("resource://formautofill/FormAutofillParent.jsm");
> +let {FormAutofillParent} = Cu.import("resource://formautofill/FormAutofillParent.jsm");

Hi Luke,

Just found out we did `let {MasterPassword} = Cu.import("resource://formautofill/MasterPassword.jsm", {});` to import object in the `test_autofillFormFields.js` file.

I think we could update this line to `let {FormAutofillParent} = Cu.import("resource://formautofill/FormAutofillParent.jsm", {});` to fix the linkt issue. What do you think? :)

[1]: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/unit/test_autofillFormFields.js#9

Comment 9

[Evan Tseng [:evanxd]]

Comment on attachment 8932777 [details]
Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

https://reviewboard.mozilla.org/r/203838/#review210074

> Hi Luke,
> 
> Just found out we did `let {MasterPassword} = Cu.import("resource://formautofill/MasterPassword.jsm", {});` to import object in the `test_autofillFormFields.js` file.
> 
> I think we could update this line to `let {FormAutofillParent} = Cu.import("resource://formautofill/FormAutofillParent.jsm", {});` to fix the linkt issue. What do you think? :)
> 
> [1]: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/unit/test_autofillFormFields.js#9

https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/unit/test_autofillFormFields.js#9

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8932777 [details]
Bug 1421551 - Make FormAutofillParent singleton and expose the initialized status.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203838/diff/2-3/

Comment 11

[Evan Tseng [:evanxd]]

The try[1] looks good. Let's land the patch. Thank you for the help, Luke.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6427e12ade5a

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f109cab61c9
Make FormAutofillParent singleton and expose the initialized status. r=MattN

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0f109cab61c9