Closed
Bug 1001566
Opened 10 years ago
Closed 10 years ago
Yahoo search plugin should use nresults param for smaller requests
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox31 fixed, firefox32 fixed)
RESOLVED
FIXED
Firefox 32
People
(Reporter: mconnor, Assigned: mconnor)
Details
Attachments
(1 file)
7.09 KB,
patch
|
Margaret
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
There's other stuff we can use (see bug 1001528 for an explanation), but for mobile we should adopt nresults right now, since we only use the first four results, and that makes the request half the size on average. I'm going to make this a MozParam so we can share between plugins/tweak the value centrally. Note that this also cleans up the search plugin to use a properly parameterized search suggestion URL, which will made testing easier (i.e. how we've done desktop in bug 998071 ) Note that this patch is untested because I still don't have a working Android build VM. Maybe next week!
Attachment #8412783 -
Flags: review?(margaret.leibovic)
Comment 1•10 years ago
|
||
Comment on attachment 8412783 [details] [diff] [review] limitYahooResultsAndroid Review of attachment 8412783 [details] [diff] [review]: ----------------------------------------------------------------- I tested this patch, and it works. ::: mobile/android/app/mobile.js @@ +239,5 @@ > > // pointer to the default engine name > pref("browser.search.defaultenginename", "chrome://browser/locale/region.properties"); > +// maximum number of search suggestions, as a string because the search service expects a string pref > +pref("browser.search.param.maxSuggestions", "4") You're missing a semicolon. You need to add that for this patch to work. @@ -523,5 @@ > -#ifdef MOZ_OFFICIAL_BRANDING > -pref("browser.search.param.yahoo-fr", "moz35"); > -pref("browser.search.param.yahoo-fr-cjkt", "moz35"); > -pref("browser.search.param.yahoo-fr-ja", "mozff"); > -#endif What were these bits used for before?
Attachment #8412783 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 2•10 years ago
|
||
Those removed bits were the desktop codes, which we just don't use now (we've got mozilla_mobile_search everywhere)
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ca97a75fa7
Comment 4•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d8ca97a75fa7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment 5•10 years ago
|
||
Mike, is this bug going to land on other branches? Currently is only on central. Asking to be sure it's safe to update l10n searchplugin on Aurora with these settings.
Flags: needinfo?(mconnor)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8412783 [details] [diff] [review] limitYahooResultsAndroid Good point, I'll ask for uplift so we can roll it in. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: unnecessary data usage Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): very minimal String or IDL/UUID changes made by this patch: n/a Uplifting this will allow us to apply this change across Yahoo plugins (which get updated on Aurora) as a part of a larger update to those plugins. Should be very very safe.
Attachment #8412783 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(mconnor)
Updated•10 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•10 years ago
|
Attachment #8412783 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•