Issue 4864 - Start using ESLint for adblockpluschrome

composer.postload.js

Index: composer.postload.js
diff --git a/composer.postload.js b/composer.postload.js
index 6b87f0ba5e2a4cc61e1c9eb0e1c4a98099966a3b..72ff69028dca922a08d81cdc6600e1c390312cfb 100644
--- a/composer.postload.js
+++ b/composer.postload.js
@@ -15,6 +15,8 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* globals checkCollapse, elemhide, getURLsFromElement, typeMap  */

[Sebastian Noack, 2017/02/09 01:04:50]

As mentioned in the other review for the base ESLint configuration, I'm opposed
to add this boilerplate. But it points out an issue: global variables aren't
great. So instead making the situation worse by explicitly/redundantly
specifying these globals, we should get rid of them. Note that if we'd use
modules for the related logic reused here, the only global we'd need is
require() which is already known.

[kzar, 2017/02/20 10:27:30]

We're not making the situation worse by documenting the global variables, we're
just making it explicit which IMO is actually an improvement.

Either way I agree that the less global state the better, so I'd also like to
reduce that. As for using modules here, how can we do that since this is a
content script and our "module framework stuff" lives in lib/compat.js which is
only included in the background. Do you mean I should add lib/compat.js to the
content scripts too?

[Sebastian Noack, 2017/03/29 17:14:07]

JavaScript is a dynamic language, which comes with some advantages including
that you can just use stuff that exists on the global (or any) object at
runtime. When we now require to formally declare the intend to use any global
name, this restricts the dynamic of the language, and I get the impression you
would prefer much to write your code in Java instead. ;)

As I said before, the job of a linter is to make the code easier to maintain,
not to bloat it with additional boilerplate. So IMO, it actually makes things
worse. I would be much less concerned if metadata like this could be separated
from the code, which however, isn't possible with ESLint. But if (and only if)
we could get rid of our globals, none of this would matter, anymore.
We could just separate these 11 lines from lib/compat.js and have buildtools add
them when bundling the modules, so that the module bundle works without any
addional support code. Then we can have one bundle for the background page, and
one for the content side.

[kzar, 2017/03/30 07:20:39]

As discussed in IRC sounds good. I've opened
https://issues.adblockplus.org/ticket/5060 for the buildtools changes, and will
make the changes to adblockpluschrome as a separate patch set but still for this
issue #4864.

+
 "use strict";
 
 // The page ID for the popup filter selection dialog (top frame only).
@@ -43,8 +45,7 @@ let lastRightClickEventIsMostRecent = false;
 function getFiltersForElement(element, callback)
 {
   let src = element.getAttribute("src");
-  ext.backgroundPage.sendMessage(
-  {
+  ext.backgroundPage.sendMessage({
     type: "composer.getFilters",
     tagName: element.localName,
     id: element.id,
@@ -52,7 +53,7 @@ function getFiltersForElement(element, callback)
     style: element.getAttribute("style"),
     classes: Array.prototype.slice.call(element.classList),
     urls: getURLsFromElement(element),
-    mediatype: typeMap[element.localName],
+    mediatype: typeMap.get(element.localName),
     baseURL: document.location.href
   },
   response =>
@@ -71,8 +72,9 @@ function getBlockableElementOrAncestor(element, callback)
     // We can't handle non-HTML (like SVG) elements, as well as
     // <area> elements (see below). So fall back to the parent element.
     if (!(element instanceof HTMLElement) || element.localName == "area")
+    {
       element = element.parentElement;
-
+    }
     // If image maps are used mouse events occur for the <area> element.
     // But we have to block the image associated with the <map> element.
     else if (element.localName == "map")
@@ -388,21 +390,15 @@ function elementPicked(event)
     if (currentlyPickingElement)
       stopPickingElement();
 
-    ext.backgroundPage.sendMessage(
-    {
+    ext.backgroundPage.sendMessage({
       type: "composer.openDialog"
     },
     popupId =>
     {
-      ext.backgroundPage.sendMessage(
-      {
+      ext.backgroundPage.sendMessage({
         type: "forward",
         targetPageId: popupId,
-        payload:
-        {
-          type: "composer.dialog.init",
-          filters: filters
-        }
+        payload: {type: "composer.dialog.init", filters}
       });
 
       // Only the top frame keeps a record of the popup window's ID,
@@ -413,14 +409,9 @@ function elementPicked(event)
       }
       else
       {
-        ext.backgroundPage.sendMessage(
-        {
+        ext.backgroundPage.sendMessage({
           type: "forward",
-          payload:
-          {
-            type: "composer.content.dialogOpened",
-            popupId: popupId
-          }
+          payload: {type: "composer.content.dialogOpened", popupId}
         });
       }
     });
@@ -461,8 +452,7 @@ function deactivateBlockElement()
 
   if (blockelementPopupId != null)
   {
-    ext.backgroundPage.sendMessage(
-    {
+    ext.backgroundPage.sendMessage({
       type: "forward",
       targetPageId: blockelementPopupId,
       payload:
@@ -504,8 +494,7 @@ if (document instanceof HTMLDocument)
     lastRightClickEvent = event;
     lastRightClickEventIsMostRecent = true;
 
-    ext.backgroundPage.sendMessage(
-    {
+    ext.backgroundPage.sendMessage({
       type: "forward",
       payload:
       {
@@ -520,9 +509,11 @@ if (document instanceof HTMLDocument)
     {
       case "composer.content.getState":
         if (window == window.top)
+        {
           sendResponse({
             active: currentlyPickingElement || blockelementPopupId != null
           });
+        }
         break;
       case "composer.content.startPickingElement":
         if (window == window.top)
@@ -572,8 +563,7 @@ if (document instanceof HTMLDocument)
         // to be careful here. (This is not perfect, but best we can do.)
         if (window == window.top && blockelementPopupId == msg.popupId)
         {
-          ext.backgroundPage.sendMessage(
-          {
+          ext.backgroundPage.sendMessage({
             type: "forward",
             payload:
             {