Closed Bug 469443 Opened 11 years ago Closed 10 years ago

Form Manager Storage should be a JavaScript-based component

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

Satchel's implementation of nsIFormHistory2 should be a JS component instead of native code. [Bug 439716 covers converting the Form Fill Controller]
Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
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.
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.
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.
Depends on: 495584
Attached patch Patch v.2 (obsolete) — Splinter Review
I think this is ready for a review pass now.
Attachment #376291 - Attachment is obsolete: true
Attachment #381157 - Flags: review?(gavin.sharp)
(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.
Attachment #381157 - Flags: review?(gavin.sharp) → review+
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).
Attached patch Patch v.3Splinter Review
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.
Attachment #381157 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Also pushed http://hg.mozilla.org/mozilla-central/rev/e3cde1ff1ef6

(Noticed a small typo, I wasn't throwing the exception. :/)
verified per code rework
Status: RESOLVED → VERIFIED
No longer blocks: 561116
You need to log in before you can comment on or make changes to this bug.