handle repeated calls to ProcessResult() [allow for incremental autocomplete search results] I found this while working on bug #389491, where I repeatedly call nsAutoCompleteController::OnSearchResult() which can call ProcessResults(). [looking at lxr, no one seems to do what I'm attempting to do.] if you call OnSearchResult() only once, like you see in lxr, the current code works because mRowCount starts at 0, so oldRowCount is 0. I'll attach a patch.

accepting for m8, as this blocks #389491 (which should land for m8)

this is the toolkit part of bug #389491 that doesn't have to do with places.

Comment on attachment 275665 [details] [diff] [review] updated patch >Index: toolkit/components/autocomplete/public/nsIAutoCompleteController.idl >+ * Stops a asynchronous search. nit: "an" >Index: toolkit/components/autocomplete/src/nsAutoCompleteController.cpp StopSearch() calls ClearSearchTimer(), so we should get rid of the calls to ClearSearchTimer() that happen before calls to StopSearch() (there are existing ones, but your patch adds one). > nsAutoCompleteController::StopSearch() >+ // stop all searches, even if mSearchStatus is not nsIAutoCompleteController::STATUS_SEARCHING >+ // we might have an incremental search running (which as already returned results. nit: "has", close parenthesis. I'm a bit worried about changing the behavior of StopSearch() to unconditionally stop all searches, because nsIAutocompleteSearch stopSearch() implementations might make assumptions about when they're called and could potentially cause bad side effects on the search. Given that the autocomplete APIs are poorly documented and not extensively tested beyond the relatively simple users in our tree, I'm not sure there's really much we can do to assuage my worry. I'm probably worrying too much :) >@@ -1173,20 +1177,20 @@ nsAutoCompleteController::ProcessResult( Seems like we invalidate the entire tree in ProcessResult(), ideally in the incremental search case we could invalidate only the rows that were added (which in most cases won't even be drawn because they're scrolled off the screen). I think the best way to do this would be to expose additional invalidate*() variants on nsIAutoCompletePopup, implement them in autocomplete.xml (which just forwards to the tree's boxobject), and then have the autocomplete controller invalidate only the parts that have changed. This might be tricky because the controller can't know how the two sets of results differ, because the history service nsIAutocompleteSearch impl just calls onSearchResult multiple times with a new autocomplete result each time, as far as I can tell, but something to look at in a followup bug, for sure (I think it would also benefit other autocomplete consumers, and probably fix bug 391889). > } else if (result == nsIAutoCompleteResult::RESULT_SUCCESS) { > // Increase the match count for all matches in this result > PRUint32 matchCount = 0; > aResult->GetMatchCount(&matchCount); >- mRowCount += matchCount; >+ mRowCount = matchCount; > if (mTree) >- mTree->RowCountChanged(oldRowCount, matchCount); >- >+ mTree->RowCountChanged(oldRowCount, mRowCount - oldRowCount); >+ > // Try to autocomplete the default index for this search > CompleteDefaultIndex(aSearchIndex); > } Looks to me like this change breaks support for multiple nsIAutoCompleteSearches (we have no in-tree consumers at the moment, that I know of, but I've been trying to make search suggest use it). mRowCount will be wrong once the second result is processed in that case, since it won't take into account the previously processed result. This is in some way incompatible with what you're doing - the controller expects each call to onSearchResult (and this ProcessResult) to receive a separate result object, but you're changing it so that each object replaces the previous one. You need to keep a list of already-returned searches (a list of indices into mSearches is probably sufficient, since ProcessResult is already passed an index) and adjust mRowCount appropriately based on whether this is a "new" result or simply an replacement for a previously processed result. r- because of this issue. I should really add some unit tests for this stuff... >Index: toolkit/content/widgets/autocomplete.xml > <handler event="contextmenu" phase="capturing" >- action="this.closePopup();"/> >+ action="this.mController.stopSearch(); this.closePopup();"/> >+ >+ <handler event="mousedown" phase="capturing" >+ action="this.mController.stopSearch();"/> I wonder whether we should just put a call to stopSearch in the popup's popuphiding handler instead of adding a handler to the input for contextmenu and mousedown.

1) >+ * Stops a asynchronous search. > nit: "an" fixed, thanks 2) > StopSearch() calls ClearSearchTimer(), so we should get rid of the calls to > ClearSearchTimer() that happen before calls to StopSearch() (there are existing > ones, but your patch adds one). fixed, thanks. 3) nit: "has", close parenthesis. fixed, thanks. 4) > I'm a bit worried about changing the behavior of > StopSearch() to unconditionally stop all searches stopSearch() did this before my patch, my patch just makes stopSearch() part of the nsIAutoCompleteController interface so that I could get to it from autocomplete.xml Or did I misunderstand you? I did fix some comments, such as: -* Stop an asynchronous search that is in progress +* Stop all searches that are in progress and -// Stop current search in case it's async. +// Stop all searches in case they are async. 5) > Seems like we invalidate the entire tree in ProcessResult() I'll log a follow up bug on this, but I think there is another bug that this would also address. (see https://bugzilla.mozilla.org/show_bug.cgi?id=389491#c32, which I'll also log as a spin off) 6) > } else if (result == nsIAutoCompleteResult::RESULT_SUCCESS) { > // Increase the match count for all matches in this result > PRUint32 matchCount = 0; > aResult->GetMatchCount(&matchCount); >- mRowCount += matchCount; >+ mRowCount = matchCount; > if (mTree) >- mTree->RowCountChanged(oldRowCount, matchCount); >- >+ mTree->RowCountChanged(oldRowCount, mRowCount - oldRowCount); >+ > // Try to autocomplete the default index for this search > CompleteDefaultIndex(aSearchIndex); > } > Looks to me like this change breaks support for multiple > nsIAutoCompleteSearches (we have no in-tree consumers at the moment, that I > know of, but I've been trying to make search suggest use it). mRowCount will be > wrong once the second result is processed in that case, since it won't take > into account the previously processed result. This is in some way incompatible > with what you're doing - the controller expects each call to onSearchResult > (and this ProcessResult) to receive a separate result object, but you're > changing it so that each object replaces the previous one. > You need to keep a > list of already-returned searches (a list of indices into mSearches is probably > sufficient, since ProcessResult is already passed an index) and adjust > mRowCount appropriately based on whether this is a "new" result or simply an > replacement for a previously processed result. r- because of this issue. I see your point, but don't I have to pass a complete result set every time? Otherwise, what will nsNavHistory::StartSearch() pass back as the previous result? 7) > I should really add some unit tests for this stuff... I'll log a spin off bug on that. 8) > I wonder whether we should just put a call to stopSearch in the popup's > popuphiding handler instead of adding a handler to the input for contextmenu > and mousedown. won't the popuphiding handler get called when we want the search to continue? a) user types google b) popup show c) user adds ffff (to get googlefff), so popup is hid while that search happens. Or am I minunderstanding what you are suggestng?

(In reply to comment #6) > > I'm a bit worried about changing the behavior of > > StopSearch() to unconditionally stop all searches > > stopSearch() did this before my patch, my patch just makes stopSearch() part of > the nsIAutoCompleteController interface so that I could get to it from > autocomplete.xml You exposed it, but you also modified it, removing the check for STATUS_SEARCHING. That change is what my comment was about. > > You need to keep a > > list of already-returned searches (a list of indices into mSearches is probably > > sufficient, since ProcessResult is already passed an index) and adjust > > mRowCount appropriately based on whether this is a "new" result or simply an > > replacement for a previously processed result. r- because of this issue. > > I see your point, but don't I have to pass a complete result set every time? > Otherwise, what will nsNavHistory::StartSearch() pass back as the previous > result? I'm not sure what you mean. Looking at the patch for bug 389491, you're calling onSearchResult multiple times (from PerformAutoComplete()) with a different result each time, even though your nsIAutocompleteSearch has only received one StartSearch() call, because you want to return results incrementally. This confuses the controller which only expects one call to nsIAutoCompleteListener::onSearchResult for each nsIAutoCompleteSearch::StartSearch() call. Given that the controller will now receive multiple "onSearchResults" for a given "StartSearch()", you need to fix it's rowcount logic to make up for it. You can't assume that each new result processed by ProcessResult replaces the previously processed results. That's all my comment was about, the controller will still pass the previous results for a given search to subsequent calls to it's StartSearch(). > > I wonder whether we should just put a call to stopSearch in the popup's > > popuphiding handler instead of adding a handler to the input for contextmenu > > and mousedown. > > won't the popuphiding handler get called when we want the search to continue? I'm not sure. I was just trying to put the call to stopSearch some place common instead of having to put it in handlers for all the events we think should stop the search. Maybe that's not feasible :)

gavin, thanks for the clarification. I understand your comment about nsAutoCompleteController::ProcessResult() now. looking into it, I noticed another problem with my change that I need to address. we use mSearchesOngoing to figure out when to call PostSearchCleanup(), but I'm calling ProcessResult() multiple times, so this won't work like it used to work.

chatted with Gavin, and to address the mSearchesOngoing, I'm going to add two status codes (RESULT_NOMATCH_ONGOING and RESULT_SUCCESS_ONGOING)

1) > You exposed it, but you also modified it, removing the check for > STATUS_SEARCHING. That change is what my comment was about. to address this comment, I made this change: + /* + * Stop all asynchronous searches (no matter what the searchStatus) + */ + void stopSearch(); 2) > You can't assume that each new result > processed by ProcessResult replaces the previously processed results. to address this comment, here's what I did: When nsAutoCompleteController::ProcessResult() is called, we used to always append it to the results. now, I first check to see if it is there already. if so, we replace instead of append. and, when adjusting row count, I use that knowledge (if the result is cached) to indicate we are replacing it, so we should adjust row count accordingly. 3) RESULT_NOMATCH_ONGOING and RESULT_SUCCESS_ONGOING Per our discussion, I've added these searchResult codes, and used them.

Comment on attachment 277178 [details] [diff] [review] updated patch clearing review request, need to fix a problem with this patch.

the problem with the current patch is that I've broken the handling of enter. with the current patch, our search status is STATUS_SEARCHING until the ongoing search fully completes. but because our status is STATUS_SEARCHING, there is existing code that waits until the search completes to handle enter, and for a large history, we need to stop the search. working on a fix.

fixed. Checking in toolkit/components/autocomplete/public/nsIAutoCompleteController.idl ; /cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteControlle r.idl,v <-- nsIAutoCompleteController.idl new revision: 1.16; previous revision: 1.15 done Checking in toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl; /cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteResult.id l,v <-- nsIAutoCompleteResult.idl new revision: 1.5; previous revision: 1.4 done Checking in toolkit/components/autocomplete/public/nsIAutoCompleteSearch.idl; /cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteSearch.id l,v <-- nsIAutoCompleteSearch.idl new revision: 1.4; previous revision: 1.3 done Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp; /cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cp p,v <-- nsAutoCompleteController.cpp new revision: 1.62; previous revision: 1.61 done Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.h; /cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.h, v <-- nsAutoCompleteController.h new revision: 1.10; previous revision: 1.9 done Checking in toolkit/content/widgets/autocomplete.xml; /cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v <-- autocomplete.x ml new revision: 1.81; previous revision: 1.80 done

landed gavin's unit test: Checking in test_autocomplete_multiple.js; /cvsroot/mozilla/toolkit/components/autocomplete/tests/unit/test_autocomplete_mu ltiple.js,v <-- test_autocomplete_multiple.js initial revision: 1.1 done

Bonsai had the wrong bug number so I accidentally left this in 389503, but this is the correct bug number. This appears to be causing a crash for me. Using 200708211545 build from here http://hourly-archive.localgho.st/, I have no crash. Using the 200708211656 build from the same location, I get a crash when the suggestions start appearing from the search bar. The only checkin during this interval is: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1187736300&maxdate=1187740559 I get the crash in normal mode, and in safe mode. However, I do not crash with a new profile. Does anyone want me to upload part of my profile so that we can see what might be causing this?

(In reply to comment #23) > This appears to be causing a crash for me. Weston, could you file a new bug on the crash? It's much easier to deal with new issues in separate bugs. Please CC Seth and I.

I filed bug 393140 to fix the commit message.

weston, can you try again with tomorrow's nightly? If you still crash, would you be willing to send me your places.sqlite file?

weston, you might have been hitting bug #393191. is browser.formfill.enable false in your profile? can you try again with a build with that fix for that bug?

(In reply to comment #27) > weston, you might have been hitting bug #393191. > > is browser.formfill.enable false in your profile? > > can you try again with a build with that fix for that bug? > Yes, browser.formfill.enable is set to false. Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082216 Minefield/3.0a8pre ID:2007082216, the crashes have gone away. It seems like 393191 was the problem. Thanks, Seth.

>> Seems like we invalidate the entire tree in ProcessResult() > I'll log a follow up bug on this, but I think there is another bug that this > would also address. spun that issue off to bug #393902