implement nsIFile.normalize for windows and os/2

RESOLVED FIXED

Status

()

Core
XPCOM
--
enhancement
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: timeless, Assigned: Brodie)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.55 KB, patch
Darin Fisher
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
I:\build\mozilla\opt-i686-pc-cygwin\dist\bin>xpcshell
js> o=Components.classes["@mozilla.org/file/local;1"].createInstance(); o
instanceof Components.interfaces.nsILocalFile;
 o.initWithPath("i:.\\xpcshell.exe"); o.exists();
true
js> o.normalize(); o.path
i:.\xpcshell.exe
js> quit()

Expected results: o.path after o.normalize() should return
I:\build\mozilla\opt-i686-pc-cygwin\dist\bin\xpcshell.exe

Comment 1

16 years ago
Yep, looks broken to me. You should get the full path to 'xpcshell.exe'.

Looks like a realpath() like libc call in windows that nsLocalFileWindows
::Normalize can implement instead of just returning NS_OK might do the trick here.

;-)

--pete


(Assignee)

Comment 2

14 years ago
Created attachment 150511 [details] [diff] [review]
patch 1

First cut at the patch. This function produces the following behaviour. First
path is the source, second path is the results.

    // path normalization
    nFail += TestNormalize( "r:\\foobar\\foo\\..", "r:\\foobar" ); 
    nFail += TestNormalize( "r:\\foobar\\..\\foo", "r:\\foo" ); 
    nFail += TestNormalize( "r:\\foobar\\.\\foo", "r:\\foobar\\foo" ); 
    nFail += TestNormalize( "r:\\foobar\\..\\.\\foo", "r:\\foo" ); 
    nFail += TestNormalize( "r:\\foobar\\.\\.\\.\\.\\.", "r:\\foobar" ); 
    nFail += TestNormalize( "r:\\foobar\\..\\..", "r:\\" );
    nFail += TestNormalize( "r:\\foobar\\test\\..\\test\\.\\..\\test",
"r:\\foobar\\test" ); 
    nFail += TestNormalize( "r:\\foobar\\\\..\\.\\\\test\\foo\\.",
"r:\\test\\foo" ); 

    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..", "\\\\svr\\shr" ); 
    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\foo",
"\\\\svr\\shr\\foo" ); 
    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\.\\foo",
"\\\\svr\\shr\\foobar\\foo" ); 
    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\.\\foo",
"\\\\svr\\shr\\foo" ); 
    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\.\\.\\.\\.\\.",
"\\\\svr\\shr\\foobar" ); 
    nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\..", "\\\\svr\\shr" );
    nFail += TestNormalize(
"\\\\svr\\shr\\foobar\\test\\..\\test\\.\\..\\test",
"\\\\svr\\shr\\foobar\\test" ); 

    // path normalization using the current directory
    char cwd[MAX_PATH], drive[_MAX_DRIVE];
    getcwd(cwd, MAX_PATH);
    _splitpath(cwd, drive, 0, 0, 0);
    nFail += TestNormalize( drive, cwd ); 
    chdir("c:\\temp");
    nFail += TestNormalize( "c:", "c:\\temp" ); 
    nFail += TestNormalize( "c:.", "c:\\temp" ); 
    nFail += TestNormalize( "c:.\\test", "c:\\temp\\test" ); 
    nFail += TestNormalize( "c:.", "c:\\temp" ); 
    chdir("c:\\");
    nFail += TestNormalize( "c:", "c:\\" ); 
    chdir(cwd);
(Assignee)

Comment 3

14 years ago
Comment on attachment 150511 [details] [diff] [review]
patch 1

Timeless: would you assign this to me please.
Attachment #150511 - Flags: review?(timeless)
Assignee: timeless → brofield
(Assignee)

Comment 4

14 years ago
Created attachment 150775 [details] [diff] [review]
patch 2

Fix a pointer problem that I noticed and cleaned up the comments.
Attachment #150511 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #150511 - Flags: review?(timeless)
(Assignee)

Updated

14 years ago
Attachment #150775 - Flags: superreview?(dougt)
Attachment #150775 - Flags: review?(timeless)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 5

14 years ago
Comment on attachment 150775 [details] [diff] [review]
patch 2

there's a bug recently discussing three or more dots in a row. do you want to
handle that?
Attachment #150775 - Flags: review?(timeless) → review?(timeless)
(Assignee)

Comment 6

14 years ago
Do you have a bug number? I can't find it, so I'm not sure what behaviour you
are after.
(Reporter)

Comment 7

14 years ago
bug 87501
(Assignee)

Comment 8

14 years ago
If I handle this case it will be to either error or ignore. I don't think that 
we should accept this as valid syntax as it is an esoteric function that few 
people know about or use and it's not cross platform. 

My vote is to ignore it (treat it like ".") when normalizing. If satisfactory 
then I'll update the patch and again ask for review.
(Assignee)

Updated

14 years ago
Attachment #150775 - Flags: superreview?(dougt)
Attachment #150775 - Flags: review?(timeless)
(Assignee)

Comment 9

14 years ago
Created attachment 151083 [details] [diff] [review]
patch 3

This version ignores path segments consisting solely of > 2 dots.
i.e. "c:\foo\bar\..." is normalized to "c:\foo\bar"
(Assignee)

Updated

14 years ago
Attachment #150775 - Attachment is obsolete: true
(Assignee)

Comment 10

14 years ago
Comment on attachment 151083 [details] [diff] [review]
patch 3

See testcases (TestNormalize) handled by this patch in bug 247456, attachment
http://bugzilla.mozilla.org/attachment.cgi?id=151082&action=view

Now can I get review?
Attachment #151083 - Flags: superreview?(dougt)
Attachment #151083 - Flags: review?(timeless)

Comment 11

14 years ago
Comment on attachment 151083 [details] [diff] [review]
patch 3

darin, not timeless, should review this.
Attachment #151083 - Flags: review?(timeless) → review?(darin)
(Assignee)

Comment 12

14 years ago
This whole thing is almost moot anyhow. We can't add relative paths using .. 
according to the spec, so it looks like this function was added before that 
rule came to be. The only thing that can be done now is adding paths with . or 
creating paths like c:foo 

Is it even correct to allow paths like c:foo? Shouldn't an nsILocalFile always 
contain a full and validly specified path? (not necessarily one in existence, 
but the format should be right). If this is correct and the set/append methods 
were correctly implemented to prevent invalid input, then this functions entire 
usefulness is simply stripping /./ from paths.

Comment 13

14 years ago
Brodie: good questions.  I know that under UNIX, we allow a path to be
initialized with "~/", so maybe that is enough reason to allow initialization
with pathes like "c:dir".  my main concern is that we have some code in
existence that tries to distinguish a URL from a local file by seeing if a
string can be used to initialize a local file.  we have to be careful not to
regress browser functionality if we make changes to the input we accept.  of
course, we can rightfully limit ourselves to single character driver letters, so
maybe it is easy to avoid the problems i'm thinking about :-/

I'd also like to know who intends to make use of this Normalize function.  Fully
implementing our frozen API is good in general, but without a clear idea of who
will benefit, isn't this just code bloat?

Comment 14

14 years ago
By the way, I looked over your patch, and it looks very solid to me.  I'd just
like to know if there are any planned consumers for this code.  Thanks!
(In reply to comment #13)
> of
> course, we can rightfully limit ourselves to single character driver letters, 

as long as you don't break unc paths :)
(Assignee)

Comment 16

14 years ago
I have no idea who wants to use it. I found the bug, had some spare time and
fixed it. After implementing it I started thinking that it was pretty but
probably useless.

Since timeless reported it and has commented a few times, I expect he should
have a better idea of who if anyone would want to use it. 
(Reporter)

Comment 17

14 years ago
well, bookmarks or whatever could use it. i certainly have uses for it in 
testcases and other places. a consistent api means less silly corner casing 
code.
(Assignee)

Comment 18

14 years ago
This implementation makes the assumption that the current working path is valid.
This assumption leaves us open to problems with the current implementation of
localFileWin because we don't rigorously check the paths passed in in the
set/append functions. It currently assumes one of the following formats:

  ""                  Empty path
  "\\server\share"    Valid UNC 
  "D:\"               Valid absolute local
  "D:"                Valid relative local

I know that there are paths that can be passed in in the set/append methods
which will invalidate this assumption. e.g. I think these semantics currently
work without error (but probably shouldn't).

  NS_NewLocalFile("", &file);
  file.Append(".");

This can't be addressed until we rigorously define what the contents of
mWorkingPath is allowed to be. It will however work in 100% of current use cases
(since the current use is 0 :-), and if the working path matches the assumptions.

Comment 19

14 years ago
Comment on attachment 151083 [details] [diff] [review]
patch 3

r=darin

perhaps it would be a good idea to add testcases for this function to
nsIFileTest.cpp

you might also want to add a XXX comment in the code corresponding to the
concern you expressed in your last comment.
Attachment #151083 - Flags: review?(darin) → review+
(Assignee)

Comment 20

14 years ago
Darin:
I'm about to leave in a couple of days and will be away for a couple of months.
I won't have time to update this or check it in before then. 

Testcases are already in the new version of the test harness (see comment #2 and
bug 247456). There is no point modifying the old test harness because it
requires a manual inspection to check the results.

Updated

14 years ago
Attachment #151083 - Flags: superreview?(dougt) → superreview+
(Assignee)

Comment 21

14 years ago
Who needs sleep? Comment added. Checked in.

Checking in xpcom/io/nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.128; previous revision: 1.127
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 22

14 years ago
> Testcases are already in the new version of the test harness (see comment #2 and
> bug 247456). There is no point modifying the old test harness because it
> requires a manual inspection to check the results.

ok.. sounds good.
You need to log in before you can comment on or make changes to this bug.