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

chrome/ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 28 matching lines @@
       // In that case we get the url from top frame of the tab, recorded by
       // the onBeforeRequest handler.
       var frames = framesOfTabs[this.id];
       if (frames)
       {
         var frame = frames[0];
         if (frame)
           return frame.url;
       }
     },
-    sendMessage: function(message, responseCallback)
+    sendMessage: function(message, responseCallback, frameId)
     {
-      chrome.tabs.sendMessage(this.id, message, responseCallback);
+      let options = {};
+      if (frameId != undefined)

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

Please use typeof to check for undefined, like we do everywhere else.

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

Whoops, Done.

+        options.frameId = frameId;

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

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.

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

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.
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.

+      chrome.tabs.sendMessage(this.id, message, options, responseCallback);
     }
   };
 
   ext.getPage = function(id)
   {
     return new Page({id: parseInt(id, 10)});
   };
 
   function afterTabLoaded(callback)
   {
@@ skipping 259 matching lines @@
         if (!items)
           return;
 
         items.forEach(function(item)
         {
           chrome.contextMenus.create({
             title: item.title,
             contexts: item.contexts,
             onclick: function(info, tab)
             {
-              item.onclick(new Page(tab));
+              item.onclick(new Page(tab), info);
             }
           });
         });
       });
     });
   };
 
   var ContextMenus = function(page)
   {
     this._page = page;
@@ skipping 239 matching lines @@
   ext.windows = {
     create: function(createData, callback)
     {
       chrome.windows.create(createData, function(createdWindow)
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 })();