Closed Bug 404201 Opened 14 years ago Closed 2 months ago

add comment to nsBrowserSearchService.idl stating that the parameters for addParam should already be uri encoded


(Firefox :: Search, defect, P3)






(Reporter: moz-bugs, Unassigned)


(Whiteboard: [fxsearch])


(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111705 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111705 Minefield/3.0b2pre

I'm using nsISearchEngine::addParam in my extension and it took me some releases and time digging through the code to figure out, that the params for it should already be URI encoded in the search engine's queryCharset. (It works with most charsets when the values are ASCII)

A comment in the idl stating the user should call nsITextToSubURI before addParam would help others not to stumble across this, I think.

If my CVS worked, I'd also attach a patch.

Reproducible: Always
Hrm, perhaps we should have addParam do the encoding? What do you think?

(Sorry for the delay in responding here)
Ever confirmed: true
Uh, yeah. That would be prettier for sure.

Though I think that some way to detect this change would be needed. (A wrappedJSObject hack might be enough - when using addEngineWithDetails there's currently no way around it anyways.)
Perhaps we could add a new method that does the encoding and just leave the existing addParam that adds parameters exactly as they're passed in, if we're really worried about compatibility?

I'm not sure how many existing users of addEngineWithDetails there really are, and how many would actually be affected by this change (since as you say, ASCII parameters/values aren't affected).
Version: unspecified → Trunk
I don't think there are too many users. I know of two extensions for adding custom search engines. (One of these uses addEngineWithDetails.) I don't think there's need for even more.

So to me, just doing the encoding in addParam and stating in a comment when that functionality was added sounds like the best option.
OK. So all we need to do is something like in addParam, right? Would you be able to attach a patch?
Attached patch patchSplinter Review
Priority: -- → P3
Whiteboard: [fxsearch]
Rank: 37

addParam is no longer in the search interface, and legacy style extensions are not supported.

Closed: 2 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.