Closed Bug 923795 Opened 11 years ago Closed 11 years ago

Add condition to MozParam to detect top 2 (or N) position

Categories

(Firefox for Android Graveyard :: General, defect)

24 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed, fennec26+)

RESOLVED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed
fennec 26+ ---

People

(Reporter: krudnitski, Assigned: blassey)

References

Details

Attachments

(1 file, 5 obsolete files)

We need to know the search position (other than the default) of a search provider based on conditional rev share agreements. ie whether a search provider provided search results when it was in position 2.
needs to be query-able
Apologies for the chatter. Bear with me.

Need: a mozparam that allows us to query whether a search provider is in position number 1 or 2.
Doesn't condition="defaultEngine" work well enough? Or do you think you need condition="secondEngine" too? This seems a little like a slippery slope. How long before "thirdEngine" ? Do we have other alternatives?
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Doesn't condition="defaultEngine" work well enough? Or do you think you need
> condition="secondEngine" too? This seems a little like a slippery slope. How
> long before "thirdEngine" ? Do we have other alternatives?

Updating the summary to reflect what I think needs to be done here.

We currently condition on "defaultEngine" because that is the only position of value in Desktop Firefox. On Android, positions 2 through X also have some value, so we need to be able to detect that and change our partner params based onit.
Summary: need to 'get search position' in the search provider list → Add condition to MozParam to detect top 2 (or N) position
Attached patch topN.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #814456 - Flags: review?(gavin.sharp)
Comment on attachment 814456 [details] [diff] [review]
topN.patch

Duplicating the "determine search engine order" code from the search service itself isn't ideal. In fact the pref-reading code is already somewhat duplicated in getDefaultEngines and _buildSortedEngineList :( At the very least you could share code with the getDefaultEngines case you're copying from.

I seem to recall that the new search prefs UI in Fennec lets you re-order engines (if not directly, then indirectly by changing default engines). Assuming that's the case, is it really desired that this logic depend on the default engine order, rather than the current engine order?
Attachment #814456 - Flags: review?(gavin.sharp) → review-
the UI doesn't let you reorder your engines, though I filed a bug 924461 earlier today to do that.

I'm not seeing the distinction of current order and default order. If they are different, how would you determine current order?
Flags: needinfo?(gavin.sharp)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6581 changes the "current order".

The "current order" is stored in the search service's _sortedEngines. But if you wanted to use that instead of the default order, there's another problem: currently all the MozParams values are set at engine parse time, which is before the sorted engine list is determined. To do that you'd need to change the MozParam values to be obtained when getSubmission is called, which would involve more search service changes (but probably isn't too complicated?).
Flags: needinfo?(gavin.sharp)
tracking-fennec: ? → 26+
Attached patch topN.patch (obsolete) — Splinter Review
Attachment #814456 - Attachment is obsolete: true
Attachment #815533 - Flags: review?(gavin.sharp)
Comment on attachment 815533 [details] [diff] [review]
topN.patch

Review of attachment 815533 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchService.js
@@ +914,5 @@
> +  },
> +
> +  reevalMozParams: function(engine) {
> +    try {
> +      for each (param in this.params) {

Drive-by: for each...in is deprecated, so you shouldn't use it:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in
(In reply to :Margaret Leibovic from comment #10)
> 
> Drive-by: for each...in is deprecated, so you shouldn't use it:
> https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/
> for_each...in

according to mdn, the replacement isn't implemented https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of?redirectlocale=en-US&redirectslug=JavaScript%2FReference%2FStatements%2Ffor...of
That page doesn't say that - for...of is certainly implemented and you should use it. That green warning box at the top is not relevant to chrome JS.
I was referring to the "Implemented in: None" in the Version info table. If it is implemented, it seems like we should fix those docs.
Comment on attachment 815533 [details] [diff] [review]
topN.patch

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>   addParam: function SRCH_EURL_addParam(aName, aValue, aPurpose) {
>-    this.params.push(new QueryParameter(aName, aValue, aPurpose));
>+    this.params[aName] = new QueryParameter(aName, aValue, aPurpose);
>   },

>+  setParam: function SRCH_EURL_setParam(aName, aValue, aPurpose) {
>+    this.params[aName] = new QueryParameter(aName, aValue, aPurpose);
>+  },

This will regress the behavior for multiple same-name parameters, which is probably undesirable (we should have a test for that if we don't already).

>+  reevalMozParams: function(engine) {
>+    try {
>+      for each (param in this.params) {
>+      let mozparam = this.mozparams[param.name];
>+      if (mozparam && mozparam.reeval) {
>+        if (mozparam.condition != null && mozparam.condition.indexOf("top") == 0) {

get rid of the "!= null", and use mozparam.condition.startsWith("top")

>+          let positionStr = mozparam.condition.substring(3, mozparam.condition.length);

mozparam.condition.slice(3)

>+          let pos = parseInt(positionStr);

parseInt(positionStr, 10);

>+          // This will throw if we're not initialized yet, just ignore and move on, we'll
>+          // have the falseValue through initialization (checking isInitialized also throws)
>+          let engines = Services.search.getVisibleEngines({});

isInitialized really shouldn't throw? Unless I suppose this is being called synchronously from under the original search service getService() call, in which case there's probably some infinite recursion protection. Shouldn't happen with async initialization, though, so would be curious if you are hitting the sync init path for some reason.

I think a better solution would ensure that this never gets called until a getSubmission call occurs (which should be guaranteed to be after service initialization is complete). My suggested change below might fix that?

(It might also be nicer to just have a SearchService._isEngineTopN(engine) helper that you can just call directly and that fails gracefully.)
 
>   _getURLOfType: function SRCH_ENG__getURLOfType(aType) {
>     for (var i = 0; i < this._urls.length; ++i) {
>-      if (this._urls[i].type == aType)
>+      if (this._urls[i].type == aType) {
>+        this._urls[i].reevalMozParams(this);

This seems like the wrong place to put this - SRCH_EURL_getSubmission seems better?

>@@ -1647,7 +1679,8 @@ Engine.prototype = {

>+          default:
>+            try {

Why try/catch?

>+              if (condition != null && condition.indexOf("top") == 0) {

>+                let mozparam = {"name": param.getAttribute("name"),
>+                                  "falseValue": param.getAttribute("falseValue"),
>+                                  "trueValue": param.getAttribute("trueValue"),
>+                                  "condition": condition,
>+                                  "reeval": true};

Rather than using "reeval", why not something more specific - "positionDependent: true". That way in reevalMozParams you don't need to check for beginsWith("top") again.
Attachment #815533 - Flags: review?(gavin.sharp) → review-
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #13)
> I was referring to the "Implemented in: None" in the Version info table. If
> it is implemented, it seems like we should fix those docs.

Oh, we just stopped issuing "versions" of JS after 1.8.5 I think. Not sure what you'd put there, seems like the template just needs changing.
Attached patch topN.patch (obsolete) — Splinter Review
Attachment #815533 - Attachment is obsolete: true
Attachment #815966 - Flags: review?(gavin.sharp)
Gavin, review ping?
Attached patch tweaked patch (obsolete) — Splinter Review
Sorry for the delay, (mini-)work week.

I made some tweaks to this; does this patch work?

Can you write a test for this functionality? See toolkit/components/search/tests/xpcshell/
Attachment #815966 - Attachment is obsolete: true
Attachment #815966 - Flags: review?(gavin.sharp)
Attachment #818727 - Flags: feedback?(blassey.bugs)
Comment on attachment 818727 [details] [diff] [review]
tweaked patch

Review of attachment 818727 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchService.js
@@ +925,5 @@
> +        } catch (ex) {
> +          LOG("reevalMozParams called before search service initialization!?");
> +          break;
> +        }
> +        let index = engines.indexOf(this); // XXX

this doesn't work. First this is a URL, not an engine. Second, the engines aren't equal. To test, I have this code:

        let index = -1;//engines.indexOf(engine); // XXX
        for (let i = 0; i < position && i < engines.length; i++) {
		        if (engine.name == engines[i].name) {
		            index = i;
                Services.console.logStringMessage("names are equal bug engines == : " + (engine == engines[i]));
                Services.console.logStringMessage("names are equal bug engines === : " + (engine === engines[i]));
            } else
                Services.console.logStringMessage(engine.name + " != " + engines[i].name);
		    }
        Services.console.logStringMessage("index: " + index);
        let isTopN = index > -1 && (index + 1) <= position;
        param.value = isTopN ? mozparam.trueValue : mozparam.falseValue;

on the console I get:
E/GeckoConsole( 9442): names are equal bug engines == : false
E/GeckoConsole( 9442): names are equal bug engines === : false
E/GeckoConsole( 9442): index: 1
Attachment #818727 - Flags: feedback?(blassey.bugs) → feedback-
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #19)
> > +        let index = engines.indexOf(this); // XXX
> 
> this doesn't work. First this is a URL, not an engine. Second, the engines
> aren't equal. To test, I have this code:

How about engines[i].wrappedJSObject == engine? I.e. engines.map((e) => e.wrappedJSObject).indexOf(engine.wrappedJSObject) might work.

Otherwise, let's just use a loop:
for (let i = 0; i < engines.length; i++)
  if (engines[i].name == engine.name) {
    index = i;
    break;
  }
}
Attached patch topN.patch (obsolete) — Splinter Review
Attachment #818727 - Attachment is obsolete: true
Attachment #820227 - Flags: review?(gavin.sharp)
Attached patch topN.patchSplinter Review
Attachment #820227 - Attachment is obsolete: true
Attachment #820227 - Flags: review?(gavin.sharp)
Attachment #820261 - Flags: review?(gavin.sharp)
Comment on attachment 820261 [details] [diff] [review]
topN.patch

Still needs a test. It would be good to have it cover that things continue to work after the engineURL is serialized/deserialized to disk.
Attachment #820261 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18)
> Can you write a test for this functionality? See
> toolkit/components/search/tests/xpcshell/

I don't see any tests for conditional mozparams there other than testing serialization to disk.
https://hg.mozilla.org/mozilla-central/rev/53382ea36e36
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment on attachment 820261 [details] [diff] [review]
topN.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): not very risky, shouldn't effect anything not using the flag
String or IDL/UUID changes made by this patch:
Attachment #820261 - Flags: approval-mozilla-aurora?
Attachment #820261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[/c/src-gecko/aurora]$ qimp 923795
Fetching... done
Parsing... done
adding 923795 to series file
renamed 923795 -> topN.patch
applying topN.patch
patching file mobile/locales/en-US/searchplugins/yahoo.xml
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file mobile/locales/en-US/searchplugins/yahoo.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh topN.patch
Comment on attachment 820261 [details] [diff] [review]
topN.patch

Now that uplift has happened, this needs to land on beta
Attachment #820261 - Flags: approval-mozilla-beta?
(In reply to Ed Morley [:edmorley UTC+1] from comment #27)
> [/c/src-gecko/aurora]$ qimp 923795
> Fetching... done
> Parsing... done
> adding 923795 to series file
> renamed 923795 -> topN.patch
> applying topN.patch
> patching file mobile/locales/en-US/searchplugins/yahoo.xml
> Hunk #1 FAILED at 10
> 1 out of 1 hunks FAILED -- saving rejects to file
> mobile/locales/en-US/searchplugins/yahoo.xml.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh topN.patch

This patch needs to land on top of bug 903082
Depends on: 903082
Attachment #820261 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [checkin-needed-beta]
Depends on: 999851
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: