Issue 4864 - Start using ESLint for adblockpluschrome

composer.postload.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
  * 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/>.
  */
 
+/* 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).
 let blockelementPopupId = null;
 
 // Element picking state (top frame only).
 let currentlyPickingElement = false;
 let lastMouseOverEvent = null;
 
 // During element picking this is the currently highlighted element. When
 // element has been picked this is the element that is due to be blocked.
 let currentElement = null;
 
 // Highlighting state, used by the top frame during element picking and all
 // frames when the chosen element is highlighted red.
 let highlightedElementsSelector = null;
 let highlightedElementsInterval = null;
 
 // Last right click state stored for element blocking via the context menu.
 let lastRightClickEvent = null;
 let lastRightClickEventIsMostRecent = false;
 
 
 /* Utilities */
 
 function getFiltersForElement(element, callback)
 {
   let src = element.getAttribute("src");
-  ext.backgroundPage.sendMessage(
-  {
+  ext.backgroundPage.sendMessage({
     type: "composer.getFilters",
     tagName: element.localName,
     id: element.id,
     src: src && src.length <= 1000 ? src : null,
     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 =>
   {
     callback(response.filters, response.selectors);
   });
 }
 
 function getBlockableElementOrAncestor(element, callback)
 {
   // We assume that the user doesn't want to block the whole page.
   // So we never consider the <html> or <body> element.
   while (element && element != document.documentElement &&
          element != document.body)
   {
     // 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")
     {
       let images = document.querySelectorAll("img[usemap]");
       let image = null;
 
       for (let currentImage of images)
       {
         let usemap = currentImage.getAttribute("usemap");
@@ skipping 295 matching lines @@
 {
   if (!currentElement)
     return;
 
   let element = currentElement.prisoner || currentElement;
   getFiltersForElement(element, (filters, selectors) =>
   {
     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,
       // so if this isn't the top frame we need to pass the ID on.
       if (window == window.top)
       {
         blockelementPopupId = popupId;
       }
       else
       {
-        ext.backgroundPage.sendMessage(
-        {
+        ext.backgroundPage.sendMessage({
           type: "forward",
-          payload:
-          {
-            type: "composer.content.dialogOpened",
-            popupId: popupId
-          }
+          payload: {type: "composer.content.dialogOpened", popupId}
         });
       }
     });
 
     if (selectors.length > 0)
       highlightElements(selectors.join(","));
 
     highlightElement(currentElement, "#fd1708", "#f6a1b5");
   });
 
@@ skipping 20 matching lines @@
 /* Core logic */
 
 // We're done with the block element feature for now, tidy everything up.
 function deactivateBlockElement()
 {
   if (currentlyPickingElement)
     stopPickingElement();
 
   if (blockelementPopupId != null)
   {
-    ext.backgroundPage.sendMessage(
-    {
+    ext.backgroundPage.sendMessage({
       type: "forward",
       targetPageId: blockelementPopupId,
       payload:
       {
         type: "composer.dialog.close"
       }
     });
 
     blockelementPopupId = null;
   }
@@ skipping 21 matching lines @@
   // this because the contextMenu API only provides a URL, not the actual DOM
   // element.
   //   We also need to make sure that the previous right click event,
   // if there is one, is removed. We don't know which frame it is in so we must
   // send a message to the other frames to clear their old right click events.
   document.addEventListener("contextmenu", event =>
   {
     lastRightClickEvent = event;
     lastRightClickEventIsMostRecent = true;
 
-    ext.backgroundPage.sendMessage(
-    {
+    ext.backgroundPage.sendMessage({
       type: "forward",
       payload:
       {
         type: "composer.content.clearPreviousRightClickEvent"
       }
     });
   }, true);
 
   ext.onMessage.addListener((msg, sender, sendResponse) =>
   {
     switch (msg.type)
     {
       case "composer.content.getState":
         if (window == window.top)
+        {
           sendResponse({
             active: currentlyPickingElement || blockelementPopupId != null
           });
+        }
         break;
       case "composer.content.startPickingElement":
         if (window == window.top)
           startPickingElement();
         break;
       case "composer.content.contextMenuClicked":
         let event = lastRightClickEvent;
         deactivateBlockElement();
         if (event)
         {
@@ skipping 29 matching lines @@
         break;
       case "composer.content.dialogOpened":
         if (window == window.top)
           blockelementPopupId = msg.popupId;
         break;
       case "composer.content.dialogClosed":
         // The onRemoved hook for the popup can create a race condition, so we
         // 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:
             {
               type: "composer.content.finished"
             }
           });
         }
         break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "composer.ready"});
 }