287286 - Need way to get properties of an image request from cache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Stuart Parmenter]

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 1

[Stuart Parmenter]

Attachment: v1

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 3

[Stuart Parmenter]

Attachment: v2

Comment 4

[Boris Zbarsky [:bzbarsky]]

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-.

Comment 5

[Stuart Parmenter]

Attachment: v3

This should address all your comments....

Comment 6

[Stuart Parmenter]

Attachment: v4

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178393 [details] [diff] [review]
v4

r=bzbarsky

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 178393 [details] [diff] [review]
v4

sr=shaver with biesi's inconsistency repaired.

Comment 10

[Stuart Parmenter]

fixed the commenting