Issue 2426 - Open block.html as a popup window

background.js

Index: background.js
diff --git a/background.js b/background.js
index 355905b59296b91571fc4739253be3b1f6dd8be7..99b462f9a992351f7c225d40303723d928251646 100644
--- a/background.js
+++ b/background.js
@@ -119,7 +119,7 @@ var contextMenuItem = {
   contexts: ["image", "video", "audio"],
   onclick: function(page)
   {
-    page.sendMessage({type: "clickhide-new-filter"});
+    page.sendMessage({type: "blockelement-context-menu-clicked"});
   }
 };
 
@@ -283,6 +283,40 @@ 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);
@@ -402,15 +436,21 @@ ext.onMessage.addListener(function (msg, sender, sendResponse)
       );
       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;
   }
@@ -419,7 +459,7 @@ ext.onMessage.addListener(function (msg, sender, sendResponse)
 // 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);
 });