bz wants a way to get a content disposition and type of an image from the cache without having to do a full load and whatnot.

Comment on attachment 178286 [details] [diff] [review] v1 So.. this patch would be a lot more readable with more context and some use of the -p flag to cvs diff. >Index: public/imgICache.idl >+ /** >+ * Find Properties >+ * Used to get properties such as 'content-type' and 'content-disposition' What will actually be returned for those properties? The header, some parsed version thereof, or something else entirely (for content-type I assume the third of these, for content-disposition I assume the first... document that, please)? >+ * If you call this before any data has been loaded from a URI, it may come back >+ * without any properties So it'll succeed, but the resulting nsIProperties may be empty? >+ * @return NS_OK if \a uri was located in the cache. >+ * NS_ERROR_NOT_AVAILABLE if \a uri was unable to be found in the cache. That's an @throws, not a @return. And what's with the "\a" thing? Would it make more sense to not throw anything and just return null? I think that makes more sense, since this is not an exceptional circumstance.... >Index: src/imgRequest.cpp >@@ -83,6 +84,7 @@ > mCacheId(0), mValidator(nsnull), mIsMultiPartChannel(PR_FALSE) > { > /* member initializers and constructor code */ >+ mProperties = do_CreateInstance("@mozilla.org/properties;1"); And on out-of-memory what? Maybe this should go in Init()? >@@ -813,6 +815,19 @@ > LOG_MSG(gImgLog, "imgRequest::OnDataAvailable", "Got content type from the channel"); > } > >+ nsCOMPtr<nsISupportsCString> contentType(do_CreateInstance("@mozilla.org/supports-cstring;1")); >+ contentType->SetData(mContentType); This needs to handle out-of-memory. >+ nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aRequest)); >+ if (httpChannel) { >+ nsCOMPtr<nsISupportsCString> contentDisposition(do_CreateInstance("@mozilla.org/supports-cstring;1")); >+ contentDisposition->SetData(disposition); Same here. Also, nsIMultiPartChannel instances can have a content-disposition as well... >Index: src/imgRequest.h >+ inline nsIProperties *GetProperties() { >+ return mProperties.get(); No need for manual .get() here. >Index: src/imgCache.cpp >+NS_IMETHODIMP imgCache::FindEntryProperties(nsIURI *uri, nsIProperties **_retval) >+{ >+ PRBool expired; >+ // This is an owning reference that must be released. >+ imgRequest *request = nsnull; >+ nsCOMPtr<nsICacheEntryDescriptor> entry; Which is? |request|? Note that you don't release it, if so.... Use an nsRefPtr, perhaps, and skip the painful and error-prone refcounting? >+ if (request) { >+ *_retval = request->GetProperties(); >+ NS_ADDREF(*_retval); Make those two be a single line, please. And if there are always properties, perhaps the method should be called Properties(), not GetProperties()? >Index: src/imgCache.h >+class nsIProperties; Why do you need this change? Doesn't forward-declaring it in the IDL handle that?

Comment on attachment 178383 [details] [diff] [review] v2 This doesn't address some of the initial review comments... if you have a good reason for not addressing them, I'd love to hear it, but until then r-.

This should address all your comments....

Comment on attachment 178393 [details] [diff] [review] v4 r=bzbarsky

Comment on attachment 178393 [details] [diff] [review] v4 + * Used to get properties such as 'mime-type' and 'content-disposition' + mProperties->Set("type", contentType); this doesn't seem to match...

Comment on attachment 178393 [details] [diff] [review] v4 sr=shaver with biesi's inconsistency repaired.

fixed the commenting