1460419 - Remove US preference code from search

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Mike Kaply [:mkaply]]

Now that we've moved default engine to list.json and search order is the same for all languages, we can remove the US specific code from the search service.

Comment 1

[Mike Kaply [:mkaply]]

Two tests, test_location_malformed_json and test_location_partner, depend on the "getIsUS() fell back to a timezone check with the result=" message coming from the isUS function.

I've tried to figure out how to rewrite these tests, but I'm not entirely clear how to.

This will have to wait until Florian gets back.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: First pass

Comment 3

[Mike Kaply [:mkaply]]

Florian:

I'm having a lot of trouble with these tests that depend on the isUS error message in order to work.

Any advice appreciated.

Comment 4

[Florian Quèze [:florian]]

Is the timezone check still needed for anything?

Comment 5

[Mike Kaply [:mkaply]]

> Is the timezone check still needed for anything?

We were considering keeping it as an extra check to make sure when we got US from the geolocation, you were actually in a US timezone (We're still having a good percentage of mismatches).

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Bug 1460419 - Remove geoSpecificPref code from search service.

Review commit: https://reviewboard.mozilla.org/r/254046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254046/

Comment 7

[Mike Kaply [:mkaply]]

I left the timezone code in because we might use it for something else.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8988926 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

https://reviewboard.mozilla.org/r/254046/#review260996

At this point the timezone code seems to only be used for telemetry (and I doubt anybody is looking at the data collected for that probe). In comment 7 you wrote we might use it for something else. Do you have something specific in mind? If not, I think we could as well cleanup further.

::: toolkit/components/search/nsSearchService.js
(Diff revision 1)
>  // Method to determine if we should be using geo-specific defaults
>  function geoSpecificDefaultsEnabled() {
>    return Services.prefs.getBoolPref("browser.search.geoSpecificDefaults", false);
>  }
>  
> -// Some notes on countryCode and region prefs:

Should we keep this comment explaining the difference between the two countryCode and region prefs? I guess only the first 2 points are still relevant. Maybe move this to above the storeCountryCode method?

Even better would be to merge these 2 prefs and get rid of the mess, but that's clearly out of scope for this bug :-).

::: toolkit/components/search/nsSearchService.js:3570
(Diff revision 1)
>            }
>          } catch (e) { }
>  
> -        let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
>          while (true) {
> -          prefName = prefNameBase + "." + (++i);
> +          prefName = `${BROWSER_SEARCH_PREF}order.{(++i)}`;

This should be ${++i} rather than {(++i)} right?

::: toolkit/components/search/nsSearchService.js:4123
(Diff revision 1)
>          }
>  
> -        let prefNameBase = getGeoSpecificPrefName(BROWSER_SEARCH_PREF + "order");
>          let i = 0;
>          while (!sendSubmissionURL) {
> -          let prefName = prefNameBase + "." + (++i);
> +          let prefName = `${BROWSER_SEARCH_PREF}order.{(++i)}`;

same here

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 8988926 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

https://reviewboard.mozilla.org/r/254046/#review260996

We had thought about using it as a simple validation against the results from the location server. I'll ask Mike C.

> Should we keep this comment explaining the difference between the two countryCode and region prefs? I guess only the first 2 points are still relevant. Maybe move this to above the storeCountryCode method?
> 
> Even better would be to merge these 2 prefs and get rid of the mess, but that's clearly out of scope for this bug :-).

Merging those two prefs is next on my list: https://bugzilla.mozilla.org/show_bug.cgi?id=1462015.

> This should be ${++i} rather than {(++i)} right?

Yeah. I had kept the old style only because I copied it.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8988926 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254046/diff/1-2/

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8988926 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

https://reviewboard.mozilla.org/r/254046/#review262238

What did Mike C say about the timezone check?

::: toolkit/components/search/nsSearchService.js:3576
(Diff revisions 1 - 2)
>              addedEngines[engine.name] = engine;
>            }
>          } catch (e) { }
>  
>          while (true) {
> -          prefName = `${BROWSER_SEARCH_PREF}order.{(++i)}`;
> +          prefName = `${BROWSER_SEARCH_PREF}order.{++i}`;

The '$' is still missing. Have you tested this part of the patch?

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 8988926 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

https://reviewboard.mozilla.org/r/254046/#review262238

We need that for telemetry anyway, so it's going to stay for now.

Comment 13

[Mike Kaply [:mkaply]]

Attachment: Bug 1460419 - Remove geoSpecificPref code from search service.

Comment 14

[Phabricator Automation]

Comment on attachment 8990793 [details]
Bug 1460419 - Remove geoSpecificPref code from search service.

Florian Quèze [:florian] has approved the revision.

https://phabricator.services.mozilla.com/D2036

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6224409094a
Remove geoSpecificPref code from search service. r=florian

Comment 16

[Cosmin Sabou [:CosminS]]

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