980036 - Move image/* preferences to gfxPrefs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

Move preferences from
image/src/DiscardTracker.cpp
image/src/ImageFactory.cpp
image/src/imgLoader.cpp
image/src/RasterImage.cpp
image/src/SurfaceCache.cpp
to use gfxPrefs class.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=49066d9b1720

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to manage imagelib prefs.

No urgency on this one.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8490770 [details] [diff] [review]
Use gfxPrefs to manage imagelib prefs.

Review of attachment 8490770 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! r+'d with the issues below addressed.

::: image/src/DiscardTracker.cpp
@@ +180,5 @@
>  DiscardTracker::Initialize()
>  {
>    // Watch the timeout pref for changes.
>    Preferences::RegisterCallback(DiscardTimeoutChangedCallback,
>                                  sDiscardTimeoutPref);

So I guess gfxPrefs can't support callbacks? It would be nice if it did; maybe file a followup.

::: image/src/RasterImage.cpp
@@ +433,5 @@
>  /* static */ void
>  RasterImage::Initialize()
>  {
> +  // This should already be initialized, but just in case:
> +  gfxPrefs::GetSingleton();

Seems like it'd be easier to just call this in nsImageModule::InitModule(), before all of the Initialize() calls there. Then we don't have to worry about calling it in multiple places, and you can remove the gfxPrefs::GetSingleton() call from e.g. ImageFactory.

::: image/src/SurfaceCache.cpp
@@ +27,5 @@
>  #include "nsSize.h"
>  #include "nsTArray.h"
>  #include "prsystem.h"
>  #include "SVGImageContext.h"
> +#include "gfxPrefs.h"

Nit: it's preferable to keep things alphabetized.

@@ +461,5 @@
>  {
>    // Initialize preferences.
>    MOZ_ASSERT(!sInstance, "Shouldn't initialize more than once");
>  
> +  // See gfxPrefs for the default values

This comment either needs rewording, since the reader will look down and say "What do you mean, I see the default values right there!", or you should remove the default value comments below.

I think it's probably better to remove the comments, because I suspect they'll inevitably start to get out of sync with the values in gfxPrefs.

::: image/src/imgLoader.cpp
@@ -1108,5 @@
> -  if (NS_SUCCEEDED(rv))
> -    sCacheMaxSize = cachesize;
> -  else
> -    sCacheMaxSize = 5 * 1024 * 1024;
> -  sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0;

We recently added this line as part of a bug fix, because code elsewhere can't handle a negative value. We still need it.

What would be considerably simpler is if the pref were just a uint32_t, but I'm not sure whether it's safe to change the type of an existing pref.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #3)
> ...
> 
> ::: image/src/imgLoader.cpp
> @@ -1108,5 @@
> > -  if (NS_SUCCEEDED(rv))
> > -    sCacheMaxSize = cachesize;
> > -  else
> > -    sCacheMaxSize = 5 * 1024 * 1024;
> > -  sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0;
> 
> We recently added this line as part of a bug fix, because code elsewhere
> can't handle a negative value. We still need it.
> 
> What would be considerably simpler is if the pref were just a uint32_t, but
> I'm not sure whether it's safe to change the type of an existing pref.

But sCacheMaxSize is uint32_t, so this:

sCacheMaxSize = sCacheMaxSize > 0 ? sCacheMaxSize : 0;

is equivalent to this, right?

sCacheMaxSize = sCacheMaxSize != 0 ? sCacheMaxSize : 0;

which is a no-op.  However, I see what we should be doing, and I'll make sure the new version does that; in the old code, it really should have been this instead:

if (NS_SUCCEEDED(rv))
  sCacheMaxSize = cachesize > 0 : cachesize : 0;
else
  sCacheMaxSize = 5 * 1024 * 1024;

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #3)
> ...
> >    Preferences::RegisterCallback(DiscardTimeoutChangedCallback,
> >                                  sDiscardTimeoutPref);
> 
> So I guess gfxPrefs can't support callbacks? It would be nice if it did;
> maybe file a followup.

Bug 1069620.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to manage imagelib prefs. Carry r=sfowler.

Address review comments.  Will wait for bug 1069582, this patch is based of the one there.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

While the changes from the previous patch were minimal, I did move where gfxPrefs are potentially initialized (at least during the test runs), so do another run just in case:
https://tbpl.mozilla.org/?tree=Try&rev=47c614ae0d62

Comment 8

[Seth Fowler [:seth] [:s2h]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> However, I see what we should be doing, and I'll make
> sure the new version does that

Thanks! I overlooked the issue you pointed out here when I reviewed that patch originally.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to manage imagelib prefs. Carry r=sfowler.

Typo in the previous patch https://tbpl.mozilla.org/?tree=Try&rev=b50ee3ee41d0

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to manage imagelib prefs. Carry r=seth.

https://tbpl.mozilla.org/?tree=Try&rev=000f4aa7d74f

Comment 11

[Carsten Book [:Tomcat]]

Hi Milan, the patch didn't apply cleanly:

patching file image/src/RasterImage.cpp
Hunk #4 FAILED at 2486
1 out of 8 hunks FAILED -- saving rejects to file image/src/RasterImage.cpp.rej
patch failed, unable to continue (try -v)

could you take a look (maybe it need a rebase against a current tree). Thanks!

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use gfxPrefs to manage imagelib prefs. Carry r=seth.

Rebased patch. https://tbpl.mozilla.org/?tree=Try&rev=c09e720a1c89

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d42f046984ce

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d42f046984ce