94454 - Have the image cache flush when memory is low.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Stuart Parmenter]

The image cache needs add its self as an observer of the memory thread so that 
it can flush the cache when memory is low.

Comment 1

[Stuart Parmenter]

See http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsIMemory.idl#40 for how 
to hookup to this.  imgCache is the class in imagelib that should listen to the 
observer.

Comment 2

[Nivedita (gone)]

Attachment: patch file to flush image cache when the memory is low

Added imagecache as an observer to the memory-pressure event.

Comment 3

[Stuart Parmenter]

Comment on attachment 65564 [details] [diff] [review]
patch file to flush image cache when the memory is low

r=pavlov

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 65564 [details] [diff] [review]
patch file to flush image cache when the memory is low

>+PR_STATIC_CALLBACK(nsresult)
>+Initialize(nsIModule* aSelf)
>+{
>+  imgCache::Init();
>+  return PR_TRUE;
>+}
>+
> PR_STATIC_CALLBACK(void)
>-ImageModuleDestructor(nsIModule *self)
>+Shutdown(nsIModule* aSelf)
> {
>   imgCache::Shutdown();
> }

These two static callbacks may be non-static on some systems, in order to be
callbacks, so they should have unique names -- prefix their given names above
with imgCache_ or some such.

>+nsresult imgCache::Init()
>+{
>+  nsresult rv;
>+  imgCache* cache = new imgCache();

Test for null result from operator new and fail with NS_ERROR_OUT_OF_MEMORY if
so.

>+
>+  nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1", &rv);
>+  if (NS_SUCCEEDED(rv) && os)
>+    os->AddObserver(cache, "memory-pressure", PR_TRUE);

Capture os->AddObserver()'s result in rv, and return it instead of NS_OK below:

>+  return NS_OK;
>+}
>+
> /* void clearCache (in boolean chrome); */
> NS_IMETHODIMP imgCache::ClearCache(PRBool chrome)
> {
>@@ -255,3 +268,12 @@
>   return PR_TRUE;
> }
> 
>+
>+NS_IMETHODIMP
>+imgCache::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aSomeData)
>+{
>+  if (nsCRT::strcmp(aTopic, "memory-pressure") == 0)

We've finally figured out that all Mozilla platforms have working strcmp (:-)
so no need for nsCRT:: here -- if you get rid of it, you'll be ahead of the
game when we soon purge most str* wrapper methods from nsCRT.

>+    ClearCache(PR_TRUE);
>+
>+  return NS_OK;
>+}

Fix these issues and I'll gladly sr=[e-mail].

/be

Comment 5

[Nivedita (gone)]

Attachment: patch file incorporating the comments given by Brenden

Incorporated the comments given by Brenden

Comment 6

[Nivedita (gone)]

Brenden,
I am not clear on the static call backs being non-static on some systems. Could 
you please elaborate on it. I am wondering in these cases how would the static 
intializers and etc be taken care of. what should be taken care while writing 
static methods or static blocks.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 66245 [details] [diff] [review]
patch file incorporating the comments given by Brenden

>+PR_STATIC_CALLBACK(nsresult)
>+imgCache_Initialize(nsIModule* aSelf)
>+{
>+  imgCache::Init();
>+  return PR_TRUE;
>+}
>+
> PR_STATIC_CALLBACK(void)
>-ImageModuleDestructor(nsIModule *self)
>+imgCache_Shutdown(nsIModule* aSelf)
> {
>   imgCache::Shutdown();
> }

See http://lxr.mozilla.org/mozilla/source/nsprpub/pr/include/prtypes.h#147 for
the macro that (on windows) defines PR_STATIC_CALLBACK to expand without the
static storage class/visibility specifier.

The callbacks on Windows are therefore extern, not static -- still singleton
functions, but with names that pollute the entire application's global
namespace, not with names scoped to the file.

>+nsresult imgCache::Init()
>+{
>+  nsresult rv;
>+  imgCache* cache = new imgCache();
>+  if(!cache) return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsCOMPtr<nsIObserverService> os = do_GetService("@mozilla.org/observer-service;1", &rv);
>+  if (NS_SUCCEEDED(rv) && os)
>+    rv = os->AddObserver(cache, "memory-pressure", PR_TRUE);
>+  return rv;
>+}

Bonus style points (change only if you agree) for moving the nsresult rv;
declaration down to just before it's actually needed.

Thanks for the nit-picks, sr=[e-mail].

/be

Comment 8

[Nivedita (gone)]

Attachment: incorporating the comment given by Brenden

defering the creation of nsresult untill needed.

Brenden,
Thanks for the info.

Comment 9

[Stuart Parmenter]

Comment on attachment 66246 [details] [diff] [review]
incorporating the comment given by Brenden

r=/sr= from previous patch

Comment 10

[Stuart Parmenter]

fix checked in.  great job nivedita!