469443 - Form Manager Storage should be a JavaScript-based component

Status: VERIFIED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Justin Dolske [:Dolske]]

Satchel's implementation of nsIFormHistory2 should be a JS component instead of native code. [Bug 439716 covers converting the Form Fill Controller]

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Work in progress, moves the autocomplete bits out of C++ and into JS, with a little bit of other cleanup. Works, but not extensively tested.

Comment 2

[Justin Dolske [:Dolske]]

It probably makes sense to mutate this bug into just doing the autocomplete parts (as the WIP patch does), and move the rest of form history storage in a second pass.

Comment 3

[Matthew N. [:MattN]]

When testing out this patch with the search bar, some search suggestions would show above the separator that divides the history from the suggestions.  This would only occur if I typed characters at a slow rate ie. 3 characters in 2 seconds.  This seems to be related to reusing the search results if there is a previous result set.  Possibly the suggestions are stored in that set with the history.  I couldn't reproduce the problem on other builds without this patch.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

I think this is ready for a review pass now.

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #3)
> When testing out this patch with the search bar, some search suggestions would
> show above the separator that divides the history from the suggestions.

This is bug 495584.

Comment 6

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

Comment on attachment 381157 [details] [diff] [review]
Patch v.2

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

>+    autoCompleteSearch : function (aInputName, aSearchString, aPreviousResult) {
>+        if (!this._enabled)
>+            return;

return null; here to avoid a strict warning (looks like the controller already deals with having a null result)

>+// nsIAutoCompleteResult implementation
>+function FormAutoCompleteResult (formHistory, entries, fieldName, fieldValue) {

why fieldValue instead of searchString?

>+FormAutoCompleteResult.prototype = {

>+    getCommentAt : function (index) {
>+    getStyleAt : function (index) {
>+    getImageAt : function (index) {

I think it probably makes sense to do index bounds checking for all of these just to match the previous behavior (can factor that out into a _checkIndex too I guess). Should probably also throw Components.Exception("", Cr.NS_ERROR_ILLEGAL_VALUE).

>+    removeValueAt : function (index, removeFromDB) {

>+        this.matchCount--;

worth making matchCount a getter for entries.length to avoid needing to adjust this manually?

>diff --git a/toolkit/components/satchel/test/test_form_autocomplete.html b/toolkit/components/satchel/test/test_form_autocomplete.html

>+    default:
>+        ok(false, "Unexpected invocataion of test #" + testNum);

This "invocataion" typo is already in the tree twice - fix it here and in those two other places as well? :)

Would be worth adding a test to ensure non-ASCII entries get restored/sorted properly via SQLITE (as discussed on IRC).

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Fixed nits, added some more tests (to also test some filtering as the user types).

Noticed that Patch v.2 was triggering the leak tests, added a xpcom-shutdown observer to flush the cached DB statements, that fixes the problem.

Comment 8

[Justin Dolske [:Dolske]]

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

Comment 9

[Justin Dolske [:Dolske]]

Also pushed http://hg.mozilla.org/mozilla-central/rev/e3cde1ff1ef6

(Noticed a small typo, I wasn't throwing the exception. :/)

Comment 10

[Tracy Walker [:tracy]]

verified per code rework