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.

-> future?

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

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.

Comment on attachment 162405 [details] [diff] [review] Patch + // realy failures, so propagate them to caller. "really"

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.

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.

David, see comment 6. What bug is that patch on? Darin, I agree. EnsureFile would be better here. I'll add that.

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?

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

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.

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

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.

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

This checkin caused bug 280206