For MailNews autocomplete migration to toolkit, we want to have multiple autocompletesearch items, i.e. multiple searches on one autocomplete. The first two of these (at least) return their results to the listener/controller straight away when startSearch is called. When we do a subsequent search (e.g. going from "S" to "St"), only the first search defined in the "autocompletesearch" attribute gets its previous result back. The second search just gets null. I'm attaching a xpcshell test that fails due to this bug. This is a real pain because we can't do efficient searching on subsequent changes.

Comment on attachment 324817 [details] [diff] [review] XPFE fix >@@ -649,7 +649,7 @@ > { > this.mLastResults[aSessionName] = null; > if (firstReturn) >- this.clearResults(false); >+ this.clearResultElements(false); Hmm. This seems to be the only place data is cleared out now. Should this be doing something to null out the param like clearResultData? > this.searchFailed(); > return; > } else if (aStatus == Feel free to remove else-after-return here. >- this.clearResults(false); // clear results, but don't repaint yet >+ this.clearResultElements(false); // clear results, but don't repaint yet Comment is better now than it was before. :) > // if all searches are done and they all failed... >- this.clearResults(true); // clear data and repaint empty >+ this.clearResultElements(true); // clear data and repaint empty Comment isn't right anymore. does this only not need to clear the data because there's no data to clear? Is that true even if called from startLookup? I'd think startLookup would want to clear any results for the session that failed. It looks like clearResults is now only called by the desctructor. Perhaps just inline clearResults to there.

(In reply to comment #2) >(From update of attachment 324817 [details] [diff] [review]) >> this.mLastResults[aSessionName] = null; >Should this be doing something to null out the param like clearResultData? param is going away with the toolkit switch, no need to worry about it. >>- this.clearResults(true); // clear data and repaint empty >>+ this.clearResultElements(true); // clear data and repaint empty >Comment isn't right anymore. does this only not need to clear the data because >there's no data to clear? Is that true even if called from startLookup? I'd >think startLookup would want to clear any results for the session that failed. You're right, this change is probably incorrect.

Fix checked in.

Comment on attachment 327390 [details] [diff] [review] [checked in] Addressed review comments > { > this.mLastResults[aSessionName] = null; > if (firstReturn) >- this.clearResults(false); >+ this.clearResultElements(false); > this.searchFailed(); > return; >- } else if (aStatus == >- Components.interfaces.nsIAutoCompleteStatus.failureItems){ >+ if (aStatus == Components.interfaces.nsIAutoCompleteStatus.failureItems) > ++this.mFailureItems; >- } You missed adding the } back in after the return statement. I've checked in a fixed as autocomplete is pretty much broken without that.

Patch checked into mozilla-central in Bug 443837

Reopening - this is NOT fixed for toolkit (which this bug was originally filed for. The testcase attached should still be valid for toolkit builds (though I haven't checked it yet.

This is the fix for toolkit. It also fixes several other bugs: - mMatchCounts was initially set to the length of the searchCount, not the capacity. I'm not sure why this hasn't really showed up at all, possibly because ClearResults() was clearing the list. - When looking up the previous results index, a pointer to the previous results was used, this may not be the case, if the caller created a new set of results and copied the relevant ones from the previous, additionally, we know the index anyway. Alongside those, the main part of this fix is to not clear out the stored results when we're just doing a follow-on search (and just zero the matches), and therefore this keeps the results available for the next search. Includes the testcase that I did when I originally filed this bug.

Comment on attachment 334328 [details] [diff] [review] Toolkit fix >+ PRUint32 length = 0; >+ mResults->Count(&length); >+ if (aSearchIndex >= (PRInt32)length) { I don't think you can guarantee the order of the results. >+ return; >+ } >+ else { Nit: else after return

(In reply to comment #10) > (From update of attachment 334328 [details] [diff] [review]) > >+ PRUint32 length = 0; > >+ mResults->Count(&length); > >+ if (aSearchIndex >= (PRInt32)length) { > I don't think you can guarantee the order of the results. Actually you can, see nsAutoCompleteController::OnSearchResult: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#668

What I mean is, what happens if search 2 returns results before search 1 does?

Comment on attachment 334328 [details] [diff] [review] Toolkit fix nsAutoCompleteController::ProcessResult( > if (aResult) > aResult->GetMatchCount(&matchCount); > >- PRInt32 oldIndex = mResults->IndexOf(aResult); >- if (oldIndex == -1) { >+ PRUint32 length = 0; >+ mResults->Count(&length); >+ if (aSearchIndex >= (PRInt32)length) { > // cache the result > mResults->AppendElement(aResult); > mMatchCounts.AppendElement(matchCount); > } > else { > // replace the cached result >- mResults->ReplaceElementAt(aResult, oldIndex); >+ mResults->ReplaceElementAt(aResult, aSearchIndex); > oldMatchCount = mMatchCounts[aSearchIndex]; >- mMatchCounts[oldIndex] = matchCount; >+ mMatchCounts[aSearchIndex] = matchCount; > } I don't understand this code much at all. Can you explain this part. What does mMatchCounts hold? Is a result just one item of output (that is, there is one result per line in the autocomplete box), or multiple items?

Here's a much simpler fix that Neil Rashbrook suggested to me over irc. mResults is an array of the results that were returned from the previous searches; each element in the array is an nsIAutoCompleteResult item returned from the search containing the individual results, so we'll get one item per search type used (e.g. history / addrbook / mydomain). This patch caches the mResults array before calling StartSearch. This means when the first search comes back and forces all the results arrays to be cleared (which could be synchronous and occur before starting the search on the second search session), we'll still have a local cache of the results to pass the previous results correctly back to the searches that haven't been started yet.

Comment on attachment 334686 [details] [diff] [review] Toolkit fix v2 Yes, this looks much better. Do we want to cleart the cached items once all searches have completed?

(In reply to comment #15) > (From update of attachment 334686 [details] [diff] [review]) > Yes, this looks much better. Do we want to cleart the cached items once all > searches have completed? > Its a cache local to the function, therefore shouldn't the array clear these for us?

My apologies, I missed including the testcase last time, not sure if you wanted to look at it again or not.

I checked this in as changeset id 379931a16cd7, I then backed out under id 20663933d4b0 due to what looked like a Lk/MH regression (all platforms, worst on Linux). However, Lk/MH didn't go back down again. So I think that the tinderbox regression was actually due to something else (although no other patches landed), so I'll try relanding in a couple of days.

Checked in again as changeset 18409:166ebc402d37 No significant changes in Lk/MH so it looks like it was originally a tinderbox issue.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/04bd0b3e026d fix xpfe autocomplete bustage from bug 438861 to be really in sync with cvs