composer.postload.js - Issue 29374674: Issue 4864 - Start using ESLint for adblockpluschrome

Delta Between Two Patch Sets: composer.postload.js

Issue 29374674: Issue 4864 - Start using ESLint for adblockpluschrome (Closed)

Left Patch Set: Use var for ext declarations again Created Feb. 8, 2017, 9:02 a.m.

Right Patch Set: Use .includes again Created March 31, 2017, 8:37 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 /*	1 /*

2 * This file is part of Adblock Plus <https://adblockplus.org/>,	2 * This file is part of Adblock Plus <https://adblockplus.org/>,

3 * Copyright (C) 2006-2016 Eyeo GmbH	3 * Copyright (C) 2006-2017 eyeo GmbH

4 *	4 *

5 * Adblock Plus is free software: you can redistribute it and/or modify	5 * Adblock Plus is free software: you can redistribute it and/or modify

6 * it under the terms of the GNU General Public License version 3 as	6 * it under the terms of the GNU General Public License version 3 as

7 * published by the Free Software Foundation.	7 * published by the Free Software Foundation.

8 *	8 *

9 * Adblock Plus is distributed in the hope that it will be useful,	9 * Adblock Plus is distributed in the hope that it will be useful,

10 * but WITHOUT ANY WARRANTY; without even the implied warranty of	10 * but WITHOUT ANY WARRANTY; without even the implied warranty of

11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

12 * GNU General Public License for more details.	12 * GNU General Public License for more details.

13 *	13 *

14 * You should have received a copy of the GNU General Public License	14 * You should have received a copy of the GNU General Public License

15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

16 */	16 */

17	17

18 /* globals checkCollapse, elemhide, getURLsFromElement, typeMap */	18 /* globals checkCollapse, elemhide, getURLsFromElement, typeMap */
Sebastian Noack 2017/02/09 01:04:50 As mentioned in the other review for the base ESLi 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 documentin Show quoted text On 2017/02/09 01:04:50, Sebastian Noack wrote: > 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. 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 Show quoted text On 2017/02/20 10:27:30, kzar wrote: > We're not making the situation worse by documenting the global variables, we're > just making it explicit which IMO is actually an improvement. 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. Show quoted text > 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? 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 Show quoted text On 2017/03/29 17:14:07, Sebastian Noack wrote: > 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. 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.
19	19

20 "use strict";	20 "use strict";

21	21

22 // The page ID for the popup filter selection dialog (top frame only).	22 // The page ID for the popup filter selection dialog (top frame only).

23 let blockelementPopupId = null;	23 let blockelementPopupId = null;

24	24

25 // Element picking state (top frame only).	25 // Element picking state (top frame only).

26 let currentlyPickingElement = false;	26 let currentlyPickingElement = false;

27 let lastMouseOverEvent = null;	27 let lastMouseOverEvent = null;

28	28

(...skipping 36 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
65 function getBlockableElementOrAncestor(element, callback)	65 function getBlockableElementOrAncestor(element, callback)

66 {	66 {

67 // We assume that the user doesn't want to block the whole page.	67 // We assume that the user doesn't want to block the whole page.

68 // So we never consider the <html> or <body> element.	68 // So we never consider the <html> or <body> element.

69 while (element && element != document.documentElement &&	69 while (element && element != document.documentElement &&

70 element != document.body)	70 element != document.body)

71 {	71 {

72 // We can't handle non-HTML (like SVG) elements, as well as	72 // We can't handle non-HTML (like SVG) elements, as well as

73 // <area> elements (see below). So fall back to the parent element.	73 // <area> elements (see below). So fall back to the parent element.

74 if (!(element instanceof HTMLElement) \|\| element.localName == "area")	74 if (!(element instanceof HTMLElement) \|\| element.localName == "area")

75 {

76 element = element.parentElement;	75 element = element.parentElement;

77 }	76

78 // If image maps are used mouse events occur for the <area> element.	77 // If image maps are used mouse events occur for the <area> element.

79 // But we have to block the image associated with the <map> element.	78 // But we have to block the image associated with the <map> element.

80 else if (element.localName == "map")	79 else if (element.localName == "map")

81 {	80 {

82 let images = document.querySelectorAll("img[usemap]");	81 let images = document.querySelectorAll("img[usemap]");

83 let image = null;	82 let image = null;

84	83

85 for (let currentImage of images)	84 for (let currentImage of images)

86 {	85 {

87 let usemap = currentImage.getAttribute("usemap");	86 let usemap = currentImage.getAttribute("usemap");

(...skipping 483 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
571 }	570 }

572 });	571 });

573 }	572 }

574 break;	573 break;

575 }	574 }

576 });	575 });

577	576

578 if (window == window.top)	577 if (window == window.top)

579 ext.backgroundPage.sendMessage({type: "composer.ready"});	578 ext.backgroundPage.sendMessage({type: "composer.ready"});

580 }	579 }

LEFT	RIGHT