100212 - xpcom depends on NECKO for url parsing

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Doug Turner (:dougt)]

Comment 1

[Alec Flett]

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.

Comment 2

[Alec Flett]

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

Comment 3

[Doug Turner (:dougt)]

great plan, Alec!

Comment 4

[Doug Turner (:dougt)]

great plan needs a great owner.  :-)

Comment 5

[Judson Valeski]

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 :-).

Comment 6

[Darin Fisher]

i like this solution too... the code should just create a STDURL and then call
GetFile().

Comment 7

[Darin Fisher]

s/should/could/

creating a STDURL may be too much overhead.

Comment 8

[Alec Flett]

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..

Comment 9

[Darin Fisher]

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.

Comment 10

[Alec Flett]

Attachment: add 2 new parsing routines to nsIIOService, add windows impl

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.

Comment 11

[Alec Flett]

oh, and nsIOService::ParseLocalURL is ripped directly from nsLocalFile::ParseURL

Comment 12

[Darin Fisher]

this approach looks good to me, but i'd really really appreciate if this patch
could wait until after 103916 lands :)

Comment 13

[Alec Flett]

Attachment: update to reflect new parsing api

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
NS_SetFileFromURL()
(you'll notice the #if 1 code here..that will be fixed up right before landing)

Comment 14

[Alec Flett]

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 15

[Darin Fisher]

Comment on attachment 58058 [details] [diff] [review]
update to reflect new parsing api

>Index: src/nsIOService.cpp

>+static NS_DEFINE_CID(kURLParserCID, NS_NOAUTHURLPARSER_CID);

how about naming this kNoAuthURLParserCID?

otherwise, sr=darin

Comment 16

[Doug Turner (:dougt)]

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?

Comment 17

[Alec Flett]

Attachment: updated patch (win32 only) - attaching so I can hack multiple platforms

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.

Comment 18

[Alec Flett]

Attachment: updated patch (win32/unix)

ok, Unix support added.

Comment 19

[Alec Flett]

adding mkaply for OS/2 support

mike: any chance you can take this patch, tweak the Makefile.in, 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.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: OS/2 implementation

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.

Comment 21

[Alec Flett]

thanks, this is fantastic.. I'll incorporate this into my patch.. I'll have mac
ready by later today...

Comment 22

[Alec Flett]

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 :(

Comment 23

[Alec Flett]

Attachment: Complete patch (win/unix/mac/os2)

Can I get a review? This covers all platforms.

Comment 24

[Doug Turner (:dougt)]

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.

getURLFromFile(

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

+NS_SetFileFromURL(nsIFile* 

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. ;-)

Comment 25

[Brendan Eich [:brendan]]

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. :-)

/be

Comment 26

[Alec Flett]

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.

Comment 27

[Brendan Eich [:brendan]]

So if it's a string getter, how about getURLSpecFromFile?

/be

Comment 28

[Alec Flett]

getURLSpecFromFile it is!

Comment 29

[Darin Fisher]

nsIOService::GetCachedURLParser can be used to avoid the hit on the service manager.

Comment 30

[Alec Flett]

Attachment: complete patch, v2

Ok, reflecting dougt's comments. Reviews?

Comment 31

[Alec Flett]

Attachment: complete implementation, v2.1

I forgot to used the cached url parsers.. now this is done right.

Comment 32

[Alec Flett]

Attachment: complete implementation, v2.1

I forgot to used the cached url parsers.. now this is done right.

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 62017 [details] [diff] [review]
complete implementation, v2.1

NS_InitFileFromURL should be NS_InitFileFromURLSpec.

with that, r=dougt

Comment 34

[Darin Fisher]

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));

indentation


>+    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);

indentation


>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

Comment 35

[Alec Flett]

Attachment: update all C++ and JS consumers

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
this?

Comment 36

[Alec Flett]

(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 37

[Doug Turner (:dougt)]

Comment on attachment 63403 [details] [diff] [review]
update all C++ and JS consumers

-  nsCOMPtr<nsIObserverService> obsService =
do_GetService("@mozilla.org/observer-service;1");
-  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
-0000
@@ -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 mozilla.org 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 38

[Alec Flett]

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 
sr=darin

Comment 39

[Darin Fisher]

Comment on attachment 63403 [details] [diff] [review]
update all C++ and JS consumers

sr=darin

Comment 40

[Alec Flett]

Attachment: finally remove the "URL" attribute

and now that all consumers have been updated, we have the final removal. Can I
get an r=dougt, sr=darin?

Comment 41

[Doug Turner (:dougt)]

Comment on attachment 64164 [details] [diff] [review]
finally remove the "URL" attribute

awesome.  r=dougt.

Comment 42

[Darin Fisher]

Comment on attachment 64164 [details] [diff] [review]
finally remove the "URL" attribute

sr=darin :-)

Comment 43

[Alec Flett]

ok, fix is finally in!

Comment 44

[Alec Flett]

actually mark fixed