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

NEW
Unassigned

Status

()

Firefox
Search
P3
trivial
Rank:
37
11 years ago
3 years ago

People

(Reporter: Malte Kraus, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

11 years ago
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
(Reporter)

Comment 4

10 years ago
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 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?
(Reporter)

Comment 6

10 years ago
Created attachment 301543 [details] [diff] [review]
patch

Updated

3 years ago
Priority: -- → P3
Whiteboard: [fxsearch]

Updated

3 years ago
Rank: 37
You need to log in before you can comment on or make changes to this bug.