Closed Bug 439716 Opened 12 years ago Closed 10 years ago

Form Manager should be a JavaScript-based component

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached file WIP (hacky, broken, incomplete) (obsolete) —
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. :)
Product: Firefox → Toolkit
Also filed bug 469443 on converting the Form Storage pieces over to JS, leaving this bug to deal with just nsFormFillController.cpp.
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.
Attached patch Patch v.1 (obsolete) — Splinter Review
zpao and I hacked away at this yesterday. Not done yet, but this passes satchel's xpcshell tests.
Assignee: nobody → dolske
Attachment #325445 - Attachment is obsolete: true
Attached patch Patch v.2 (obsolete) — Splinter Review
Now mochitests pass.
Attachment #441885 - Attachment is obsolete: true
Attached patch Patch v.3 (obsolete) — Splinter Review
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.
Attachment #442221 - Attachment is obsolete: true
Attachment #442547 - Flags: review+
Attachment #442547 - Flags: review?(paul)
Attachment #442547 - Flags: review?(gavin.sharp)
(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 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.
Attachment #442547 - Flags: review?(gavin.sharp) → review+
Blocks: 552828
(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?
Attached patch Patch v.4Splinter Review
Updated with gavin's review comments.
Attachment #442547 - Attachment is obsolete: true
(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.
(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.
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.
Pushed http://hg.mozilla.org/mozilla-central/rev/cabbe056fd9c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Keywords: dev-doc-needed
Note to self: do not comment on people's bugs after 1:30AM. :-)
Keywords: dev-doc-needed
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 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.
Depends on: 568256
Blocks: 568587
No longer blocks: 568587
(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?
You need to log in before you can comment on or make changes to this bug.