Closed Bug 1461347 Opened 4 years ago Closed 4 years ago
Remove unused lines from mobile JS file
59 bytes, text/x-review-board-request
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?
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/d797ab8b1fd9 Remove unused enginename from JS, but add to properties. r=nalexander
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8647cfb89b8 Remove unused enginename from JS, but add to properties. r=nalexander
You need to log in before you can comment on or make changes to this bug.