Regional adblock #4489
Regional adblock #4489
Conversation
bbondy
force-pushed
the
regional-adblock
branch
from
0a04758
to
1dd8a6d
Compare
|
@bsclifton will review and I'll do changes in a follow up. |
| @@ -10,6 +10,18 @@ const ABPFilterParser = ABPFilterParserLib.ABPFilterParser | |||
| const FilterOptions = ABPFilterParserLib.FilterOptions | |||
| const DataFile = require('./dataFile') | |||
| const Filtering = require('./filtering') | |||
| const appConfig = require('../js/constants/appConfig') | |||
| const debounce = require('../js/lib/debounce.js') | |||
There was a problem hiding this comment.
doh that was from copy/paste, I'll get rid of all files at once when I fix this to avoid that
| @@ -31,6 +31,7 @@ const app = electron.app | |||
| const uuid = require('node-uuid') | |||
| const path = require('path') | |||
| const getOrigin = require('../js/state/siteUtil').getOrigin | |||
| const {adBlockResourceName} = require('./adBlock') | |||
There was a problem hiding this comment.
Small style question- our project has a mix of:
- destructuring assignments with and without spaces around the item(s) (see line 34)
- including using require and then referencing the method name (see line 33)
Is there a preferred syntax?
There was a problem hiding this comment.
Newer code mostly should use destructuring when possible and no space around { and }.
| @@ -571,7 +581,10 @@ module.exports.isResourceEnabled = (resourceName, url) => { | |||
| } | |||
|
|
|||
| if ((resourceName === appConfig.resourceNames.ADBLOCK || | |||
| resourceName === appConfig.resourceNames.TRACKING_PROTECTION)) { | |||
| appConfig[resourceName] && | |||
There was a problem hiding this comment.
It was there before, but has unnecessary double parens
if((condition === value)) { }
There was a problem hiding this comment.
Yep it was probably there because at one point they mattered and then after a bugfix they were left behind. Fixed as part of:
ecc67f2
| etag: string, // last downloaded data file etag | ||
| lastCheckVersion: string, // last checked data file version | ||
| lastCheckDate: number, // last checked data file date.getTime() | ||
| enabled: boolean, // Enable adblocking | ||
| enabled: boolean, // Enable the resoruce |
| /** | ||
| * Dispatches an event to the renderer process to register or deregister a datafile | ||
| * | ||
| * @param {rules} ABP filter syntax rule string |
There was a problem hiding this comment.
might be a copy/paste thing, but are these comments right? to register or deregister? (looks like it's updating existing rules that are in place)
|
LGTM 😄 great job w/ the tests ❤️ |
git rebase -ito squash commits (if needed).Test Plan: