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

Status: RESOLVED WONTFIX

(Firefox :: Search, defect, P3)

Description

[Malte Kraus]

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

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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

(Sorry for the delay in responding here)

Comment 2

[Malte Kraus]

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.)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 4

[Malte Kraus]

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.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

OK. So all we need to do is something like http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/search/nsSearchService.js&rev=1.108#2161 in addParam, right? Would you be able to attach a patch?

Comment 6

[Malte Kraus]

Attachment: patch

Comment 7

[Mark Banner (:standard8)]

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