When bug 114951 lands, the implementations of GetRelativeSpec and GetCommonSpec in nsStandardURL probably won't handle every case they should. For example, a relative spec result could be something like: "../../foo/bar.html"

in fact, it is a serious ommision. this should get high priority since w/ this bug, GetRelativeSpec fails to satisfy: relativeSpec = url.getRelativeSpec(url2) <==> url2 = url.resolve(relativeSpec) and it's really not good to freeze an interface w/ a broken impl.

need to fix this for 1.0

This is the test file I am using for testing the implementations of GetCommonBaseSpec and GetRelativeSpec. Note that I am adding the file as "text/plain" instead of "text/html" in the hopes that it will display the source rather than the html (since you won't be able to use the html version without putting it in chrome anyways). Darin--could you look over this file and verify that it is accurate? Is it desirable to check this into necko somewhere? Can it stay in html format?

i usually just write .js files that can be executed using xpcshell to test out such things, but .html is fine. please add the test file to netwerk/test, thx!

fixes port matching (so default port matches absence of port in url); fixes relativize to use "this" as base url (rather than the parameter); fixes relativize to now do "../" appropriately interesting things I notice in my testing: http://foo.com:8080/ http://foo.com:80/ gave me http://foo.com/ (no port)

that makes sense since :80 is the default port. http://foo.com:80/ is likely normalized before the call to GetRelativeSpec.

Comment on attachment 76465 [details] [diff] [review] fixes to getCommonBaseSpec and getRelativeSpec I have reviewed and tested this patch on Windows and it works great except for the case when the drive letters (or volume names for Mac?) are different. E.g.: if the base url spec is "file:///M:/temp/foo.html", and I have an image with the absolute src =file:///N:/Graphics/1.gif", this will yield the relative url spec: "../../N:/Graphics/1.gif" This is not valid relative path for Windows. Interestingly, mozilla actually handles this type of path just fine -- the image is made absolute by the image loader so it displays correctly, but that image it does not display correctly in IE. What needs to be done is to test for "file" schemes, and get the "volume" from each url. If the volumes of the base and input url don't match, we should not relativize. I'm not sure what api we have for obtaining the volume/drive letters. Darin? I vote that this code is accepted as is, however, for 1.0, since it is extremely useful for most of the problems we need it for *right now.* Brade can give examples of that. In Composer, we have a MakeRelativeUrl() method in JS that is working fine and we can continue to use that as we do now. So with that caveat, r=cmanske

i think we should add #ifdef XP_WIN code that checks for drive letters. there aren't any utility functions to help here, but it should be trivial enough to check for a drive letter. keep in mind that the ':' is commonly replaced with a '|', so you need to check for "/X:/" or "/X|/" to know if 'X' should be treated a a drive letter. i'm willing to accept the patch w/o this for 1.0, but then again... it wouldn't be that much work to fix this problem.

Comment on attachment 76465 [details] [diff] [review] fixes to getCommonBaseSpec and getRelativeSpec Index: mozilla/netwerk/base/src/nsStandardURL.cpp >- && (Port() == stdurl2->Port()); >+ && ((Port() == stdurl2->Port()) >+ || (mPort == -1 && stdurl2->mPort == stdurl2->mDefaultPort) >+ || (stdurl2->mPort == -1 &&mPort == mDefaultPort)); i don't understand this. if the URLs have the same scheme, then they will be similarly constructed. in other words, if one has mDefaultPort set, then the other will to. so, this additional check should be unnecessary. Port() == stdurl2->Port() should be sufficient. please eliminate this code if possible. otherwise, the code looks good. please be sure to rigorously test this. thx! darin

Comment on attachment 77300 [details] [diff] [review] updated to patch to handle windows volumes sr=darin (looks good.. just make sure to get plenty of testing on this)

adt1.0.0+ (on ADT's behalf) for checkin into 1.0.

Comment on attachment 77300 [details] [diff] [review] updated to patch to handle windows volumes I've applied the patch and tested it by using it in Composer's "MakeRelative()" JS routine used by the image and link dialogs. I works great, but shouldn't the #ifdef be for both Windows and Mac? Don't you hve the same problem when trying to relativize across Mac drive volumes? r=cmanske for the code, once this is resolved.

I want to talk to some other people regarding Mac handling of this issue (given the multiple OS). I will track the issue and if changes need to be made (change the #ifdef in this patch), then I will file a new bug for that issue. I really don't want to hold up this work any longer than necessary.

fix checked in

*** Bug 134940 has been marked as a duplicate of this bug. ***

brade: agreed... necko currently only special cases windows drive numbers, and this patch is consistent with that. if there is need to do something similar with mac file URLs, then that will likely effect other parts of the code (beyond GetRelativeSpec), so pushing that to another bug is the right thing to do IMO.

Charles: If this does everything you need for composer, can you mark this as VERIFIED?

-> composer qa. Filed by composer, no blackbox testcase. Send it back to networking qa if you want me to verify, please include steps. After friday, you will have to wait until Nov. Otherwise, I have to clear my branch fixed verification list this week.

I will not be able to verify this bug...Kathy, can you verify this one ? thanks.

v

Thanks!