697377 - (asyncFormAC) Form Autocomplete should use asynchronous storage API

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Felix Fung (:felix)]

This bug is (in spirit) very similar to bug 566746. We want to use asynchronous queries to the underlying data during auto-completion. This is mostly independent of making an async form history because of the way frecency is done. We're exposing the DBConnection so we can directly modify autocomplete to do async queries.

Comment 1

[Felix Fung (:felix)]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory

Summary of Changes:
In nsIFormAutoComplete.idl
- Changed nsIFormAutoComplete to have an nsIFormAutoCompleteObserver to listen for results rather than return a value on autoCompleteSearch
- Added stopAutoCompleteSearch

In nsFormAutoComplete.js
- Replace nsIFormHistory2 with AsyncFormHistory
- _dbCreateStatement -> _dbCreateAsyncStatement
- Stopped storing dbStmts since form-history already memoizes them

In nsFormFillController.js
- Modified StartSearch to handle new call signature to FormAutocomplete
- Modified StopSearch to interrupt pending async requests
- Supports new interface to listen to autocomplete results

Comment 2

[Marco Bonardo [:mak]]

does form autocomplete support inline autocomplete?
If so you may likely have the same issues as the locationbar, where typing quickly may replace chars wrongly, if not nevermind.

Comment 3

[Felix Fung (:felix)]

Form history doesn't have inline autocomplete =)

Comment 4

[Felix Fung (:felix)]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory

- Takes care of bug 393231

Comment 5

[Felix Fung (:felix)]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory

Additional Changes:
- Update caller to match newer API

Comment 6

[Felix Fung (:felix)]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory

Additional Changes:
- Bugfixes

Comment 7

[Felix Fung (:felix)]

Attachment: Convert test_autocomplete.js to Use Asynchronous APIs

- Use add_test/run_next_test pattern
- Use FormHistory.jsm from bug 566746 in addition to above patch

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 582618 [details] [diff] [review]
Convert test_autocomplete.js to Use Asynchronous APIs

Review of attachment 582618 [details] [diff] [review]:
-----------------------------------------------------------------

pseudo-r+ with these fixes

::: toolkit/components/satchel/test/unit/test_autocomplete.js
@@ +43,1 @@
>  var testnum = 0;

testnum is now unused so delete it.

@@ +49,4 @@
>  const DEFAULT_EXPIRE_DAYS = 180;
>  
>  function countAllEntries() {
> +    let stmt = FormHistory.DBConnection.createStatement("SELECT COUNT(*) as numEntries FROM moz_formhistory");

Nit: mind capitalizing "as" while you're here?

@@ +147,5 @@
> +          });
> +      },
> +
> +      onFailure : function(aError) {
> +          do_throw("B");

The messages to throw should be more descriptive.  Perhaps something like "failure counting ('field1'|all) entries".

@@ +197,5 @@
> +add_test(function test5() {
> +    do_log_info("Begin tests with constant use dates and varying timesUsed");
> +    do_log_info("Check search result ordering with empty search term");
> +
> +    let timesUsedSamples = 20;

Nit: Hoist timesUsedSamples to the top-level so it doesn't need to be defined again below.

Comment 9

[Felix Fung (:felix)]

Attachment: Use Asynchronous FormHistory.jsm for test_form_autocomplete.html

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 583603 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_form_autocomplete.html

Review of attachment 583603 [details] [diff] [review]:
-----------------------------------------------------------------

pseudo-r+ with these fixes

::: toolkit/components/satchel/test/test_form_autocomplete.html
@@ +97,4 @@
>  
> +  FormHistory.updateFormHistory(
> +    [
> +      { op : "remove" }, 

Nit: there's a trailing space on the remove line

@@ +102,5 @@
> +      { op : "addOrModify", fieldname : "field1", value : "value2" },
> +      { op : "addOrModify", fieldname : "field1", value : "value3" },
> +      { op : "addOrModify", fieldname : "field1", value : "value4" },
> +      { op : "addOrModify", fieldname : "field2", value : "value1" },
> +      { op : "addOrModify", fieldname : "field2", value : "value1" },

Was field2/value1 added a second time intentionally? I don't see a test change to confirm that it's handled properly, although it may be covered already in this file or another.

@@ +106,5 @@
> +      { op : "addOrModify", fieldname : "field2", value : "value1" },
> +      { op : "addOrModify", fieldname : "field3", value : "a" },
> +      { op : "addOrModify", fieldname : "field3", value : "aa" },
> +      { op : "addOrModify", fieldname : "field3", value : "aaz" },
> +      { op : "addOrModify", fieldname : "field3", value : "aa\xe6" },

Nit: the comment probably didn't need to go but it's a minor detail

@@ +124,5 @@
> +      { op : "addOrModify", fieldname : "field9", value : "value" }
> +    ], {
> +      onSuccess : aCallback,
> +      onFailure : function(aError) {
> +        do_throw("C");

The thrown message should be more descriptive such as including aError

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 583603 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_form_autocomplete.html

Moving more stale reviews to zpao (who may want to load-balance with mattn too ;).

Might be better in future patches to just lump all the test changes together, but paul/matt can make that call.

Comment 12

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

Comment on attachment 583603 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_form_autocomplete.html

Review of attachment 583603 [details] [diff] [review]:
-----------------------------------------------------------------

Everything MattN said :)

Comment 13

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

Comment on attachment 582618 [details] [diff] [review]
Convert test_autocomplete.js to Use Asynchronous APIs

Review of attachment 582618 [details] [diff] [review]:
-----------------------------------------------------------------

Again, what MattN said and a couple other small things.

::: toolkit/components/satchel/test/unit/test_autocomplete.js
@@ +36,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You're not using XPCOMUtils (anymore?). As such, defining Cu won't save you anything anymore, but leaving it in is fine.

@@ +243,3 @@
>  
> +add_test(function test7() {
> +    do_log_info("Check that \"senior citizen\" entries get a bonus (browser.formfill.agedBonus)");

Nit: Just use single quotes around senior citizen - escaping isn't necessary (here and other log messages)

Comment 14

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

Comment on attachment 582617 [details] [diff] [review]
Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 582617 [details] [diff] [review]:
-----------------------------------------------------------------

In general it looks good, but I'd like Matt to give it a closer look before I come back to it.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +154,2 @@
>       *
>       * Returns: an nsIAutoCompleteResult

It no longer returns an nsIAutoCompleteResult

@@ +210,5 @@
> +            result = new FormAutoCompleteResult(FormHistory, [], aInputName, aUntrimmedSearchString);
> +            this.getAutoCompleteValues(aInputName, searchString,
> +                function (aEntries) {
> +                  if (aField && aField.maxLength > -1) {
> +                    result.wrappedJSObject.entries = aEntries.filter(function (el) { return el.text.length <= aField.maxLength; });

Nit: this line is way long now.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +586,5 @@
> +      formAutoComplete->AutoCompleteSearch(aSearchParam,
> +                                           aSearchString,
> +                                           mFocusedInput,
> +                                           aPreviousResult,
> +                                           static_cast<nsIFormAutoCompleteObserver *>(this));

I'm not super familiar with our C++, but this cast feels wrong. I _think_ you want to QI to the right thing.

Matt: do you know?

@@ +669,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = inputListAutoComplete->AutoCompleteSearch(resultParam,
> +                                                 searchString,
> +                                                 mFocusedInput,
> +                                                 getter_AddRefs(result));

Do we want to NS_ENSURE_SUCCESS here?

@@ +1137,5 @@
>    kSatchelCIDs,
>    kSatchelContracts
>  };
>  
>  NSMODULE_DEFN(satchel) = &kSatchelModule;

The whitespace change can probably be left out, but I won't care if you sneak it in.

::: toolkit/components/satchel/nsIFormAutoComplete.idl
@@ +49,5 @@
> +    void autoCompleteSearch(in AString aInputName,
> +                            in AString aSearchString,
> +                            in nsIDOMHTMLInputElement aField,
> +                            in nsIAutoCompleteResult aPreviousResult,
> +                            in nsIFormAutoCompleteObserver aListener);

Why not re-use nsIAutoCompleteObserver? It is a bit more complex, but a wheel already exists.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 582617 [details] [diff] [review]
Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 582617 [details] [diff] [review]:
-----------------------------------------------------------------

I worry about race conditions stopping search in both nsFormAutoComplete and nsFormFillController and with the mLast* references but the autocomplete delay may wallpaper over that?

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +304,5 @@
> +            stmt.params["tokenBoundary" + i] =  "% " + escapedToken + "%";
> +            stmt.params["tokenContains" + i] = "%" + escapedToken + "%";
> +          }
> +        } else {
> +          // no addional params need to be substituted into the query when the

Nit: While you're touching this code do you mind fixing the typo on "additional".

@@ +311,5 @@
>  
> +        this.stopAutoCompleteSearch();
> +
> +        let self = this;
> +        self._pendingQuery =

Are we sure that cancelling the pending query is worthwhile (I don't know the implementation) and doesn't lead to reduced perceived performance.  It does make sense to stop pending searches that a user would never see but I also think showing a slightly stale result is better than showing nothing sometimes.

This is probably a non-issue (fast query times compared to the autocomplete delay) so it can be addressed in a follow-up if it's noticeable.

@@ +331,5 @@
> +
> +                },
> +
> +                handleError : function (aError) {
> +                    self._pendingQuery = null;

I'm not sure what types of errors we would get but can we keep the this.log(...) call that was in the catch block before.  It's only output when the debug pref is flipped so not a big overhead.

@@ +336,5 @@
> +                },
> +
> +                handleCompletion : function (aReason) {
> +                    if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
> +                      self._pendingQuery = null;

Shouldn't we also be finalizing the statement here?

@@ +350,3 @@
>          if (params) {
> +            for (let i in params) {
> +                stmt.params[i] = params[i];

You removed the property lookup perf optimization.  The only reason I'm mentioning it is because I remember being told to do the optimization for this loop. I just paying the review comment forward. :P  Unfortunately that review comment isn't in Bug 370117 so I don't know how big of a deal it is.  Some quick measurements on jsperf.com make it seem negligible.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +586,5 @@
> +      formAutoComplete->AutoCompleteSearch(aSearchParam,
> +                                           aSearchString,
> +                                           mFocusedInput,
> +                                           aPreviousResult,
> +                                           static_cast<nsIFormAutoCompleteObserver *>(this));

I'm not confident but I think this is fine since we control `this` and it will always succeed with static_cast unless we move the observer.  It seems like a common pattern in the tree https://mxr.mozilla.org/mozilla-central/search?string=static_cast.*(this)&regexp=1

::: toolkit/components/satchel/nsFormFillController.h
@@ +70,1 @@
>                               public nsIDOMEventListener,

Nit: move nsIFormAutoCompleteObserver below nsIDOMEventListener to be alphabetical and for consistency with the .cpp.

::: toolkit/components/satchel/nsIFormAutoComplete.idl
@@ +42,3 @@
>  interface nsIDOMHTMLInputElement;
>  
>  [scriptable, uuid(997c0c05-5d1d-47e5-9cbc-765c0b8ec699)]

This UUID needs to be changed.

@@ +58,5 @@
> +[scriptable, uuid(604419ab-55a0-4831-9eca-1b9e67cc4751)]
> +interface nsIFormAutoCompleteObserver : nsISupports
> +{
> +  /*
> +   * Called when a search is complete and the results are ready

Could you be explicit about the fact that it will be called for empty results?  The name is a bit misleading... I think "onSearchComplet(e/ion)" is more clear but if it's documented it's ok with me. It's also confusing because OnSearchResult is also a callback on nsIAutoCompleteObserver.

@@ +60,5 @@
> +{
> +  /*
> +   * Called when a search is complete and the results are ready
> +   *
> +   * @param search - The search object that processed this search

I don't see the "search" param. anymore.

Comment 16

[Matthew N. [:MattN]]

Hey Felix, do you think you'll get a chance to work on a new revision shortly? It's just the one patch left for this bug.

Comment 17

[Felix Fung (:felix)]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory

Addressed most comments.

> Why not re-use nsIAutoCompleteObserver? It is a bit more complex, but a
> wheel already exists.

It's not just that the interface is a bit more complex, the design would only weirdly fit. To implement nsIAutoCompleteObserver, you're supposed to pass back an nsIAutoCompleteSearch object, which implements StartSearch / StopSearch. Our options would be to pass back a filler object (lame) or implement it through nsFormAutoComplete.js. So now nsFormAutoComplete.js would be implementing both nsIAutoCompleteSearch as well as nsIFormAutoComplete who overlap kinda and StartSearch just wouldn't make sense. The REAL problem is that we have all these things that are named very similarly so it would seem that they should be compatible... but they are not.

With regards to the static_cast in comment 14. Looking at other code reviews, static_cast seems like the right thing to do. Afaik, it's because we're inheriting nsISupports from multiple places. If we still wanted to QueryInterface you would run into compile errors unless you performed a static_cast. You would get something like below, which just seems kinda needless.

+      nsCOMPtr<nsIFormAutoCompleteObserver> formAutoCompleteObserver =
+        do_QueryInterface(static_cast<nsIFormAutoCompleteObserver *>(this));
+      formAutoComplete->AutoCompleteSearch(aSearchParam,
+                                           aSearchString,
+                                           mFocusedInput,
+                                           aPreviousResult,
+                                           formAutoCompleteObserver);

Comment 18

[Felix Fung (:felix)]

> Are we sure that cancelling the pending query is worthwhile (I don't know
> the implementation) and doesn't lead to reduced perceived performance.  It
> does make sense to stop pending searches that a user would never see but I
> also think showing a slightly stale result is better than showing nothing
> sometimes.

If we don't cancel the pending query, I'm not sure that we can be certain that onSearchCompletion will be called in order (i.e. what if a stale query overwrites a newer query?)

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 604350 [details] [diff] [review]
Update nsFormAutoComplete to use AsyncFormHistory

Everything looks good except for the one review comment not addressed:

(Quoting Matthew N. [:MattN] from comment #15)
> ::: toolkit/components/satchel/nsFormAutoComplete.js
> @@ +336,5 @@
> > +                },
> > +
> > +                handleCompletion : function (aReason) {
> > +                    if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
> > +                      self._pendingQuery = null;
> 
> Shouldn't we also be finalizing the statement here?

Did you get a chance to investigate this review comment.  If you have questions about finalizing the statement, you could ask Mak.

r+ once this is addressed.

Comment 20

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

Comment on attachment 604350 [details] [diff] [review]
Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 604350 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for reasoning out why not nsIAutoCompleteObserver.

I didn't have any more concerns, so just what Matt said for his review.

Comment 21

[Neil Deakin]

Attachment: Use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

Changes:

- fix tests to work asynchronously. A few setTimeout calls still exist.
- mostly switch to using SpecialPowers instead of enablePrivilege
- switch specialpowersAPI.js addAutoCompletePopupEventListener method to use popupshown event which is what should actually have been used when that method was added.
- add SpecialPowers.formHistory
- switch change observer to use correct data names
- add entryExists method
- use new formhistory component to initialize form history in tests

Comment 22

[Neil Deakin]

Attachment: Update nsFormAutoComplete to use AsyncFormHistory, v2

Changes:

- switch to use new form history component (bug 566746)
- fix to ensure that the inputlist-autocomplete is called even when autocomplete is disabled

Comment 23

[Neil Deakin]

Comment on attachment 688760 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

David, Gavin says you might be willing to re-review these patches.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

I am, but I am currently in overdrive until Wednesday. I will try and review it earlier, but I can make no promise.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

I'm on it. Are you sure you want to review code that uses FormHistory.jsm before landing that module, though?

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 688761 [details] [diff] [review]
Update nsFormAutoComplete to use AsyncFormHistory, v2

Review of attachment 688761 [details] [diff] [review]:
-----------------------------------------------------------------

You are refactoring code that is largely undocumented, so I hope you do not mind my asking questions.
Also, I am not enthusiastic about the fact that listeners may or may not be called. Is there a good reason to do so?

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +30,5 @@
>      _timeGroupingSize   : 7 * 24 * 60 * 60 * 1000 * 1000,
>      _expireDays         : null,
>      _boundaryWeight     : 25,
>      _prefixWeight       : 5,
> +    _pendingQuery       : null,

I realize that not much is documented here, but could you please take the opportunity to document |_pendingQuery|?

@@ +50,1 @@
>          Services.obs.addObserver(this.observer, "profile-before-change", true);

Do we still need this?

@@ +88,5 @@
>                          self._prefixWeight = self._prefBranch.getIntPref(prefName);
>                          break;
>                      default:
>                          self.log("Oops! Pref not handled, change ignored.");
>                  }

Nice cleanup. I assume that this is due to the fact that asynchronous statements behave more nicely than synchronous ones?

@@ +133,5 @@
>  
>          // don't allow form inputs (aField != null) to get results from search bar history
>          if (aInputName == 'searchbar-history' && aField) {
>              this.log('autoCompleteSearch for input name "' + aInputName + '" is denied');
> +            return;

So the listener is never called? That looks weird.

@@ +175,5 @@
> +            result = new FormAutoCompleteResult(FormHistory, [], aInputName, aUntrimmedSearchString);
> +            this.getAutoCompleteValues(aInputName, searchString,
> +                function (aEntries) {
> +                  if (aField && aField.maxLength > -1) {
> +                    result.wrappedJSObject.entries =

Since you have created |result| as a jsval, that |wrappedJSObject| doesn't look necessary.

@@ +276,3 @@
>          }
>  
> +        this.stopAutoCompleteSearch();

What happens to the listeners when you do this?

@@ -304,5 @@
> -        // Memoize the statements
> -        if (!stmt) {
> -            this.log("Creating new statement for query: " + query);
> -            stmt = this._formHistory.DBConnection.createStatement(query);
> -            this._dbStmts[query] = stmt;

We seemed to do some caching here, but we don't anymore. What was/is the rationale?

@@ +429,5 @@
>  
>          let [removedEntry] = this.entries.splice(index, 1);
>  
> +        if (removeFromDB) {
> +          this.formHistory.updateFormHistory([{ op: "remove", 

Nit: trailing whitespace.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +611,5 @@
>                                           aPreviousResult,
>                                           mFocusedInput,
>                                           getter_AddRefs(result));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    aListener->OnSearchResult(this, result);

I have difficulties following this refactoring. Why did you move this OnSearchResult?

@@ +626,5 @@
> +                                           mFocusedInput,
> +                                           aPreviousResult,
> +                                           static_cast<nsIFormAutoCompleteObserver *>(this));
> +      mLastFormAutoComplete = formAutoComplete;
> +    }

Nit: in this file, I believe that previous authors wrote
  } else {

@@ +643,5 @@
> +        nsCOMPtr<nsIDOMHTMLElement> list;
> +        mFocusedInput->GetList(getter_AddRefs(list));
> +
> +        nsCOMPtr<nsINode> node = do_QueryInterface(list);
> +        if (node) {

That looks much simpler than the original algorithm. What has changed?

@@ +717,5 @@
> +
> +////////////////////////////////////////////////////////////////////////
> +//// nsIFormAutoCompleteObserver
> +
> +NS_IMETHODIMP

I believe that this method deserves a high-level overview of what it is meant to do.

::: toolkit/components/satchel/nsFormFillController.h
@@ +89,5 @@
>    nsCOMPtr<nsISupportsArray> mPopups;
>  
>    //these are used to dynamically update the autocomplete
>    nsCOMPtr<nsIAutoCompleteResult> mLastSearchResult;
>    nsCOMPtr<nsIAutoCompleteObserver> mLastListener;

Could you take the opportunity to document what mLastListener is? The name is quite unintuitive.

::: toolkit/components/satchel/nsIFormAutoComplete.idl
@@ +18,5 @@
> +    void autoCompleteSearch(in AString aInputName,
> +                            in AString aSearchString,
> +                            in nsIDOMHTMLInputElement aField,
> +                            in nsIAutoCompleteResult aPreviousResult,
> +                            in nsIFormAutoCompleteObserver aListener);

I believe that no known add-on uses this interface, so this change is probably ok. However, I would have rather chosen a different technique: keep |autoCompleteSearch| as is plus a deprecation warning, and implement |autoCompleteSearchAsync| or |autoCompleteSearch2| with the callback.

@@ +25,3 @@
>  };
> +
> +[scriptable, uuid(604419ab-55a0-4831-9eca-1b9e67cc4751)]

Given that there is only one method, this should probably be labelled [function], too.

@@ +28,5 @@
> +interface nsIFormAutoCompleteObserver : nsISupports
> +{
> +  /*
> +   * Called when a search is complete and the results are ready even if the
> +   * result set is empty.

However, it seems that the callback is simply never called if the query is canceled for instance. I am not sure I like this behavior, but regardless, it definitely needs to be documented.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 688760 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

Review of attachment 688760 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling r? until we are done with the other patch. Do not hesitate to ask me again once we have finished with the other patch.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +836,2 @@
>                                                             listener,
>                                                             false);

You mention in comment 21 that popupshown is the event that should have been used in the first place. Could you tell me why? Just curious.

Comment 28

[Neil Deakin]

> You are refactoring code that is largely undocumented, so I hope you do not mind my asking questions.

Since I didn't write this patch and just updated it, and fixed one bug with it, I don't actually have any answer beyond what you might ascertain yourself from looking at the code.

But I'll try to address the comments with a followup patch.

> You mention in comment 21 that popupshown is the event that should have been used in the > first place. Could you tell me why? Just curious.

popupshowing fires before the popup opens, and popupshown fires afterwards.

Technically for the test that added that function it doesn't matter which is used, since the test just wants to know if an attempt was made to open the popup. Thinking about it more though popupshowing might be slightly better for that test as it checks earliear in the process before a listener could cancel the event. I'll change this function to just take an argument for the event name.

Comment 29

[Neil Deakin]

> +        this.stopAutoCompleteSearch();
> What happens to the listeners when you do this?
> However, it seems that the callback is simply never called if the query is canceled for instance. I am not sure I like this behavior, but regardless, it definitely needs to be documented.

The listener is used to perform the next part of the search algorithm, scanning for input datalists which are added to the found results. If the search is cancelled, the next part of the search doesn't need to happen either.

They are ignored, since a new search is going to occur 
> We seemed to do some caching here, but we don't anymore. What was/is the rationale?

I would guess the caching was only used to finalize the statements.

> I have difficulties following this refactoring. Why did you move this OnSearchResult?

It is called in one codepath at the end of OnSearchCompletion. The next patch iteration will make this clearer.

Comment 30

[[e-mail]]

The static cast is unnecessary, as the class inherits from the interface, so the compiler can work out how to cast from one to the other without your help.

For when you have to worry about multiple interface inheritance (including those cases where you have to worry about someone deriving from your class) use nsCOMPtr<nsIFoo> foo(do_QueryObject(this));

Comment 31

[Neil Deakin]

Attachment: Part 1.3 - Update nsFormAutoComplete to use AsyncFormHistory

Comment 32

[Neil Deakin]

Attachment: Part 2.3 - use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

Comment 33

[Neil Deakin]

Attachment: Part 3.3 - Use Asynchronous FormHistory.jsm for other tests

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 728928 [details] [diff] [review]
Part 1.3 - Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 728928 [details] [diff] [review]:
-----------------------------------------------------------------

I am still nervous about reviewing code that depends on code that hasn't landed.
And not just because I had to dig the source code of FormHistory.jsm from bug 566746.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +34,5 @@
>  
> +    // Only one query is performed at a time, which will be stored in _pendingQuery
> +    // while the query is being performed. It will be cleared when the query finishes,
> +    // is cancelled, or an error occurs. If a new query occurs while one is already
> +    // pending, the existing one is cancelled.

Nit: Could you document the type of this field?

@@ +127,5 @@
>       * aUntrimmedSearchString -- current value of the input
>       * aField -- nsIDOMHTMLInputElement being autocompleted (may be null if from chrome)
>       * aPreviousResult -- previous search result, if any.
> +     * aListener -- nsIFormAutoCompleteObserver that listens for the nsIAutoCompleteResult
> +     *              that may be returned asynchronously

Please document the argument passed to |aListener|.
Also, please document |aAsync|.

@@ +132,1 @@
>       */

Is this a public API? Otherwise, it might be useful to prefix this with a _. Your call.

@@ +137,5 @@
>  
> +        let result = null;
> +        if (!this._enabled) {
> +            result = new FormAutoCompleteResult(FormHistory, [], aInputName, aUntrimmedSearchString);
> +            aListener.onSearchCompletion(result);

Looks like this will fail if |aListener| is |null|. Given that |autoCompleteSearch| seems to pass |null| as |aListener|, there is something fishy.

@@ +191,5 @@
> +            let entries = aAsync ? [] : this.getAutoCompleteValues(aInputName, searchString);
> +            result = new FormAutoCompleteResult(FormHistory, entries, aInputName, aUntrimmedSearchString);
> +
> +            let processEntry = function(aEntries)
> +            {

In this file, { seem to go at the end of the line.

@@ +194,5 @@
> +            let processEntry = function(aEntries)
> +            {
> +              if (aField && aField.maxLength > -1) {
> +                result.entries =
> +                  aEntries.filter(function (el) { return el.text.length <= aField.maxLength; });

The original version used |result.wrappedJSObject.entries|. Is that change by design?

@@ +196,5 @@
> +              if (aField && aField.maxLength > -1) {
> +                result.entries =
> +                  aEntries.filter(function (el) { return el.text.length <= aField.maxLength; });
> +              } else {
> +                result.entries = aEntries;

In the original version, this |result.entries = aEntries| didn't seem necessary. Is that change by design?

@@ +225,2 @@
>  
> +    getAutoCompleteValues : function (fieldName, searchString, aCallback) {

Could you take the opportunity to document this method?

@@ +244,5 @@
> +            aCallback(aResults);
> +          },
> +          onFailure: function(aError) {
> +            self.log("getAutocompleteValues failed: " + aError.message);
> +            self._pendingQuery = null;

I seem to remember being explained that the callback is not called by design. Could you add a comment to that effect?

@@ +247,5 @@
> +            self.log("getAutocompleteValues failed: " + aError.message);
> +            self._pendingQuery = null;
> +          }
> +        };
> +          

Nit: Whitespace.

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +603,5 @@
>      rv = mLoginManager->AutoCompleteSearch(aSearchString,
> +                                           aPreviousResult,
> +                                           mFocusedInput,
> +                                           getter_AddRefs(result));
> +    NS_ENSURE_SUCCESS(rv, rv);

I seem to remember that |NS_ENSURE_SUCCESS| is deprecated
(appears several times in this code)

@@ +604,5 @@
> +                                           aPreviousResult,
> +                                           mFocusedInput,
> +                                           getter_AddRefs(result));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    aListener->OnSearchResult(this, result);

I would be more comfortable if we checked that |aListener| is not null.

@@ -624,2 @@
>  
> -    mLastSearchResult = formHistoryResult;

I may be wrong, but I have the impression that you do not update |mLastSearchResult| anymore.

::: toolkit/components/satchel/nsFormFillController.h
@@ +101,2 @@
>    nsCOMPtr<nsIAutoCompleteObserver> mLastListener;
> +  nsCOMPtr<nsIFormAutoComplete> mLastFormAutoComplete;

Do I understand correctly that we hold upon this nsIFormAutoComplete solely to be able to cancel it?

I see that it is cleared by StopSearch. Do we have guarantees that this method is called at the completion of every search? Otherwise, we might want to remove the reference once we are satisfied that the search is complete.

::: toolkit/components/satchel/nsIFormAutoComplete.idl
@@ +19,5 @@
>       */
> +    void autoCompleteSearch(in AString aInputName,
> +                            in AString aSearchString,
> +                            in nsIDOMHTMLInputElement aField,
> +                            in nsIAutoCompleteResult aPreviousResult);

So this method does not return anymore? That looks a little weird.

@@ +30,5 @@
> +                                 in nsIDOMHTMLInputElement aField,
> +                                 in nsIAutoCompleteResult aPreviousResult,
> +                                 in nsIFormAutoCompleteObserver aListener);
> +
> +    void stopAutoCompleteSearch();

This needs more documentation. When should it be called? What happens if there is no search in progress? etc.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 728930 [details] [diff] [review]
Part 2.3 - use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

Review of attachment 728930 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/unit/test_autocomplete.js
@@ +84,5 @@
> +              onSuccess : function(aFieldEntries) {
> +                  do_check_true(aFieldEntries > 0);
> +                  run_next_test();
> +              },
> +              

Nit: whitespace

@@ +141,4 @@
>  
> +add_test(function test5() {
> +    do_log_info("Begin tests with constant use dates and varying timesUsed");
> +    do_log_info("Check search result ordering with empty search term");

Shouldn't that do_log_info go to test6()?

Comment 36

[Neil Deakin]

Yeah, the patches in this bug really should just be part of bug 566746 as they can't be landed separately.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 728931 [details] [diff] [review]
Part 3.3 - Use Asynchronous FormHistory.jsm for other tests

Review of attachment 728931 [details] [diff] [review]:
-----------------------------------------------------------------

I will try and review test_form_* tomorrow.

::: toolkit/components/satchel/test/browser/browser_privbrowsing_perwindowpb.js
@@ +17,5 @@
>        aWindow.gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
>  
> +      let checks = 0;
> +      function doneCheck()
> +      {

Nit: doneCheck() {

@@ +23,5 @@
> +        if (checks == 2) {
> +          executeSoon(aCallback);
> +        }
> +      }
> +

I understand how your code implements the comment below, but I don't understand how the previous code did. What am I missing?

::: toolkit/components/satchel/test/test_bug_511615.html
@@ +241,5 @@
>      case 20:
>          // We're privileged for this test, so open the popup.
>          checkPopupOpen(false);
>          checkForm("");
> +        expectingPopup = true;

A bit hackish. Could you perhaps check that expectingPopup is not true already?

Also, this pattern looks hard to debug if the popup for some reason fails to appear. Perhaps you could
- info(...) the fact that you are now waiting for a popup to appear;
- info(...) in popupShownListener the fact that a popup has appeared.

::: toolkit/components/satchel/test/test_form_autocomplete.html
@@ +12,5 @@
>  <p id="display"></p>
>  
> +<!-- We presumably can't hide the content for this test. The large top padding is to allow
> +     listening for scrolls to occur. -->
> +<div id="content" style="padding-top: 2000px;">

If we absolutely need scrolling, 2000px might not be sufficient for very long. Perhaps multiply by 10?

@@ +192,5 @@
> +
> +var expectingPopup = false;
> +function popupShownListener()
> +{
> +  if (expectingPopup) {

As in previous test, some info(...) here and in the part of the code that sets |expectingPopup| to |true| would be useful.

@@ +226,5 @@
>      case 1:
>          // Make sure initial form is empty.
>          checkForm("");
>          // Trigger autocomplete popup
> +        expectingPopup = true;

Couldn't this be done by |restoreForm()|?

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 728928 [details] [diff] [review]
Part 1.3 - Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 728928 [details] [diff] [review]:
-----------------------------------------------------------------

David's review looks quite well done, the global change looks clean to me and I don't have much comments so far.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +217,5 @@
>      },
>  
> +    stopAutoCompleteSearch : function () {
> +        if (this._pendingQuery) {
> +            this._pendingQuery.cancel();

just to be sure we are on the same boat, cancelling a pending query won't immediately stop sending back results, but will definitely fire onCompletion with reason CANCELED after a little while

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +627,2 @@
>  
> +      // Handle the inputlist even if autocomplete is disabled

when commenting code, please always express the reason behind choices, in future it won't be that clear why it is done

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 728931 [details] [diff] [review]
Part 3.3 - Use Asynchronous FormHistory.jsm for other tests

Review of attachment 728931 [details] [diff] [review]:
-----------------------------------------------------------------

Here we go. Don't forget the first part of the review on comment 37.

::: toolkit/components/satchel/test/test_form_autocomplete_with_list.html
@@ +82,5 @@
> +var testNum = 0;
> +
> +var prevValue;
> +
> +var expectingPopup = false;

Same remark as in previous tests.

@@ +102,5 @@
>  *
> +* This is a bit hacky, as many operations happen asynchronously.
> +* Various mechanisms call runTests as a result of operations:
> +*   - set expectingPopup to true, and the next test will occur when the autocomplete popup is shown
> +*   - call waitForMenuChange(x) to run the next test when the autocomplete popup to have x items in it 

Nit: whitespace.

@@ +121,1 @@
>          restoreForm();

As previously, looks like you cold merge restoreForm and expectingPopup = true;

Comment 40

[Neil Deakin]

Attachment: Part 1.4 - Update nsFormAutoComplete to use AsyncFormHistory

I reworked this a bit to properly handle the synchronous version of the api, however since its deprecated I just use an event loop.


@@ +127,5 @@
> 	* aUntrimmedSearchString -- current value of the input
> 	* aField -- nsIDOMHTMLInputElement being autocompleted (may be null if
from chrome)
> 	* aPreviousResult -- previous search result, if any.
> +	* aListener -- nsIFormAutoCompleteObserver that listens for the
nsIAutoCompleteResult
> +	*	       that may be returned asynchronously

> Please document the argument passed to |aListener|.

I would think that the documentation for nsIFormAutoCompleteObserver was sufficient.


::: toolkit/components/satchel/nsFormFillController.cpp
@@ +603,5 @@
>      rv = mLoginManager->AutoCompleteSearch(aSearchString,
> +					      aPreviousResult,
> +					      mFocusedInput,
> +					      getter_AddRefs(result));
> +    NS_ENSURE_SUCCESS(rv, rv);

> I seem to remember that |NS_ENSURE_SUCCESS| is deprecated (appears several times in this code)

Not that I'm aware of.


::: toolkit/components/satchel/nsFormFillController.h
@@ +101,2 @@
>    nsCOMPtr<nsIAutoCompleteObserver> mLastListener;
> +  nsCOMPtr<nsIFormAutoComplete> mLastFormAutoComplete;

> Do I understand correctly that we hold upon this nsIFormAutoComplete solely to
> be able to cancel it?

Yes, and as far as I know, StopSearch is always called.


@@ +226,5 @@
>      case 1:
> 	   // Make sure initial form is empty.
> 	   checkForm("");
> 	   // Trigger autocomplete popup
> +	   expectingPopup = true;

> Couldn't this be done by |restoreForm()|?

Several callers only do one or the other.


::: toolkit/components/satchel/test/browser/browser_privbrowsing_perwindowpb.js

> I understand how your code implements the comment below, but I don't understand
> how the previous code did. What am I missing?

I don't either, but I'll investigate.

Comment 41

[Neil Deakin]

Attachment: Part 2.4 - use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

OK, it turns out this test wasn't even running, so I fixed it up. No wonder it seemed to not fail.

Comment 42

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

(In reply to Neil Deakin from comment #40)
> > I seem to remember that |NS_ENSURE_SUCCESS| is deprecated (appears several times in this code)
> 
> Not that I'm aware of.

Not quite "deprecated", but bsmedberg isn't a fan, and has been discouraging its use in favor of explicit returns (i.e. |if (NS_FAILED(rv)) return rv;|).

Comment 43

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

(In reply to Neil Deakin from comment #40)
> I reworked this a bit to properly handle the synchronous version of the api,
> however since its deprecated I just use an event loop.

Where is this used in practice? Are we planning on eventually completely removing the sync API?

Comment 44

[Neil Deakin]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #43)
> (In reply to Neil Deakin from comment #40)
> > I reworked this a bit to properly handle the synchronous version of the api,
> > however since its deprecated I just use an event loop.
> 
> Where is this used in practice? Are we planning on eventually completely
> removing the sync API?

I noticed today that there a couple of usages still that need to be converted over, which I will fix and some addons might use it. The synchronous version prints a deprecated warning and will removed in the future.

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 731244 [details] [diff] [review]
Part 1.4 - Update nsFormAutoComplete to use AsyncFormHistory

Review of attachment 731244 [details] [diff] [review]:
-----------------------------------------------------------------

For the moment, f+, until the issue of API clients is resolved.

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ +11,5 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/Deprecated.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
> +                                  "resource://gre/modules/Deprecated.jsm");

You are importing Deprecated.jsm twice. I suspect that the lazy getter might be sufficient.

@@ +116,5 @@
>      },
>  
>  
> +    autoCompleteSearch : function (aInputName, aUntrimmedSearchString, aField, aPreviousResult) {
> +      Deprecated.warning("nsIFormAutoComplete::autoCompleteSearch is deprecated");

Deprecated.warning also takes a non-optional URL. If you have not provided the URL, an exception has been thrown, so you should check out the test suite.

@@ +129,5 @@
> +      let thread = Components.classes["@mozilla.org/thread-manager;1"].getService().currentThread;
> +      while (!result && this._pendingQuery) {
> +        thread.processNextEvent(true);
> +      }
> +

I realize the need, but that is rather ugly. I believe that this is acceptable if all m-c clients of nsFormAutocomplete have been ported to the async version. Can you confirm that this is the case?

Also,
https://mxr.mozilla.org/addons/search?string=nsFormAutocomplete
suggests that the FirefoxOS simulator uses nsFormAutocomplete. Could you check this out quickly that they do not use the sync API? Otherwise, I believe that fixing FFOS simulator is a blocker for this bug.

::: toolkit/components/satchel/nsFormFillController.h
@@ +101,2 @@
>    nsCOMPtr<nsIAutoCompleteObserver> mLastListener;
> +  nsCOMPtr<nsIFormAutoComplete> mLastFormAutoComplete;

Could you add a comment stating that this is cleared by |StopSearch| and that if |StopSearch| is not called, we are going to leak?

Comment 46

[Neil Deakin]

(In reply to David Rajchenbach Teller [:Yoric] from comment #45)
> I realize the need, but that is rather ugly. I believe that this is
> acceptable if all m-c clients of nsFormAutocomplete have been ported to the
> async version. Can you confirm that this is the case?
> 

Yes.

> Also,
> https://mxr.mozilla.org/addons/search?string=nsFormAutocomplete
> suggests that the FirefoxOS simulator uses nsFormAutocomplete. Could you
> check this out quickly that they do not use the sync API? Otherwise, I
> believe that fixing FFOS simulator is a blocker for this bug.

That looks to be implementing its own form history autocomplete component. It also looks to be nsIInputListAutoComplete which isn't changed here to be asynchronous.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 731247 [details] [diff] [review]
Part 2.4 - use Asynchronous FormHistory.jsm for test_form_autocomplete.html, v2

Review of attachment 731247 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Not a blocker, but if you find time, it would be nice to document which property is tested by which test.

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 731244 [details] [diff] [review]
Part 1.4 - Update nsFormAutoComplete to use AsyncFormHistory

In that case, moving to r+, pending the minor changes mentioned previously.

Comment 49

[Neil Deakin]

This was checked in on inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5901830c7a9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/44b897958ad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3c196b31af

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this had to be backed out for frequent OSX debug mochitest-5 crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc39d8b649a

https://tbpl.mozilla.org/php/getParsedLog.php?id=21685513&tree=Mozilla-Inbound

08:34:56     INFO -  171399 INFO TEST-INFO | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | expecting popup for test 300
08:34:56     INFO -  Assertion failure: mPresContext->mLayoutPhaseCount[eLayoutPhase_Reflow] == 0 (painting in the middle of reflow), at ../../../layout/base/nsAutoLayoutPhase.cpp:37
08:34:59  WARNING -  TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Exited with code 1 during test run
08:34:59     INFO -  INFO | automation.py | Application ran for: 0:13:05.919909
08:34:59     INFO -  INFO | zombiecheck | Reading PID log: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpQu_7Wkpidlog
08:35:15  WARNING -  PROCESS-CRASH | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | application crashed [@ nsAutoLayoutPhase::Enter()]
08:35:15     INFO -  Crash dump filename: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpkZJoD6/minidumps/E350A22D-DAF2-4ECB-8FBD-E4FDA6BED10E.dmp
08:35:15     INFO -  Operating system: Mac OS X
08:35:15     INFO -                    10.7.2 11C74
08:35:15     INFO -  CPU: amd64
08:35:15     INFO -       family 6 model 23 stepping 10
08:35:15     INFO -       2 CPUs
08:35:15     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
08:35:15     INFO -  Crash address: 0x0
08:35:15     INFO -  Thread 0 (crashed)
08:35:15     INFO -   0  XUL!nsAutoLayoutPhase::Enter() [nsAutoLayoutPhase.cpp:0b669f2d896a : 34 + 0x0]
08:35:15     INFO -      rbx = 0x00007fff759f1630   r12 = 0x00007fff5fbfb4d8
08:35:15     INFO -      r13 = 0x000000010ca78de0   r14 = 0x00007fff5fbfb508
08:35:15     INFO -      r15 = 0x000000010ca780b0   rip = 0x00000001013fbe3f
08:35:15     INFO -      rsp = 0x00007fff5fbfb2f0   rbp = 0x00007fff5fbfb300
08:35:15     INFO -      Found by: given as instruction pointer in context
08:35:15     INFO -   1  XUL!PresShell::Paint(nsView*, nsRegion const&, unsigned int) [nsPresShell.cpp:0b669f2d896a : 5492 + 0x4]
08:35:15     INFO -      rip = 0x00000001013de9c0   rsp = 0x00007fff5fbfb310
08:35:15     INFO -      rbp = 0x00007fff5fbfb4b0
08:35:15     INFO -      Found by: stack scanning

Comment 51

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

Re-landed this as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/555f2b757639

Comment 52

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

(try run with that patch: https://tbpl.mozilla.org/?tree=Try&rev=1ad668e3d7a3)

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backout:
https://hg.mozilla.org/mozilla-central/rev/555f2b757639

Comment 54

[Mark Hammond [:markh] [:mhammond]]

via bug 566746

try at https://tbpl.mozilla.org/?tree=Try&rev=da094345346f

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b21e902f7a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/96fe69d53f35

Comment 55

[[e-mail]]

Comment on attachment 731244 [details] [diff] [review]
Part 1.4 - Update nsFormAutoComplete to use AsyncFormHistory

>-      rv = formAutoComplete->AutoCompleteSearch(aSearchParam,
>+      formAutoComplete->AutoCompleteSearchAsync(aSearchParam,
Any reason you've stopped propagating exceptions?

Comment 56

[[e-mail]]

Comment on attachment 731244 [details] [diff] [review]
Part 1.4 - Update nsFormAutoComplete to use AsyncFormHistory

>+nsFormFillController::OnSearchCompletion(nsIAutoCompleteResult *aResult)
>+{
>+  nsCOMPtr<nsIAutoCompleteResult> resultParam = do_QueryInterface(aResult);
>+
>+  nsAutoString searchString;
>+  resultParam->GetSearchString(searchString);
>+  mLastSearchResult = aResult;
>+  mLastSearchString = searchString;
>+
>+  return PerformInputListAutoComplete(resultParam);
>+}
Why write three lines of code when six will do?

Comment 57

[Mark Hammond [:markh] [:mhammond]]

(In reply to [e-mail] from comment #55 and comment #56)

I guess you noticed that this landed 3 months ago, so the feedback is a little late for this bug.  Please open another if you think there are issues that need to be addressed by this.