The use case is that we cases where we want to return a different result from getSubmission, based on what the submission is being used for (e.g. keyword search vs. normal search bar search). In bug 587719 I'm using a different Url "type", but that's kind of a hack, since it's supposed to indicate the type of the response rather than the purpose of the request. There are several ways we could implement this on the back-end. One way would be to allow having the "purpose" argument affect the value of parameters (e.g. <MozParam purpose="keywordsearch" name="iskeywordsearch" value="true"> that's only present in submissions returned from getSubmission("foo", "text/html", "keywordsearch")). Another would be to have it actually select different <Url>s to use (e.g. based on their "purpose" attribute - a possible addition to OpenSearch).

Comment on attachment 594286 [details] [diff] [review] WIP Part 1 - IDL and nsSearchService.js I think this approach makes sense and it makes for simpler search plugins compared to duplicating <Url>s like we currently do for Google. I'll work on tests.

Comment on attachment 612109 [details] [diff] [review] Part 2 - Tests for purpose argument >diff --git a/toolkit/components/search/tests/xpcshell/data/engine.xml b/toolkit/components/search/tests/xpcshell/data/engine.xml > <Param name="rls" value="{moz:distributionID}:{moz:locale}:{moz:official}"/> >diff --git a/toolkit/components/search/tests/xpcshell/test_purpose.js b/toolkit/components/search/tests/xpcshell/test_purpose.js >+ let base = "http://www.google.com/search?q=foo&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:unofficial&client=firefox"; // TODO: this will fail on different "rls" Let's just remove that parameter from the test plugin? I don't really like that it's an exact copy of the Google plugin to begin with :)

* Replaced responseType argument (which I just added in bug 724116) with purpose on BrowserSearch.loadSearch. * Added support for purpose="" to support Bing's context-specific param. purpose="" means that the param is added if no purpose is specified (null or "").

Added tests for params which apply for the default purpose (when not specified).

Searched for x-moz-keywordsearch and x-moz-contextsearch to find callers to change. I'll push these patches to try to see if I missed anything (which tests catch). Bug 590068 is already on file for about:home and I'll look at that tomorrow.

My tree was a few weeks old so my previous patch didn't account for changes from bug 699856.

Just a rebase that takes bug 557890 into account.

Comment on attachment 612421 [details] [diff] [review] Part 1 v.3 Methods which accept purpose - update for bug 699856 >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js >+ getSubmission: function SRCH_EURL_getSubmission(aSearchTerms, aEngine, aPurpose) { >+ var purpose = (aPurpose === null ? "" : aPurpose); var purpose = aPurpose || ""; ? > switch (param.getAttribute("condition")) { >+ case "purpose": >+ url.addParam(param.getAttribute("name"), >+ param.getAttribute("value"), >+ param.getAttribute("purpose")); Hrm, doesn't this also need to call _addMozParam for the JSON serializing/deserializing to work correctly? I guess that's not actually necessary, since we only need to know name/value/purpose to later re-add this param (and those are all serialized normally when JSONing QueryParameter objects), but in that case your change to _initWithJSON should be dead code (param.mozparam will never be true with param.condition == "purpose", since _serializeToJSON will serialize these parameters as normal parameters), and you'll need to add a "param.purpose" to its "else this.addParam" branch. If I understand this all correctly, this bug means that purpose parameters work the first time (when engines are loaded from XML), but not the second (when engines are loaded from the JSON cache). We should probably have some tests that cover the second search service initialization (from the JSON cache) in addition to the first. I'm not sure the best way to do that - IIRC the current tests all use the same profile, do they each clear the sqlite/json files before running? Can one test start from scratch and then not clean up its search.json, so that the next test uses it? (we'd need to ensure proper test ordering of course).

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10) > If I understand this all correctly, this bug means that purpose parameters > work the first time (when engines are loaded from XML), but not the second > (when engines are loaded from the JSON cache). We should probably have some > tests that cover the second search service initialization (from the JSON > cache) in addition to the first. I'm not sure the best way to do that - IIRC > the current tests all use the same profile, do they each clear the > sqlite/json files before running? Can one test start from scratch and then > not clean up its search.json, so that the next test uses it? (we'd need to > ensure proper test ordering of course). Yeah, that is the case that I thought I was fixing with this revision but I guess I didn't test properly. I was also planning on making a test for this case. I was thinking about just adding a search.json for the test engine to the test suite and then have the search service use it and check that it's correct. I think that's cleaner than re-using the search.json from a previous test.

Yeah, that works too, but it means that we're only testing the "read from cache" code, and have no coverage for the "generate the cache" code.

Comment on attachment 612424 [details] [diff] [review] Part 3 v.2 Update consumers of the search service (Rebase) >diff --git a/browser/locales/en-US/searchplugins/google.xml b/browser/locales/en-US/searchplugins/google.xml > #expand __GOOGLE_PARAMS__ > #expand __GOOGLE_CLIENT_PARAM__ We can get rid of these now, right? We should get some tests specifically for Google and Bing to ensure that we don't break the required parameters (it would also be useful to confirm that these changes don't break them).

Comment on attachment 612132 [details] [diff] [review] Part 2 v.2 - Tests for purpose argument + default purpose Review of attachment 612132 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/tests/xpcshell/test_purpose.js @@ +55,5 @@ > + break; > + case "engine-current": > + break; > + case "engine-removed": > + break; why handling these if you don't do anything? all of this switch could be a simple if (aData != "engine-added") return; @@ +71,5 @@ > + httpServer.registerDirectory("/", do_get_cwd()); > + > + let search = Services.search; // Cause service initialization > + > + do_register_cleanup(cleanup); if you inline the function here, you don't need to declare a global httpserver var.

Comment on attachment 612421 [details] [diff] [review] Part 1 v.3 Methods which accept purpose - update for bug 699856 Review of attachment 612421 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/nsSearchService.js @@ +932,4 @@ > var url = ParamSubstitution(this.template, aSearchTerms, aEngine); > + // Default to an empty string if the purpose is not provided so that default purpose params > + // (purpose="") work consistently rather than having to define "null" and "" purposes. > + var purpose = (aPurpose === null ? "" : aPurpose); this doesn't seem to handle aPurpose = undefined, is that expected? if so would be nice to state that in the comment.

Changed notInToLoad to notInCachePath so we don't have to assume the order of the directories is the same. We already check that cachePaths.length == toLoad.length to use the cache.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > Comment on attachment 612424 [details] [diff] [review] > Part 3 v.2 Update consumers of the search service (Rebase) > > >diff --git a/browser/locales/en-US/searchplugins/google.xml b/browser/locales/en-US/searchplugins/google.xml > > > #expand __GOOGLE_PARAMS__ > > #expand __GOOGLE_CLIENT_PARAM__ > > We can get rid of these now, right? Yes. > We should get some tests specifically for Google and Bing to ensure that we > don't break the required parameters (it would also be useful to confirm that > these changes don't break them). I'll do this in a separate patch.

I made a browser chrome test for now since that's what the others are. I can make a new xpcshell directory for this if you prefer. If this pattern is fine, I'll do the same for Bing.

Any updates? Are you still waiting for the reviews?

Yes, I believe I'm waiting on Gavin's reviews but since this is mostly code cleanup it hasn't been a priority compared to other projects.

Removed bitrot.

Comment on attachment 618093 [details] [diff] [review] Part 4 v.1 Test search.json Stealing these off Gavin's plate.

Comment on attachment 709069 [details] [diff] [review] Part 1 v.5 Methods which accept purpose Review of attachment 709069 [details] [diff] [review]: ----------------------------------------------------------------- Patch header needs a little cleanup…! ::: toolkit/components/search/nsSearchService.js @@ +902,3 @@ > }, > > + // Note: This method requires that aObj has a unique name or MozParams will be overwritten. Strictly speaking, "or the previous mozparams entry with that name will be overwritten".

Comment on attachment 612132 [details] [diff] [review] Part 2 v.2 - Tests for purpose argument + default purpose Review of attachment 612132 [details] [diff] [review]: ----------------------------------------------------------------- +1 to mak's comments. ::: toolkit/components/search/tests/xpcshell/test_purpose.js @@ +27,5 @@ > + do_check_eq(base, url); > + url = engine.getSubmission("foo", null, "contextmenu").uri.spec; > + do_check_eq(base + "&channel=rcs", url); > + url = engine.getSubmission("foo", "text/html", "contextmenu").uri.spec; > + do_check_eq(base + "&channel=rcs", url); I think we can make this all a little neater: function check(text, type, purpose, expected) { do_check_eq(engine.getSubmission(text, type, purpose).uri.spec, base + expected); } … … check("foo", "text/html", undefined, ""); check("foo", null, "contextmenu", "&channel=rcs"); … @@ +62,5 @@ > + > +function run_test() { > + > + removeMetadata(); > + createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "2"); After Bug 809920, we can now do: Cu.import("resource://testing-common/AppInfo.jsm"); updateAppInfo();

Comment on attachment 620488 [details] [diff] [review] Part 3 v.3 Update consumers of the search service (remove Google defines) Review of attachment 620488 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty confident in this, but flagging Gavin to double-check. (BECAUSE REVENUE)

Comment on attachment 618093 [details] [diff] [review] Part 4 v.1 Test search.json Review of attachment 618093 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see this switched to use add_test rather than futzing with do_test_pending, and clean up the observer. See inline. ::: toolkit/components/search/nsSearchService.js @@ +2673,5 @@ > > // Write to the cache file asynchronously > NetUtil.asyncCopy(data, ostream, function(rv) { > + if (Components.isSuccessCode(rv)) { > + gObsSvc.notifyObservers(null, This was bitrotted by Bug 726279. You should use Services.obs. @@ +2681,1 @@ > LOG("_buildCache: failure during asyncCopy: " + rv); Let's add {} to the other arm of the `else` while we're here. ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +15,4 @@ > var gXULAppInfo = null; > > /** > * Creates an nsIXULAppInfo Follow-up: can we kill this, or reuse something from Bug 809920? ::: toolkit/components/search/tests/xpcshell/test_json_cache.js @@ +22,5 @@ > + > +/** > + * Read a JSON file and return the JS object > + */ > +function readJSONFile(aFile) { It gently pains me that we probably have quite a few implementations of this around the tree :/ (I know, because we have two in services code -- one using nsIFile, and one using OS.File!) Can you lift this into head_search, or even higher? @@ +29,5 @@ > + let json = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON); > + > + try { > + stream.init(aFile, MODE_RDONLY, PERMS_FILE, 0); > + return json.decodeFromStream(stream, stream.available()); You might want to touch base with Yoric for Bug 832664. @@ +46,5 @@ > + */ > +let _dirSvc = null; > +function getDir(aKey, aIFace) { > + if (!aKey) > + FAIL("getDir requires a directory key!"); Stylistically I would prefer new code to (a) not use aFoo naming, and (b) always use curly braces, even for one-line conditionals. These are two style changes that I think are worth mentioning because they're decent wins for error-catching and readability. But as they say, "not my circus, not my monkey", so I defer to Gavin and those with more of a sense of ownership over t/c/search! @@ +104,5 @@ > + ostream.DEFER_OPEN); > + converter.charset = "UTF-8"; > + let data = converter.convertToInputStream(JSON.stringify(cacheTemplate)); > + > + do_test_pending(); Split here, use add_test and run_next_test rather than do_test_pending. I've fixed so many intermittent oranges just by rewriting code to not use do_test_pending/do_test_finish…. @@ +122,5 @@ > + ostream.DEFER_OPEN); > + converter.charset = "UTF-8"; > + let data = converter.convertToInputStream(JSON.stringify(gMetadata)); > + > + NetUtil.asyncCopy(data, ostream, afterCacheCopy); Make `afterCacheCopy` a separate test, and use run_next_test here. @@ +143,5 @@ > + > +/** > + * Start the search service and confirm the engine properties match the expected values. > + */ > +function afterCacheCopy(rv) { add_test(function test_cached_engine_properties() { @@ +164,5 @@ > + removeMetadata(); > + removeCacheFile(); > + add_test(test_cache_write); > + run_next_test(); > + do_test_finished(); O.O @@ +190,5 @@ > + if (aTopic != "browser-search-service") > + return; > + if (aVerb != "write-cache-to-disk-complete") > + return; > + do_print("Cache write complete"); Remove the observer here. Otherwise you're going to add a second test and wonder why it gets marked as finished without it doing anything… @@ +210,5 @@ > + Services.search.QueryInterface(Ci.nsIObserver).observe(null, "browser-search-engine-modified" , "engine-removed"); > + }); > +} > + > +let gExpectedEngine = { const EXPECTED_ENGINE

Comment on attachment 620980 [details] [diff] [review] Part 5 v.1 Test Google default search plugin Review of attachment 620980 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine, but it has enough open questions out of my realm of knowledge that I'm going to leave it on Gavin's plate. Thanks for the drive-by opportunity! ::: browser/components/search/test/browser_google.js.in @@ +22,5 @@ > +function getLocale() { > + const localePref = "general.useragent.locale"; > + var locale = getLocalizedPref(localePref); > + if (locale) > + return locale; let not var, curly braces (and same previous style comment applies to this new file). @@ +35,5 @@ > + * The name of the pref to get. > + * @returns aDefault if the requested pref doesn't exist. > + */ > +function getLocalizedPref(aPrefName, aDefault) { > + const nsIPLS = Ci.nsIPrefLocalizedString; This seems like overkill! @@ +40,5 @@ > + try { > + return Services.prefs.getComplexValue(aPrefName, nsIPLS).data; > + } catch (ex) {} > + > + return aDefault; Just return inside the catch block. That way we don't get lint complaining about empty catch blocks :D @@ +51,5 @@ > + let distributionID = MOZ_DISTRIBUTION_ID; > + try { > + distributionID = Services.prefs.getCharPref(BROWSER_SEARCH_PREF + "distributionID"); > + } > + catch (ex) {} Cuddle brace. @@ +53,5 @@ > + distributionID = Services.prefs.getCharPref(BROWSER_SEARCH_PREF + "distributionID"); > + } > + catch (ex) {} > + > + let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8&aq=t&rls={moz:distributionID}:{moz:locale}:{moz:official}&client=firefox-a"; // TODO don't hard code rls? Please either file a follow-up for the TODO, and reference the bug number, or deal with it somehow. ::: browser/components/search/test/head.js @@ +3,5 @@ > + > +/** > + * Recursively compare two objects > + */ > +function compareObjects(actualObj, expectedObj, name) { Yay no aFoo! :D @@ +13,5 @@ > + compareObjects(actualObj[prop], expectedObj[prop], name + "[" + prop + "]"); > + } else { > + is(actualObj[prop], expectedObj[prop], name + "[" + prop + "]"); > + } > + } We have this well-tested code hanging around already, which you might want to convert to `is`: deepEquals: function eq(a, b) { // If they're triple equals, then it must be equals! if (a === b) return true; // If they weren't equal, they must be objects to be different if (typeof a != "object" || typeof b != "object") return false; // But null objects won't have properties to compare if (a === null || b === null) return false; // Make sure all of a's keys have a matching value in b for (let k in a) if (!eq(a[k], b[k])) return false; // Do the same for b's keys but skip those that we already checked for (let k in b) if (!(k in a) && !eq(a[k], b[k])) return false; return true; },

Oh, and something that just occurred to me: please use strict mode for new files.

Comment on attachment 618093 [details] [diff] [review] Part 4 v.1 Test search.json Review of attachment 618093 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Richard. ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +15,4 @@ > var gXULAppInfo = null; > > /** > * Creates an nsIXULAppInfo Filed bug 839670. ::: toolkit/components/search/tests/xpcshell/test_json_cache.js @@ +46,5 @@ > + */ > +let _dirSvc = null; > +function getDir(aKey, aIFace) { > + if (!aKey) > + FAIL("getDir requires a directory key!"); RE (a): I prefer aFoo and it's the standard for the component and Mozilla[1], plus I think that it improves readability and error-catching. [1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Prefixes

Addressed feedback from rnewman.

(In reply to Matthew N. [:MattN] from comment #32) > Filed bug 839670. Thanks! > RE (a): I prefer aFoo and it's the standard for the component and > Mozilla[1], Oh, then you should also have Vim and Emacs modelines in every file :D https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line Our coding standards are kinda bitrotted, both forward and backward in time -- I see lots of code using non-braced conditionals (which is called out as wrong by the style guide, and most reviewers reject), as well as lots of new code I've seen that diverged in other positive ways (converging on what I term "modern JavaScript"), and the developer guide simply hasn't kept up. (I'd go so far as to say that pseudo-Hungarian is no longer "Mozilla style" for JavaScript or Java code, and the Developer Guide is simply stale, and we ought to fix that.) But as I mentioned, this isn't my component, so I'm fine with you doing whatever the module owner wants to see happen going forward. I know Gavin is generally anti a-prefix: https://bugzilla.mozilla.org/show_bug.cgi?id=813833#c28 so I mentioned it because I thought that might be the direction Search is heading for new code. NMC,NMM!

Comment on attachment 712070 [details] [diff] [review] Part 4 v.2 Test search.json Review of attachment 712070 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty happy with this (I expect Gavin to catch some things that it's too late at night for me to catch!), but see comments! ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +136,5 @@ > + try { > + stream.init(aFile, MODE_RDONLY, FileUtils.PERMS_FILE, 0); > + return parseJsonFromStream(stream, stream.available()); > + } catch(ex) { > + dumpn("readJSONFile: Error reading cache file: " + ex); Message is no longer quite accurate. @@ +156,5 @@ > + do_check_eq(expectedObj[prop], actualObj[prop]); > + } > + } > +} > + This does not do what one might expect it to do: it tests a kind of object subsetness, rather than equality. (Or phrased differently, you don't define what kind of comparison you're doing!) That is: compareObjects({ "foo": 1, }, { "foo": 1, "bar": 2, }); will succeed. You should either name it "isSubObjectOf", or use a more thorough definition of equality, like the deepEquals method I mentioned in a previous review. ::: toolkit/components/search/tests/xpcshell/test_json_cache.js @@ +177,5 @@ > + Services.search.QueryInterface(Ci.nsIObserver).observe(null, "browser-search-engine-modified" , "engine-removed"); > + }); > +}); > + > +let EXPECTED_ENGINE = { const.

Matt, Gavin: what can we do to get this landed ASAP? It's one of the pieces we need to get search provider reporting in FHR for 21.

I'll attach an updated part 5 today

Comment on attachment 620488 [details] [diff] [review] Part 3 v.3 Update consumers of the search service (remove Google defines) Let's make sure we have a separate bug on file specifically to QA the URLs used when these various search points are triggered, on all channels (i.e. once this reaches beta).

(i.e. a bug that has a complete test matrix that QA can go through to make sure we're not changing URLs)

Comment on attachment 620980 [details] [diff] [review] Part 5 v.1 Test Google default search plugin >diff --git a/browser/components/search/test/Makefile.in b/browser/components/search/test/Makefile.in I think there are better ways to do the preprocessing now (bug 770426). Ask a build peer (gps/mhommey). >diff --git a/browser/components/search/test/browser_google.js.in b/browser/components/search/test/browser_google.js.in >+ { >+ "name": "client", >+ "value": "firefox-a", >+ mozparams: { >+ "client": { >+ "name": "client", >+ "falseValue": "firefox", >+ "trueValue": "firefox-a", >+ "condition": "defaultEngine", These values depend on the update channel, so you'll need to adjust the expected output depending on update channel to avoid the test failing on uplift to aurora. It's a bit of a PITA to duplicate data from the search engine definition in this test, but it's better than not having this kind of test coverage. It's worth being a little paranoid with this stuff.

So, we're fast tracking this bug for Firefox 21 because we feel we want to use the "purpose" argument for FHR and have the search service centrally interface with FHR rather than leaving it up to every search initiator to call out to FHR to report a search. I want to verify this is the right thing to do. If we proceed with this bug, presumably the "purpose" argument will contain something identifying where the search initiated (FHR needs to differentiate between {search bar, url bar, context menu, about:home}). When getSubmission() is called, we will increment a counter in FHR. The giant assumption here is that getSubmission() means that a search has been initiated and worth reporting to FHR. However, it's actually up to the caller to complete the search (by loading the URI in the returned nsISearchSubmission). Is this a safe assumption? Or, should we abandon this bug for the purposes of FHR and leave it up to call sites to interface with FHR?

Good question. Looking at the existing callers of getSubmission that don't actually result in searches, we have: - two callers in search.xml that just want a host to pre-connect to - the caller in AboutHomeUtils that hackily extracts the search URL for use by about:home (to be fixed in bug 590068) - the caller in KeywordURLResetPrompter.jsm that just wants to compare the keyword.URL pref's host with that of the default search engine's (to be removed once we drop support for keyword.URL) - an android caller that wants to pass the URL to the Java UI for use there We're going to be getting rid of most of those, I think, but probably not soon enough to block FHR on, and in any case doing this kind of work in a rush isn't ideal to begin with. It's also true that we may never have sufficient information within getSubmission() to properly distinguish the various cases we care about. So I think your suggested alternative of just annotating the various callers we care about is probably the most expedient (and maybe also best long-term) option.

Two of those four callers (1 & 3) probably don't need to pass a purpose argument to achieve what they're doing and therefore could be ignored by FHR. (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #42) > - the caller in AboutHomeUtils that hackily extracts the search URL for use > by about:home (to be fixed in bug 590068) Note that bug 590068 wasn't actually fixing this.

(In reply to Richard Newman [:rnewman] from comment #35) > Comment on attachment 712070 [details] [diff] [review] > Part 4 v.2 Test search.json > > This does not do what one might expect it to do: it tests a kind of object > subsetness, rather than equality. (Or phrased differently, you don't define > what kind of comparison you're doing!) > > That is: > > compareObjects({ > "foo": 1, > }, { > "foo": 1, > "bar": 2, > }); > > will succeed. > > You should either name it "isSubObjectOf", or use a more thorough definition > of equality, like the deepEquals method I mentioned in a previous review. This was intentional because there are a bunch of properties that don't need to be tested (e.g. properties on the nsIURI for engine.iconURI and many more on the _wrappedJSObject). I've changed the function name to isSubObjectOf and updated the comment to make this more clear.

We can handle Bing in a follow-up IMO. (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #40) > Comment on attachment 620980 [details] [diff] [review] > Part 5 v.1 Test Google default search plugin > > These values depend on the update channel, so you'll need to adjust the > expected output depending on update channel to avoid the test failing on > uplift to aurora. Do we ever run tests on builds where Google is not the default engine? This patch changes the client based on the update channel but doesn't check the default engine.

(In reply to Matthew N. [:MattN] from comment #46) > Do we ever run tests on builds where Google is not the default engine? In practice, no. But it would be easy to have the test fail gracefully when defaultenginename != "Google", right?

Comment on attachment 712809 [details] [diff] [review] Part 5 v.2 Test Google default search plugin Or maybe it's better to just fail more obviously (rather than pass silently without testing anything). I guess I don't feel strongly.

Try results look good: https://tbpl.mozilla.org/?tree=Try&rev=a799109407c4

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48) > Or maybe it's better to just fail more obviously (rather than pass silently > without testing anything). I guess I don't feel strongly. I added an explicit check that Google is the default engine: > is(Services.search.originalDefaultEngine, engine, "Check that Google is the default search engine");

https://hg.mozilla.org/integration/mozilla-inbound/rev/5548449c7931 https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb308b8d436 https://hg.mozilla.org/integration/mozilla-inbound/rev/19c2ed845e67 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa0a72ac95d https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba8ddf7bef0

https://hg.mozilla.org/mozilla-central/rev/5548449c7931 https://hg.mozilla.org/mozilla-central/rev/7fb308b8d436 https://hg.mozilla.org/mozilla-central/rev/19c2ed845e67 https://hg.mozilla.org/mozilla-central/rev/ffa0a72ac95d https://hg.mozilla.org/mozilla-central/rev/1ba8ddf7bef0