Issue 2426 - Open block.html as a popup window

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 101 matching lines @@
 // See http://crbug.com/68705.
 var noStyleRulesHosts = ["mail.google.com", "mail.yahoo.com", "www.google.com"];
 
 var htmlPages = new ext.PageMap();
 
 var contextMenuItem = {
   title: ext.i18n.getMessage("block_element"),
   contexts: ["image", "video", "audio"],
   onclick: function(page)
   {
-    page.sendMessage({type: "clickhide-new-filter"});
+    page.sendMessage({type: "blockelement-context-menu-clicked"});
   }
 };
 
 // Adds or removes browser action icon according to options.
 function refreshIconAndContextMenu(page)
 {
   var whitelisted = !!checkWhitelisted(page);
   updateIcon(page, whitelisted);
 
   // show or hide the context menu entry dependent on whether
@@ skipping 143 matching lines @@
     }
   }
 
   return {filters: filters, exceptions: exceptions};
 }
 
 ext.onMessage.addListener(function (msg, sender, sendResponse)
 {
   switch (msg.type)
   {
+    case "blockelement-open-popup":
+      var url = ext.getURL("block.html") + "?targetPageId=" + sender.page._id +

[Sebastian Noack, 2016/02/10 12:26:56]

Alternatively, you could send this data to the popup from the callback below,
similar to how we do it with the options page when opened to add a specific
subscription. I'm not entirely sure what's better, but at least you wouldn't
need to manually serialize/deserialize the data when using messaging.

[kzar, 2016/02/10 14:20:16]

I think this would work for Chrome but not Safari. In Safari the callback could
be performed before the new page has finished loading, so the message might be
sent too early. (I agree that sending a message would be cleaner.)

The other option I tried was to have the popup send a message to request the
filters, but that led to even more mess than just passing the filters through in
the URL. Worse I still had to pass the page ID in the URL anyway, so it just
meant we did things in both ways.

[Sebastian Noack, 2016/02/10 14:43:24]

As I said, we already do so for the options page. So apparently it works on
Safari.

[Sebastian Noack, 2016/02/12 17:00:00]

So what's about that comment?

[kzar, 2016/02/13 14:18:12]

Well I tried it and it didn't work for me.

Looking at the code to open the options page it looks like we do something else,
like ensuring the tab is active, before performing the callback that sends the
message. Maybe that helped prevent the race condition from troubling us?

I could wait 100ms before performing the callback if you like, seems like a hack
though.

[Sebastian Noack, 2016/02/15 14:50:17]

Well, when opening the options page, we look for an open option page an activate
it if there is one. But otherwise we simply delegate to ext.pages.open().

So if it doesn't work here, it only works coincidentally for the option page, if
it does. And whatever solution we come up with, we should consistently apply,
for the options page too.

I mildly lend towards just waiting until the page is complete on Safari as well,
before calling the callback passed to ext.pages.open.

+                  "&filters=" + encodeURIComponent(JSON.stringify(msg.filters));
+      ext.windows.create({
+        url: url,
+        left: 50,
+        top: 50,
+        width: 420,
+        height: 200,
+        focused: true,
+        type: "popup"
+      },
+      function (popupPage) {
+        var popupPageId = popupPage._id;
+        function onRemoved(removedPageId)
+        {
+          if (popupPageId == removedPageId)
+          {
+            sender.page.sendMessage({
+              type: "blockelement-popup-closed",
+              popupId: popupPageId
+            });
+            ext.pages.onRemoved.removeListener(onRemoved);
+          }
+        }
+        ext.pages.onRemoved.addListener(onRemoved);
+
+        sender.page.sendMessage({
+          type: "blockelement-popup-opened",
+          popupId: popupPageId
+        });
+      });
+      return true;

[Sebastian Noack, 2016/02/10 12:26:56]

By returning true but never calling sendResponse(), the underlying port will be
kept open forever introducing memory leaks.

[kzar, 2016/02/10 14:20:16]

Done.

+      break;
     case "get-selectors":
       var selectors = [];
       var trace = devtools && devtools.hasPanel(sender.page);
 
       if (!checkWhitelisted(sender.page, sender.frame,
                             RegExpFilter.typeMap.DOCUMENT |
                             RegExpFilter.typeMap.ELEMHIDE))
       {
         var noStyleRules = false;
         var specificOnly = checkWhitelisted(sender.page, sender.frame,
@@ skipping 99 matching lines @@
         frame: sender.frame
       }));
       break;
     case "trace-elemhide":
       devtools.logHiddenElements(
         sender.page, msg.selectors,
         extractHostFromFrame(sender.frame)
       );
       break;
     case "forward":
-      if (sender.page)
+      var targetPage;
+      if (msg.targetPageId)
+        targetPage = ext._getPage(msg.targetPageId);

[Sebastian Noack, 2016/02/10 12:26:56]

I guess it's time, to remove the prefix from ext._getPage() and Page._id.
Oterwise this code looks a tad hacky, which it in fact is if we still consider
these APIs private.

[kzar, 2016/02/10 14:20:16]

Done.

+      else
+        targetPage = sender.page;
+
+      if (targetPage)
       {
         if (msg.expectsResponse)
         {
-          sender.page.sendMessage(msg.payload, sendResponse);
+          targetPage.sendMessage(msg.payload, sendResponse);
           return true;
         }
 
-        sender.page.sendMessage(msg.payload);
+        targetPage.sendMessage(msg.payload);
       }
       break;
   }
 });
 
 // update icon when page changes location
 ext.pages.onLoading.addListener(function(page)
 {
-  page.sendMessage({type: "clickhide-deactivate"});
+  page.sendMessage({type: "blockelement-finished"});
   refreshIconAndContextMenu(page);
   showNextNotificationForUrl(page.url);
 });