Issue #350 - iFrame block element improvements

350 - iFrame block element improvements

https://issues.adblockplus.org/ticket/350
https://github.com/kzar/adblockpluschrome/tree/350-iframe-block-element

If you would like to test this out here's a test page I made http://static.kzar.co.uk/iframe-test/

[kzar, 2015-01-16 20:32:23]

On 2015/01/16 15:11:49, kzar wrote:

As requested I have separated out the unrelated changes into separate reviews:

 - http://codereview.adblockplus.org/6636194259533824/
 - http://codereview.adblockplus.org/4916569079545856/

[Sebastian Noack, 2015-01-16 22:45:31]

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/back...
File background.js (right):

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/back...
background.js:569: case "clickHide-showDialog":
It seems those message types are unnecessary and redundant. There is already the
generic "forward" message type, which you can use instead.

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
include.postload.js:598: lastRightClickEventRandom = Math.random();
I don't really like the approach here to generate a random number for each frame
to distinguish them. A random number isn't supposed to be a unique identifier.

I don't think you need to distinguish frames in the first place, but can just
protect the variable by a flag:

  lastRightClickEvent = e;
  lastRightClickEventValid = true;
  ext.backgroundPage.sendMessage({type: "clickhide-forget-contextmenu"});

  ...

  case "clickhide-forget-contextmenu":
    if (lastRightClickEventValid)
      lastRightClickEventValid = false;
    else
      lastRightClickEvent = null;

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
include.postload.js:715: case "clickHide-showDialog":
For consistency with the message types implemented above, use dashes instead
camelcase here, and write "clickhide" lowercase as one word, same below.

[kzar, 2015-01-16 23:03:45]

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/back...
File background.js (right):

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/back...
background.js:569: case "clickHide-showDialog":
On 2015/01/16 22:45:31, Sebastian Noack wrote:
> It seems those message types are unnecessary and redundant. There is already
the
> generic "forward" message type, which you can use instead.

Done.

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
include.postload.js:598: lastRightClickEventRandom = Math.random();
On 2015/01/16 22:45:31, Sebastian Noack wrote:
> I don't really like the approach here to generate a random number for each
frame
> to distinguish them. A random number isn't supposed to be a unique identifier.
> 
> I don't think you need to distinguish frames in the first place, but can just
> protect the variable by a flag:
> 
>   lastRightClickEvent = e;
>   lastRightClickEventValid = true;
>   ext.backgroundPage.sendMessage({type: "clickhide-forget-contextmenu"});
> 
>   ...
> 
>   case "clickhide-forget-contextmenu":
>     if (lastRightClickEventValid)
>       lastRightClickEventValid = false;
>     else
>       lastRightClickEvent = null;

Done.

http://codereview.adblockplus.org/6264613016436736/diff/5741031244955648/incl...
include.postload.js:715: case "clickHide-showDialog":
On 2015/01/16 22:45:31, Sebastian Noack wrote:
> For consistency with the message types implemented above, use dashes instead
> camelcase here, and write "clickhide" lowercase as one word, same below.

Done.

[Sebastian Noack, 2015-01-17 08:18:30]

LGTM