Closed Bug 208882 Opened 21 years ago Closed 21 years ago

remove preference imageblocker.enabled


(Core :: Graphics: Image Blocking, defect)

Not set





(Reporter: danielwang, Assigned: mconnor)




(Keywords: polish)


(2 files, 2 obsolete files)

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:

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?
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...
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....
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.
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?
useless prefs are lame

-> me
Assignee: security-bugs → mconnor
Attached patch remove the pref (obsolete) — Splinter Review
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 on attachment 139850 [details] [diff] [review]
remove the pref

about time I attached a patch that removes bloat :)
Attachment #139850 - Flags: review?(dwitte)
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
Comment on attachment 139850 [details] [diff] [review]
remove the pref

dwitte seems pretty busy.  mvl?
Attachment #139850 - Flags: review?(dwitte) → review?(mvl)
pretty busy eh? mumble mumble... ;)

but yeah, please review this mvl, you like xul/js fu more than i do. :)
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

fix that, and r=mvl. For now, i don't like useless stuff floating around ;)
Attachment #139850 - Flags: review?(mvl) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #139850 - Attachment is obsolete: true
Comment on attachment 140160 [details] [diff] [review]
patch v2

added the entries in Thunderbird and Calendar to this one
Attachment #140160 - Flags: review?(mvl)
Comment on attachment 140160 [details] [diff] [review]
patch v2

Attachment #140160 - Flags: review?(mvl) → review+
Attachment #140160 - Flags: superreview?(bz-vacation)
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....
Attachment #140160 - Flags: superreview?(bz-vacation) → superreview?(alecf)
Attachment #140160 - Attachment is obsolete: true
Attachment #140160 - Flags: superreview?(alecf)
Attached patch patch v3Splinter Review
missed a spot
Attachment #140326 - Flags: superreview?(alecf)
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

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.


(by the way, when are we moving all this permission management stuff out of
Attachment #140326 - Flags: superreview?(alecf) → superreview+
checked in for mconnor. thx for the patch!
Closed: 21 years ago
Resolution: --- → FIXED
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[';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();
>-      }
>     }
for completeness, this pref was partially brought back to life on the 1.7 branch


it will not be brought back to life on the trunk.
You need to log in before you can comment on or make changes to this bug.