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)
Tracking
(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed, fennec26+)
RESOLVED
FIXED
Firefox 27
People
(Reporter: krudnitski, Assigned: blassey)
References
Details
Attachments
(1 file, 5 obsolete files)
3.67 KB,
patch
|
Gavin
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
needs to be query-able
Reporter | ||
Comment 2•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
(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
Assignee | ||
Comment 5•11 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #814456 -
Flags: review?(gavin.sharp)
Comment 6•11 years ago
|
||
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-
Assignee | ||
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
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)
Updated•11 years ago
|
tracking-fennec: ? → 26+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #814456 -
Attachment is obsolete: true
Attachment #815533 -
Flags: review?(gavin.sharp)
Comment 10•11 years ago
|
||
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
Assignee | ||
Comment 11•11 years ago
|
||
(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
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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-
Comment 15•11 years ago
|
||
(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.
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #815533 -
Attachment is obsolete: true
Attachment #815966 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 17•11 years ago
|
||
Gavin, review ping?
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
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-
Comment 20•11 years ago
|
||
(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; } }
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #818727 -
Attachment is obsolete: true
Attachment #820227 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #820227 -
Attachment is obsolete: true
Attachment #820227 -
Flags: review?(gavin.sharp)
Attachment #820261 -
Flags: review?(gavin.sharp)
Comment 23•11 years ago
|
||
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+
Assignee | ||
Comment 24•11 years ago
|
||
(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.
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53382ea36e36
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Updated•11 years ago
|
status-firefox26:
--- → affected
status-firefox27:
--- → fixed
Assignee | ||
Comment 26•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #820261 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•11 years ago
|
||
[/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
Keywords: branch-patch-needed
Assignee | ||
Comment 28•11 years ago
|
||
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?
Assignee | ||
Comment 29•11 years ago
|
||
(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
Keywords: branch-patch-needed
Updated•11 years ago
|
Attachment #820261 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Updated•11 years ago
|
Whiteboard: [checkin-needed-beta]
Comment 30•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/2c4fd4ea9c0e
Whiteboard: [checkin-needed-beta]
Comment 31•11 years ago
|
||
Automated merge from beta to b2g26. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2c4fd4ea9c0e
status-b2g-v1.2:
--- → fixed
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
•