1117153 - no "purpose" sent for location bar searches triggered by search engine keyword

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

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

See bug 1057166 comment 35.

We don't send a purpose param for searches triggered by search engine keywords in the location bar (e.g. assigning the "b" keyword to bing and entering "b foo bar" in the location bar to search). We probably should just use the "keyword" purpose to match the non-keyword triggered location bar searches.

mconnor, do you agree?

(Note that this currently only affects Bing and DDG, because they are the only engines that use "purpose" params.)

Comment 1

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

Should just be a matter of adding the "purpose" argument in this getSubmission call:

https://hg.mozilla.org/mozilla-central/annotate/57e4e9c33bef/browser/base/content/browser.js#l2085

Ideally we would also adjust the "behavior" tests at https://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/ to include coverage for this case. That involves adding an alias to the engines (and register a cleanup function to remove it afterwards), and then adding a variant of the existing "keyword" test (https://hg.mozilla.org/mozilla-central/annotate/57e4e9c33bef/browser/components/search/test/browser_amazon_behavior.js#l40) that uses that alias instead of "?" to trigger the search.

Comment 2

[Chris]

I'd be interested in taking a look at this.

Comment 3

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

Awesome! Let me know if you need more info to get started. Let's go with the plan from comment 0 (using "keyword" as the purpose parameter) for now, and we can easily revise that if mconnor insists we should use a different value.

Comment 4

[Chris]

Attachment: bug-1117153.patch

Comment 5

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

Comment on attachment 8543680 [details] [diff] [review]
bug-1117153.patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-    let submission = engine.getSubmission(param);
>+    let purpose = "keyword";
>+    let submission = engine.getSubmission(param, "", purpose);

Let's use |null| for the responseType argument here (more technically correct per API definition, even though it doesn't matter in practice), and just hardcode in "keyword" rather than introducing the "purpose" variable (makes it easier to see what's going on when grepping for getSubmission calls).

>diff --git a/browser/components/search/test/browser_amazon_behavior.js b/browser/components/search/test/browser_amazon_behavior.js

>   registerCleanupFunction(function () {
>+    engine.alias = undefined;

Hmm, looking at the engine alias setter (https://hg.mozilla.org/mozilla-central/annotate/55f3224d7513/toolkit/components/search/nsSearchService.js#l4856), it looks like this might result in the engine getting an alias of "undefined" (the string). "null" might have the same problem. Can you verify this? Using an empty string ("") might be a better option - there isn't really a way to clear the engine aliases completely with an exposed API, as far as I can tell. We can probably live with it not being perfect, though, as long as it doesn't interfere with other tests.

(Same comment applies to all of the tests.)

Apart from those nits, this looks great - thanks for jumping on this, Chris!

Comment 6

[Chris]

Attachment: bug-1117153.patch

I checked the contents of the engine alias using console.log.  It looks like using either null or undefined in the cleanup function will set it to null, using an empty string will set it to an empty string.  Given that it's original state is null I'm more inclined towards setting it to either null or undefined.

Comment 7

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

Comment on attachment 8543709 [details] [diff] [review]
bug-1117153.patch

You're right about the alias behavior. Thanks again!

Comment 8

[Chris]

Thanks for the review Gavin, do you want to wait until MConnor's input or do you want me to go ahead and post a patch for checkin?

Comment 9

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

Let's get it posted for checkin, we can let mconnor weigh in and change things later as needed.

Comment 10

[Chris]

Attachment: bug-1117153.patch

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/41656484aab8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41656484aab8

Comment 13

[Petruta Horea [:phorea], Desktop QA]

Using Nightly 37.0a1 2014-01-09 I see no data in about:healthreport (under org.mozilla.searches.counts) when performing searches with keywords in url bar. I tried with Bing and DDG, but also with other search engines.
Am I missing something? Thanks

Comment 14

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

(In reply to Petruta Rasa [QA] [:petruta] from comment #13)
> Using Nightly 37.0a1 2014-01-09 I see no data in about:healthreport (under
> org.mozilla.searches.counts) when performing searches with keywords in url
> bar. I tried with Bing and DDG, but also with other search engines.
> Am I missing something? Thanks

You're not - that's a different bug, though (not saving these searches in FHR). I will file a separate bug about that.

To verify this bug, you should ensure that the correct "purpose" parameters are being passed when the searches are triggered. Bing and DDG are the only ones who have "purpose" params. The expected URLs for a location bar keyword-triggered search of "test" are:

http://www.bing.com/search?q=test&pc=MOZI&form=MOZLBR
https://duckduckgo.com/?q=test&t=ffab

Without this patch, those URLs would have been:

http://www.bing.com/search?q=test&pc=MOZI
https://duckduckgo.com/?q=test

Comment 15

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

(In reply to :Gavin Sharp [email: [e-mail]] from comment #14)
> You're not - that's a different bug, though (not saving these searches in
> FHR). I will file a separate bug about that.

Bug 1121133

Comment 16

[Petruta Horea [:phorea], Desktop QA]

Thanks a lot, Gavin!

Verified as fixed using Firefox Developer edition 37.0a2 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

Comment 17

[Mike Connor [:mconnor]]

For clarity: I 100% agree with the choices here. Thanks!