587780 - add "purpose" argument to getSubmission

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: WIP Part 1 - IDL and nsSearchService.js

Comment 2

[Matthew N. [:MattN]]

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 3

[Matthew N. [:MattN]]

Attachment: Part 2 - Tests for purpose argument

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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 :)

Comment 5

[Matthew N. [:MattN]]

Attachment: Part 1 v.2 Methods which accept purpose

* 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 "").

Comment 6

[Matthew N. [:MattN]]

Attachment: Part 2 v.2 - Tests for purpose argument + default purpose

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

Comment 7

[Matthew N. [:MattN]]

Attachment: Part 3 v.1 Update consumers of the search service

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.

Comment 8

[Matthew N. [:MattN]]

Attachment: Part 1 v.3 Methods which accept purpose - update for bug 699856

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

Comment 9

[Matthew N. [:MattN]]

Attachment: Part 3 v.2 Update consumers of the search service (Rebase)

Just a rebase that takes bug 557890 into account.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 11

[Matthew N. [:MattN]]

(In reply to Gavin Sharp (use [e-mail] 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.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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 13

[:Gavin Sharp [email: [e-mail]]]

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 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 16

[Matthew N. [:MattN]]

Attachment: Part 4 v.1 Test search.json

Comment 17

[Matthew N. [:MattN]]

Attachment: Part 1 v.4 Methods which accept purpose

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.

Comment 18

[Matthew N. [:MattN]]

Attachment: Part 3 v.3 Update consumers of the search service (remove Google defines)

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 19

[Matthew N. [:MattN]]

Attachment: Part 5 v.1 Test Google default search plugin

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.

Comment 20

[Marco Castelluccio [:marco]]

Any updates? Are you still waiting for the reviews?

Comment 21

[Matthew N. [:MattN]]

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.

Comment 22

[Gregory Szorc [:gps]]

Attachment: Part 1 v.5 Methods which accept purpose

Removed bitrot.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 618093 [details] [diff] [review]
Part 4 v.1 Test search.json

Stealing these off Gavin's plate.

Comment 24

[Richard Newman [:rnewman]]

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 25

[Richard Newman [:rnewman]]

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("[e-mail]", "XPCShell", "1", "2");

After Bug 809920, we can now do:

  Cu.import("resource://testing-common/AppInfo.jsm");
  updateAppInfo();

Comment 26

[Richard Newman [:rnewman]]

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 27

[Richard Newman [:rnewman]]

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 28

[Richard Newman [:rnewman]]

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;
  },

Comment 29

[Matthew N. [:MattN]]

Attachment: Part 1 v.5.1 Methods which accept purpose

Comment 30

[Matthew N. [:MattN]]

Attachment: Part 2 v.3 - Tests for purpose argument + default purpose

Comment 31

[Richard Newman [:rnewman]]

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

Comment 32

[Matthew N. [:MattN]]

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

Comment 33

[Matthew N. [:MattN]]

Attachment: Part 4 v.2 Test search.json

Addressed feedback from rnewman.

Comment 34

[Richard Newman [: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 35

[Richard Newman [:rnewman]]

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.

Comment 36

[Richard Newman [:rnewman]]

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.

Comment 37

[Matthew N. [:MattN]]

I'll attach an updated part 5 today

Comment 38

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 39

[:Gavin Sharp [email: [e-mail]]]

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

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 41

[Gregory Szorc [:gps]]

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?

Comment 42

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 43

[Matthew N. [:MattN]]

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 [e-mail] 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.

Comment 44

[Matthew N. [:MattN]]

Attachment: Part 4 v.3 Test search.json

(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.

Comment 45

[Matthew N. [:MattN]]

Attachment: Part 4 v.3.1 Test search.json

Comment 46

[Matthew N. [:MattN]]

Attachment: Part 5 v.2 Test Google default search plugin

We can handle Bing in a follow-up IMO.

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 47

[:Gavin Sharp [email: [e-mail]]]

(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 48

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 49

[Matthew N. [:MattN]]

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

Comment 50

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (use [e-mail] 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");

Comment 51

[Matthew N. [:MattN]]

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

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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