This patch removes some code duplication in the search service tests, in preparation for the addition of new tests in bug 1040721. It also fixes a failure resulting from two of the tests not having been converted to use a dynamic HTTP port.

Comment on attachment 8459589 [details] [diff] [review] The patch Review of attachment 8459589 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. It's something that has bothered me in the past. I would also like to use it for bug 1007979 which would mean uplifting to 33. r=me assuming this passes Try. ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +167,5 @@ > + let httpServer = new HttpServer(); > + httpServer.start(-1); > + httpServer.registerDirectory("/", do_get_cwd()); > + gDataUrl = "http://localhost:" + httpServer.identity.primaryPort + "/data/"; > + do_register_cleanup(() => httpServer.stop(() => {})); Nit: Could you return the server from the function so that consumers can register other things e.g. registerContentType @@ +185,5 @@ > + * except for the engine name. Alternative to xmlFileName. > + * } > + */ > +function addTestEngines(aItems) { > + return new Promise((resolve, reject) => { Ideally this function would also register a cleanup function to remove the engines after. We currently delete leftovers at the beginning of every test but we shouldn't rely on that.

Comment on attachment 8459589 [details] [diff] [review] The patch Review of attachment 8459589 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +224,5 @@ > + Services.search.addEngine(gDataUrl + item.srcFileName, > + Ci.nsISearchEngine.DATA_TEXT, > + gDataUrl + item.iconFileName, false); > + } else { > + Services.search.addEngineWithDetails(item.name, ...item.details) Nit: missing semicolon.

Comment on attachment 8459589 [details] [diff] [review] The patch Review of attachment 8459589 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +217,5 @@ > + > + for (let item of aItems) { > + do_print("Adding engine: " + item.name); > + if (item.xmlFileName) { > + Services.search.addEngine(gDataUrl + item.xmlFileName, This method (addTestEngines) relies on gDataURL from useHttpServer which may be non-obvious. I think this method should throw if it tries to use gDataURL and it's undefined.

(In reply to Matthew N. [:MattN] from comment #1) > Nit: Could you return the server from the function so that consumers can > register other things e.g. registerContentType Good idea, will do, though if you only need to do registerContentType("sjs", "sjs") I recommend doing that in the head function itself, so that all tests run in the same environment. I think in the future useHttpServer should be implicit for all tests, and the same for environment initialization and cleanup, but I haven't worked on it in this bug to avoid scope creep. Only a bunch of tests don't add new engines. > @@ +185,5 @@ > > +function addTestEngines(aItems) { > > + return new Promise((resolve, reject) => { > > Ideally this function would also register a cleanup function to remove the > engines after. We currently delete leftovers at the beginning of every test > but we shouldn't rely on that. Filed bug 1043379.

Comment on attachment 8461549 [details] [diff] [review] Updated patch 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 by bug 1007979, which is necessary 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]: this patch itself is tests, which are running on 34 already; see also try link above [Risks and why]: low risk, tests only; we want about:home/newtab search suggestions on 33 as stated in bug 1054516 [String/UUID change made/needed]: none