Closed Bug 1421551 Opened 2 years ago Closed 2 years ago

Expose the availability of Form Autofill through a member method

Categories

(Toolkit :: Form Autofill, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [preference-spotlight])

Attachments

(1 file)

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.
Status: NEW → ASSIGNED
Attachment #8932777 - Flags: review?(selee)
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.
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.
(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);
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 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.
Attachment #8932777 - Flags: review?(MattN+bmo) → review+
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 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
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
Keywords: checkin-needed
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f109cab61c9
Make FormAutofillParent singleton and expose the initialized status. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f109cab61c9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Whiteboard: [preference-spotlight]
You need to log in before you can comment on or make changes to this bug.