Add "purpose" argument for searches coming from outside Firefox

RESOLVED FIXED in Firefox 41

Status

()

defect
P1
normal
Rank:
11
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Dolske, Assigned: florian)

Tracking

unspecified
Firefox 43
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

(Whiteboard: [win10][fxsearch])

Attachments

(1 attachment, 3 obsolete attachments)

From bug 1177237. We want to redirect searches coming from Windows (which always uses Bing) to use the user's default search engine in Firefox. I believe we'll want a new "purpose" param for this context. Notably, Yahoo is returning suboptimal / old-style searches when it doesn't have a valid "purpose" param in the search URL. (This would be nice to have Yahoo fix!)

I think this is as simple as adding a new purpose-type to browser/locales/en-US/searchplugins/yahoo.xml, and making sure Yahoo is cool with that.

We should also fix doSearch() so that the existing "firefox -search kittens" uses this too.
...and by "simple" I guess I really mean adding it to ddg.xml and bing.xml too, and probably all the localized flavors too?

Yahoo seems more important at the moment, just because this bug means you get poor results. But maybe Yahoo can just fix that server-side.
Posted patch Patch v.1 WIP (obsolete) — Splinter Review
Something like this. Not sure if we need to differentiate "ossearch" from "cmdline", but why not...
I don't think it makes sense to split those out, particularly.  They're just different entry points depending on the OS.  I'd prefer to collate those entry points into "system" (I doubt "cmdline" is worth having a different param from all providers.)

I know MattN had considered making purpose a mandatory arg. I think defaulting to "searchbar" if it's unspecified is sufficient.  That's the right fix (Yahoo uses the yhs-* parameter to identify the correct YHS config to use, so if we omit it we're missing data they need to pick a result format).
Blocks: 1177237
Looks like bug 925967 is the other bug.
Just to be clear -- are we ok to actually ship with "searchbar" as the arg here (at least initially) for bug 1177237?
Flags: needinfo?(mconnor)
Following up, we discussed internally (within BD) and agreed we would like a unique code for Yahoo.  I will follow-up once we have that.
(In reply to Justin Dolske [:Dolske] from comment #5)
> Just to be clear -- are we ok to actually ship with "searchbar" as the arg
> here (at least initially) for bug 1177237?

@mconnor, correct me if I'm wrong here but I think we can ship the with the "searchbar" code for now.  Noting comment 6 where Joanne says that we do want (and are in the progress of getting) a new code for Yahoo.
@mconnor is on PTO this week.  If we do not get you the Y code in time, please ship with "searchbar" code for now. What is the deadline for getting the new code in?
This is more complicated than it should be, unfortunately.  Especially since I'm not 100% sure which partner builds use/used per-SAP purposes, and we can't easily update those in the field.  Given the timeline for Win10, I'd recommend the following approach to minimize risk (where risk means "searches won't be counted"):

1) Implement "system" as the purpose for these code paths.
2) In the search service, I'd assume around http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1155 we'll need to handle a fallback so we use the "searchbar" purpose if "system" isn't specified in the plugin.  Yes, this will be a relatively ugly hack, and yes, we'll want to write some tests to ensure this works.

If we get a separate code from partners, we'll add the "system" purpose to the plugin, and then we won't hit the fallback case.

This will ensure that a) we always get the right SERP (no weird behaviour from missing params) b) plugins in distro builds won't get under-counted and c) we'll have the ability to start splitting out system searches correctly on a per-plugin/partner basis.
Flags: needinfo?(mconnor)
Received the OS level search tag from Yahoo:  

HSimp=07

For example: https://search.yahoo.com/yhs/search?p=ipad&ei=UTF-8&hspart=mozilla&hsimp=yhs-007.  This will also allow us to track metrics separately via Yahoo’s type tag reports.  That’s how we differentiate all the other SAPs traffic.
Flags: firefox-backlog+
Whiteboard: [fxsearch]
Rank: 3
Whiteboard: [fxsearch] → [fxsearch][win10]
Rank: 3 → 10
Priority: -- → P1
Whiteboard: [fxsearch][win10] → [win10][fxsearch]
Rank: 10 → 12
Rank: 12 → 11
Posted patch system-purpose (obsolete) — Splinter Review
Implemented as specified in comment 9, using the yhs-007 (I'm surprised there's no trace of a yhs-006 anywhere) code for a new 'system' purpose and falling back to the 'searchbar' purpose if 'system' is missing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adb77c0d81d7
Assignee: nobody → florian
Attachment #8626205 - Attachment is obsolete: true
Attachment #8644311 - Flags: review?(MattN+bmo)
Posted patch Patch v2 (obsolete) — Splinter Review
Fixed browser/components/search/test/browser_yahoo.js failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86983840a783
Attachment #8644311 - Attachment is obsolete: true
Attachment #8644311 - Flags: review?(MattN+bmo)
Attachment #8644327 - Flags: review?(MattN+bmo)
Hi Florian,

Just responding to your comment 11 statement - there is a reason for using yhs-007 instead of -006, too lengthy to write here.  If you want details, ping me outside the bug and I can explain it to you, if you're interested.  Thanks, Joanne
Status: NEW → ASSIGNED
Summary: Add "purpose" parm for searches coming from outside Firefox → Add "purpose" argument for searches coming from outside Firefox
Comment on attachment 8644327 [details] [diff] [review]
Patch v2

Review of attachment 8644327 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserContentHandler.js
@@ +270,5 @@
>  function doSearch(searchTerm, cmdLine) {
>    var ss = Components.classes["@mozilla.org/browser/search-service;1"]
>                       .getService(nsIBrowserSearchService);
>  
> +  var submission = ss.defaultEngine.getSubmission(searchTerm, null, "system");

I'm a bit surprised we don't want a separate purpose here to differentiate these but that's what mconnor said so it's fine with me.

::: toolkit/components/search/nsSearchService.js
@@ +1324,5 @@
>      var purpose = aPurpose || "";
>  
> +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> +    if (purpose == "system" &&
> +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))

Can't `p.purpose !== undefined && p.purpose == "system"` be simplified to `p.purpose == "system"`? Then you can put this on one line probably.

@@ +1325,5 @@
>  
> +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> +    if (purpose == "system" &&
> +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> +      purpose = "searchbar";

Nit: My understanding is that we're preferring braces for single-statement blocks now.
Attachment #8644327 - Flags: review?(MattN+bmo) → review+
Blocks: 1177246
Comment on attachment 8644327 [details] [diff] [review]
Patch v2

Review of attachment 8644327 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchService.js
@@ +1322,5 @@
>      // 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 || "";
>  
> +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.

Maybe this should be a generic fallback, so that "searchbar" is used whenever a unknown purpose is specified?

I don't know if that's a good idea or bad idea, but I also don't know why Yahoo has chosen to give different results when there isn't a known param in the resulting URL. :/
(I suppose bug 1181645 is looking at that issue, a little more broadly.)
(In reply to Matthew N. [:MattN] from comment #15)
> Comment on attachment 8644327 [details] [diff] [review]

Thanks for the review!

> ::: toolkit/components/search/nsSearchService.js
> @@ +1324,5 @@
> >      var purpose = aPurpose || "";
> >  
> > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> > +    if (purpose == "system" &&
> > +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> 
> Can't `p.purpose !== undefined && p.purpose == "system"` be simplified to
> `p.purpose == "system"`? Then you can put this on one line probably.

I copied this from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=3dff06c8adbc#1155
I think this first check is to avoid a "use of undefined value" JS warning. We could change |p.purpose !== undefined| to just |p.purpose| but I prefer being consistent.

> @@ +1325,5 @@
> >  
> > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> > +    if (purpose == "system" &&
> > +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> > +      purpose = "searchbar";
> 
> Nit: My understanding is that we're preferring braces for single-statement
> blocks now.

For old code, I still try to preserve consistency with the surrounding code. This method has 2 other single-statement if blocks without braces.
(In reply to Justin Dolske [:Dolske] from comment #16)
> Comment on attachment 8644327 [details] [diff] [review]

> ::: toolkit/components/search/nsSearchService.js
> @@ +1322,5 @@
> >      // 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 || "";
> >  
> > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> 
> Maybe this should be a generic fallback, so that "searchbar" is used
> whenever a unknown purpose is specified?

I considered doing this, and then decided I would prefer that broader behavior change to be done in a separate patch, so that we can decide separately whether we want to uplift that change. It will be a trivial code change anyway, just replace:
    if (purpose == "system" &&
        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
with:
    if (!this.params.some(p => p.purpose !== undefined && p.purpose == purpose))

And yes, bug 1181645 is a good place to do that change :).
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> (In reply to Matthew N. [:MattN] from comment #15)

> > ::: toolkit/components/search/nsSearchService.js
> > @@ +1324,5 @@
> > >      var purpose = aPurpose || "";
> > >  
> > > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> > > +    if (purpose == "system" &&
> > > +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> > 
> > Can't `p.purpose !== undefined && p.purpose == "system"` be simplified to
> > `p.purpose == "system"`? Then you can put this on one line probably.
> 
> I copied this from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/
> nsSearchService.js?rev=3dff06c8adbc#1155
> I think this first check is to avoid a "use of undefined value" JS warning.
> We could change |p.purpose !== undefined| to just |p.purpose| but I prefer
> being consistent.

Looking at this again, it's possible the existing '!== undefined' check was to treat the empty string differently than undefined.
(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> (In reply to Florian Quèze [:florian] [:flo] from comment #18)
> > (In reply to Matthew N. [:MattN] from comment #15)
> 
> > > ::: toolkit/components/search/nsSearchService.js
> > > @@ +1324,5 @@
> > > >      var purpose = aPurpose || "";
> > > >  
> > > > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> > > > +    if (purpose == "system" &&
> > > > +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> > > 
> > > Can't `p.purpose !== undefined && p.purpose == "system"` be simplified to
> > > `p.purpose == "system"`? Then you can put this on one line probably.
> > 
> > I copied this from
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/
> > nsSearchService.js?rev=3dff06c8adbc#1155
> > I think this first check is to avoid a "use of undefined value" JS warning.
> > We could change |p.purpose !== undefined| to just |p.purpose| but I prefer
> > being consistent.
> 
> Looking at this again, it's possible the existing '!== undefined' check was
> to treat the empty string differently than undefined.

That's what I recall. I think you should use the simplified version unless there actually is a strict mode warning.
(In reply to Matthew N. [:MattN] from comment #15)

> > +    // If the 'system' purpose isn't defined in the plugin, fallback to 'searchbar'.
> > +    if (purpose == "system" &&
> > +        !this.params.some(p => p.purpose !== undefined && p.purpose == "system"))
> 
> Can't `p.purpose !== undefined && p.purpose == "system"` be simplified to
> `p.purpose == "system"`? Then you can put this on one line probably.

Yes, the purpose value is always set by the QueryParameter constructor, so even when it's undefined it shouldn't cause warnings. Thanks for catching this :).
Attachment #8644327 - Attachment is obsolete: true
Attachment #8646913 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d6108eb0d284
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8646913 [details] [diff] [review]
Patch v3 (for check-in)

Approval Request Comment
[Feature/regressing bug #]: part of the Windows 10 search effort; I think this was wanted for Firefox 40 and had to be cut.
[User impact if declined]: I think no user impact, but our search partners will not receive the right tags in search requests from the Windows search UI.
[Describe test coverage new/current, TreeHerder]: the patch contains unit tests.
[Risks and why]: reasonable, the patch is self contained and straight forward.
[String/UUID change made/needed]: none.
Attachment #8646913 - Flags: approval-mozilla-beta?
Attachment #8646913 - Flags: approval-mozilla-aurora?
Comment on attachment 8646913 [details] [diff] [review]
Patch v3 (for check-in)

Approved for uplift to Aurora. I would like this patch to stabilize on Aurora for a day or two and then be uplifted to Beta.
Attachment #8646913 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8646913 [details] [diff] [review]
Patch v3 (for check-in)

Approving for Beta uplift given that the patch baked on Nightly and then on Aurora.
Attachment #8646913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.