133591 - make implementations of GetRelativeSpec & GetCommonSpec match spec

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Kathleen :Brade]

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"

Comment 1

[Darin Fisher]

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.

Comment 2

[Kathleen :Brade]

need to fix this for 1.0

Comment 3

[Kathleen :Brade]

Attachment: test html file (needs to be in chrome to use)

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?

Comment 4

[Darin Fisher]

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!

Comment 5

[Kathleen :Brade]

Attachment: fixes to getCommonBaseSpec and getRelativeSpec

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)

Comment 6

[Darin Fisher]

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

Comment 7

[Charles Manske]

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

Comment 8

[Darin Fisher]

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 9

[Darin Fisher]

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 10

[Kathleen :Brade]

Attachment: updated to patch to handle windows volumes

Comment 11

[Darin Fisher]

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)

Comment 12

[Jaime Rodriguez, Jr.]

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

Comment 13

[Charles Manske]

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.

Comment 14

[Kathleen :Brade]

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.

Comment 15

[Kathleen :Brade]

fix checked in

Comment 16

[Charles Manske]

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

Comment 17

[Darin Fisher]

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.

Comment 18

[benc]

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

Comment 19

[benc]

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

Comment 20

[sujay]

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

thanks.

Comment 21

[Kathleen :Brade]

v

Comment 22

[benc]

Thanks!