Closed Bug 56354 Opened 24 years ago Closed 23 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: 23 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: