174687 - Show domain in the context menu for image blocking

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[Radu Cornea]

This is an enhancement request for image blocking.

It would be nice if "Block images from this server" could be replaced with
"Block images from www.site.com" in the image conext menu. That's how it is
implemented in galeon and I find it very handy particularly with all these news
sites that tend to include ads from couple of different sites, but sometimes
they also host them locally. To block only the images coming from a different
domain, you need to go to image properties and check them first to make sure you
are not blocking valid images. This just adds another step that can be skipped
if the domain is specified in the context menu.

I think this should be trivial to fix, as the image information is available at
the time the image menu is created.

Comment 1

[David Tenser [:djst]]

> This is an enhancement request for image blocking.
Then why didn't you select Enhancement instead of Normal bug?

However, I agree that this would be very useful and probably very easy to
implement as well. Right now, I have to go to the advanced preferences and see
what actual domain I blocked (e.g. "was it www.site.com or ad.site.com?")

Comment 2

[eric lindvall]

Attachment: Patch to show hostname in context menu

This patch displays the hostname that will be blocked in the context menu, such
as:

"Block Images from mozilla.org"

Comment 3

[Peter]

Eric got his fix in first... I got a "mid air collision" from Bugzilla but
having spent some time playing with this I'm posting anyway!

---------------------------------------------------------------------------

This is the first time I have tried to submit a fix, and I know that a
differences file is expected, preferably against the latest csv code.  I'm not
ready for that yet...

However, I think I have a fix, tested on Phoenix 0.5, by editing the browser.js
in the browser.jar, as follows.

The function initMiscItems sets up assorted context menu things, including the
presence or absence of the check (tick) on the Block Image menu item
"context-blockimage" as shown in this snippet of code:

if (this.onImage) {
    var blockImage = document.getElementById("context-blockimage");
    if (this.isImageBlocked()) {
        blockImage.setAttribute("checked", "true");
    }
    else
        blockImage.removeAttribute("checked");
}

To me this seems like the best place to also set the menu's caption to include
the site of the image's URL.

Adding this line shows the entire image URL,

blockImage.setAttribute("label", "Block images from " + this.imageURL);
          
But we only want to show the site part of the image's URL.  I suspect that there
is a "proper" way to get the site part from a URL (if not, there should be!). 
However, I did by by some string manipulation.

To apply my fix, replace the code snippet above with this:

        if (this.onImage) {
          var blockImage = document.getElementById("context-blockimage");
          if (this.isImageBlocked()) {
            blockImage.setAttribute("checked", "true");
          }
          else
            blockImage.removeAttribute("checked");
          
          //Bugzilla Bug 174687 START
          //Make the menu item's caption "Block images from www.site.com"
          //Note there is no accelerator key to worry about
          
          //Set a sensible default value in case the code below breaks:
          blockImage.setAttribute("label", "Block images from this site");
          
  	  var characterposition; //temp integer variable
  	  var imagesite;         //temp string variable
  	  
  	  //Remove leading http:// or ftp:// or similar
  	  //Have to escape / char as \/
  	  characterposition = this.imageURL.indexOf( "\/\/" );
  	  if ( characterposition > 0 ) {
  	  	//Add the length of // string plus one
	  	imagesite = this.imageURL.substr ( characterposition + 2 );
  	  } else {
  	  	imagesite = this.imageURL;
  	  };
  	  
  	  //Remove anything after a /
  	  //Have to escape / char as \/
  	  //indexOf returns -1 if string not present
  	  characterposition = imagesite.indexOf( "\/" );
	  if ( characterposition >= 0 ) {
	  	imagesite = imagesite.substr ( 0, characterposition );
	  };

  	  //imagesite should now be just www.site.com
  	  blockImage.setAttribute("label", "Block images from " + imagesite);
          
          //Bugzilla Bug 174687 END

        };

NOTE - This has some hard coded text strings, but I don't know enough about
Mozilla/Phoenix yet to try and do that properly.

Hopefully someone else can take this and polish it off properly!

----------------------------------------------------------------------

Hmm.  My version is actually pretty close to Eric's.  That makes me feel better :)

Peter

Comment 4

[Peter]

See also Bugzilla Bug 93390 which is a very similar entry for Mozilla, which 
also has a patch.

According to the comments, it also handles local files as special case, and 
limits length of host name to 25 characters.

Thanks to nicubunu on the MozillaZine Forums for pointing this out, as I failed 
to find it by myself.

http://www.mozillazine.org/forums/viewtopic.php?t=3923

Comment 5

[David Tenser [:djst]]

Reassigning to ben. I really want this implemented! :)

Comment 6

[eric lindvall]

what do i need to do to get this patch reviewed?

Comment 7

[Blake Ross]

Hey noririty, can you review this patch and check it in if it's good? (test with
long site urls, etc.)

Comment 8

[noririty]

Checked in.
But we still have bug 206595.

Comment 9

[Simon Paquet [:sipaq]]

Taking QA Contact as designated owner of Firebird-Menus. Sorry for bugspam.

Comment 10

[Simon Paquet [:sipaq]]

verified with 2003010 W2K build.