Issue 3138 - Improve how context menu "block element" handles iframes

Issue 3138 - Improve how context menu "block element" handles iframes

[kzar, 2017-01-10 06:40:15]

Patch Set 1

[Sebastian Noack, 2017-01-11 18:15:11]

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
chrome/ext/background.js:52: if (frameId != undefined)
Please use typeof to check for undefined, like we do everywhere else.

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
chrome/ext/background.js:53: options.frameId = frameId;
This isn't supported in Microsoft Edge. Regardless, I'm not too happy with
extending ext.* but would rather prefer to finally target the browserext API.

https://codereview.adblockplus.org/29370947/diff/29370948/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29370947/diff/29370948/composer.postload.j...
composer.postload.js:441: if (event)
How is it possible that there is no event here?

[kzar, 2017-01-12 07:24:04]

Patch Set 2 : Addressed feedback

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
chrome/ext/background.js:52: if (frameId != undefined)
On 2017/01/11 18:15:11, Sebastian Noack wrote:
> Please use typeof to check for undefined, like we do everywhere else.

Whoops, Done.

https://codereview.adblockplus.org/29370947/diff/29370948/chrome/ext/backgrou...
chrome/ext/background.js:53: options.frameId = frameId;
On 2017/01/11 18:15:11, Sebastian Noack wrote:
> This isn't supported in Microsoft Edge.

Damn, looking at the docs it seems like you're right. Apparently browserext does
specify the frameId option but Edge didn't implement that yet. Without this I
don't see how we can work around the problem for Edge. (Well actually I do, we
could have a similar workaround as on Safari where each frame generates a random
number, sends that to the background and then the background includes that in
messages sent to frames. Seems like overkill here however...) For now I suppose
we could add a check in the content script to avoid using the frame element if
we don't think per frame messaging is working.

> Regardless, I'm not too 
> happy with extending ext.* but would rather prefer to finally 
> target the browserext API.

Well I'd prefer not to extend ext further too but for now I can't see how else
to do it.

Switching to browserext is a huge job which while I've made some progress
towards, is not even ready for review yet. Then we still need to remove ext
which is another huge job as far as I can see. 

If you'd rather mark this issue as being blocked by the migration then fair
enough, but it seems overkill IMO.

https://codereview.adblockplus.org/29370947/diff/29370948/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29370947/diff/29370948/composer.postload.j...
composer.postload.js:441: if (event)
On 2017/01/11 18:15:11, Sebastian Noack wrote:
> How is it possible that there is no event here?

If the composer.content.contextMenuClicked handler calls this function using the
new logic there is no right click event we can use here. (That's the whole
problem, our right click event handler got trashed so we don't get those
events.)

[kzar, 2017-08-09 15:29:24]

Patch Set 3 : Rebased

https://codereview.adblockplus.org/29370947/diff/29510606/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29370947/diff/29510606/composer.postload.j...
composer.postload.js:543: if (!target && window.frameElement && typeof chrome !=
"undefined")
Note: This check `typeof chrome != "undefined"` is wrong, we should be using the
info module to check if the platform is edge. Unfortunately we can't do that
until the content scripts start using the module system however.

[kzar, 2017-10-18 14:57:26]

Patch Set 4 : Rebased, used info.application, improved comment

https://codereview.adblockplus.org/29370947/diff/29510606/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29370947/diff/29510606/composer.postload.j...
composer.postload.js:543: if (!target && window.frameElement && typeof chrome !=
"undefined")
On 2017/08/09 15:29:24, kzar wrote:
> Note: This check `typeof chrome != "undefined"` is wrong, we should be using
the
> info module to check if the platform is edge. Unfortunately we can't do that
> until the content scripts start using the module system however.

Done.

[kzar, 2017-10-19 10:57:52]

Patch Set 5 : Use messaging instead of requiring the info module