Closed Bug 1007979 Opened 10 years ago Closed 10 years ago

refactor nsSearchSuggestions to use a reusable JSM

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.1
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: mconnor, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 16 obsolete files)

55.79 KB, patch
MattN
: review+
Details | Diff | Splinter Review
Attached patch newSuggestAPI (obsolete) — Splinter Review
To avoid duplicating a bunch of this code in order to fix bug 612453 (and a future, similar bug for the new about:newtab implementation), we should have a single implementation for returning a mix of local and remote suggestions for various consumers and UIs.

First part of this bug is creating a new SearchSuggest.jsm containing all of the logic.

As a note, I've simplified the backoff behaviour substantially (the old logic was... weird).  This isn't quite exponential backoff, but we can discuss it (it's pretty isolated from the rest of the JSM, so we can tweak it as a followup).  I've also trimmed out a bunch of cruft, and made a bunch of pseudo-consts into real consts to make it obvious what's going on.
Attachment #8419782 - Flags: feedback?(MattN+bmo)
This guts nsSearchSuggestions and leaves the logic for SearchSuggest.jsm to control.
Attachment #8419784 - Flags: feedback?(MattN+bmo)
Attachment #8419784 - Attachment is patch: true
Comment on attachment 8419784 [details] [diff] [review]
use SearchSuggest.jsm for the searchbar

This is missing the new module.

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>+ * We do it this way since the AutoCompleteController in Mozilla requires a 
>+ * unique XPCOM Service for every search provider, even if the logic for two 
>+ * providers is identical.

I have no idea what this comment is referring to - I think it's just wrong. SearchSuggestAutoComplete and SuggestAutoComplete are both singletons AFAICT.

>+  onResultsReturned: function(results) {

>+    // If form history has results, add them to the list.
>+    if (results.local) {

>         // we don't want things to appear in both history and suggestions
>-        var dupIndex = results.indexOf(term);
>+        var dupIndex = results.remote.indexOf(term);

Shouldn't this logic live in SearchSuggest.jsm?

>+  onResultsReady: function(searchString, results, comments) {

>-      var result = new FormAutoCompleteResult(
>+      let result = new FormAutoCompleteResult(

>-          formHistoryResult);
>+          null);

IIRC not passing in the previous result might lead to inefficiencies somewhere? I forget, would need to look it up.
Oh, missed the separate patch.

Bad form to combine a refactor with a behavior change. Can you split those patches up and file a new bug to simplify backoff?
Yeah, I don't know if that comment is still correct.  I just tweaked the wording to leave implementation details to SearchSuggest.jsm.

The de-duping does exist in SearchSuggest.jsm, I'll clean that up.

Passing the previous result doesn't change perf based on the JSM implementation, it just appends the previous results to the result object as .entries.  In the older version it was a shortcut to return a subset of the previous entries, which likely was a bigger deal in the bad old days of Mork.  Note that the current comment says it's unused, and I can't find anything on the caller side caching it.  I might be missing something in the indirection.

I can restore the old, oddly behaved backoff code.  I'm actually thinking we should implement something closer to Google's current docs anyway.  Fodder for a followup.
Attached patch newSuggestAPI (obsolete) — Splinter Review
Addressing gavin's comments, a few other tweaks.  Time to go under the microscope.
Attachment #8419782 - Attachment is obsolete: true
Attachment #8419782 - Flags: feedback?(MattN+bmo)
Attachment #8422125 - Flags: review?(MattN+bmo)
Attached patch useNewSuggestAPISearchbar (obsolete) — Splinter Review
And part 2.
Attachment #8419784 - Attachment is obsolete: true
Attachment #8419784 - Flags: feedback?(MattN+bmo)
Attachment #8422126 - Flags: review?(MattN+bmo)
(In reply to Mike Connor [:mconnor] from comment #4)
> Passing the previous result doesn't change perf based on the JSM
> implementation, it just appends the previous results to the result object as
> .entries.  In the older version it was a shortcut to return a subset of the
> previous entries, which likely was a bigger deal in the bad old days of
> Mork.  Note that the current comment says it's unused, and I can't find
> anything on the caller side caching it.  I might be missing something in the
> indirection.

I was confused about which "previous result" this was - it's the one passed to FormAutoCompleteResult, not the one passed to nsIAutoCompleteSearch::startSearch.

FormAutoCompleteResult uses it to forward removeEntryAt calls. It looks like your change will break that, thus breaking the ability to delete form history entries from searches that also have suggestions?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #7)
> I was confused about which "previous result" this was - it's the one passed
> to FormAutoCompleteResult, not the one passed to
> nsIAutoCompleteSearch::startSearch.

(it's actually not a "previous result" at all, it's the underlying result object for the form history portion of the current results)
Attached patch newSuggestAPI (obsolete) — Splinter Review
Good catch from Gavin, the "prevResult" naming in nsFormAutoCompleteResult.jsm threw me off.  Deletion now works again!
Attachment #8422125 - Attachment is obsolete: true
Attachment #8422125 - Flags: review?(MattN+bmo)
Attachment #8422630 - Flags: review?(MattN+bmo)
Attached patch useNewSuggestAPISearchbar (obsolete) — Splinter Review
matching patch.
Attachment #8422126 - Attachment is obsolete: true
Attachment #8422126 - Flags: review?(MattN+bmo)
Attachment #8422631 - Flags: review?(MattN+bmo)
Blocks: 1010561
I wouldn't be surprised if this breaks some extensions.
Keywords: addon-compat
Comment on attachment 8422630 [details] [diff] [review]
newSuggestAPI

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

OOC, why is code movement done over two patches? It makes it harder to review. Please combine them into one before landing so it's an atomic commit that will help code archaeology.

It seems like there are still backoff changes here e.g. _serverErrorEngine and _nextRequestTime code are gone. Is that intentional? I was hoping this was just code movement and refactor, not other changes.

I don't think the SearchSuggest object should be a singleton since it seems reasonable to want to get suggestions from multiple providers at once but now they would cancel each other's requests and have messed up state variables. In fact, I think Bryan experimented on this at one point.

Overall, splitting this code out into definitely makes sense.

::: toolkit/components/search/SearchSuggest.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// "use strict";

Did you find something that didn't work in strict mode or were you being conservative?

@@ +15,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;
> +const Cu = Components.utils;

Nit: nowadays you can do this easily on a single line:
const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Note that Cr isn't even used so can be omitted.

@@ +23,5 @@
> +const HTTP_BAD_GATEWAY           = 502;
> +const HTTP_SERVICE_UNAVAILABLE   = 503;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/nsFormAutoCompleteResult.jsm");

A quick find-in-page indicates this may not be used. Correct me if I'm wrong.

@@ +43,5 @@
> +this.SearchSuggest = {
> +  /**
> +   * Record errors, and stop making requests when the server is failing
> +   */
> +  _serverErrorLog: [],

I thought the existing comment that you nuked was useful.

@@ +46,5 @@
> +   */
> +  _serverErrorLog: [],
> +  _nextRequest: 0,
> +  _serverErrorTimeout: 0,
> +  _resetStatus: function () {

Put a newline to separate the primitives from the functions.

I think this function name is quite ambiguous in a SearchSuggest object. The old name more clearly indicated this was about server errors.

@@ +51,5 @@
> +    this._serverErrorLog = [];
> +    this._nextRequest = 0;
> +    this._serverErrorTimeout = 0;
> +  },
> +  /**

Nit: missing newline. I'm not sure if you're trying to group the error handling stuff together without spaces. It may be cleaner to put the error handling properties/methods on a new object (either top-level or as a property on this object). Splitting all the networking and autocomplete stuff on separate objects would be nice.

@@ +70,5 @@
> +                                 SERVER_TIMEOUT_INCREMENT;
> +      this._nextRequest = currentTime + this._serverErrorTimeout;
> +    }
> +  },
> +  _canMakeRequest: function () {

missing newline here too and many other places below

@@ +79,5 @@
> +   * The XMLHttpRequest object.
> +   * @private
> +   */
> +  _request: null,
> +  

Nit: trailing whitespace in a few places

@@ +94,5 @@
> +      this._request.abort();
> +    }
> +    this._reset();
> +  },
> +  _reset: function () {

Nit: I'm not a fan of leaving spaces between "function" and "(" as it always seems like there's something missing to me. Most code I see doesn't do that anymore.

@@ +108,5 @@
> +  },
> +
> +  // callers can change these values to get bigger or smaller result sets
> +  localResults: 7,
> +  remoteResults: 10,

Um, I don't think it's a good idea to have these shared as essentially global state by all consumers.

@@ +270,5 @@
> +    formHistory.startSearch(searchString, searchParam, this._formHistoryResult, this);
> +  },
> +  _formHistoryEntries: [],
> +  _formHistoryTimer: null,
> +  _formHistoryResult: null,

Nit: We normally put non-function properties at the top of the object. If you're doing this to keep them close to related code, it's another point in favour of splitting this into multiple objects.
Attachment #8422630 - Flags: review?(MattN+bmo) → review-
Comment on attachment 8422630 [details] [diff] [review]
newSuggestAPI

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

::: toolkit/components/search/SearchSuggest.jsm
@@ +9,5 @@
> +const SEARCH_RESPONSE_SUGGESTION_JSON = "application/x-suggestions+json";
> +
> +const BROWSER_SUGGEST_PREF              = "browser.search.suggest.enabled";
> +const XPCOM_SHUTDOWN_TOPIC              = "xpcom-shutdown";
> +const NS_PREFBRANCH_PREFCHANGE_TOPIC_ID = "nsPref:changed";

I think the above three consts are unused.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Comment on attachment 8422631 [details] [diff] [review]
useNewSuggestAPISearchbar

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

There's still quite a bit to review in both patches since stuff was rewritten.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +8,5 @@
>  
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  const Cu = Components.utils;

Nit: I wouldn't mind if you switched these C* consts to the newer object destructuring syntax since the blame on these lines isn't important.

@@ +20,5 @@
>  /**
>   * SuggestAutoComplete is a base class that implements nsIAutoCompleteSearch
> + * and can collect results for a given search by using SearchSuggest.jsm
> + * We do it this way since the AutoCompleteController in Mozilla requires a 
> + * unique XPCOM Service for every search provider, even if the logic for two 

Nit: trailing whitespace and I believe Gavin's comment that this is incorrect wasn't addressed.
Attachment #8422631 - Flags: feedback+
Comment on attachment 8422631 [details] [diff] [review]
useNewSuggestAPISearchbar

I think it makes sense to wait for a new patch and replies
Attachment #8422631 - Flags: review?(MattN+bmo)
Attached patch newSuggestAPI (obsolete) — Splinter Review
unified patch, comments addressed (except for the nsIAutoComplete one).  To the best of my understanding, what that comment was trying to imply is that we have to register as a unique implementation for the autocomplete code to pick up the right implementation.  I'll figure out some wording that's more accurate, but I don't think I wrote the comment (it might have been inherited from some code Ben Goodger wrote...)
Attachment #8422630 - Attachment is obsolete: true
Attachment #8422631 - Attachment is obsolete: true
Attachment #8425933 - Flags: review?(MattN+bmo)
Comment on attachment 8425933 [details] [diff] [review]
newSuggestAPI

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

In manual testing, it's apparent something is broken with the form history results. The form history results only intermittently appears with this patch but appeared fine without it.

::: toolkit/components/search/SearchSuggest.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["SearchSuggest"];
> +
> +const SEARCH_RESPONSE_SUGGESTION_JSON = "application/x-suggestions+json";
> +
> +const DEFAULT_FORM_HISTORY_PARAM        = "searchbar-history";

Nit: align "="

@@ +40,5 @@
> +this.SearchSuggest.prototype = {
> +  /**
> +   * Record errors, and stop making requests when the server is failing
> +   */
> +  _serverErrorLog: [],

I still think the comment describing the data in this array is useful

@@ +41,5 @@
> +  /**
> +   * Record errors, and stop making requests when the server is failing
> +   */
> +  _serverErrorLog: [],
> +  _nextRequest: 0,

…along with describing what this number is for.

@@ +79,5 @@
> +   * The XMLHttpRequest object.
> +   * @private
> +   */
> +  _request: null,
> +  

Nit: trailing space

@@ +136,5 @@
> +    }
> +    if (!this.localResults && !this.remoteResults) {
> +      throw("Zero results expected, what are you trying to do?");
> +    }
> +    

ditto

@@ +214,5 @@
> +
> +    if (this._searchString != serverResults[0]) {
> +      // something is wrong here, bail out and just return local results
> +      Cu.reportError("Unexpected response, this._searchString does not match API response");
> +            

ditto

::: toolkit/components/search/nsSearchSuggestions.js
@@ +20,5 @@
>  /**
>   * SuggestAutoComplete is a base class that implements nsIAutoCompleteSearch
> + * and can collect results for a given search by using this._suggestjsm
> + * We do it this way since the AutoCompleteController in Mozilla requires a 
> + * unique XPCOM Service for every search provider, even if the logic for two 

Nit: trailing whitespace
Attachment #8425933 - Flags: review?(MattN+bmo) → review-
This refactor would be a great opportunity to write some SearchSuggest.jsm unit tests!
/me mumbles something about gift horses and mouths and starts thinking about how to unit test this sanely.
Comment on attachment 8425933 [details] [diff] [review]
newSuggestAPI

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

(In reply to Matthew N. [:MattN] from comment #17)
> In manual testing, it's apparent something is broken with the form history
> results. The form history results only intermittently appears with this
> patch but appeared fine without it.

Weird, I can't reproduce this anymore.

I'll try finish reviewing after dinner.

::: toolkit/components/search/SearchSuggest.jsm
@@ +44,5 @@
> +  _serverErrorLog: [],
> +  _nextRequest: 0,
> +  _serverErrorTimeout: 0,
> +
> +  _resetStatus: function () {

I still think the error handling should be in a (sub-)object to keep it self-contained. Having both _resetStatus and _reset is confusing.

@@ +265,5 @@
> +  _formHistoryEntries: [],
> +  _formHistoryTimer: null,
> +  _formHistoryResult: null,
> +  /**
> +   * This is the callback for that the form history service uses to

s/for//
Comment on attachment 8425933 [details] [diff] [review]
newSuggestAPI

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

Where did the engine switching logic (e.g. checkForEngineSwitch) go? I think the backoff should be for a specific engine. r- for that.

I realize some feedback is on code that was copied.

I agree that tests would be nice, especially since this wasn't just moving code around.

::: toolkit/components/search/SearchSuggest.jsm
@@ +126,5 @@
> +    // here as there is when looking through history/form data because the
> +    // result set returned by the server is different for every typed value -
> +    // "ocean breathes" does not return a subset of the results returned for
> +    // "ocean", for example. This does nothing if there is no current request.
> +    this.stop();

Nit: newline above for readability

@@ +156,5 @@
> +        this._request.channel.setPrivate(privateMode);
> +      }
> +      this._request.mozBackgroundRequest = true; // suppress dialogs and fail silently
> +
> +      this._request.onreadystatechange = () => this.onReadyStateChange();

this.onReadyStateChange.bind(this); seems clearer to me. I almost asked you to remove this extra function until I remembered about |this| working differently with arrow functions.

@@ +170,5 @@
> +  },
> +
> +  /**
> +   * Called when the 'readyState' of the XMLHttpRequest changes. We only care
> +   * about state 4 (COMPLETED) - handle the response data.

Nit: replace "state 4 (COMPLETED)" with "the request when it's done" as the "4" is an XHR implementation detail

@@ +173,5 @@
> +   * Called when the 'readyState' of the XMLHttpRequest changes. We only care
> +   * about state 4 (COMPLETED) - handle the response data.
> +   * @private
> +   */
> +  onReadyStateChange: function() {

Since this is private, prefix with an underscore.

@@ +180,5 @@
> +
> +    try {
> +      var status = this._request.status;
> +    } catch (e) {
> +      // The XML HttpRequest can throw NS_ERROR_NOT_AVAILABLE.

Nit: s/XML HttpRequest/XMLHttpRequest/

@@ +205,5 @@
> +
> +    this._resetStatus();
> +
> +    try {
> +      var serverResults = JSON.parse(responseText);

I filed bug 1016808 to change this XHR to use responseType = "json" so this._request.response would already be JSON.

@@ +221,5 @@
> +
> +    // If form history is enabled and has results, add them to the list.
> +    if (this.localResults && this._formHistoryEntries) {
> +      for (let i = 0; i < this._formHistoryEntries.length; ++i) {
> +        let term = this._formHistoryEntries[i];

These two lines can be replaced with:
  for (let term of this._formHistoryEntries) {
while we're cleaning up and writing tests ;-)

@@ +256,5 @@
> +    }
> +  },
> +
> +  _startHistorySearch: function (searchString, searchParam) {
> +    var formHistory =

May as well get rid of the remaining few |var| in this file while you're at it.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +32,4 @@
>    _init: function() {
>      this._addObservers();
>      this._suggestEnabled = Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF);
> +    this._suggest = new SearchSuggest(obj => this.onResultsReturned(obj));

Here too I think this.onResultsReturned.bind(this) is more clear but maybe this is a pattern for arrow functions that I'm not understanding. In this case, I think there is only one caller for onResultsReturned so making a wrapper function for it feels weird to me.

@@ +82,5 @@
> +    if (results.remote && results.remote.length) {
> +      let comments = [];  // "comments" column values for suggestions
> +      // fill out the comment column for the suggestions
> +      for (let i = 0; i < results.remote.length; ++i)
> +        comments.push("");

Since you're touching this, you can convert it to JS of the future with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill although you'd be one of the first users in browser/. (you could do the same for finalComments above too).
Flags: firefox-backlog+
Assignee: mconnor → nobody
Iteration: --- → 33.2
Points: --- → 3
QA Whiteboard: [qa+]
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
final version, has some review comments addressed, MattN and I discussed and agreed on dropping backoff entirely (other browsers don't back off, we're just hurting ourselves here, really).  Would have been cleaner as a pre-req patch, but not worth it at this point, IMO.
Iteration: 33.2 → 33.3
* Addressed remaining review comments
* removed backoff
* moved member variables to top of SearchSuggest instead of mixing them in with methods
* re-ordered search arguments since engine was optional and optional args should be last. I made it not optional for now since I'm not sure if this is a good idea.
* Switch from readystatechange to newer load event that handles state changes and network errors for us.

Tests will be in a separate patch.
Attachment #8425933 - Attachment is obsolete: true
Attachment #8447361 - Attachment is obsolete: true
Attachment #8459968 - Flags: review?(adw)
Iteration: 33.3 → 34.1
Keywords: dev-doc-needed
Promises make testing much easier.
Attachment #8459968 - Attachment is obsolete: true
Attachment #8459968 - Flags: review?(adw)
Attachment #8460240 - Flags: review?(adw)
Attached patch Tests - WIP (obsolete) — Splinter Review
Attachment #8460242 - Flags: feedback?(adw)
Comment on attachment 8460240 [details] [diff] [review]
v.7 Promise.all to fix execution order issue & remove more state properties

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

::: toolkit/components/search/SearchSuggest.jsm
@@ +22,5 @@
> + * search suggestions from a given engine, regardless of the base implementation. Much of this
> + * code was originally in nsSearchSuggestions.js until it was refactored to separate it from the
> + * nsIAutoCompleteSearch dependency.
> + * For the best results, instances of SearchSuggest should not be shared between fields since
> + * form history results are cached.

This should also say that one SearchSuggest instance should be used per field, so it's a one-to-one mapping.

@@ +37,5 @@
> +
> +this.SearchSuggest.prototype = {
> +  // Callers can change these values to get bigger or smaller result sets.
> +  localResults: 7,
> +  remoteResults: 10,

Nit: would be great to name these {local,remote}ResultsCount or num{Local,Remote}Results.  It's pretty confusing at first since you would expect these to be the results themselves, not numbers.

@@ +39,5 @@
> +  // Callers can change these values to get bigger or smaller result sets.
> +  localResults: 7,
> +  remoteResults: 10,
> +
> +  // Private properties

Nit: To make it easier for people writing clients to scan the code, list the private properties after public methods and properties.  I only mention it because you mentioned "moved member variables to top of SearchSuggest instead of mixing them in with methods."  Feel free to ignore.

@@ +61,5 @@
> +  /**
> +   * The main API for SearchSuggest
> +   *
> +   * @param searchTerm
> +   *        the term to provide suggestions for

Nit: This method comment looks different from the others: it doesn't have param {types}, and the param descriptions don't start on the same line as the @params.

@@ +79,5 @@
> +
> +    this.stop();
> +
> +    if (!Services.search.isInitialized) {
> +      throw("Search not initialized yet (how did you get here?)");

Throw new Error()s here so that they're real Errors with stacks.

@@ +134,5 @@
> +      onSearchResult: function(search, result) {
> +        that._formHistoryResult = result;
> +        switch (result.searchResult) {
> +          case Ci.nsIAutoCompleteResult.RESULT_SUCCESS:
> +          case Ci.nsIAutoCompleteResult.RESULT_NOMATCH:

Is nsIAutoCompleteResult.RESULT_IGNORED also possible?

@@ +160,5 @@
> +                            acSearchObserver);
> +    return deferredFormHistory.promise;
> +  },
> +
> +  _startRequest: function(searchTerm, engine, privateMode) {

Nit: "request" is pretty generic.  _startRemoteSearch (to balance _startHistorySearch) or something similar would be better.

@@ +166,5 @@
> +    this._request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
> +                    createInstance(Ci.nsIXMLHttpRequest);
> +    let submission = engine.getSubmission(searchTerm,
> +                                          SEARCH_RESPONSE_SUGGESTION_JSON);
> +    this._suggestURI = submission.uri;

Nit: _suggestURI is too generic as well.

@@ +173,5 @@
> +    if (this._request.channel instanceof Ci.nsIPrivateBrowsingChannel) {
> +      this._request.channel.setPrivate(privateMode);
> +    }
> +    this._request.mozBackgroundRequest = true; // suppress dialogs and fail silently
> +    this._request.timeout = SUGGESTION_TIMEOUT;

This whole module is about suggestions, so a better name for this constant would be REMOTE_SEARCH_TIMEOUT or something similar.

@@ +180,5 @@
> +    this._request.addEventListener("error", (evt) => deferredResponse.resolve("HTTP error"));
> +    this._request.addEventListener("timeout", (evt) => deferredResponse.resolve("HTTP timeout"));
> +    // Reject for an abort assuming it's always from .stop() in which case we shouldn't return local
> +    // or remote results for existing searches.
> +    this._request.addEventListener("abort", (evt) => deferredResponse.reject("HTTP request aborted"));

stop()ing the suggestions aborts an outstanding remote request, which rejects the remote request promise here, which means that the client callback isn't called -- but if there's only an outstanding form history search, it's not aborted and there's no rejected promise, so the client callback ends up getting called.  It looks like that's the current behavior, so it's not really the fault of this patch, but ideally the behavior would be consistent for both types of searches.

@@ +253,5 @@
> +    }
> +
> +    // We don't want things to appear in both history and suggestions so remove entries from
> +    // remote results that are alrady in local.
> +    if (results.remote && results.local) {

Looks like this will always be true: both properties are initialized to [] above, and when those properties are overwritten in the previous block, `result.result` is always an array in both the remote and local cases.

@@ +264,5 @@
> +      }
> +    }
> +
> +    // Trim the number of results to the max requested (now that we've pruned dupes).
> +    results.remote = results.remote.splice(0, this.remoteResults);

Nit: slice() is much better suited to this than splice().  You're setting results.remote, so there's no need to remove from that same array first on the right-hand side.  If you want to use splice, then results.remote.splice(this.remoteResults); would be a more faithful use.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +19,4 @@
>  
>  /**
>   * SuggestAutoComplete is a base class that implements nsIAutoCompleteSearch
> + * and can collect results for a given search by using this._suggest

Nit: need a period at the end of the sentence.

@@ +71,3 @@
>  
> +    // If form history has results, add them to the list.
> +    if (results.local && results.local.length) {

Nit: the results.local.length term isn't necessary.
Attachment #8460240 - Flags: review?(adw) → review+
Comment on attachment 8460242 [details] [diff] [review]
Tests - WIP

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

Looks good.

::: toolkit/components/search/tests/xpcshell/data/searchSuggestions.sjs
@@ +7,5 @@
> +
> +function handleRequest(request, response) {
> +  // Get the query parameters from the query string.
> +  let query = {};
> +  request.queryString.split('&').forEach(function (val) {

I find it's easier to make the query string be JSON than to do manual parsing like this.

::: toolkit/components/search/tests/xpcshell/test_searchSuggest.js
@@ +68,5 @@
> +check capping of local #
> +check capping of remote #
> +check de-duping
> +search term escaping
> +*/

Ought to check private mode, too, if possible.

@@ +83,5 @@
> +  suggest.search("no remote", false, testEngine);
> +  yield deferred.promise;
> +});
> +
> +add_task(function* simple_no_result_callback_and_promise() {

This function covers the simple_no_result_callback and simple_no_result_promise cases, so I think the latter two can be removed without loss of coverage.  Or, this function can be removed, which would probably be better in order to keep these scoped small.
Attachment #8460242 - Flags: feedback?(adw) → feedback+
Attached patch v.8 Review comments addressed (obsolete) — Splinter Review
I made enough changes that you may want to look over it again.
Attachment #8460240 - Attachment is obsolete: true
Attachment #8460530 - Flags: review?(adw)
Oops, I left some dump() in my patch. Please ignore.

(In reply to Drew Willcoxon :adw from comment #27)
> Comment on attachment 8460242 [details] [diff] [review]
> Tests - WIP
> 
> Review of attachment 8460242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: toolkit/components/search/tests/xpcshell/data/searchSuggestions.sjs
> @@ +7,5 @@
> > +
> > +function handleRequest(request, response) {
> > +  // Get the query parameters from the query string.
> > +  let query = {};
> > +  request.queryString.split('&').forEach(function (val) {
> 
> I find it's easier to make the query string be JSON than to do manual
> parsing like this.

Good idea. I think you still need to deal with decoding so I'm not sure it saves much. I got this from many other tests in m-c.

> ::: toolkit/components/search/tests/xpcshell/test_searchSuggest.js
> @@ +68,5 @@
> > +check capping of local #
> > +check capping of remote #
> > +check de-duping
> > +search term escaping
> > +*/
> 
> Ought to check private mode, too, if possible.

True. I'll try figure out if that's possible in xpcshell.

> @@ +83,5 @@
> > +  suggest.search("no remote", false, testEngine);
> > +  yield deferred.promise;
> > +});
> > +
> > +add_task(function* simple_no_result_callback_and_promise() {
> 
> This function covers the simple_no_result_callback and
> simple_no_result_promise cases, so I think the latter two can be removed
> without loss of coverage.  Or, this function can be removed, which would
> probably be better in order to keep these scoped small.

I wanted it to make sure both the callback and promise got results.
Attachment #8460530 - Attachment is obsolete: true
Attachment #8460530 - Flags: review?(adw)
Attachment #8460550 - Flags: review?(adw)
(In reply to Matthew N. [:MattN] from comment #29)
> Good idea. I think you still need to deal with decoding so I'm not sure it
> saves much. I got this from many other tests in m-c.

The client has to encodeURIComponent and the server has to decodeURIComponent, but it saves constructing and destructing query strings with &'s and ='s, which is the main thing.
Attachment #8460550 - Flags: review?(adw) → review+
Attached patch v.9 Folded with tests (obsolete) — Splinter Review
This now depends on two helpers from bug 1041534 but I can fork those temporarily if this needs to land before that does.
Attachment #8460242 - Attachment is obsolete: true
Attachment #8460550 - Attachment is obsolete: true
Attachment #8461392 - Flags: review?(adw)
Removing add-on compat since we are still implementing nsIAutoCompleteSearch and the behaviour hasn't really changed significantly to the outside.

dev-doc-needed for the new module which add-ons and other default UI can use for search suggestions (including search history)

QA work would involve making sure search suggestions (from the provider [on supporting engines] and search history) continue to work in the search box.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=fd8fdca9af5a
Depends on: 1041534
Keywords: addon-compat
Comment on attachment 8461392 [details] [diff] [review]
v.9 Folded with tests

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

feedback+, I'll wait for the new patch for r+.
Attachment #8461392 - Flags: review?(adw) → feedback+
Based on discussion with Gavin and Drew, we decided to stick with the existing behaviour even though it adds some complexity because it's unclear what the best behaviour is.

I hear the 10th patch is the charm…
Attachment #8461392 - Attachment is obsolete: true
Attachment #8462920 - Flags: review?(adw)
Comment on attachment 8462920 [details] [diff] [review]
v.10 Switch back to starting the timeout after form history results

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

Yay!  But it looks like the test is still failing on debugs.

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +53,5 @@
> +  _formHistoryResult: null,
> +
> +  /**
> +   * The remote server timeout timer, if applicable. The timer starts when form history
> +   * results are retrieved.

Nit: "Are retrieved" is kind of ambiguous because it could also mean when formHistory.startSearch is called.  "Are ready", "are complete", or "are returned from the autocomplete search"?

@@ +152,5 @@
> +    let deferredFormHistory = Promise.defer();
> +    let that = this;
> +    let acSearchObserver = {
> +      // Implements nsIAutoCompleteSearch
> +      onSearchResult: function(search, result) {

If you use an arrow function here instead (or bind this function to `this`), you should be able to reference `this` inside and have it refer to the suggestion controller.  I didn't test with your patch, but I tested with a similar case in a JS shell.  It makes sense that it would work since the `this` in the lexical scope of the onSearchResult function is the controller, not acSearchObserver.

@@ +171,5 @@
> +            let maxHistoryItems = Math.min(result.matchCount, that.maxLocalResults);
> +            for (let i = 0; i < maxHistoryItems; ++i) {
> +              fhEntries.push(result.getValueAt(i));
> +            }
> +            deferredFormHistory.resolve({

Did we decide to reject the promise here if the result's search string doesn't match this._searchString?  (To avoid the case where stop is called before form history finishes and there's no remote request, so the client ends up getting notified... at least I think that was the case.)  I don't mean to add more work, but maybe instead you could keep a reference to the deferredFormHistory like you do in the remote case and reject it when stop is called.  Anyway, if you like that idea, a follow-up is fine.

@@ +207,5 @@
> +      this._request.channel.setPrivate(privateMode);
> +    }
> +    this._request.mozBackgroundRequest = true; // suppress dialogs and fail silently
> +
> +    this._request.addEventListener("load", this._onRemoteLoaded.bind(this, deferredResponse));

Nit: You could use an arrow function here to be consistent with the other listeners.
Attachment #8462920 - Flags: review?(adw) → review+
QA Contact: petruta.rasa
I fixed the simple test xpcshell failure so this should be good to land.
Attachment #8462920 - Attachment is obsolete: true
Attachment #8463699 - Flags: review+
Points: 3 → 5
Flags: in-testsuite+
Keywords: checkin-needed
sorry had to back this out for crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=44791112&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
(In reply to Carsten Book [:Tomcat] from comment #40)
> sorry had to back this out for crashes like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44791112&tree=Fx-Team

Sorry, I accidentally uploaded the unfixed version of the patch.

Re-pushed: https://hg.mozilla.org/integration/fx-team/rev/54d57bd38f51
Attachment #8463699 - Attachment is obsolete: true
Attachment #8464148 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/54d57bd38f51
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
(In reply to Matthew N. [:MattN] from comment #33)
> QA work would involve making sure search suggestions (from the provider [on
> supporting engines] and search history) continue to work in the search box.

I performed a small smoke using 2014-07-31 Nightly 34.0a1 under Win 7 64-bit, Ubuntu 12.10 32-bit and Mac OSX 10.9.4  to make sure nothing has regressed and found no issues. Will continue testing more deeply on Monday.

Seems like bug 1046214 looks better too, I'll update them both next week.
Blocks: 1046943
Depends on: 1048198
Marking as verified considering that the new issues (such as bug 1048198) will be tracked separately.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Comment on attachment 8464148 [details] [diff] [review]
v.11 Fix the shutdown failure. r=adw

This is applies cleanly to Aurora/33.

Bug 1054516 is the uplift bug for this feature.

mozilla-aurora try push with this and patches for other bugs mentioned in bug 1054516: https://tbpl.mozilla.org/?tree=Try&rev=fba51f37ff40

Approval Request Comment
[Feature/regressing bug #]:
this bug is needed to uplift search suggestions in about:home/newtab (bug 612453, bug 1028985)

[User impact if declined]:
no search suggestions in about:home/newtab

[Describe test coverage new/current, TBPL]:
currently covered by automated tests on 34; this patch adds tests to 33; see also try link above

[Risks and why]:
moderate risk; this modifies how search suggestions are retrieved, which affects suggestions shown in the main search box in chrome; we want about:home/newtab search suggestions on 33 as stated in bug 1054516

[String/UUID change made/needed]:
none
Attachment #8464148 - Flags: approval-mozilla-aurora?
Attachment #8464148 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Performed search smoke-tests using latest Aurora 33.0a2 (20140828004002) under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.8.5. Marking as verified considering this is also covered by automation tests.
Depends on: 1060675
Depends on: 1060846
Depends on: 1083990
Depends on: 1113561
Matthew, you added mozBackgroundRequest after xhr.open(). It caused test failures in bug 1132347. Note that mozBackgroundRequest after xhr.open() has never had effect from the start. Bug 1132347 just made it explicit.
Why did you add it? Is mozBackgroundRequest needed here?
Flags: needinfo?(MattN+bmo)
See Also: → 1132347
(In reply to Masatoshi Kimura [:emk] from comment #50)
> Matthew, you added mozBackgroundRequest after xhr.open(). It caused test
> failures in bug 1132347. Note that mozBackgroundRequest after xhr.open() has
> never had effect from the start. Bug 1132347 just made it explicit.
> Why did you add it? 

I was porting https://hg.mozilla.org/releases/mozilla-aurora/rev/e2efba644fb3#l3.477 and didn't realize that mozBackgroundRequest had to be used before .open().

> Is mozBackgroundRequest needed here?

Yes, we don't want dialogs popping up from this request.

I will r+ a fix in bug 1132347 if you want.
Flags: needinfo?(MattN+bmo)
Depends on: 1176381
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: