Closed Bug 677260 Opened 9 years ago Closed 9 years ago

Need a way to hash on URIs ignoring a fragment identifier

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: bzbarsky, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 7 obsolete files)

For example, I could use this in bug 676740.
Blocks: 676740
For example, take a URI with y://z.com/a/b/c?foo#bar and return y://z.com/a/b/c?foo
Status: NEW → ASSIGNED
Proposed signatures (with bz) are:
SpecIgnoringRef() and also HasRef() (readonly boolean attribute)
Attachment #551782 - Flags: review?(bzbarsky)
Comment on attachment 551782 [details] [diff] [review]
provide URI-without-ref and test for existance of a ref

Drive-by comments on this beautiful pony:

>--- a/netwerk/base/public/nsIURI.idl
>+++ b/netwerk/base/public/nsIURI.idl

Rev uuid

>+NS_IMETHODIMP
>+nsSimpleURI::GetHasRef(PRBool *result)
>+{
>+    if (mIsRefValid)
>+        *result = PR_TRUE;
>+    else
>+        *result = PR_FALSE;

*result = mIsRefValid;

>+nsStandardURL::GetHasRef(PRBool *result)
>+{
>+    if (mRef.mPos >= 0)
>+        *result = PR_TRUE;
>+    else
>+        *result = PR_FALSE;

*result = mRef.mPos >= 0;
*result = (mRef.mPos >= 0);

Pick one :)
Attachment #551782 - Attachment is obsolete: true
Attachment #551782 - Flags: review?(bzbarsky)
Comment on attachment 552108 [details] [diff] [review]
provide URI-without-ref and test for existance of a ref

Updated to resolve ms2ger's nits (thanks)
Attachment #552108 - Flags: review?(bzbarsky)
Comment on attachment 552108 [details] [diff] [review]
provide URI-without-ref and test for existance of a ref

> +    if (mRef.mLen >= 0) {

Should that be testing mPos?  Seems like it.

Also, please make specIgnoringRef a readonly attribute in the IDL.  And add it to the end of nsIURI, not in the middle.

You should also rev whatever interfaces inherit from nsIURI.  http://people.mozilla.org/~sfink/uploads/update-uuids is a good tool for that.

r=me with those nits or a good explanation of why you want mLen and not mPos above.
Attachment #552108 - Flags: review?(bzbarsky) → review+
Blocks: 225910
Attached patch patch with updated UUIDs (obsolete) — Splinter Review
Nice tool!
Attachment #552108 - Attachment is obsolete: true
Attachment #552841 - Attachment is obsolete: true
<segment>.mLen == -1 is used throughout nsStandardURL to indicate that a segment doesn't exist.  mRef.mLen == 0 means the ref is "#"

I also added one more test to check that specIgnoringRef == spec when there is no ref.
Comment on attachment 552894 [details] [diff] [review]
patch with all nits addressed

Carrying forward r=bz
Attachment #552894 - Flags: review+
Attachment #552894 - Flags: superreview?(dbaron)
Comment on attachment 552894 [details] [diff] [review]
patch with all nits addressed

Switching sr= to :biesi
Attachment #552894 - Flags: superreview?(dbaron) → superreview?(cbiesinger)
Now that I've been poked:  one thing I wonder is whether the specIgnoringRef attribute would be better called preRef, given the existing prePath/path pair.  I'm ok with it either way, though.
I'm neutral: you have CloneIgnoringRef, and you have prePath/path.  I chose specIgnoringRef because it was more obvious as to the function; I would find preRef less intuitive.  But the difference is marginal, and it's trivial to change.  Up to bz/dbaron/biesi; just let me know.
> <segment>.mLen == -1 is used throughout nsStandardURL to indicate that a
> segment doesn't exist.

Then shouldn't you test that in GetHasRef instead of testing mPos?  My point was ustthat GetSpecIgnoringRef and GetHasRef should be consistent in the test they use...
Attachment #552894 - Attachment is obsolete: true
Attachment #552894 - Flags: superreview?(cbiesinger)
Comment on attachment 553004 [details] [diff] [review]
patch with all nits addressed. including mRef.mPos

Correct - that mref.mPos use *was* wrong, updated.  I'll throw some tests for hasRef in too in a bit.
Attachment #553004 - Attachment is obsolete: true
Comment on attachment 553015 [details] [diff] [review]
patch with all nits addressed, including mRef.mPos, and hasRef tests added

Tests hasRef false and true, and I purposely mis-set the tests to make sure it would kick on hasRef being wrong.

Carrying forward r=bz, and sr=? to biesi
Attachment #553015 - Flags: superreview?(cbiesinger)
Attachment #553015 - Flags: review+
I realize one very minor nit: it's canonically more correct (though not an issue here) to use "if (!(typeof aTest.hasRef === "undefined"))" instead of "if (aTest.hasRef != undefined)".  If no one chimes in and says to rev it, I'll leave it as-is.
Comment on attachment 553015 [details] [diff] [review]
patch with all nits addressed, including mRef.mPos, and hasRef tests added

Review of attachment 553015 [details] [diff] [review]:
-----------------------------------------------------------------

Please also add a test that checks that an empty ref returns true for hasRef (http://foo/bar#)

::: caps/src/nsNullPrincipalURI.cpp
@@ +205,5 @@
> +// result may contain unescaped UTF-8 characters
> +NS_IMETHODIMP
> +nsNullPrincipalURI::GetSpecIgnoringRef(nsACString &result)
> +{
> +  result = mScheme + NS_LITERAL_CSTRING(":") + mPath;

I'd just make this "return GetSpec(result);", the code is the same.

::: netwerk/base/src/nsStandardURL.cpp
@@ +1004,5 @@
> +nsStandardURL::GetSpecIgnoringRef(nsACString &result)
> +{
> +    // URI without ref is 0 to one char before ref
> +    if (mRef.mLen >= 0) {
> +        URLSegment noRef(0,mRef.mPos-1);

spaces after , and around - please

::: netwerk/test/unit/test_URIs.js
@@ +692,5 @@
>    do_check_property(aTest, URI, "username");
>    do_check_property(aTest, URI, "password");
>    do_check_property(aTest, URI, "host");
> +  do_check_property(aTest, URI, "specIgnoringRef");
> +  if (aTest.hasRef != undefined) {

if ("hasRef" in aTest)
Attachment #553015 - Flags: superreview?(cbiesinger) → superreview+
I guess I should also mention that I'm not terribly happy with uglifying this interface further but URI classes need a rework anyway.
OS: Mac OS X → All
Hardware: x86 → All
Attachment #553015 - Attachment is obsolete: true
Attachment #553251 - Attachment is obsolete: true
Comment on attachment 553252 [details] [diff] [review]
patch with biesi's nits addressed (inc. space)

Carrying forward r+ for bz, sr+ for biesi
Attachment #553252 - Flags: superreview+
Attachment #553252 - Flags: review+
Checked in: http://hg.mozilla.org/mozilla-central/rev/a8e6a5bebe87
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Please set the target milestone to whatever version this got fixed in?  Hard to set tracking flags on regressions otherwise....
Depends on: 682031
This change made FF8 (barely)
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.