1177443 - Add "purpose" argument for searches coming from outside Firefox

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Justin Dolske [:Dolske]]

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

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 WIP

Something like this. Not sure if we need to differentiate "ossearch" from "cmdline", but why not...

Comment 3

[Mike Connor [:mconnor]]

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

Comment 4

[Justin Dolske [:Dolske]]

Looks like bug 925967 is the other bug.

Comment 5

[Justin Dolske [:Dolske]]

Just to be clear -- are we ok to actually ship with "searchbar" as the arg here (at least initially) for bug 1177237?

Comment 6

[Joanne Nagel]

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.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

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

Comment 8

[Joanne Nagel]

@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?

Comment 9

[Mike Connor [:mconnor]]

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.

Comment 10

[Joanne Nagel]

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.

Comment 11

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: system-purpose

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

Comment 12

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: Patch v2

Fixed browser/components/search/test/browser_yahoo.js failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86983840a783

Comment 13

[Joanne Nagel]

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

Comment 15

[Matthew N. [:MattN]]

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.

Comment 16

[Justin Dolske [:Dolske]]

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. :/

Comment 17

[Justin Dolske [:Dolske]]

(I suppose bug 1181645 is looking at that issue, a little more broadly.)

Comment 18

[Florian Quèze [:florian] (on leave until October 9)]

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

Comment 19

[Florian Quèze [:florian] (on leave until October 9)]

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

Comment 20

[Florian Quèze [:florian] (on leave until October 9)]

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

Comment 21

[Matthew N. [:MattN]]

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

Comment 22

[Florian Quèze [:florian] (on leave until October 9)]

Attachment: Patch v3 (for check-in)

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

Comment 23

[Pulsebot]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6108eb0d284

Comment 25

[Florian Quèze [:florian] (on leave until October 9)]

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.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/700a10008300

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23555d3d2787