1461347 - Remove unused lines from mobile JS file

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

[Mike Kaply [:mkaply]]

When I removed defaultenginename from search services, I missed a preference file in mobile.

These lines are not used at all - we only read defaultenginename in the distribution case.

See:

https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2793

Comment 1

[Mike Kaply [:mkaply]]

I also need to put back our default engine for now into region.properties on mobile until browsersearch.json is fixed.

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Mike Kaply [:mkaply] from comment #1)
> I also need to put back our default engine for now into region.properties on
> mobile until browsersearch.json is fixed.

Does it mean that we still need it for l10n too? It hasn't been removed, but it would affect brand new locales.

Comment 3

[Mike Kaply [:mkaply]]

> Does it mean that we still need it for l10n too? It hasn't been removed, but it would affect brand new locales.

Sadly yes it does. I'm investigating what to do about browsersearch.json. I didn't realize it had such a tight dependency on list.properties.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Bug 1461347 - Remove unused enginename from JS, but add to properties.

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8975514 [details]
Bug 1461347 - Remove unused enginename from JS, but add to properties.

https://reviewboard.mozilla.org/r/243770/#review249642

This looks fine to me.

::: mobile/android/app/mobile.js:269
(Diff revision 1)
>  
>  // Market-specific search defaults
>  pref("browser.search.geoSpecificDefaults", true);
>  pref("browser.search.geoSpecificDefaults.url", "https://search.services.mozilla.com/1/%APP%/%VERSION%/%CHANNEL%/%LOCALE%/%REGION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%");
>  
> -// US specific default (used as a fallback if the geoSpecificDefaults request fails).
> +// US specific default

nit: full sentence.

::: mobile/locales/en-US/chrome/region.properties:5
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Default search engine - Used only to generate browsersearch.json

nit: full sentence, lower case "u" for "Used".

Comment 6

[Nick Alexander :nalexander [he/him]]

mkaply: can you point me to the latest thinking about the (Desktop) search direction (specifically for the search JSON bits)?  It feels like we should get rid of browsersearch.json on Android and unify on whatever Desktop is doing (and hopefully, what other products are doing).  However, it's not clear to me that we should be doing that on any particular timeframe, given that we're not investing much in Fennec right now.  Can you tell me what the plans are?

Comment 7

[Mike Kaply [:mkaply]]

You already asked this :)

https://bugzilla.mozilla.org/show_bug.cgi?id=1437942#c7

I'm investigating removing browsersearch.json right now.

Near as I can tell, it's only used now to report the default search engine to telemetry (which is wrong - it's reporting the original default search engine regardless of what the user changes"

I'll be opening a bug to (hopefully) completely remove SearchEngineManager.java.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 8975514 [details]
Bug 1461347 - Remove unused enginename from JS, but add to properties.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d797ab8b1fd9
Remove unused enginename from JS, but add to properties. r=nalexander

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8647cfb89b8
Remove unused enginename from JS, but add to properties. r=nalexander

Comment 11

[Cosmin Sabou [:CosminS]]

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

Comment 12

[Natalia Csoregi [:nataliaCs]]

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