nsIURL::GetRelativeSpec documentation is incorrect

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla1.9.3a4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
In nsIURL.idl:

    /**
     * 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 uri spec is returned.  If they are identical, "" is returned.
     * Filename, query, etc are always returned except when uris are identical.
     */
    AUTF8String getRelativeSpec(in nsIURI aURIToCompare);

Note that the documentation says that the return value is a substring of |this|.

Test case:

  nsCOMPtr<nsIURI> uri1, uri2; 
  NS_NewURI(getter_AddRefs(uri1), 
            NS_LITERAL_STRING("http://foo.com/")); 
  NS_NewURI(getter_AddRefs(uri2), 
            NS_LITERAL_STRING("http://foo.com/bar")); 
  nsCOMPtr<nsIURL> url1(do_QueryInterface(uri1)), 
                   url2(do_QueryInterface(uri2)); 
 
  nsCAutoString rel; 
  url1->GetRelativeSpec(url2, rel); 
  printf("url1->GetRelativeSpec(url2) = '%s'\n", rel.get()); // prints "bar"
  url2->GetRelativeSpec(url1, rel);
  printf("url2->GetRelativeSpec(url1) = '%s'\n", rel.get()); // prints ""
Hm, yeah. It's incorrect anyway in that it doesn't necessarily return a substring (it may add some ../)
(Assignee)

Updated

8 years ago
Summary: nsIURL::GetRelativeSpec documentation inverts its parameters → nsIURL::GetRelativeSpec documentation is incorrect
(Assignee)

Comment 2

8 years ago
Created attachment 434016 [details] [diff] [review]
Patch

Christian, what do you think of this comment?
Attachment #434016 - Flags: review?(cbiesinger)
(Assignee)

Comment 3

8 years ago
Created attachment 434020 [details]
Testcase

This is the code I used to check the comment.
(Assignee)

Updated

8 years ago
Attachment #434020 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 434016 [details] [diff] [review]
Patch

Can you turn your C++ testing code into an xpcshell test for netwerk/test/unit?
Attachment #434016 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 5

8 years ago
Created attachment 434546 [details] [diff] [review]
Patch with testcase
Attachment #434016 - Attachment is obsolete: true
Attachment #434020 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
Created attachment 434547 [details] [diff] [review]
Patch with testcase v1.1

'this' -> '|this|' in the comment, which is clearer, I think.
Assignee: nobody → justin.lebar+bug
Attachment #434546 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 434547 [details] [diff] [review]
Patch with testcase v1.1

I think makeURL is a more common name for this kind of function
Attachment #434547 - Flags: review+
(Assignee)

Comment 8

8 years ago
Created attachment 434551 [details] [diff] [review]
Patch for checkin
Attachment #434547 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/282d7009f226
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.