110500 - nsStandardURL should implement SetFilePath

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Currently, nsStandardURL::SetFilePath returns NS_ERROR_NOT_IMPLEMENTED

This function should be implemented.

Bug 100944 depends on this function to work.

Comment 1

[Darin Fisher]

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

Comment 2

[Darin Fisher]

-> me

Comment 3

[Håkan Waara]

This broke HTML signatures.

Comment 4

[Andreas Otte]

Attachment: quick fix, please check if it does the job

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 6

[Andreas Otte]

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.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 8

[Darin Fisher]

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

Comment 9

[Andreas Otte]

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. 

Comment 10

[Andreas Otte]

Attachment: SetFilePath - work in progress

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.

Comment 11

[Darin Fisher]

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.

Comment 12

[Andreas Otte]

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.

Comment 13

[Andreas Otte]

Attachment: better version of SetFilePath

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.

Comment 14

[Darin Fisher]

andreas: yeah, good point.

Comment 15

[Andreas Otte]

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

Comment 16

[Darin Fisher]

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

Comment 17

[Darin Fisher]

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.

Comment 18

[Darin Fisher]

Attachment: patch: simplified version of SetFilePath

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

Comment 19

[Andreas Otte]

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. 

Comment 20

[Andreas Otte]

For the same reason calling SetPreHost in SetUsername is wrong.

Comment 21

[Darin Fisher]

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

Comment 22

[Darin Fisher]

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.

Comment 23

[Darin Fisher]

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

Comment 24

[Darin Fisher]

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.

Comment 25

[Darin Fisher]

FIXED now that the patch for bug 110846 has landed.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

vrfy