Closed Bug 1117153 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan

People

(Reporter: Gavin, Assigned: chrishood, Mentored)

Details

Attachments

(1 file, 2 obsolete files)

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.)
Flags: qe-verify+
Flags: needinfo?(mconnor)
Flags: firefox-backlog+
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.
Mentor: gavin.sharp
I'd be interested in taking a look at this.
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.
Attached patch bug-1117153.patch (obsolete) — Splinter Review
Attachment #8543680 - Flags: review?(gavin.sharp)
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!
Attachment #8543680 - Flags: review?(gavin.sharp) → feedback+
Assignee: nobody → chrishood
Attached patch bug-1117153.patch (obsolete) — Splinter Review
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.
Attachment #8543680 - Attachment is obsolete: true
Attachment #8543709 - Flags: review?(gavin.sharp)
Comment on attachment 8543709 [details] [diff] [review]
bug-1117153.patch

You're right about the alias behavior. Thanks again!
Attachment #8543709 - Flags: review?(gavin.sharp) → review+
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?
Let's get it posted for checkin, we can let mconnor weigh in and change things later as needed.
Attachment #8543709 - Attachment is obsolete: true
Attachment #8545058 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/41656484aab8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Iteration: --- → 37.3 - 12 Jan
Attachment #8545058 - Flags: checkin?
QA Contact: petruta.rasa
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
(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
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
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.
Status: RESOLVED → VERIFIED
For clarity: I 100% agree with the choices here. Thanks!
Flags: needinfo?(mconnor)
You need to log in before you can comment on or make changes to this bug.