make implementations of GetRelativeSpec & GetCommonSpec match spec

VERIFIED FIXED in mozilla1.0

Status

()

Core
Networking
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Brade, Assigned: Brade)

Tracking

Trunk
mozilla1.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: publish [adt2])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
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"
(Assignee)

Updated

16 years ago
Keywords: nsbeta1
Whiteboard: publish
Target Milestone: --- → mozilla1.0.1

Comment 1

16 years ago
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.
(Assignee)

Comment 2

16 years ago
need to fix this for 1.0
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0.1 → mozilla1.0
(Assignee)

Comment 3

16 years ago
Created attachment 76419 [details]
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

16 years ago
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!
(Assignee)

Comment 5

16 years ago
Created attachment 76465 [details] [diff] [review]
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

16 years ago
that makes sense since :80 is the default port.  http://foo.com:80/ is likely
normalized before the call to GetRelativeSpec.
(Assignee)

Updated

16 years ago
Blocks: 133803

Updated

16 years ago
Attachment #76465 - Flags: review+

Comment 7

16 years ago
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

16 years ago
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

16 years ago
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
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0
(Assignee)

Comment 10

16 years ago
Created attachment 77300 [details] [diff] [review]
updated to patch to handle windows volumes
Attachment #76465 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Whiteboard: publish → publish [adt2]

Comment 11

16 years ago
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)
Attachment #77300 - Flags: superreview+
(Assignee)

Updated

16 years ago
Blocks: 134940

Comment 12

16 years ago
adt1.0.0+ (on ADT's behalf) for checkin into 1.0.
Keywords: adt1.0.0 → adt1.0.0+

Comment 13

16 years ago
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.
Attachment #77300 - Flags: review+
(Assignee)

Comment 14

16 years ago
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.

Updated

16 years ago
Attachment #77300 - Flags: approval+
(Assignee)

Comment 15

16 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

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

Comment 17

16 years ago
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.
(Assignee)

Updated

16 years ago
Keywords: fixed1.0.0

Comment 18

16 years ago
Charles: If this does everything you need for composer, can you mark this as
VERIFIED?
Keywords: verifyme

Comment 19

16 years ago
-> 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.
QA Contact: benc → sujay

Comment 20

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

thanks.
(Assignee)

Comment 21

16 years ago
v
Status: RESOLVED → VERIFIED

Comment 22

16 years ago
Thanks!
Keywords: fixed1.0.0, verifyme → verified1.0.0
You need to log in before you can comment on or make changes to this bug.