Closed Bug 1182308 Opened 9 years ago Closed 9 years ago

Windows 10 search rewrite should handle more Bing domains

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox40 --- verified
firefox41 --- verified
firefox42 --- verified

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 1 obsolete file)

The initial landing of bug 1177237 only rewrites certain searches from "https://www.bing.com". Bug 1177237 comment 33 reports that this doesn't always work, and cites an example Windows search going to "https://cn.bing.com" (in China).

So, we should broaden the matching we do for the rewrite. Either by adding this (and any other?) as explicit additional checks, or by loosening the check to match more loosely.

AFAIK, Bing doesn't appear to do the plethora of multiple-TLDs like Google (google.ca, google.cn, google.co.uk, etc), and other *.bing.com sites seem to just redirect to www.bing.com. I have a suspicion that "cn.bing.com" may be unique, but I'm also leaning towards just matching *.bing.com.

We talked about checking with our localization community / general outreach to see if this matches reality, and we should probably do that first. Wasn't expecting to get a bug report so fast. ;-)
Okay, now by viewing commands in task manager I found that in mainland china search link is started with "http://cn.bing.com/search", which will be automatically redirected to https://cn.bing.com/search after the browser is opened.
How about this?

--- a/browser/components/nsBrowserContentHandler.js
+++ b/browser/components/nsBrowserContentHandler.js
@@ -740,17 +740,17 @@ nsDefaultCommandLineHandler.prototype =
     try {
       var ar;
       while ((ar = cmdLine.handleFlagWithParam("url", false))) {
         var uri = resolveURIInternal(cmdLine, ar);

         // Searches in the Windows 10 task bar searchbox simply open the default browser
         // with a URL for a search on Bing. Here we extract the search term and use the
         // user's default search engine instead.
-        if (redirectWinSearch && uri.spec.startsWith("https://www.bing.com/search")) {
+        if (redirectWinSearch && uri.spec.includes(".bing.com/search")) {
           try {
             var url = uri.QueryInterface(Components.interfaces.nsIURL);
             var params = new URLSearchParams(url.query);
             // We don't want to rewrite all Bing URLs coming from external apps. Look
             // for the magic URL parm that's present in searches from the task bar.
             // (Typed searches use "form=WNSGPH", Cortana voice searches use "FORM=WNSBOX")
             var formParam = params.get("form");
             if (!formParam) {
(In reply to zhoubcfan from comment #2)
> How about this?
> 
> --- a/browser/components/nsBrowserContentHandler.js
> +++ b/browser/components/nsBrowserContentHandler.js
> @@ -740,17 +740,17 @@ nsDefaultCommandLineHandler.prototype =
>      try {
>        var ar;
>        while ((ar = cmdLine.handleFlagWithParam("url", false))) {
>          var uri = resolveURIInternal(cmdLine, ar);
> 
>          // Searches in the Windows 10 task bar searchbox simply open the
> default browser
>          // with a URL for a search on Bing. Here we extract the search term
> and use the
>          // user's default search engine instead.
> -        if (redirectWinSearch &&
> uri.spec.startsWith("https://www.bing.com/search")) {
> +        if (redirectWinSearch && uri.spec.includes(".bing.com/search")) {
>            try {
>              var url = uri.QueryInterface(Components.interfaces.nsIURL);
>              var params = new URLSearchParams(url.query);
>              // We don't want to rewrite all Bing URLs coming from external
> apps. Look
>              // for the magic URL parm that's present in searches from the
> task bar.
>              // (Typed searches use "form=WNSGPH", Cortana voice searches
> use "FORM=WNSBOX")
>              var formParam = params.get("form");
>              if (!formParam) {

What I mind is whether it will affect users searching with Bing in firefox. You know, sometimes users don't use their default search engine to search. So you need to test whether this method will also redirect search requests in this situation.
Flags: needinfo?(zhoubcfan)
(In reply to zhoubcfan from comment #2)
> How about this?
...
> +        if (redirectWinSearch && uri.spec.includes(".bing.com/search")) {

Close, but a substring match is too loose, it would catch things like "http://site.com/article.html?referrer=www.bing.com/search".


(In reply to leichixian from comment #3)
> What I mind is whether it will affect users searching with Bing in firefox.
> You know, sometimes users don't use their default search engine to search.
> So you need to test whether this method will also redirect search requests
> in this situation.

This code is only looking at URLs passed in from the command line, so searches (or any other browsing) from within Firefox itself never goes through this code.
Flags: needinfo?(zhoubcfan)
Attached patch Patch v.1 (obsolete) — Splinter Review
My Windows machine is currently doing on OS update, so I haven't tested this beyond staring at it carefully. Will test shortly via Try. :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aea2bb383c6
Attachment #8634500 - Flags: review?(jaws)
Comment on attachment 8634500 [details] [diff] [review]
Patch v.1

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

::: browser/components/nsBrowserContentHandler.js
@@ +754,5 @@
> +        // 1182308 reports a Chinese edition of Windows 10 using
> +        // "http://cn.bing.com/search...", so be a bit flexible in what we match.
> +        if (redirectWinSearch &&
> +            (uriScheme == "http" || uriScheme == "https") &&
> +            uriHost.endsWith("bing.com") && uriPath.startsWith("/search")) {

Speaking of too-loose matches, this should be ".bing.com".
Comment on attachment 8634500 [details] [diff] [review]
Patch v.1

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

::: browser/components/nsBrowserContentHandler.js
@@ +754,5 @@
> +        // 1182308 reports a Chinese edition of Windows 10 using
> +        // "http://cn.bing.com/search...", so be a bit flexible in what we match.
> +        if (redirectWinSearch &&
> +            (uriScheme == "http" || uriScheme == "https") &&
> +            uriHost.endsWith("bing.com") && uriPath.startsWith("/search")) {

I would prefer Services.eTLD.getBaseDomainFromHost(uriHost).startsWith("bing.")

That way it will work with "bing.co.uk" and any other potential ccTLDs that we may not anticipate.
Attachment #8634500 - Flags: review?(jaws) → review+
Priority: -- → P1
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)

> > +            uriHost.endsWith("bing.com") && uriPath.startsWith("/search")) {
> 
> I would prefer
> Services.eTLD.getBaseDomainFromHost(uriHost).startsWith("bing.")
> 
> That way it will work with "bing.co.uk" and any other potential ccTLDs that
> we may not anticipate.

I thought about that, but so far there's no sign they're actually using those domains. I'd rather start off conservative with what we're rewriting because using the eTLD service is a pretty big hammer... There are thousands of eTLDs, and only a fraction of those are actually going to be Bing.
url:        https://hg.mozilla.org/integration/fx-team/rev/403f2f4cd9783f19abf010c2bde33679aaabe00e
changeset:  403f2f4cd9783f19abf010c2bde33679aaabe00e
user:       Justin Dolske <dolske@mozilla.com>
date:       Thu Jul 16 17:29:32 2015 -0700
description:
Bug 1182308 - Windows 10 search rewrite should handle more Bing domains.  r=jaws
I tested this fix by running:

  ./firefox -url "http://cn.bing.com/search?q=kittens&FORM=WNSGPH"

and making sure it loaded the search in Firefox's default search engine.
Approval Request Comment
[Feature/regressing bug #]: 1177237 followup
[User impact if declined]: win10 search rewrites don't work for some users
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low risk, small change
[String/UUID change made/needed]: n/a
Attachment #8634500 - Attachment is obsolete: true
Attachment #8635048 - Flags: approval-mozilla-beta?
Attachment #8635048 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/403f2f4cd978
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
QA Contact: cornel.ionce
(In reply to Justin Dolske [:Dolske] from comment #9)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> 
> > > +            uriHost.endsWith("bing.com") && uriPath.startsWith("/search")) {
> > 
> > I would prefer
> > Services.eTLD.getBaseDomainFromHost(uriHost).startsWith("bing.")
> > 
> > That way it will work with "bing.co.uk" and any other potential ccTLDs that
> > we may not anticipate.
> 
> I thought about that, but so far there's no sign they're actually using
> those domains. I'd rather start off conservative with what we're rewriting
> because using the eTLD service is a pretty big hammer... There are thousands
> of eTLDs, and only a fraction of those are actually going to be Bing.

Works for me :)
Comment on attachment 8635048 [details] [diff] [review]
Patch v.2 (as landed)

Small, isolcated fix that dolske verified locally. This is only visible on Windows 10. Let's get the fix into beta6. Beta+ Aurora+
Attachment #8635048 - Flags: approval-mozilla-beta?
Attachment #8635048 - Flags: approval-mozilla-beta+
Attachment #8635048 - Flags: approval-mozilla-aurora?
Attachment #8635048 - Flags: approval-mozilla-aurora+
Confirming the fix for Windows 10 64-bit using:
- 42.0a1 Nightly, build ID: 20150727030216;
- 41.0a2 Aurora, build ID: 20150727004009;
- 40.0b8, build ID: 20150727174134.
You need to log in before you can comment on or make changes to this bug.