Skip to content

/ ghostery-extension

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding return after rejects in a few places. Implementing Ghostery side of GH-885. Fix for GH-871. #10

Merged
merged 10 commits into from Mar 23, 2018
+64 −41
Merged

Adding return after rejects in a few places. Implementing Ghostery side of GH-885. Fix for GH-871. #10

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
54b5286
Adding return after rejects in a few places.
Mar 20, 2018
6078357
Implementing flag for site-specific unblocks.
Mar 22, 2018
c81bd41
Using abbreviations for seconds, removing unnecessary comment.
Mar 22, 2018
0745205
Requested changes.
Mar 23, 2018
61a1e5c
Implementing suggested changes.
Mar 23, 2018
40c1d48
Update policy block reason name.
jsignanini Mar 23, 2018
6bf01a9
Update block reason names. Call allowedOnce check only when needed.
jsignanini Mar 23, 2018
6ee32ae
Update JSDocs and add missing block reason.
jsignanini Mar 23, 2018
b523b46
Update JSDocs and update block reason c2p names.
jsignanini Mar 23, 2018
73cc0eb
Fix typo.
jsignanini Mar 23, 2018
File filter
Filter file types
Clear filters
Jump to
Jump to file
Failed to load files.

Always

Just for now

Prev Next
Loading status checks…
Implementing flag for site-specific unblocks.
  • Loading branch information
Serge Zarembsky
Serge Zarembsky committed Mar 22, 2018
commit 60783575e889594f690ee8b6f32302df95fbb0a7
20 src/classes/EventHandlers.js
View file
Open in desktop
@@ -378,15 +378,21 @@ class EventHandlers {
let tab_host;
let fromRedirect;
let block;
let ss_unblock;
if (bug_id) {
app_id = bugDb.db.bugs[bug_id].aid;
cat_id = bugDb.db.apps[app_id].cat;
incognito = tabInfo.getTabInfo(tab_id, 'incognito');
tab_host = tabInfo.getTabInfo(tab_id, 'host');
fromRedirect = globals.REDIRECT_MAP.has(request_id);
block = this._checkBlocking(app_id, cat_id, tab_id, tab_host, page_url, request_id);
const retVal = this._checkBlocking(app_id, cat_id, tab_id, tab_host, page_url, request_id);

This comment has been minimized.

Sign in to view
@christophertino

christophertino Mar 23, 2018
Member

const {block, ss_unblock} = this._checkBlocking(app_id, cat_id, tab_id, tab_host, page_url, request_id);

This comment has been minimized.

Sign in to view
@zarembsky

zarembsky Mar 23, 2018 •
edited
Author Contributor

The problem with the code const {block, ss_unblock} = is that block should be visible out of scope if(bug_id) { }. We can get away with extra assignment, like:
const {block1, ss_unblock} = ...
block = block1;
block = retVal.block;
/* eslint prefer-destructuring: 0 */
if (retVal.ss_unblock) {
// The way to pass this flag to Cliqz handlers
details.ghosteryWhitelisted = true;
}
}

// Latency initialization needs to be synchronous to avoid race condition with onCompleted, etc.
// TODO can URLs repeat within a redirect chain? what are the cases of repeating URLs (trackers only, ...)?
if (block === false) {
@@ -400,6 +406,8 @@ class EventHandlers {
page_url,
incognito
};

// Find out if block false because of site_specific_unblock
}

// process the tracker asynchronously
@@ -767,10 +775,10 @@ class EventHandlers {
* @param {number} app_id tracker id
* @param {number} cat_id tracker category id
* @param {number} tab_id tab id
* @param {string} tab_host tab url host
* @param {string} page_url full tab url
* @param {string} tab_host tab url host
* @param {string} page_url full tab url
* @param {number} request_id request id
* @return {boolean}
* @return {Object} {block, ss_unblock}
*/
_checkBlocking(app_id, cat_id, tab_id, tab_host, page_url, request_id) {

This comment has been minimized.

Sign in to view
@christophertino

christophertino Mar 23, 2018
Member

this should have an object return, correct? looks to just be returning block at the moment, instead of return {block:true, ss_unblock:false} etc

See line 793

This comment has been minimized.

Sign in to view
@zarembsky

zarembsky Mar 23, 2018
Author Contributor

block IS object, as it is returned by shouldBlock, which returns object.
const fromRedirect = globals.REDIRECT_MAP.has(request_id);
@@ -779,7 +787,7 @@ class EventHandlers {
// If we let page-level c2p trackers through, we don't want to block it
// along with all subsequent top-level redirects.
if (fromRedirect && globals.LET_REDIRECTS_THROUGH) {
block = false;
block = { block: false };
} else {
block = this.policy.shouldBlock(app_id, cat_id, tab_id, tab_host, page_url);
}
29 src/classes/Policy.js
View file
Open in desktop
@@ -100,35 +100,42 @@ class Policy {
* @param {number} tab_id tab id
* @param {string} tab_host tab url host
* @param {string} tab_url tab url
* @return {boolean}
* @return {Object} {block, ss_unblock}
*/
shouldBlock(app_id, cat_id, tab_id, tab_host, tab_url) {
if (globals.SESSION.paused_blocking) {
return false;
return { block: false };

This comment has been minimized.

Sign in to view
@christophertino

christophertino Mar 23, 2018
Member

we should return {block, ss_unblock} in all cases to prevent undefined errors

This comment has been minimized.

Sign in to view
@zarembsky

zarembsky Mar 23, 2018
Author Contributor

You mean if we ever call shouldBlock elsewhere (but we don't)

This comment has been minimized.

Sign in to view
@zarembsky

zarembsky Mar 23, 2018 •
edited
Author Contributor

The reason I did not go with {{block, ss_unblock} was that it is performance-critical, and most of the time ss_unblock is not needed.
I'll make the requested changes though.
}

const ss_unblock = conf.toggle_individual_trackers && conf.site_specific_unblocks.hasOwnProperty(tab_host) && conf.site_specific_unblocks[tab_host].includes(+app_id);

if (ss_unblock && !this.blacklisted(tab_url) && !this.whitelisted(tab_url)) {
return { block: false, ss_unblock: true };
}

if (conf.selected_app_ids.hasOwnProperty(app_id)) {
if (conf.toggle_individual_trackers && conf.site_specific_unblocks.hasOwnProperty(tab_host) && conf.site_specific_unblocks[tab_host].includes(+app_id)) {
if (ss_unblock) {
if (this.blacklisted(tab_url)) {
return !c2pDb.allowedOnce(tab_id, app_id);
return { block: !c2pDb.allowedOnce(tab_id, app_id) };
}
return false;
return { block: false };
}
if (this.whitelisted(tab_url)) {
return false;
return { block: false };
}
return !c2pDb.allowedOnce(tab_id, app_id);
return { block: !c2pDb.allowedOnce(tab_id, app_id) };
}
// We get here when app_id is not selected for blocking
if (conf.toggle_individual_trackers && conf.site_specific_blocks.hasOwnProperty(tab_host) && conf.site_specific_blocks[tab_host].includes(+app_id)) {
if (this.whitelisted(tab_url)) {
return false;
return { block: false };
}
return !c2pDb.allowedOnce(tab_id, app_id);
return { block: !c2pDb.allowedOnce(tab_id, app_id) };
}
if (this.blacklisted(tab_url)) {
return !c2pDb.allowedOnce(tab_id, app_id);
return { block: !c2pDb.allowedOnce(tab_id, app_id) };
}
return false;
return { block: false };
}
}

ProTip! Use n and p to navigate between commits in a pull request.