>@@ -693,157 +331,44 @@ nsChromeRegistry::ConvertChromeURL(nsIUR >+ rv = GetBaseURIFromPackage(package, provider, path, &baseURI); That's a really confusing signature. Followup to just return nsIURI?

being that this is just code clean up, I don't think it should block

Currently the code can return: NS_ERROR_NOT_INITIALIZED NS_ERROR_FAILURE NS_OK and in the NS_OK case, baseURI might be set to nsnull or a value actually pointing at something. Are we sure we want it to just be able to return nsnull or a value?

> + NS_ASSERTION(baseURI, "GetBaseURIFromPackage returned nsnull"); Uh... why the heck is that ok? Shouldn't the original author of this code be reviewing this, by the way?

And also as far as comment 1 goes... this is "just code cleanup" that was a precondition for a patch landing. Things like that are why we shouldn't allow the "land now, address comments later" crap.

(In reply to comment #3) > > + NS_ASSERTION(baseURI, "GetBaseURIFromPackage returned nsnull"); > > Uh... why the heck is that ok? > > Shouldn't the original author of this code be reviewing this, by the way? Good question, which behavior would you prefer, returning an error on nsnull or ignoring the result letting the existing code cope with it?

I wrote the code that this review comment was about. GetBaseURIFromPackage callers handle a null baseURI case, so scratch the assertion, please.

(In reply to comment #6) > I wrote the code that this review comment was about. GetBaseURIFromPackage > callers handle a null baseURI case, so scratch the assertion, please. So it is ok that it doesn't return an error in the cases that it did before? (see comment #2)

Yes, it's fine.

Comment on attachment 482830 [details] [diff] [review] Change as requested (fits m-c from Oct 13 2010) I hear a volunteer!

(In reply to comment #9) > Comment on attachment 482830 [details] [diff] [review] > Change as requested (fits m-c from Oct 13 2010) > > I hear a volunteer! Wait 10-20 min, and I'll have an updated patch without the assert

Comment on attachment 482936 [details] [diff] [review] Now without the assert, but otherwise the same as preivous patch minor change, addresses bz review comments.

Pushed: http://hg.mozilla.org/mozilla-central/rev/7896216a2724