Closed Bug 1461347 Opened 4 years ago Closed 4 years ago

Remove unused lines from mobile JS file

Categories

(Firefox :: Search, enhancement)

60 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file)

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
I also need to put back our default engine for now into region.properties on mobile until browsersearch.json is fixed.
(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.
> 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 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".
Attachment #8975514 - Flags: review?(nalexander) → review+
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?
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/d797ab8b1fd9
Remove unused enginename from JS, but add to properties. r=nalexander
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8647cfb89b8
Remove unused enginename from JS, but add to properties. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/d797ab8b1fd9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.