Closed Bug 100212 Opened 23 years ago Closed 23 years ago

xpcom depends on NECKO for url parsing


(Core :: XPCOM, defect, P1)

Windows 2000





(Reporter: dougt, Assigned: alecf)



(Whiteboard: fix in hand)


(3 files, 8 obsolete files)

Blocks: 66759
So I thought about this on the way into work today.
Here's what I think we should do: Eliminate the concept of URLs from the
nsI*File interfaces, and then provide a helper function in nsNetUtils.h (in
necko) to initialize an nsI*File object from a url. Something like

nsresult NS_InitializeFileFromUrl(nsIFile* aFile, const char *aURL);

That way, xpcom could continue having nsIFile, and we'd still have the
capability to use URLs in nsI*File stuff, but it would be up to the consumer of
nsIFile to establish this dependency.
oh, and the mechanism that would make this work would basically be for the
helper method to derive a native path, and then call nsI*File's "path" attribute
via SetPath

We'd also probably want
NS_GetUrlFromFile(nsIFile *aFile, char **aUrl);

which would retrieve the url via a similar mechanism
great plan, Alec!
great plan needs a great owner.  :-)
Assignee: dougt → alecf
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
I like this idea too. Rather than explicitly binding an interface (and
subsequently a dependency) to another interface, push it off to the consumer.

nit. Maybe replace "URL" w/ "string" in the function names though. "URL"
translates to "nsIURL" in my mind; not that my mind is sane :-).
Blocks: 104523
i like this solution too... the code should just create a STDURL and then call

creating a STDURL may be too much overhead.
It turns out there's a lot of per-platform parsing code that is too big to be
inlined....I'm essentially going to have to move the platform-specific
native-file parsing into some service in necko, probably in nsIOService. Then I
will add two inline methods to nsNetUtil.h:
nsresult NS_GetURLFromFile(nsIFile* aFile, char **aUrl);
nsresult NS_SetFileFromURL(nsIFile* aFile, const char *aUrl);

which will make similar calls into nsIIOService... I could also move this into
nsIURLParser, if that made sense?
unfortunately, this is turning out to be more work than I can do in the next 36
hours - moving to 0.9.7

cc'ing darin in case he has any thoughts on this. It seems bad to keep piling
methods into nsIIOService, but I don't see another likely place..
Priority: P2 → P1
Target Milestone: mozilla0.9.6 → mozilla0.9.7
nsIIOService is fine for now.  we already need to do some house cleaning there,
but it isn't going to happen anytime soon.

my patch for bug 103916 (which will hopefully be landing for 0.9.6) touches
nsIURLParser as well as the current nsIFile::[GS]etURL implementation.  so, it's
probably best that this bug wait until that patch lands.
Here's a patch which adds two new routines to nsIOService, and includes a
windows implementation of them. It also adds 2 new convenience routines to
nsNetUtil.h, in the spirit of the functions that are already there - that will
make it much easier to fix the C++ that calls the old Get/SetURL()

You'll notice some asymmetry between the two new routines with nsIFile vs.
nsILocalFile - the problem is that SetFileFromURL() is called by
nsIOService::NewFileURI() so it requires an nsIFile. The GetURLFromFile()
implementation requires SetNativePath(), which is a part of nsILocalFile.

I basically just copied the windows implementation from nsLocalFileWin.cpp, and
#ifndef XP_WIN'ed them for the other platforms.

Darin, can you take a look at this to make sure we're on the same page w.r.t.
the approach I'm taking here?

Regarding the use of nsIFile vs. nsILocalFile, the possibilities I see are:
1) use nsIFile in SetFileFromURL and GetURLFromFile and QI() in
nsIOService::SetURLFromFile() - there is actually only one implementation
(nsLocalFile) so the QI is guaranteed to work in the current codebase.
2) use nsILocalFile and do a QI() inside of nsIOService::NewFileURI(). Again
the QI will always succeed in the current codebase.

I'm open to either possibility.
oh, and nsIOService::ParseLocalURL is ripped directly from nsLocalFile::ParseURL
this approach looks good to me, but i'd really really appreciate if this patch
could wait until after 103916 lands :)
Depends on: 103916
now that darin has landed, I updated my patch... note that this is windows-only
and that I won't check in any consumers of NS_GetURLFromFile() or
(you'll notice the #if 1 code here..that will be fixed up right before landing)
Attachment #57820 - Attachment is obsolete: true
I could use some reviewers here. Again, I copied the implementation of
nsIOService::ParseFileURL() from nsLocalFile::ParseURL() (most recently updated
by darin) could I get r=dougt, sr=darin?

What I'd like to do is land this patch, which simply adds the API and a windows
implementation, and then do the mac/linux implementation (due to the tricky
nature of the patch, I want to make sure I have the infrastructure landed first)
- then I'll switch all the callers over to start using the new API, and remove
the old one from nsLocalFile.
Comment on attachment 58058 [details] [diff] [review]
update to reflect new parsing api

>Index: src/nsIOService.cpp


how about naming this kNoAuthURLParserCID?

otherwise, sr=darin
Attachment #58058 - Flags: superreview+
Comment on attachment 58058 [details] [diff] [review]
update to reflect new parsing api

SetFileFromURL - Make the first parameter a nsIFile.  At some point I want to
make a nsIFile that abstracts FTP and HTTP.

What about the mac and 'nix?
Attachment #58058 - Flags: needs-work+
this patch is by no means complete, but I want to put it here so I can apply it
on other platforms, and add to the final patch.
Attachment #58058 - Attachment is obsolete: true
Attachment #60877 - Flags: needs-work+
Attached patch updated patch (win32/unix) (obsolete) — Splinter Review
ok, Unix support added.
Attachment #60877 - Attachment is obsolete: true
adding mkaply for OS/2 support

mike: any chance you can take this patch, tweak the, and create a
new nsIOServiceOS2?

What's basically going on here is that we're consolidating URL parsing stuff in
necko, and taking it out of XPCOM. 

:To do the conversion, all you pretty much have to do is:
- copy nsLocalFile::GetURL and ::SetFile, change them to
nsIOService::GetURLFromFile and SetFileFromURL, where they will take an extra
param, (nsIFile* aFile)
- From there, you just change all file-related APIs like GetPath to
aFile->GetPath, and call ParseFileURL instead of ParseURL.
- QueryInterface from nsIFile to nsILocalFile and add an assertion in order to
call InitWithPath - that way the APIs still use nsIFile, and dougt can add FTP
support for nsIFile later

Meanwhile, I'll work on mac support.
Attached patch OS/2 implementation (obsolete) — Splinter Review
The only real difference between Windows and OS/2 is that we don't have
IsDBCSLeadByte - I had to roll my own (stolen from nsLocalFileOS2). I've argued
with frank before that we should have an XP IsDBCSLeadByte somewhere for
Windows and OS/2 to share - it would allow us to share a little more code.

Anyway, I also attached a Makefile change to pick the right CPP on the compile.
thanks, this is fantastic.. I'll incorporate this into my patch.. I'll have mac
ready by later today...

mac project changes interrupted my mac work.. and I realized that even adding
the new API doesn't completely solve this bug just yet.. I still have to convert
all the consumers of nsIFile.URL

so I'm moving to 0.9.8 :(
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Can I get a review? This covers all platforms.
Attachment #60881 - Attachment is obsolete: true
Attachment #61098 - Attachment is obsolete: true
Whiteboard: fix in hand
Comment on attachment 61786 [details] [diff] [review]
Complete patch (win/unix/mac/os2)

 interface nsIFile;
+interface nsILocalFile;

I think that you can just include nsILocalFile as it inludes nsIFile.


why does the API name suggest that a URL is the result, when it is a string? 
Can we just return a nsIURI?


How about NS_InitFileWithURL instead?

Coud you ditch the 
+#if 0

Please cache the nsIURLParser.	Or tell me it is not called alot.

myPLstrcpy should be named PascalStringCopy or something.

Take a look in bug 106815 for a simple optimization you can do when escaping
the path.  

+static int isleadbyte(int c)

This function name doesn't follow common style, nit at best. ;-)
Attachment #61786 - Flags: needs-work+
dougt wrote:
> interface nsIFile;
>+interface nsILocalFile;
>I think that you can just include nsILocalFile as it inludes nsIFile.

Don't overinclude -- don't include when a forward decl will do.

Do state all dependencies fully -- don't "bootleg" nsIFile via nsILocalFile if
there is a true dependency (not just one that a fwd decl satisfies) on the
interface definition for nsIFile as well as for nsILocalFile.

Thus endeth today's dependency sermon. :-)

so much of this code was just copied verbatim from nsLocalFile<Platform>.cpp -
I'll clean up the code as requested though..

I like "init" rather than "set" but I'm not sure about "get" - the result of the
function is in fact a url string, and I'd like to keep it that way because most
of the existing consumers are dealing with the string.. nsIURI would be overkill
for them.

I'll ditch the #if 0, that was left over cruft. As for the nsILocalFile forward
decl, that wasn't even needed, also left over cruft that I hadn't noticed.

As for caching the url parser - no it's not called a lot and not in an loops
that I've found, I personally don't think its worth the overhead. In any case
we're using it as a service so the worst its only a service lookup, not repeated
object creation.

isleadbyte, copied from nsLocalFileMac, is a private (static) helper function
and personally I prefer the lower-case to indicate this...

revised patch coming up.
So if it's a string getter, how about getURLSpecFromFile?

getURLSpecFromFile it is!
nsIOService::GetCachedURLParser can be used to avoid the hit on the service manager.
Attached patch complete patch, v2 (obsolete) — Splinter Review
Ok, reflecting dougt's comments. Reviews?
Attachment #61786 - Attachment is obsolete: true
Attached patch complete implementation, v2.1 (obsolete) — Splinter Review
I forgot to used the cached url parsers.. now this is done right.
I forgot to used the cached url parsers.. now this is done right.
Attachment #61987 - Attachment is obsolete: true
Attachment #62016 - Attachment is obsolete: true
Comment on attachment 62017 [details] [diff] [review]
complete implementation, v2.1

NS_InitFileFromURL should be NS_InitFileFromURLSpec.

with that, r=dougt
Comment on attachment 62017 [details] [diff] [review]
complete implementation, v2.1

>+    /**
>+     * initFileFromURL
>+     * Sets the native path of the file given the url string
>+     *
>+     */
>+    void initFileFromURL(in nsIFile file, in string url);

how about adding some @param's to this.

>Index: src/nsIOService.cpp

>+	nsXPIDLCString scheme;
>+    rv = ExtractScheme(inURL, nsnull, nsnull, getter_Copies(scheme));


>+    if (NS_FAILED(rv)) return rv;
>+    if (nsCRT::strcmp(scheme.get(), "file") != 0) {
>+        NS_ASSERTION(0, "must be a file:// url");

how about NS_ERROR instead?

>Index: src/nsIOService.h

>+    nsresult ParseFileURL(const char* inURL,
>+                                 char **outHost,
>+                                 char **outDirectory,
>+                                 char **outFileBaseName,
>+                                 char **outFileExtension);


>Index: src/nsIOServiceMac.cpp

>+static void SwapSlashColon(char * s)
>+	while (*s)
>+	{
>+		if (*s == '/')
>+			*s++ = ':';
>+		else if (*s == ':')
>+			*s++ = '/';
>+		else
>+			*s++;
>+	}
>+static void PascalStringCopy(Str255 dst, const char* src)
>+	int srcLength = strlen(src);
>+	NS_ASSERTION(srcLength <= 255, "Oops, Str255 can't hold >255 chars");
>+	if (srcLength > 255)
>+		srcLength = 255;
>+	dst[0] = srcLength;
>+	memcpy(&dst[1], src, srcLength);

this file has wacky inconsistent indentation... looks like this was probably
inherited from nsLocalFileMac.cpp... mind fixing it?

otherwise, sr=darin
Attachment #62017 - Flags: superreview+
Attachment #62017 - Flags: review+
Ok, now that we have an implementation in nsIIOService, I've gone and updated
all consumers in the tree. In the case of the C++, I simply removed the "URL"
attribute from nsIFile, and compiled, fixing bustage as I went along. In the
case of JavaScript, I searched for all instances of ".URL" and fixed them on a
case by case basis.. I'm in the process of testing all the JS ones right now,
to make sure I fixed it the right way... but can I get an r=dougt, sr=darin on
(I won't actually check anything in until I've finished testing, I'm just trying
to hedge my bets by getting reviews and testing done in parallel)
Comment on attachment 63403 [details] [diff] [review]
update all C++ and JS consumers

-  nsCOMPtr<nsIObserverService> obsService =
-  if (obsService)
-  {
-    obsService->RemoveObserver(this, "quit-application");
-    obsService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
-  }

Are you sure you want to mix this in with this patch?

--- netwerk/protocol/res/src/nsResProtocolHandler.cpp	19 Dec 2001 00:11:58
-0000	   1.45
+++ netwerk/protocol/res/src/nsResProtocolHandler.cpp	3 Jan 2002 20:22:01
@@ -11,7 +11,7 @@
  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
  * for the specific language governing rights and limitations under the
  * License.
- *
+n *
  * The Original Code is code.
  * The Initial Developer of the Original Code is 

remove 'n'.

fix that and assuming jag is happy with the string fu, r=dougt.
Comment on attachment 63403 [details] [diff] [review]
update all C++ and JS consumers

dougt: yes, I do want to incorporate that change.. all it does is remove a
service manager warning on exit.

the "n" has been removed (stupid keyboard!) so I just need an sr=jag or 
Comment on attachment 63403 [details] [diff] [review]
update all C++ and JS consumers

Attachment #63403 - Flags: superreview+
and now that all consumers have been updated, we have the final removal. Can I
get an r=dougt, sr=darin?
Comment on attachment 64164 [details] [diff] [review]
finally remove the "URL" attribute

awesome.  r=dougt.
Attachment #64164 - Flags: review+
Comment on attachment 64164 [details] [diff] [review]
finally remove the "URL" attribute

sr=darin :-)
Attachment #64164 - Flags: superreview+
ok, fix is finally in!
actually mark fixed
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.