While playing with the gopher integration, I noticed that the directory viewer code does a string comparison in nsHTTPIndex::isWellknownContainerURI for each entry. I have some hacked up code to get this from what was parsed orignally (and use an RDF attribute) but it doesn't quite work properly. The string parsing code can't be removed, because its (possibly) needed for the initial url entered (and definitely needed for bookmarks and anywhere else using rdf:httpindex), but I should be able to remove most of the protocol specific code, and make it more general.

The attached patch: 1. Cleans up the string foo - I don't know why I thought nsXPIDLCString couldn't do this. 2. Caches whether the given uri is a directory or not. This is both more correct (if the server says it is a directory, then it is), and makes it more obvious what is going on. The fallback code can't be removed though - see the comments in the bug. ccing waterson for review.

Looks good. Could you scrub per below, and update the patch? Also cc'ing dougt who's been working on this file, too (but I think in a different part). nsHTTPIndexParser::OnStartRequest(nsIRequest *request, nsISupports* aContext) { + nsresult rv; Why the whitespace? { + NS_ASSERTION(context, "context is null"); + nsXPIDLCString encodingStr; Inconsistent indentation. If context is null, are you going to crash later? If so, add an early return here. NS_IF_RELEASE(kNC_loading); NS_IF_RELEASE(kNC_URL); + NS_IF_RELEASE(kNC_IsContainer); NS_IF_RELEASE(kTrueLiteral); More kooky indents. mDirRDF->GetResource(NC_NAMESPACE_URI "loading", &kNC_loading); mDirRDF->GetResource(NC_NAMESPACE_URI "URL", &kNC_URL); + mDirRDF->GetResource(NC_NAMESPACE_URI "IsContainer", &kNC_IsContainer); Ibid. nsCAutoString tmp; tmp.AssignWithConversion(uri); - cUri = ToNewCString(tmp); + dest = tmp.ToNewCString(); } Oy. beard added the assignment operator and it's been haunting us ever since. Source of many, many string leaks. Say this instead: *getter_Copies(dest) = tmp.ToNewCString() to make it clear that you know what's going on with the buffer ownership. (I know it's uglier, but it'll make it easier when jag and scc clean up this mess someday.) (Nice fix for the ownership problem, BTW.) @@ -1203,15 +1215,14 @@ } if ((uri) && (!strncmp(uri,kGopherProtocol, sizeof(kGopherProtocol)-1))) { char* pos = PL_strchr(uri+sizeof(kGopherProtocol)-1, '/'); - if (pos) { - if (pos == nsnull || *(pos+1) == '\0' || *(pos+1) == '1') - isContainerFlag = PR_TRUE; - } + // Note - not type 7. Searches should not be autoexpanded, + // because they are not (by themselves) containers. + if (!pos || pos[1] == '\0' || pos[1] == '1') + isContainerFlag = PR_TRUE; Add an example of what a ``gopher:'' directory URL should look like in a comment so that the fancy code can be understood without referring to an RFC. + if (aSource) + httpIndex->GetDestination(aSource,uri); Local style nit: space after `,'. + + rv = NS_NewURI(getter_AddRefs(url), uri.get()); Could `uri' be null at this point? If so, will NS_NewURI() choke? nsIRDFResource *kNC_URL; + nsIRDFResource *kNC_IsContainer; nsIRDFLiteral *kTrueLiteral; More kooky indenting.

The indenting is because the file is currently mostly tabs, so when diff places a character before each line, the spaces move up one, but the tabs stay the same. It all looks aligned (on the screen) when tabs expand to 4 spaces. > If context is null, are you going to crash later? If so, add an early return here. I won't crash - I do_QueryInterface on it, and return NS_ERROR_UNEXPECTED if the QI fails. It shouldn't be null, because I never pass a null value in to AsyncOpen. I added that in to check something - I may as well just remove it. Our context behaviour has Issues though, in general. The string fix came from a discussion with scc the other night, when he made me realise that: a) I didn't want to do what I thought I wanted to do (in terms of wanting strings that could keep track of what encoding they were assigned into) b) I could assign a char* to an xpidl[c]string. > *getter_Copies(dest) = tmp.ToNewCString() Thats just confusing :) You're relying on the fact that getter_Copies returns a private class which has a char** operator to the actual string. I see what its doing, and how it works, and the fact that its guaranteed to work, but it doesn't strike me as any more documented. Even though thats how all getter_*() stuff works. I'll change it though. I'd still prefer to be able to append to the xpidlcstring automatically - thats apparently coming. A gopher directory url has first character 1. Thats it. I'll add a comment. > Could `uri' be null at this point? If so, will NS_NewURI() choke? xpidlcstrings can't contain null, can they? In any event, it can't - GetDestination will always put something in there. I'll put up a new patch tomorrow.

> > If context is null, are you going to crash later? If so, add an early > > return here. > > I won't crash - I do_QueryInterface on it, and return NS_ERROR_UNEXPECTED > if the QI fails. Ok, then put your assertion after the QI(). > > Could `uri' be null at this point? If so, will NS_NewURI() choke? > > xpidlcstrings can't contain null, can they? Yes, they can. Furhtermore, you do an unchecked GetValueConst() in GetDestination(): if that method fails, you'll end up with a null in the nsXPIDLCString.

if ((uri) && (!strncmp(uri,kGopherProtocol, sizeof(kGopherProtocol)-1))) { char* pos = PL_strchr(uri+sizeof(kGopherProtocol)-1, '/'); - if (pos) { - if (pos == nsnull || *(pos+1) == '\0' || *(pos+1) == '1') - isContainerFlag = PR_TRUE; - } + // Note - not searches. Searches should not be autoexpanded, + // because they are not (by themselves) containers. + // The only thing that we want to expand are directories + // which either have no selector at all, or have a first + // character of 1 + if (!pos || pos[1] == '\0' || pos[1] == '1') I guess I'd still like to see a comment here that says, ``gopher URLs are of the form [blah blah blah]. A search is indicated by [blah blah blah], which we won't expand''. Write the code as if you're writing it for the sorry sap that's going to be fixing bugs in this stuff (or adding functionality) down the road. nsIRDFResource *kNC_loading; - nsIRDFResource *kNC_URL; + nsIRDFResource *kNC_URL; + nsIRDFResource *kNC_IsContainer; nsIRDFLiteral *kTrueLiteral; Still some goofy indentation. Fix those two things, and r/sr=waterson, whichever helps.

if (NS_FAILED(rv = nsComponentManager::CreateInstance(kRDFInMemoryDataSourceCID, - nsnull, NS_GET_IID(nsIRDFDataSource), (void**) getter_AddRefs(mInner)))) - { + nsnull, + NS_GET_IID(nsIRDFDataSource), + getter_AddRefs(mInner)))) { Since you're touching that, could you replace that with: mInner = do_CreateInstance(kRDFInMemoryDataSourceCID, &rv); if (NS_FAILED(rv)) return rv; (Or better yet, use the contract id instead of the ClassID) That file is _so_ style-screwed :-/

Fix checked in.