Currently, nsStandardURL::SetFilePath returns NS_ERROR_NOT_IMPLEMENTED This function should be implemented. Bug 100944 depends on this function to work.

should be a quick fix... didn't mean to leave this out of the patch!

-> me

This broke HTML signatures.

Andreas: Doing this also removes the query and hash. In my opinion, SetFilePath should keep them; because if you'd not want them, you would use SetPath.

Are you sure? It was implemented this way in good old nsStdURL too and as far as I know SetPath does not remove query and ref. If SetPath does this, it's a bug.

I tried your patch, and query and ref are removed. Also, documentation for nsIURI suggests that SetPath should remove query and ref, because for nsIURI, they're part of the path. http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsIURI.idl#51 So I think that SetFilePath should not remove query and ref, because it would then be redundant with SetPath.

i agree with christian on this... path is everything after the authority, whereas filepath is only that containing the directory and filename.

Hmmm ... If I remember correctly just calling SetPath inside SetFilePath was a quick hack in nsStdURL and that status I wanted to get back with this patch. This could not have worked prior to darins work too. You are right, SetFilePath *should* leave param, query and ref untouched in the base url.

A first cut on a real SetFilePath implementation. It still has problems with the leading slash in the directory and other stuff. One thing for sure: This implementation of nsIURL maybe faster but it is a lot more complicated to change than the old one.

andreas... yeah... the new impl complicates the url part setters for sure, but i think we can simplify your patch by checking if the param, query, and ref are all absent. then SetPath can simply be called. andreas: at the time when i was working on these standard url changes, i considered keeping an array of the URL parts and only recombining them on demand. this would have probably resulted in a less complex implementation.

If SetFilePath is called we explicitly state that there are no param, query or ref present in the string, so everthing gets folded into directory, filebasename and fileextension. When getting them back in the spec ; ? and # will all get escaped as part of the filebasename for example to mask them.

This version now can deal with a missing leading /. However I still see an assertion when both directory and filebasename are present in the string given to SetFilePath: ###!!! ASSERTION: nsDependentCString must wrap only null-terminated strings: '!*aEndPtr', file ../../dist/include/string/nsDependentString.h, line 160 ###!!! Break: at file ../../dist/include/string/nsDependentString.h, line 160 Reagarding my prior comment, I hoped # ; and ? would get escaped, but that does not happen, still investigating.

andreas: yeah, good point.

Hmmm ... all setters except SetSpec are missing the escaping step, the normalized url contained in mSpec can only be escaped. This is serious ...

andreas: you're right... i've filed bug 110846 to cover that issue.

andreas: i think it might be better to just call build up a new url and call SetSpec on the resulting url. i don't think we need to worry about making SetFilePath so efficient. i'll attach a patch that does this.

this one just calls SetSpec... see my patch for bug 110846 to see why i'm not calling SetPath.

If you call SetFilePath on http://www.mozilla.org/path/path2/filebase.extension;xxxx?gggg#hhhh with "aaaaa/file#fffff" you get http,,,www.mozilla.org,-1,/aaaaa/,file,,,,fffff;xxxx?gggg#hhhh,http://www.mozilla.org/aaaaa/file#fffff;xxxx?gggg#hhhh which is wrong, because SetSpec which you use finds the first # that is part of the filename (and using SetFilePath we said it is!). Instead it should have been escaped by the setter. So using SetSpec is wrong as was using SetPath in old nsStdURL and in my first patch.

For the same reason calling SetPreHost in SetUsername is wrong.

your right... i need to parse the filepath before building up the new candidate spec. new patch en route.

i don't see any problem with SetUsername... it only calls SetPreHost when the given username is NULL or zero length. in other words, it only calls SetPreHost to remove the prehost, because without a username there's no point having a password in the URL. so, i don't think the same problem exists with SetUsername.

nevermind... ic the problem with SetUsername now :P

marking this bug as dependent on bug 110846... actually, that bug will contain the fix for this bug. the patch for this bug is going to need some of the changes in the patch for bug 110846, so they might as well be combined.

FIXED now that the patch for bug 110846 has landed.

vrfy