*** |_addMatch| is used in three places: |_onResultRow|; "recursively" for frecnecy (see below); and at the end of |execute| to pick up fallback frecency results. However, it throws |StopIteration| (necessary for the |_onResultsRow| case) when there are enough item in places. It seems it's |_onResultRow| that should throw |StopIteration|. *** // We keep this array in reverse order, so we can walk it and remove stuff // from it in one pass. Notice that for frecency reverse order means from // lower to higher. this._frecencyMatches.sort((a, b) => a.frecency - b.frecency); ... if (this._frecencyMatches) { for (let i = this._frecencyMatches.length - 1; i >= 0 ; i--) { if (this._frecencyMatches[i].frecency > match.frecency) { this._addMatch(this._frecencyMatches.splice(i, 1)[0]); } } } So: 1) What's done here isn't trivial ("Yes, here comes a result, but maybe better results came from some other place while we were waiting for this one, so let's do them first"). Please document the logic. 2) no real harm, but, |_addMatch| for a frecency match also iterates the frecnecy matches array. 3) It seems the only thing that stops this from generating too many results is |StopIteration|. Keep that in mind when addressing the previous issue. 4) I think that a while-loop + Array.pop usage would make this code much easier to follow. 5) The array is sorted, so the iteration-continuation can be conditioned. *** // If we didn't find enough matches and we have some frecency-driven // matches, add them. if (this._frecencyMatches) { this._frecencyMatches.forEach(this._addMatch, this); } Hmm, |_frecencyMatches| is lower-to-higher, so this seems wrong, but it isn't breaking anything due to the "recursive" |_addMatch| for frecency matches. *** With all the above in mind, |_addFrecencyMatch| needs a better name. Or maybe it's |_addMatch|. *** /** * Used to cancel this search, will stop providing results. */ cancel: function () { if (this._sleepTimer) this._sleepTimer.cancel(); if (this._sleepDeferred) { this._sleepDeferred.resolve(); this._sleepDeferred = null; } delete this._pendingQuery; }, /** * Whether this search is running. */ get pending() !!this._pendingQuery, ... if (this._result.matchCount == Prefs.maxRichResults || !this.pending) { // We have enough results, so stop running our search. this.cancel(); // This tells Sqlite.jsm to stop providing us results and cancel the // underlying query. throw StopIteration; } 1) |cancel| is misleading both by its name and by its documentation. 2) I'm not sure I understand then "!this.pending" check. |!pending| is possible only once cancel is called.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #0) > *** > |_addMatch| is used in three places: |_onResultRow|; "recursively" for > frecnecy (see below); and at the end of |execute| to pick up fallback > frecency results. However, it throws |StopIteration| (necessary for the > |_onResultsRow| case) when there are enough item in places. It seems it's > |_onResultRow| that should throw |StopIteration|. this must be addressed, it looks indeed wrong. > ... > if (this._result.matchCount == Prefs.maxRichResults || !this.pending) { > // We have enough results, so stop running our search. > this.cancel(); > // This tells Sqlite.jsm to stop providing us results and cancel the > // underlying query. > throw StopIteration; > } > > 1) |cancel| is misleading both by its name and by its documentation. > 2) I'm not sure I understand then "!this.pending" check. |!pending| is > possible only once cancel is called. We have a pending check now at the beginning of the method so this can be removed. The other comments are obsolete since _addFrecencyMatch has gone

Hi Marco, can you provide a point value.

I didn't rename cancel cause I don't have better ideas... Though now it should be clearer and it is doing the expected thing.

Comment on attachment 8497128 [details] [diff] [review] patch v1 Review of attachment 8497128 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/UnifiedComplete.js @@ +635,2 @@ > */ > cancel: function () { s/can/may/ @@ +1058,2 @@ > // We have enough results, so stop running our search. > + // We don't need to notifyResults in this case, cause the main promise to call notifyResult / to notify results