Closed
Bug 469443
Opened 16 years ago
Closed 15 years ago
Form Manager Storage should be a JavaScript-based component
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: Dolske, Assigned: Dolske)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Satchel's implementation of nsIFormHistory2 should be a JS component instead of native code. [Bug 439716 covers converting the Form Fill Controller]
Assignee | ||
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 4•15 years ago
|
||
I think this is ready for a review pass now.
Attachment #376291 -
Attachment is obsolete: true
Attachment #381157 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 5•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #381157 -
Flags: review?(gavin.sharp) → review+
Comment 6•15 years ago
|
||
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).
Assignee | ||
Comment 7•15 years ago
|
||
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
Assignee | ||
Comment 8•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 9•15 years ago
|
||
Also pushed http://hg.mozilla.org/mozilla-central/rev/e3cde1ff1ef6
(Noticed a small typo, I wasn't throwing the exception. :/)
You need to log in
before you can comment on or make changes to this bug.
Description
•