114951 - Network interfaces should have a "MakeRelativeUrl()" method.

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Charles Manske]

Given an absolute URL string (eg: "http://www.mysite.com/mypages/foo.html"
and a base URL string (eg: "http://www.mysite.com/mypages/subdir/index.html"), 
we need a method resolve the relative URL, which is "../foo.html" for this
example. Note that a filename is ignored in the base URL, which is usually the
location of a web page or "href" value from a <base> tag.
We need this in Composer, so we implemented a version in Java Script (see 
"MakeRelativeUrl(url) and related methods in editorUtilities.js), but it seems 
that this facility should be supplied by nsIOServices or nsIURI interfaces.

Comment 1

[Darin Fisher]

resolving a relative URL is protocol specific (nsIURI::resolve), so it seems
logical that generating relative URLs would also be protocol specific... hence,
we should probably have something like

  string nsIURI::foo(in nsIURI baseURI);

where "foo" could be "diff" or "subtract" or "getRelativeURI" ... etc.  still
thinking of a good name for this.

cc'ing some other folks.

Comment 2

[Gagan]

->darin

Comment 3

[Andreas Otte]

Nice idea, although very very specific. I agree this should be part of the
nsIURI interface or at least nsIURL since nsIURI describes simple URIs that
can't be relative by design.

Comment 4

[Darin Fisher]

andreas: then perhaps nsIURI::resolve should be nsIURL::resolve ?

Comment 5

[Andreas Otte]

I think it's okay to have ::Resolve in nsIURI, it's implementation is just very
simple: Either NOT_IMPLEMENTED or return the given url disregarding the base url
because the given url is already absolute. 
It might be very similar with MakeRelative. What is the return value of calling
MakeRelative with two absolute urls with different authoritys? That should also
be the default implementation of nsIURI::MakeRelative, since in that case the
urls are not compareable by design.
So I guess we should have ::MakeRelative in nsIURI it would just be a very
simple implementation, just like ::Resolve.
 

Comment 6

[Charles Manske]

No, it cannot be a "very simple implementation" like ::Resolve
Please look at what we do in Composer code: 
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/editorUtilities.js#395

Comment 7

[Darin Fisher]

cmanske: i think what andreas meant was that URI's that don't have any notion of
relative specs would have a very simple implementation.  this applies to, for
example, nsSimpleURI.  nsStandardURL, on the other hand, would have an impl that
looks very much like your JS code.  so it sounds like having a method on nsIURI
is appropriate.  that means that this bug needs to be resolved within the moz1.0
timeframe due to API freezing considerations.

Comment 8

[Andreas Otte]

exactly, we should add the method ASAP with NOT_IMPLEMENTED to allow freeze of
the APIs/interfaces. 

Comment 9

[Charles Manske]

cc'ing brade, who is probably going to have to implement this.

Comment 10

[Darin Fisher]

-> 1.0

Comment 11

[Kathleen :Brade]

upping priority due to approaching api freeze
reassign to myself

proposal is to add something like the following to nsIURI.idl:
 string GetCommonBaseURL(string/nsIURI?) // returns common base if any
 string GetRelativeURL(string/nsIURI?)   // returns relative url or complete url

note: for files, you might get a common base url of "file:///"

Darin--did I forget about anything else that we discussed?

Suggestions for exact api?

Comment 12

[Charles Manske]

This sounds good, and it would be good to not always require having an nsIURI 
object. For many uses in Composer, we are dealing with just URL strings, e.g., 
after user types in an input field or the result of filepicker.
What is the overhead of creating a temp nsIURI for a task like this?
For the nsIURI version, obviously it would use the current "spec" as the base.
For a purely string version, it would need the base supplied:
MakeRelativeUrl(in string baseUrl, in string url);
Maybe the latter should be with similar methods in nsIOServices, which simply 
do string manipulation like the "getUrlPart" routines?

Comment 13

[Kathleen :Brade]

I disagree (not that I've verified this though); often Composer has an nsIURI. 
Usually any string is coming from nsIURI.spec.

Comment 14

[Charles Manske]

Yes, often the string we want to relativize is comming from a URI, e.g., from the
filepicker, but the *base* is usually the document we are editing, and we don't 
cache its URI object. So we'd have to create a temp one using the docUrl from 
GetDocumentUrl().

Comment 15

[Darin Fisher]

as a matter of good practice, you should try to hold onto nsIURI's instead of
URI strings.  this is important for example if the nsIURI references a file
since the nsIURI impl will hold onto the underlying nsIFile.  remember a file://
URL is not necessarily sufficient to identify a file on the mac because two
volumes could share the same name!

also, nsStandardURL (the most common nsIURI impl) adds very little footprint
over and above the size of the URI.  it has many advantages because the URI is
already parsed, and you will not need to reparse it again.

Comment 16

[Kathleen :Brade]

Attachment: first stab at uri changes (some cleanup still required)

Comment 17

[Kathleen :Brade]

Attachment: patch to add api and basic implementation

Implementation should move to a new bug; this is the API stuff.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 75465 [details] [diff] [review]
patch to add api and basic implementation

I am happy with getCommonBaseSpec.  If follows that the common base of a URI is
always either nothing, some base which includes the identical string.

I am not happy with getRelativeSpec.  If their is nothing in common, either an
error (throw) or an empty string should be returned.  If they are identical, I
don't want to have some special character tell me this.  In this case, I am not
exactly sure what the best approach would be right now.  I think that maybe
this should also return an error....  In any case, we need to think this out
since these two function will be frozen in a matter of days.

Also, change the name of the parameter to getRelativeSpec.  The current name
implies that the parameter is going to be mutated.

Comment 19

[Darin Fisher]

the return value of GetRelativeSpec should satisfy the following relation:

  result = baseURL.getRelativeSpec(URL)
 <==>
  baseURL.resolve(result) == URL.spec

i don't recall if result='.' correctly satisfies this relation.

Comment 20

[Charles Manske]

If getRelativeSpec cannot resolve to a true relative URL, it should return the
orgininal absolute spec

Comment 21

[Kathleen :Brade]

Attachment: partial patch with just idl

Comment 22

[Darin Fisher]

Comment on attachment 75603 [details] [diff] [review]
partial patch with just idl

>Index: nsIURL.idl

>+    /**
>+     * This method takes a uri and compares the two.
>+     * The common uri portion is returned as a string.
>+     * If no commonality is found, "" is returned.
>+     * If they are identical, the whole path is returned (including file/query/etc.).
>+     * For file uris, it is expected that the common spec would be at least "file:///". 
>+     */
>+    AUTF8String getCommonBaseSpec(in nsIURI aURIToCompare);

you should elaborate on what qualifies as common... e.g., the hostnames have to
be the same.
maybe it would be good to provide some examples.

http://www.mozilla.org/foo/bar
http://www.mozillazine.org/

common part = ""

or whatever... 

otherwise, this interface looks good to me.

Comment 23

[rpotts (gone)]

I guess the APIs look ok...

some implementation notes:

It looks like both GetCommonBaseSpec(...) and GetRelativeSpec(...) leak
'stdurl2' because they don't call NS_RELEASE(...)

also, the check for (*thatIndex != '/0) is redundant... it can just be:

+    while ((*thisIndex == *thatIndex) && *thisIndex)

Comment 24

[Kathleen :Brade]

Attachment: updated patch

reviews/comments please

Comment 25

[Darin Fisher]

Comment on attachment 76206 [details] [diff] [review]
updated patch


>Index: mozilla/netwerk/base/public/nsIURL.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/public/nsIURL.idl,v
>retrieving revision 1.24
>diff -u -2 -r1.24 nsIURL.idl
>--- mozilla/netwerk/base/public/nsIURL.idl	12 Mar 2002 01:03:12 -0000	1.24
>+++ mozilla/netwerk/base/public/nsIURL.idl	26 Mar 2002 16:49:40 -0000
>@@ -149,3 +149,37 @@
>      */
>     attribute AUTF8String fileExtension;
>+
>+    /**
>+     * This method takes a uri and compares the two.
>+     * The common uri portion is returned as a string.
>+     * The minimum common uri portion is the protocol, and any of these
>+     *   if present: login, password, host and port 
>+     * If no commonality is found, "" is returned.
>+     * If they are identical, the whole path with file/ref/etc. is returned.
>+     * For file uris, it is expected that the common spec would be at least 
>+     * "file:///" since '/' is a shared common root.

perhaps you could eliminate some newlines here?  and wrap to 80 chars.	i 
think it would make the comments look cleaner and more consistent with existing
comments in nsIURL.idl.


>+     *  3) http://foo.com:80/bar/    http://foo.com/bar/           ""

does this really make sense?  shouldn't these be the same?


>+    /**
>+     * This method takes a uri and returns a substring of this if it can be
>+     * made relative to the uri passed in.
>+     * If no commonality is found, the entire (absolute) uri spec is returned.
>+     * If they are identical, "" is returned.
>+     * Filename, query, etc. are always returned except when uris are identical.
>+     */

likewise, please wrap the text into a paragraph.


>Index: mozilla/netwerk/base/src/nsStandardURL.cpp

>+nsStandardURL::GetCommonBaseSpec(nsIURI *uri2, nsACString &aResult)

>+    if (!isEquals)
>+    {
>+        aResult = "";

aResult.Truncate() is better.

also, this early return causes stdurl2 to be leaked.


>+    // backup to just after previous slash

this comment doesn't really add much... how about saying why you're
doing this, e.g.:

      // the commmon base spec should not contain the filename part.

>+    while ((*(thisIndex-1) != '/') && (thisIndex != startCharPos))
>+        thisIndex--;
>+
>+    // grab spec from beginning to thisIndex
>+    aResult.SetCapacity(mSpec.Length() + PR_MIN(32, mSpec.Length()/10));

this seems wrong... you know the exact capacity... why estimate it?
in fact, this call to SetCapicity is unnecessary since you will be making
one assignment into aResult:

>+    aResult = Substring(mSpec, mScheme.mPos, thisIndex - mSpec.get());
>+
>+    NS_RELEASE(uri2);

need to release stdurl2.


>+nsStandardURL::GetRelativeSpec(nsIURI *uri2, nsACString &aResult)

>+        return GetSpec(aResult);

leaks stdurl2


>+    // backup to just after previous slash

again, add a comment to explain why you are doing this.  it would be
much more informative then a simple summary of what the code is doing.

>+    while ((*(thisIndex-1) != '/') && (thisIndex != startCharPos))
>+        thisIndex--;

again, SetCapacity should not be called.

>+    return rv;

leaking stdurl2

Comment 26

[Kathleen :Brade]

Attachment: updated patch

* all changes are at 80 or less
* examples modified (using 8080 for port example)
* use of Truncate instead of assigning ""
* note: there is no leak here since we haven't even seen stdurl2 yet
* SetCapacity calls removed
* leaks fixed:	NS_RELEASE calls changed to stdurl2 instead of uri2 
  (doh! I copied and pasted the wrong string; sorry!)
* comments expanded; note that the backing up is meant to grab only complete
path segments, not partial segments (it's not just for chopping off filenames)

Comment 27

[Darin Fisher]

Comment on attachment 76266 [details] [diff] [review]
updated patch

so you're wanting to checkin this impl of GetRelativeSpec even though it is
incomplete?  i guess that's ok, so long as the full impl lands for moz 1.0.  i
don't want to freeze this interface w/ a broken impl if we can help it.

r=darin

Comment 28

[rpotts (gone)]

Comment on attachment 76266 [details] [diff] [review]
updated patch

sr=[e-mail]

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 76266 [details] [diff] [review]
updated patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 30

[Kathleen :Brade]

idl and related changes checked in; bug 133591 will cover remaining
implementation issues

Comment 31

[benc]

+verifyme
charles: QA is looking to verify. Does this do everything you wanted? If so, you
can verify the as the reporter.

Comment 32

[benc]

brade: you are still here right?

Does this work the way you want? Can I verify?

Comment 33

[Kathleen :Brade]

v