208882 - remove preference imageblocker.enabled

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Daniel Wang]

in /extensions/cookie/nsImgManager.cpp revision 1.19, checked in by bzbarsky,
per bug 191380, preference "imageblocker.enabled" appears out of nowhere. This
pref appears to be a relic from Netscape 7. Several bug reports seem to indicate
that Netscape 7 users have trouble with this "hidden" pref. I suggest that we
remove this preference.

I could only find two source files using this pref:
/extensions/cookie/nsImgManager.cpp
/extensions/cookie/nsImgManager.h

bugs mentioning this pref:
bug 33139 bug 35981 bug 43195 bug 43209
bug 51958 bug 36514 bug 91881 bug 66645
bug 47628 bug 47253 bug 74369 bug 41057
bug 112520 bug 62615 bug 84224

btw, shouldn't bug 33576 be moved to image:blocking component?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hint:  ccing the patch author and reviewer is a great deal more useful than
ccing the person who checked in the patch.  The former may even have an idea of
what's in the code or something...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and as for source files using the pref... you clearly missed all.js,
pref-images.xul, cookieContextOverlay.xul, cookieNavigatorOverlay.xul.

Sure looks to me like the pref does something useful....

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

I personaly won't mind removing the pref. But i don't know why it is there. IT
already appears in version 1.1 of the (now removed) file nsImages.cpp.
So, if anyone knows why it is there, please let me know :) Otherwise, i will be
glad to see it gone.

Comment 4

[[e-mail]]

i don't see the point behind this pref... it's just used to disable the blocking
functions in the "images" prefpane. would we not always want them on?

as long as they're not useful to mozilla, we should be able to remove it...
embeddors should be providing their own impl of nsImgManager if they want random
other prefs.

mvl, if you agree, want to whip up a patch to remove it?

Comment 5

[Mike Connor [:mconnor]]

useless prefs are lame

-> me

Comment 6

[Mike Connor [:mconnor]]

Attachment: remove the pref

As far as I can make out, only Netscape ever used this pref.  Firebird has it
in firebird.js, but doesn't use it.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 139850 [details] [diff] [review]
remove the pref

about time I attached a patch that removes bloat :)

Comment 8

[Mike Connor [:mconnor]]

cc-ing project owners for the other two products that include this preference.

mscott: should all.js be removed from thunderbird now that you have a
thunderbird.js?

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 139850 [details] [diff] [review]
remove the pref

dwitte seems pretty busy.  mvl?

Comment 10

[[e-mail]]

pretty busy eh? mumble mumble... ;)

but yeah, please review this mvl, you like xul/js fu more than i do. :)

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 139850 [details] [diff] [review]
remove the pref

>Index: mozilla/extensions/cookie/nsImgManager.cpp
>-  if (PREF_CHANGED(kImageBlockerPrefName) &&
>-      NS_SUCCEEDED(aPrefBranch->GetBoolPref(kImageBlockerPrefName, &val)))
>-    mBlockerPref = val;

You should remove mBlockerPref from the .h too.

>Index: mozilla/extensions/cookie/resources/content/cookieNavigatorOverlay.xul

>     // for some unexplainable reason, CheckForImage() keeps getting called repeatedly
>     // as we mouse over the task menu.  IMO, that shouldn't be happening.  To avoid
>     // taking a performance hit due to this, we will set the following flag to avoid
>     // reexecuting the routine
>     var alreadyCheckedForImage = false;

You can remove the comment as alreadyCheckedForImage isn't needed anymore.

>@@ -87,22 +82,14 @@
>       // determine if image manager should be in the UI
>       if (alreadyCheckedForImage) {
>         return;
>       }
>       alreadyCheckedForImage = true;
>-      // remove image functions (unless overruled by the "imageblocker.enabled" pref)
>-      try {
>-        if (!pref.getBoolPref("imageblocker.enabled")) {
>-          HideImage();
>-        }
>-      } catch(e) {
>-        HideImage();
>-      }
>     }

because it was just to not run this block, which is removed now anyway.

Also, dont forget
http://lxr.mozilla.org/mozilla/source/calendar/sunbird/app/profile/all.js#472

fix that, and r=mvl. For now, i don't like useless stuff floating around ;)

Comment 12

[Mike Connor [:mconnor]]

Attachment: patch v2

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 140160 [details] [diff] [review]
patch v2

added the entries in Thunderbird and Calendar to this one

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 140160 [details] [diff] [review]
patch v2

r=mvl

Comment 15

[Boris Zbarsky [:bzbarsky]]

I may well not get to this in time for 1.7a; if you can find anyone else to do
the sr, that would be much appreciated....

Comment 16

[Mike Connor [:mconnor]]

Attachment: patch v3

missed a spot

Comment 17

[Alec Flett]

Comment on attachment 140326 [details] [diff] [review]
patch v3

I remember the history on this - basically the Netscape marketing department
didn't think it was a good idea to include image blocking, because their
advertisers wouldn't like it. (because obviously the point of this is to block
ads!)

but hey, good riddens... 

but before you continue any further, can you move mPermissionManager to be
declared before all the PRPackedBool/PRUint8  attributes in nsImgManager.h? It
doesn't affect us now, but if we add/delete other PRPackedBool's, I'd like to
see them declared at the end of the class.

sr=alecf

(by the way, when are we moving all this permission management stuff out of
extensions/cookie?)

Comment 18

[Mike Connor [:mconnor]]

Attachment: patch with move of mPermissionManager per SR comments

Comment 19

[[e-mail]]

checked in for mconnor. thx for the patch!

Comment 20

[[e-mail]]

Comment on attachment 140327 [details] [diff] [review]
patch with move of mPermissionManager per SR comments

This local variable can go too, it's not doing anything (in fact it could have
gone before, we already have a global of the same name...)
> 
>       var pref;
>       pref = Components.classes['@mozilla.org/preferences-service;1'];
>       pref = pref.getService();
>       pref = pref.QueryInterface(Components.interfaces.nsIPrefBranch);
>-
>-      // determine if image manager should be in the UI
>-      if (alreadyCheckedForImage) {
>-        return;
>-      }
>-      alreadyCheckedForImage = true;
>-      // remove image functions (unless overruled by the "imageblocker.enabled" pref)
>-      try {
>-        if (!pref.getBoolPref("imageblocker.enabled")) {
>-          HideImage();
>-        }
>-      } catch(e) {
>-        HideImage();
>-      }
>     }

Comment 21

[(not reading, please use [e-mail] instead)]

for completeness, this pref was partially brought back to life on the 1.7 branch
only.

see http://bugzilla.mozilla.org/show_bug.cgi?id=244531

it will not be brought back to life on the trunk.