Yahoo search plugin should use nresults param for smaller requests

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconnor, Assigned: mconnor)

Tracking

27 Branch
Firefox 32
All
Android
Points:
---

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
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 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

5 years ago
Those removed bits were the desktop codes, which we just don't use now (we've got mozilla_mobile_search everywhere)
https://hg.mozilla.org/mozilla-central/rev/d8ca97a75fa7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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

5 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)
Attachment #8412783 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.