439716 - Form Manager should be a JavaScript-based component

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Justin Dolske [:Dolske]]

Now that the login manager is JS based (bug 374723), and I've been poking around more in the form manager code, I'm thinking that it would be nice to convert that pile of C++ code to JS, add tests, etc.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: WIP (hacky, broken, incomplete)

I got bored one weekend and whipped this up to get a feel for the scope of the problem. It's mostly just a straight "write this line of C++ in JS" conversion, is extremely rough, and not at all functional.

Backing it up here on Bugzilla because I keep thinking I accidentally deleted it. :)

Comment 2

[Justin Dolske [:Dolske]]

Also filed bug 469443 on converting the Form Storage pieces over to JS, leaving this bug to deal with just nsFormFillController.cpp.

Comment 3

[Justin Dolske [:Dolske]]

Mutating again to cover porting the storage and API parts; since 469443 actually only porting the autocomplete-related bits, and I don't expect that nsFormFillController will be ported in the foreseeable future.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

zpao and I hacked away at this yesterday. Not done yet, but this passes satchel's xpcshell tests.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Now mochitests pass.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Finished patch. Zpao and I went through this and reviewed the parts written by the other, but to avoid any sketchiness with this approach I'd like to have gavin make a final r/rs.

Comment 7

[Justin Dolske [:Dolske]]

(Also note that bug 557620 is racing with this patch, for a small change to nsStorageFormHistory.cpp Whoever lands last will need to include that change in nsFormHistory.js)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 442547 [details] [diff] [review]
Patch v.3

>diff --git a/toolkit/components/satchel/src/nsFormFillController.cpp b/toolkit/components/satchel/src/nsFormFillController.cpp

>-    NS_FORMHISTORY_CID, 

Should remove this from nsToolkitCompsCID.h as well.

>diff --git a/toolkit/components/satchel/src/nsFormAutoComplete.js b/toolkit/components/satchel/src/nsFormAutoComplete.js

>-    _debug              : false, // mirrors browser.formfill.debug
>+    _debug              : true, // mirrors browser.formfill.debug

Why this change? I noticed you defaulted debug to true in the other one as well, despite the default pref value being false - is there a reason for that?

>diff --git a/toolkit/components/satchel/src/nsFormHistory.js b/toolkit/components/satchel/src/nsFormHistory.js

>+FormHistory.prototype = {

>+    QueryInterface   : XPCOMUtils.generateQI([Ci.nsIFormHistory2, Ci.nsIFormSubmitObserver]),

Hmm, I think technically you need to be an nsIObserver too (since you're being passed to addObserver for earlyformsubmit, despite the notifiers only using nsIFormSubmitObserver)... I wonder whether this warns as-is?

>+    init : function() {

>+        this.prefBranch.addObserver("", function() { self.updatePrefs() }, false);

Probably wouldn't hurt to make this a weak ref, given that you hold on to the branch?

>+    removeEntriesByTimeframe : function (beginTime, endTime) {

>+        try {
>+            stmt = this.dbCreateStatement(query, params);
>+            stmt.executeStep();
>+        } catch (e) {
>+            this.log("removeEntriesByTimeframe failed: " + e);
>+            throw e;
>+        } finally {
>+            stmt.reset();
>+        }
>+
>+        this.sendIntNotification("removeEntriesByTimeframe", beginTime, endTime);

Why not sendNotification inside the try block like the other methods, so that you don't notify if it fails?

>+    notify : function(form, domWin, actionURI, cancelSubmit) {

>+            let docURI = Services.io.newURI(form.ownerDocument.documentURI, null, null);

form.ownerDocument.documentURIObject

>+                if (input.type.toLowerCase() != "text")
>+                    continue;

Needs to be nsIDOMNSHTMLInputElement::mozIsTextField now, per bug 557620

>+    countAllEntries : function () {

I noticed this method doesn't really have any tests, might not hurt to add some (though I guess it isn't directly exposed via any API).

(The old version of it with COUNT(*) is used for tests, should they be updated too? I must admit I don't know the difference between COUNT(*) and COUNT(1).)

>+    dbInit : function () {

>+        // Note: Firefox 3 didn't set a schema value, so it started from 0.
>+        // So we can't depend on a simple version == 0 check
>+        if (version == 0 && !this.dbConnection.tableExists("moz_formhistory"))
>+            this.dbCreate();
>+        else if (version != DB_VERSION)
>+            this.dbMigrate(version);

Is there no benefit to putting DB creation in a transaction? Old code did.

>+    dbCreate: function () {
>+        this.log("Creating Database");
>+        this.dbCreateTables();
>+        this.dbCreateIndices();

I guess this is a common pattern, but it seems like it would be clearer to just have these two functions inlined.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #8)
> >-    _debug              : false, // mirrors browser.formfill.debug
> >+    _debug              : true, // mirrors browser.formfill.debug
> 
> Why this change? I noticed you defaulted debug to true in the other one as
> well, despite the default pref value being false - is there a reason for that?

Only a very weak reason, zpao noticed that it's easier to force logging on this way, since you can just comment out the getPref call. (vs. the 2 lines that check _debug when logging :).

Setting defaults in the code is a bit silly to begin with, since they'll just end up out-of-sync with the real prefs. Bug 563725 will save us! (I hope.)

> >+    QueryInterface   : XPCOMUtils.generateQI([Ci.nsIFormHistory2, Ci.nsIFormSubmitObserver]),
> 
> Hmm, I think technically you need to be an nsIObserver too (since you're being
> passed to addObserver for earlyformsubmit, despite the notifiers only using
> nsIFormSubmitObserver)... I wonder whether this warns as-is?

Huh, that's interesting. I'll go ahead and add it. And maybe file a bug to just get rid of nsIFormSubmitObserver -- seems silly to have a special interface when a regular observer ought to work just as well.

> >+        this.prefBranch.addObserver("", function() { self.updatePrefs() }, false);
> 
> Probably wouldn't hurt to make this a weak ref, given that you hold on to the
> branch?

Hrm, well, that means I couldn't use a plain function here, as the observer would need to implement nsISupportsWeakReference...

Though I guess if we're going to have an unused observe() from adding nsIObserver, I might as well make use of it!


> >+                if (input.type.toLowerCase() != "text")
> >+                    continue;
> 
> Needs to be nsIDOMNSHTMLInputElement::mozIsTextField now, per bug 557620

Updated.

> 
> >+    countAllEntries : function () {
> 
> I noticed this method doesn't really have any tests, might not hurt to add some
> (though I guess it isn't directly exposed via any API).

Yeah, hasEntries() already exists and uses it.

> (The old version of it with COUNT(*) is used for tests, should they be updated
> too? I must admit I don't know the difference between COUNT(*) and COUNT(1).)

AIUI COUNT(*) was supposed to be less efficient than COUNT(1), but now I see there can be subtle differences between the two (though I'm not sure they matter here). Will look into this.

> Is there no benefit to putting DB creation in a transaction? Old code did.

Hmm, not sure. I'll benchmark, but I suspect the difference is minuscule because the DB is empty.

> >+    dbCreate: function () {
> >+        this.log("Creating Database");
> >+        this.dbCreateTables();
> >+        this.dbCreateIndices();
> 
> I guess this is a common pattern, but it seems like it would be clearer to just
> have these two functions inlined.

zpao: Any comment on this? Are you still reading this comment? :) I think you had a reason for wanting to keep these separate?

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Updated with gavin's review comments.

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #9)
> > (The old version of it with COUNT(*) is used for tests, should they be updated
> > too? I must admit I don't know the difference between COUNT(*) and COUNT(1).)
> 
> AIUI COUNT(*) was supposed to be less efficient than COUNT(1), but now I see
> there can be subtle differences between the two (though I'm not sure they
> matter here). Will look into this.

I seem to recall there being a reason I did this for PW manager. I thought this was optimized in some way because sqlite wouldn't have to consider columns at all... That said, for my test case just now using signons.sqlite, COUNT(1) had 18 VM operations vs 17 with COUNT(*) - no idea if that correlates to performance.

The internet tells me there's no real difference and that both should be optimized anyway. I'll ask Shawn tomorrow.

> > Is there no benefit to putting DB creation in a transaction? Old code did.
> 
> Hmm, not sure. I'll benchmark, but I suspect the difference is minuscule
> because the DB is empty.

I think there's really only a benefit if you do a transaction around many statements. AIUI (and again, something to ask Shawn), sqlite actually puts a transaction around each statement. We only have a single table so no gain. Even a couple more tables & indexes, I'd bet the gain would be negligible.

> > >+    dbCreate: function () {
> > >+        this.log("Creating Database");
> > >+        this.dbCreateTables();
> > >+        this.dbCreateIndices();
> > 
> > I guess this is a common pattern, but it seems like it would be clearer to just
> > have these two functions inlined.
> 
> zpao: Any comment on this? Are you still reading this comment? :) I think you
> had a reason for wanting to keep these separate?

I think we had talked about it, but we already took out one step (dbCreateSchema). The two functions are short enough that inlining makes sense here. If either were particularly complicated I might argue against it, but let's do it.

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #9)

> > Is there no benefit to putting DB creation in a transaction? Old code did.
> 
> Hmm, not sure. I'll benchmark, but I suspect the difference is minuscule
> because the DB is empty.

I benchmarked the time to do 1000 DB creation loops, with and without a transaction, and averaged 5 runs of each. Without a transaction, creating the DB took ~8.5ms. With a transaction, it was 4.2ms. Doesn't seem worth adding the code for a 1-time 4ms savings.

Comment 13

[Justin Dolske [:Dolske]]

Oh, and the difference between COUNT(*) and COUNT(1) doesn't seem to matter for us. The intarwebs say there can be a slight difference in that COUNT(*) does not include nulls in the count, whereas COUNT(1) does... But either this doesn't apply to sqlite, or it's confusing COUNT(1) with COUNT(aColumnName).

sqlite> .dump
BEGIN TRANSACTION;
CREATE TABLE foo (blah TEXT);
INSERT INTO "foo" VALUES('one');
INSERT INTO "foo" VALUES('two');
INSERT INTO "foo" VALUES(NULL);
COMMIT;
sqlite> SELECT COUNT(1) FROM foo;
3
sqlite> SELECT COUNT(*) FROM foo;
3
sqlite> SELECT COUNT(blah) FROM foo;
2

I suspect it's web confusion over COUNT(aColumnName), but in any case our DB's tend to have a non-null ID PRIMARY KEY, and form history in particular has a NOT NULL column constraint on the name/value, so this potential difference is irrelevant.

COUNT(1) is supposed to have the potentially-better performance of the two, so I'll stick with that.

And that should address all the issues, so I'm going to land this now.

Comment 14

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/cabbe056fd9c

Comment 15

[(no longer active)]

Note to self: do not comment on people's bugs after 1:30AM. :-)

Comment 16

[Justin Dolske [:Dolske]]

grumble stupid package manifest grumble...

Guess this is tradition now (see bug 374723 comment 29). :P

Pushed http://hg.mozilla.org/mozilla-central/rev/21ca905f5b3e

and for comm-central:

http://hg.mozilla.org/comm-central/rev/7249cc641f4b

Comment 17

[[e-mail]]

Comment on attachment 446432 [details] [diff] [review]
Patch v.4

>+    notify : function(form, domWin, actionURI, cancelSubmit) {
I'm not sure how you propose to squeeze this through observe() ;-)

>+        if (form.hasAttribute("autocomplete") &&
>+            form.getAttribute("autocomplete").toLowerCase() == "off")
That two-stage check is annoying. If only there was a boolean property ;-)

>+        } catch (e) {
>+            // Empty
>+        } finally {
>+            // Save whatever we've added so far.
>+            this.dbConnection.commitTransaction();
>+        }
Not quite sure what the utility of combining catch-all with finally is.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> (From update of attachment 446432 [details] [diff] [review])
> >+    notify : function(form, domWin, actionURI, cancelSubmit) {
> I'm not sure how you propose to squeeze this through observe() ;-)

I don't understand this comment. Can you elaborate?