Darin Fisher Reporter
	Description • 23 years ago

nsStandardURL::Equals should use case sensitive compares...

except if the URL scheme is file, in which case we should really be using
nsIFile::equals.  perhaps we just GetFile unconditionally, and if non-null,
use nsIFile::equals.  this would take care of resource:// URLs and other local URls.

	Darin Fisher Reporter
	Comment 1 • 23 years ago

-> future?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 20 years ago

Note that this is breaking session history in some cases (see bug 99091), so it
really would be nice to fix it...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 20 years ago

Comment on attachment 162405 [details] [diff] [review]
Patch

I left the host compare as case-insensitive, because I'm not sure we
canonicalize hosts.

The nsResURL change is needed because nsResURL::Clone created and
nsStandardURL, and then GetFile() failed.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 5 • 20 years ago

Comment on attachment 162405 [details] [diff] [review]
Patch

+	 // realy failures, so propagate them to caller.

"really"

	Darin Fisher Reporter
	Comment 6 • 20 years ago

Comment on attachment 162405 [details] [diff] [review]
Patch

dbaron has a patch for the nsResURL clone problem.  it's included in his patch
to make dynamic skin changes work properly.  since your solution here is
slightly different, i think you should talk to him and come to an agreement on
how this should be done.

	Darin Fisher Reporter
	Comment 7 • 20 years ago

Comment on attachment 162405 [details] [diff] [review]
Patch

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

>+    if (mSupportsFileURL) {
>+        // Assume not equal for failure cases... but failures in GetFile are
>+        // realy failures, so propagate them to caller.
>+        // XXXbz should failures here log something via NSPR logging?
>+        *result = PR_FALSE;
>+        nsCOMPtr<nsIFile> thisFile;
>+        rv = GetFile(getter_AddRefs(thisFile));

calling GetFile is somewhat costly since it creates a clone of mFile.
maybe it would be better to add an EnsureFile method.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 20 years ago

David, see comment 6.  What bug is that patch on?

Darin, I agree. EnsureFile would be better here.  I'll add that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 20 years ago

This is not synced to dbaron's changes yet, so no ready for review.

bsmedberg, this change makes nsResURL cache the file pointer the first time
GetFile() is called on it.  I assume that this cache needs to be invalidated in
some cases?  Should nsResURL observe some notification or something like that?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 20 years ago

Note to self.  dbaron's patch is in bug 252703 

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 20 years ago

So could we decide the direction we want to jump on the changes to
nsResURL::Clone?  I'd sorta like to get this patch fixed up and checked in.

	Darin Fisher Reporter
	Comment 14 • 20 years ago

Comment on attachment 167694 [details] [diff] [review]
Patch updated to the bug 252703 changes

>Index: netwerk/base/src/nsStandardURL.h

>-
>+    

nit: looks like typo adding extra spaces.


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

>+    // Now canonicalize the host to lowercase
>+    nsCString::iterator iter;
>+    mSpec.BeginWriting(iter);
>+    net_ToLowerCase(iter.get() + mHost.mPos, mHost.mLen);

or this:

  net_ToLowerCase(mSpec.BeginWriting() + mHost.mPos, mHost.mLen);


>+    if (mSupportsFileURL) {
>+        // Assume not equal for failure cases... but failures in GetFile are
>+        // really failures, so propagate them to caller.
>+        // XXXbz should failures here log something via NSPR logging?

nit: yeah, sure.. use the LOG() macro defined in this file :)


>+        *result = PR_FALSE;
>+        rv = EnsureFile();
>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

nit: style for this file is no brackets if not needed.


>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

nit: ditto


>+    if (NS_FAILED(rv)) {
>+        return rv;
>     }

nit: ditto


r+sr=darin with those nits fixed

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 20 years ago

I realized that a resource:// and file:// url pointing to the same file should
be considered different, so I added a check for schemes being the same in the
branch when both are file urls.  This is what I just checked in.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 20 years ago

Fixed on trunk (with another tweak to make this build on Windows).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 20 years ago

This checkin caused bug 280206