Closed Bug 56354 Opened 25 years ago Closed 24 years ago

implement nsIFile url attribute

Categories

(Core :: XPCOM, defect, P1)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: warrensomebody, Assigned: dougt)

References

Details

(Whiteboard: intern?)

Attachments

(1 file)

patch to implement this on 3 platforms
21.82 KB, patch
Details | Diff | Splinter Review
We need to move the GetFile/SetFile functionality from nsStdURL into nsIFile. The reason is that only the nsIFile implementations for each platform can know how to convert to and from their native platform path representation into a file: url. This problem is evident in the fact that the code in nsStdURL is ifdef'd for different platforms. To implement this, we need to move the url escaping/unescaping code from necko into xpcom (stuff in nsURLHelper.cpp) so that it can be used by nsLocalFile. Note that there's currently an old nsEscape.cpp in xpcom/io that still getting used in various places -- we should eliminate that too. We should also eliminate nsIFileURL from necko (in nsIURL.idl) -- it will become unnecessary. Note that when implemented, this functionality should get used in the installer in place of hack_nsIFile2URL (implemented to avoid making the installer depend on necko) (in xpinstall/src/nsRegisterItem.cpp, line 72).
Already working on it. Need some help in CVS moving nsURLHelper to xpcom dir. Talk to leaf about it?
Status: NEW → ASSIGNED
I think we should just yank the nsEscape/nsUnescape routines out of nsURLHelper.cpp and put them in nsEscape.cpp. The trick is figuring out how the flags map for the places calling the xpcom nsEscape routine already.
They don't! The masks in nsURLHelper-Escape work on parts of an url, the masks in nsEscape work on the whole URL. However it should be possible to add the old nsEscape masks to the existing masks in nsURLHelper-Escape and make it's different function very evident in the documentation.
Andreas: I don't see why other pieces of code using nsEscape wouldn't need exactly the same flags.
The masks in nsURLHelper were specifically designed for the escaping/unescaping in nsStdURL where we deal with the different parts of an url and we already know which part we are dealing with. There are however situations where we have to deal with whole urls and we should not apply the part-specific masks because they are sometimes to strong. The masks in nsEscape are somewhat lighter because they have to apply to the whole url, but with a little risk to do not enough escaping to satisfy the urlparser (cornercases, but they do exist!). I think we should integrate the nsEscape mask into nsURLHelper by adding them to the already existing masks. Doing that url_Forced (which is already a special thing) should be moved up to make room for the new masks.
It just seems that existing uses of nsEscape are wrong, and should be replaced by calls to necko to build well-formed urls.
I should agree, but I had to accept that there are certain cases where you just have to deal with whole urls. For example in embedded links in html documents. Oh yes, they should already be properly escaped ... but the reality is very different. In order to get to the url parts to escape them properly you have to parse the url, which needs an already escaped url. That's where the dog bites it's tail. I don't see a way around that.
The routines from nsURLHelper have more capability, and they work. What I did was move them (and defined equiv flags) to nsEscape.h/cpp. I changed the name of the escape and unescape routines that I got from nsURLHelpers and the new code in nsLocalFileXXX.cpp calls that code. We can shift callers over to the better escape routines later but for this nsIFile needs to use exactly what nsIFileURL used. This work is mostly completed. I need to test on all platforms (ugh)
Conrad, can you attach the diffs from your changes to this bug? I will do some testing on linux ...
One question before I put up diffs: See http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStdURL.cpp#991. Why do we have this platform inconsistency? Why are we giving PC users the benefit of the doubt but not people on other platforms. Is there any code which depends on this? I hope not - it would break on other platforms. Seems like preferential platform BS to me. Anybody else have an opinion on this?
At one point Mozilla required three slashes on windows, but since communicator had been more lenient people screamed blody murder and it was switched back. I don't know if it broke actual code or if it was just a bunch of annoyed users.
2 or 3 slashes should work on all platforms, not just windows.
well, it doesn't. file:// didn't work on the Mac in 4.x when I tried it, and does not work now in Mozilla. By letting file:// equate to file:/// on windows are we eliminating the possibility of making UNC paths work?
file:// should be a local protocol with no host, but unfortunaly it isn't. The spec says the can be a host component, and "localhost" or the real hostname should work. The problem is: How do we tell if something between to slashes is a host or not? On unix or mac I see no way to do that for the parser, on windows there is little help, because the usual drive names are not valid hostnames, so if we find a drive, we can be sure it is no host. We are trying to do the best thing here, this has nothing to do with os preference (certainly not from me ;)) Also we do not eliminate working UNC file paths. If we can't identify the text after the / as drive name we deal with it as on the other platforms.
file:// as well as file:/// was easy to deal with on the Mac. After parsing the URL, if I end up with a host component, I check to see if the host matches a drive name (easy file system call) and if so, it is used as that. Although nearly complete, I had to back-burner this for a little because it is only a P3 bug. I hope to finish it in a week or so.
Got this done for Mac, Win, & Unix. Will put up patch soon.
The above patch does it. After geting andreas' feedback, decided to drop support for file:// (only 2 '/'s) on Unix. There is no safe way to distiguish from the host. This is done on Mac (very certain) and Win (two-letter hack). The escape routines were lifted straight from URLHelpers, flags & all but renamed so we can transition gradually to them.
Any status on getting this checked in? It's currently blocking the Unix/Mac implementations of my Document Inspector tool (http://www.joehewitt.com/inspector)
Assignee: ccarlen → dougt
Status: ASSIGNED → NEW
-> dougt
Joe, have you verified that this works for you.
Adding me and blizzard to the CCs, blizz, how about a review for this? Axel
Keywords: review
looks good to me. I spoke with conrad and he said that he has tested it on three platforms and it was ready to go. There was some debate about merging the escape table, but I say we can do that later. Is anyone up for a last time merge and verification?
It looks good to me.
*** Bug 64122 has been marked as a duplicate of this bug. ***
Question: why does nsIFile need Get/SetURL at all? It seems to me that this encourages use of URL as a surrogate for file paths, which, as we know, are bad for Macintosh. It also requires a lot of URL escaping code to be put in XPCOM which should really live in necko. In the absence of nsIFile.Get/SetURL, you can always create a standard URI, QI to nsIFileURL, set its nsIFile, and request back the spec. So I would lean towards removing Get/SetURL on nsIFile. Comments?
Why don't we just invent a new URL which contains the volume number and propose it in a RFC?
Simon, the problem is that the installers need to do some file:// URL stuff before Necko is present (mainly chrome registration I think). Currently we have our own copy of the StdURL conversion stuff, Warren was proposing moving this to XPCOM/nsIFile where it could be shared.
Conrad, Are you still waiting to check this in?
Assignee: dougt → ccarlen
Whoops, I thought I gave this one to you a while back. But, I see it's still assigned (or was bounced back) to me. ??
Setting to 1.0. Work is done but I'm too swamped to verify on all platforms. Any takers?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
kandrot, implemention of this will spend up callers that have a URI and need an nsIFile
Assignee: ccarlen → kandrot
Status: ASSIGNED → NEW
Blocks: 73214
retargeting.
Assignee: kandrot → dougt
Target Milestone: mozilla1.0 → mozilla0.9.2
.
Assignee: dougt → gagan
Whiteboard: intern?
Blocks: 83898
I own bug 83898 which requires I implement GetURL on Unix to fix a file:// URI fixup problem. I have a patch which does this more or less the same way Conrad does in his patch. Politics aside, are there any objections to be me checking this in? If not can I have a review on it please? Thanks
Adam, Be my guest. Update the patch to the tip, reattach it, and I will review it.
Assignee: gagan → adamlock
Doug, patch is attached to the bug 83898: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37171 It should apply cleanly since nsLocalFileUnix.cpp hasn't changed in the last 3 weeks.
what about the other three platforms? Lets do this all in one sweep. Are you up for that?
Doug, that's what I thought. That's why I pointed out the patch that's here - because it does it for 3 platforms. I'd suggest using the GetURL code here, and turning the url attribute on nsIFile into a read-only attribute. I'm not sure we need SetURL and it drags in necko in order to parse the incoming URL.
Chiming in that file:/// (UNC path on win32) is still not working. Clicking on a file URL in an HTML page does nothing. win32 2001061204
No, we want SetURL.
adam, this bug is blocking me now. Ironic since I tried passing it off. Are you going to be checking this in soon? If not please reassign it to me.
actually, the patch is a bit wrong in the way that it invokes the url parser. I am reassigning this to myself.
Assignee: adamlock → dougt
Priority: P3 → P1
fixed checked in.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: