Closed Bug 1417678 Opened 2 years ago Closed 2 years ago

Make search default changes in tree

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 + wontfix
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

This bug is to make the new search default changes in the tree.

[Tracking Requested - why for this release]: We want this to go in any point release that happens for 57.
See Also: → 1417687
There are test changes for this as well. i will have those tomorrow.
Tracking to make sure we remember to uplift this work once the patches land
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review208518

Ship it!
Attachment #8928713 - Flags: review?(mconnor) → review+
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review208526

I see the "google2018" and "google-nocode" behaviors being tested. Where is the "google" behavior (used in Europe) tested?

::: browser/components/search/test/browser_google.js:23
(Diff revision 2)
>    // Test search URLs (including purposes).
>    url = engine.getSubmission("foo").uri.spec;
>    is(url, base, "Check search URL for 'foo'");
>    url = engine.getSubmission("foo", null, "contextmenu").uri.spec;
>    is(url, base, "Check context menu search URL for 'foo'");
> -  url = engine.getSubmission("foo", null, "keyword").uri.spec;
> +   url = engine.getSubmission("foo", null, "keyword").uri.spec;

This added space seems to be a typo.
(In reply to Florian Quèze [:florian] [:flo] from comment #6)

> I see the "google2018" and "google-nocode" behaviors being tested. Where is
> the "google" behavior (used in Europe) tested?
Flags: needinfo?(mozilla)
Europe behavior is tested in browser_google_codes.js
Flags: needinfo?(mozilla)
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review208566

Ok. Looking at the code shows that the location is set to US at https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/testing/profiles/prefs_general.js#321 for the tests that don't specify it. The patch looks good to me.
Attachment #8928713 - Flags: review?(florian) → review+
This is failing on the try server and I don't know why:

https://treeherder.mozilla.org/logviewer.html#?job_id=147920274&repo=try&lineNumber=1859

I haven't been able to recreate locally on Windows or Mac.
So based on this in the log:

https://treeherder.mozilla.org/logviewer.html#?job_id=147920274&repo=try&lineNumber=1841

It appears the very act of showing the homepage is causing sync init (ContentSearch.jsm).
Priority: -- → P1
Whiteboard: [fxsearch]
Latest patch finally passed all the tests, and I'm much happier with how it looks test wise.

Just need florian to review the test changes.
Flags: needinfo?(florian)
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review210996

::: browser/components/search/test/browser_google.js:14
(Diff revision 4)
>  
>  function test() {
>    let engine = Services.search.getEngineByName("Google");
>    ok(engine, "Google");
>  
> -  let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8";
> +  let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8&client=firefox-b-1";

Why do we have two tests left in this folder that hardcode US specific urls? Shouldn't we run these for all the three regions too?

::: browser/components/search/test/browser_google.js:23
(Diff revision 4)
>    // Test search URLs (including purposes).
>    url = engine.getSubmission("foo").uri.spec;
>    is(url, base, "Check search URL for 'foo'");
>    url = engine.getSubmission("foo", null, "contextmenu").uri.spec;
>    is(url, base, "Check context menu search URL for 'foo'");
> -  url = engine.getSubmission("foo", null, "keyword").uri.spec;
> +   url = engine.getSubmission("foo", null, "keyword").uri.spec;

I already mentioned this typo in comment 6.

::: browser/components/search/test/google_2018/browser.ini:4
(Diff revision 4)
> +[DEFAULT]
> +prefs =
> +  browser.search.geoip.url='data:application/json,{}'
> +  browser.search.geoSpecificDefaults.url='data:application/json,{}'

Why do we need to set these preferences in each folder? Shouldn't this be set by default for all browser mochitests already?

::: browser/components/search/test/google_2018/browser.ini:5
(Diff revision 4)
> +[DEFAULT]
> +prefs =
> +  browser.search.geoip.url='data:application/json,{}'
> +  browser.search.geoSpecificDefaults.url='data:application/json,{}'
> +  browser.search.region='US'

Can we keep the normal parent folder for the US case?

Each new folder causes the browser to fully restart when running the test suite, and this takes some time, so it's better to limit restarts to only what we really need.

::: browser/components/search/test/google_2018/browser.ini:6
(Diff revision 4)
> +[DEFAULT]
> +prefs =
> +  browser.search.geoip.url='data:application/json,{}'
> +  browser.search.geoSpecificDefaults.url='data:application/json,{}'
> +  browser.search.region='US'
> +  browser.search.countryCode='US'

I would prefer if we could limit ourselves to setting only one pref in each folder. Can we skip the region or countryCode pref?
Attachment #8928713 - Flags: review+ → review-
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] from comment #15)
> Why do we have two tests left in this folder that hardcode US specific urls?
> Shouldn't we run these for all the three regions too?

Because these tests are an absolute mess. browser_google.js duplicates what we're doing in the google_codes test, but also does the EXPECTED_ENGINE test which makes it difficult to do for multiple regions. browser_google_behavior technically doesn't need to exist. It was created because of artifact issues with searchEngineBehavior, but then searchEngineBehavior was turned off for artifact as well.

> ::: browser/components/search/test/google_2018/browser.ini:4
> (Diff revision 4)
> > +[DEFAULT]
> > +prefs =
> > +  browser.search.geoip.url='data:application/json,{}'
> > +  browser.search.geoSpecificDefaults.url='data:application/json,{}'
> 
> Why do we need to set these preferences in each folder? Shouldn't this be
> set by default for all browser mochitests already?

You are correct about browser.search.geoip.url - I can remove that.
browser.search.geoSpecificDefaults.url was set in the tests, but we've switched to setting region/countryCode because we can't set the correct JSON in browser.ini. So we at least need to set that so we don't hit the absearch server.

> ::: browser/components/search/test/google_2018/browser.ini:5
> (Diff revision 4)
> > +[DEFAULT]
> > +prefs =
> > +  browser.search.geoip.url='data:application/json,{}'
> > +  browser.search.geoSpecificDefaults.url='data:application/json,{}'
> > +  browser.search.region='US'
> 
> Can we keep the normal parent folder for the US case?

Probably.

> I would prefer if we could limit ourselves to setting only one pref in each
> folder. Can we skip the region or countryCode pref?

I'll check.
I think this is what you were thinking. All issues addressed.
Too late for 57, please fill an uplift request to 58 when ready.
Florian:

Are you getting notifications of the new review requests?
Flags: needinfo?(florian)
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review211856

::: browser/components/search/test/browser_google.js:10
(Diff revision 5)
>   * Test Google search plugin URLs
>   */
>  
>  "use strict";
>  
> -function test() {
> +const EXPECTED_ENGINE_BASE = {

Please make this: let expectedEngine = 
and get rid of EXPECTED_ENGINE_BASE completely.

::: browser/components/search/test/browser_google.js:58
(Diff revision 5)
>  
> -  isSubObjectOf(EXPECTED_ENGINE, engine, "Google");
> +let expectedEngine = EXPECTED_ENGINE_BASE;
> +
> +function test() {
> +  let engine = Services.search.getEngineByName("Google");
> +  ok(engine, "Google");

This string should be a lot more descriptive, eg. "found the Google engine"

::: browser/components/search/test/browser_google.js:61
(Diff revision 5)
> +function test() {
> +  let engine = Services.search.getEngineByName("Google");
> +  ok(engine, "Google");
> +
> +  let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8";
> +  let keywordAddl = "";

This variable name is hard to read. Maybe 'keywordSuffix'?

::: browser/components/search/test/browser_google.js:64
(Diff revision 5)
> +
> +  let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8";
> +  let keywordAddl = "";
> +
> +  let countryCode = Services.prefs.getCharPref("browser.search.countryCode");
> +  switch (countryCode) {

This whole block has more code duplication than I'm willing to r+.

Suggestion:

let code = "";
switch (countryCode)
  case US
    code = "firefox-b-1";
    break;
  case DE
    code = "firefox-b";
    
let base = "https://www.google.com/search?q=foo&ie=utf-8&oe=utf-8";
let keywordBase = base;
let urlParams = expectedEngine.wrappedJSObject._urls[1].params;

if (code) {
  let suffix = `&client=${code}`
  base += suffix;
  keywordBase += suffix + "-ab";
  expectedEngine.searchForm += suffix;
  urlParams.push(...
}

::: browser/components/search/test/browser_google.js:95
(Diff revision 5)
> +            "name": "client",
> +            "value": "firefox-b",
> +            "purpose": "searchbar",
> +          });
> +      break;
> +  }

I would probably add:
  case "RU":
    // We also cover this country in the test, but it doesn't require any partner code.
    break;

Just to make it very explicit that we are covering all cases and not accidentally forgetting one.

::: browser/components/search/test/browser_google.js:103
(Diff revision 5)
> +  let keywordBase = base + keywordAddl;
> +
> +  let url;
> +
> +  // Test search URLs (including purposes).
> +  url = engine.getSubmission("foo").uri.spec;

I'm going to pretend I haven't seen the code duplication here because it's not new, you are just moving this ugly code :-).

::: browser/components/search/test/browser_google.js:130
(Diff revision 5)
> +     "Check alternate domain");
> +
> +  // Check all other engine properties.
> +
> +
> +  isSubObjectOf(expectedEngine, engine, "Google");

I see no reason for the 2 empty lines between the comment and this instruction.

::: browser/components/search/test/browser_google_behavior.js:24
(Diff revision 5)
>      submission: "",
>    },
>    name: "Google",
>  }];
>  
> +let searchEngineDetails = SEARCH_ENGINE_DETAILS;

What's the point of this change / the new variable?

::: browser/components/search/test/browser_google_behavior.js:27
(Diff revision 5)
>  }];
>  
> +let searchEngineDetails = SEARCH_ENGINE_DETAILS;
> +
> +let countryCode = Services.prefs.getCharPref("browser.search.countryCode");
> +switch (countryCode) {

let code = "";

::: browser/components/search/test/browser_google_behavior.js:29
(Diff revision 5)
> +let searchEngineDetails = SEARCH_ENGINE_DETAILS;
> +
> +let countryCode = Services.prefs.getCharPref("browser.search.countryCode");
> +switch (countryCode) {
> +  case "US":
> +    searchEngineDetails[0].codes.context = "&client=firefox-b-1";

code = "firefox-b-1";

::: browser/components/search/test/browser_google_behavior.js:40
(Diff revision 5)
> +    searchEngineDetails[0].codes.context = "&client=firefox-b";
> +    searchEngineDetails[0].codes.newTab = "&client=firefox-b";
> +    searchEngineDetails[0].codes.submission = "&client=firefox-b";
> +    searchEngineDetails[0].codes.keyword = "&client=firefox-b-ab";
> +    break;
> +}

I would also add a comment about the RU case here.

if (code) {
  let codes = searchEngineDetails[0].codes;
  code = "&client=" + code;
  codes.context = code;
  ...
  codes.keyword = code + "-ab";
}
Attachment #8928713 - Flags: review?(florian) → review-
(In reply to Mike Kaply [:mkaply] from comment #20)
> Florian:
> 
> Are you getting notifications of the new review requests?

Yes (and I read all bugmail for bugs I'm involved/cc'ed on).
Flags: needinfo?(florian)
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review208526

browser_google_codes.js tests that behavior.
Thanks for the input, florian. There definitely was too much duplication. Thank you for giving me ideas to solve it.
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review212030

Looks much better.

::: browser/components/search/test/browser_google.js:80
(Diff revisions 5 - 6)
> +  if (code) {
> +    let suffix = `&client=${code}`;
> +    base += suffix;
> +    keywordBase += `${suffix}-ab`;
> +    expectedEngine.searchForm += suffix;
> -      expectedEngine.wrappedJSObject._urls[1].params.push({
> +    expectedEngine.wrappedJSObject._urls[1].params.push({

let urlParams = expectedEngine.wrappedJSObject._urls[1].params;

::: browser/components/search/test/browser_google.js:81
(Diff revisions 5 - 6)
> +    let suffix = `&client=${code}`;
> +    base += suffix;
> +    keywordBase += `${suffix}-ab`;
> +    expectedEngine.searchForm += suffix;
> -      expectedEngine.wrappedJSObject._urls[1].params.push({
> +    expectedEngine.wrappedJSObject._urls[1].params.push({
> -            "name": "client",
> +          "name": "client",

2 space indent is enough. You don't need the quotes around the field names (it's a JS object, not JSON data).

::: browser/components/search/test/browser_google.js:101
(Diff revisions 5 - 6)
> -  is(url, base, "Check search URL for 'foo'");
> -  url = engine.getSubmission("foo", null, "contextmenu").uri.spec;
> -  is(url, base, "Check context menu search URL for 'foo'");
> +  for (let purpose of purposes) {
> +    url = engine.getSubmission("foo", null, purpose).uri.spec;
> +    is(url, base, `Check ${purpose} search URL for 'foo'`);
> +  }
>    url = engine.getSubmission("foo", null, "keyword").uri.spec;
> -  is(url, keywordBase, "Check keyword search URL for 'foo'");
> +  is(url, keywordBase, `Check  search URL for 'foo'`);

nit: double space. Or actually, it seems you missed the word "keyword".

Also, please avoid template strings and use standard double quotes when there's no replacement.
Attachment #8928713 - Flags: review?(florian) → review-
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

https://reviewboard.mozilla.org/r/199974/#review212054

Looks good to me now. Assuming it's green on try, r=me.
Attachment #8928713 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/19a5f1cecfc3
New search defaults for browser and mobile. r=florian,mconnor
https://hg.mozilla.org/mozilla-central/rev/19a5f1cecfc3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Please request Beta approval on this when you get a chance.
Flags: needinfo?(mozilla)
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

Approval Request Comment
[Feature/Bug causing the regression]: Move our default search into the tree
[User impact if declined]: Possibility of not getting correct search engine in some cases.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Very well tested, small actual change.
[String changes made/needed]: None
Flags: needinfo?(mozilla)
Attachment #8928713 - Flags: approval-mozilla-beta?
Comment on attachment 8928713 [details]
Bug 1417678 - New search defaults for browser and mobile.

Move new search default changes in the tree. Beta58+.
Attachment #8928713 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi, this needs l10n+ because of changes to mobile's region.properties
Flags: needinfo?(lebedel.delphine)
Note the region.properties changes aren't mandatory (because I don't query them due to the pref changes).

I wonder if they need to stay behind because of the unified l10n trees?
(Stealing delphine's NI)

Go ahead and land, this doesn't have any practical effect on localization. Change to region.properties only affects en-US, that file isn't exposed to localization tools and any change requires approval from l10n-drivers before landing.
Flags: needinfo?(lebedel.delphine)
You need to log in before you can comment on or make changes to this bug.