Issue 5593 - Use messaging in popup for prefs, whitelisting, and stats

popup.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
-
-const {require} = ext.backgroundPage.getWindow();
 
 let tab = null;

[Manish Jethani, 2017/09/21 08:02:39]

So now it's just a lightweight tab object and createPageObject is gone.

 
 function getPref(key, callback)

[Manish Jethani, 2017/09/21 08:06:25]

By the way, since we moved getPref and togglePref into popup.js, now we're
getting a linter error in stats.js for these functions as they are not declared
global.

Any thoughts on how to deal with this?

[Sebastian Noack, 2017/09/21 22:57:17]

Where accessing globals from other scripts cannot be avoided, it is possible to
annotate these at the top of the script like this:

  /* global getPref, toggelPref */

However, we should avoid cases like this as much as possible. And FWIW, I don't
see why the code for the popup UI is spread across 3 scripts in the first place.
So please consider merging these files, but this should probably happen in a
separate review.

[Manish Jethani, 2017/09/24 22:37:27]

Done.
OK, I'll look into this separately.

 {
   chrome.runtime.sendMessage({type: "prefs.get", key}, callback);
 }
 
 function togglePref(key, callback)
 {
   chrome.runtime.sendMessage({type: "prefs.toggle", key}, callback);
 }
 
 function isPageWhitelisted(callback)

[Manish Jethani, 2017/09/21 08:02:39]

I wonder if we should always use promises here.

 {
   chrome.runtime.sendMessage({type: "filters.isWhitelisted", tab}, callback);
 }
 
 function whenPageReady()
 {
-  let pageId = tab.id;

[Sebastian Noack, 2017/09/21 22:57:18]

Why do we have to chache tab.id into a local variable here?

[Manish Jethani, 2017/09/24 22:37:27]

It was not necessary, removed.

-
   return new Promise(resolve =>
   {
-    let handlePageReady = () =>

[Sebastian Noack, 2017/09/21 22:57:17]

I'm not sure if it's worth to move these two trivial lines into a function. I
feel that the code is slightly easier to follow with this code inlined.

[Manish Jethani, 2017/09/24 22:37:26]

Done.

-    {
-      ext.onMessage.removeListener(onMessage);
-      resolve();
-    };
-
-    let onMessage = (message, sender) =>
+    function onMessage(message, sender)
     {
       if (message.type == "composer.ready" && sender.page &&
-          sender.page.id == pageId)
+          sender.page.id == tab.id)
       {
-        handlePageReady();
+        ext.onMessage.removeListener(onMessage);
+        resolve();
       }
-    };
+    }
 
     ext.onMessage.addListener(onMessage);
 
-    chrome.runtime.sendMessage({type: "composer.isPageReady", pageId}, ready =>
+    chrome.runtime.sendMessage({
+      type: "composer.isPageReady",
+      pageId: tab.id
+    },
+    ready =>
     {
       if (ready)
-        handlePageReady();
+      {
+        ext.onMessage.removeListener(onMessage);
+        resolve();
+      }
     });
   });
 }
 
 function onLoad()
 {
   chrome.tabs.query({active: true, lastFocusedWindow: true}, tabs =>
   {
-    tab = tabs[0] && {id: tabs[0].id, url: tabs[0].url};

[Manish Jethani, 2017/09/21 08:02:37]

I don't like to hold on to an object passed in by the browser, as it may cause
memory leaks, especially when it represents a browser tab, so I'm creating a new
"tab" object with just the id and the url (both primitive types).

[Sebastian Noack, 2017/09/21 22:57:18]

Alternatively, you could use two global variables, `tabUrl` and `tabId`. As a
side effect, this would also simplify checks like below, instead for `tab &&
tab.url`, you could just check for `tabUrl` there then.

[Manish Jethani, 2017/09/24 22:37:27]

We could do that, but we're using the tab object in isPageWhitelisted and
toggleEnabled, so then we'd have to recreate that object locally.

[Sebastian Noack, 2017/09/25 17:50:52]

I see, I don't have a strong opinion then.

+    if (tabs.length > 0)
+      tab = {id: tabs[0].id, url: tabs[0].url};
 
-    let url = tab && tab.url && new URL(tab.url);
+    let urlProtocol = tab && tab.url && new URL(tab.url).protocol;
 
     // Mark page as 'local' to hide non-relevant elements
-    if (!url || (url.protocol != "http:" && url.protocol != "https:"))
+    if (urlProtocol != "http:" && urlProtocol != "https:")
     {
       document.body.classList.add("local");
       document.body.classList.remove("nohtml");
     }
     else
     {
       whenPageReady().then(() =>
       {
         document.body.classList.remove("nohtml");
       });
@@ skipping 26 matching lines @@
     "click", toggleEnabled, false
   );
   document.getElementById("clickhide").addEventListener(
     "click", activateClickHide, false
   );
   document.getElementById("clickhide-cancel").addEventListener(
     "click", cancelClickHide, false
   );
   document.getElementById("options").addEventListener("click", () =>
   {
-    ext.showOptions();
+    ext.showOptions(window.close);
   }, false);
 
   // Set up collapsing of menu items
   for (let collapser of document.getElementsByClassName("collapse"))
   {
     collapser.addEventListener("click", toggleCollapse, false);
     getPref(collapser.dataset.option, value =>
     {
       if (value)
       {
@@ skipping 10 matching lines @@
   let disabled = document.body.classList.toggle("disabled");
   chrome.runtime.sendMessage({
     type: disabled ? "filters.whitelist" : "filters.unwhitelist",
     tab
   });
 }
 
 function activateClickHide()
 {
   document.body.classList.add("clickhide-active");
   chrome.tabs.sendMessage(tab.id, {

[Manish Jethani, 2017/09/21 08:02:38]

I forgot to account for this earlier. It needs to use tabs.sendMessage directly.

     type: "composer.content.startPickingElement"
   });
 
   // Close the popup after a few seconds, so user doesn't have to
-  activateClickHide.timeout = window.setTimeout(ext.closePopup, 5000);
+  activateClickHide.timeout = window.setTimeout(window.close, 5000);
 }
 
 function cancelClickHide()
 {
   if (activateClickHide.timeout)
   {
     window.clearTimeout(activateClickHide.timeout);
     activateClickHide.timeout = null;
   }
   document.body.classList.remove("clickhide-active");
   chrome.tabs.sendMessage(tab.id, {type: "composer.content.finished"});
 }
 
 function toggleCollapse(event)
 {
   let collapser = event.currentTarget;
   let collapsible = document.getElementById(collapser.dataset.collapsible);
   collapsible.classList.toggle("collapsed");
   togglePref(collapser.dataset.option);
 }
 
 document.addEventListener("DOMContentLoaded", onLoad, false);