Closed Bug 1460419 Opened Last year Closed Last year

Remove US preference code from search

Categories

(Firefox :: Search, enhancement, P1)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P1
Blocks: 1437942
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.
Attached patch First pass (obsolete) — Splinter Review
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.
Flags: needinfo?(florian)
Is the timezone check still needed for anything?
Flags: needinfo?(florian)
> 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).
Depends on: 1463162
I left the timezone code in because we might use it for something else.
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
Attachment #8988926 - Flags: review?(florian) → review-
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 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?
Attachment #8988926 - Flags: review?(florian) → review-
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.
Attachment #8988926 - Attachment is obsolete: true
Attachment #8976137 - Attachment is obsolete: true
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
Attachment #8990793 - Flags: review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/f6224409094a
Remove geoSpecificPref code from search service. r=florian
https://hg.mozilla.org/mozilla-central/rev/f6224409094a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.