Issue 3860 - Move request blocking logic to a seperate module

lib/requestBlocker.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
@@ skipping 17 matching lines @@
 let {port} = require("messaging");
 let devtools = require("devtools");
 
 ext.webRequest.getIndistinguishableTypes().forEach(types =>
 {
   for (let i = 1; i < types.length; i++)
     RegExpFilter.typeMap[types[i]] = RegExpFilter.typeMap[types[0]];
 });
 
 function onBeforeRequestAsync(page, url, type, docDomain,
-                              thirdParty, key, specificOnly,
-                              filter)
+                              thirdParty, sitekey,
+                              specificOnly, filter)
 {
   if (filter)
     FilterNotifier.triggerListeners("filter.hitCount", filter, 0, 0, page);
 
   if (devtools)
     devtools.logRequest(
       page, url, type, docDomain,
-      thirdParty, key, specificOnly,
-      filter
+      thirdParty, sitekey,
+      specificOnly, filter
     );
 }
 
 ext.webRequest.onBeforeRequest.addListener((url, type, page, frame) =>
 {
   if (checkWhitelisted(page, frame))
     return true;
 
   let urlString = stringifyURL(url);
   let docDomain = extractHostFromFrame(frame);
   let thirdParty = isThirdParty(url, docDomain);
-  let key = getKey(page, frame);
+  let sitekey = getKey(page, frame);
 
   let specificOnly = !!checkWhitelisted(
     page, frame, RegExpFilter.typeMap.GENERICBLOCK
   );
 
   let filter = defaultMatcher.matchesAny(
     urlString, RegExpFilter.typeMap[type],
-    docDomain, thirdParty, key, specificOnly
+    docDomain, thirdParty, sitekey, specificOnly
   );
 
   setTimeout(onBeforeRequestAsync, 0, page, urlString,
                                       type, docDomain,
-                                      thirdParty, key,
+                                      thirdParty, sitekey,
                                       specificOnly, filter);
 
   return !(filter instanceof BlockingFilter);
 });
 
 port.on("filters.collapse", (message, sender) =>
 {
   if (checkWhitelisted(sender.page, sender.frame))
     return false;
 
   let typeMask = RegExpFilter.typeMap[message.mediatype];
   let documentHost = extractHostFromFrame(sender.frame);
   let sitekey = getKey(sender.page, sender.frame);

[kzar, 2016/03/23 15:36:00]

Nit: the name `sitekey` seems inconsistent with the name `key` used above in the
request listener. 

[Sebastian Noack, 2016/03/23 16:44:48]

Well, it was inconsistent before. But fine with me using sitekey (like in other
modules) everywhere. 

   let blocked = false;
 
   let specificOnly = checkWhitelisted(
     sender.page, sender.frame,
     RegExpFilter.typeMap.GENERICBLOCK
   );
 
   for (let url of message.urls)
   {
     let urlObj = new URL(url, message.baseURL);
     let filter = defaultMatcher.matchesAny(
       stringifyURL(urlObj),
       typeMask, documentHost,
       isThirdParty(urlObj, documentHost),
       sitekey, specificOnly
     );
 
-    if (!(filter instanceof BlockingFilter))

[kzar, 2016/03/23 15:35:59]

I think it would be clearer to check if the filter is an instance of
BlockingFilter, removing the need for the continue. Something like this

if (filter instanceof BlockingFilter)
{
  blocked = true;
  if (filter.collapse != null)
    return filter.collapse;
}

[Sebastian Noack, 2016/03/23 16:44:47]

I'm not sure whether I agree, that this is any easier to understand as it
requires another block level, but done.

-      continue;
-    if (filter.collapse != null)
-      return filter.collapse;
-
-    blocked = true;
+    if (filter instanceof BlockingFilter)
+    {
+      if (filter.collapse != null)
+        return filter.collapse;
+      blocked = true;
+    }
   }
 
   return blocked && Prefs.hidePlaceholders;
 });
 
 let ignoreFilterNotifications = false;
 FilterNotifier.addListener((action, arg) =>

[Sebastian Noack, 2016/03/23 14:02:06]

I hope you don't mind that I revisited the logic to detect relevant filter
changes, since it turned out that both the logic in webrequest.js as well in
whitelisting.js didn't consider some cases, and even worse it was inconsistent.
So I used the chance to fix that.

[kzar, 2016/03/23 15:35:59]

Acknowledged.

 {
-  // Handler behavior is already going to be changed. We do so asynchronously, to

[kzar, 2016/03/23 15:35:59]

Nit: Mind wrapping this long line?

[kzar, 2016/03/23 15:35:59]

I don't really understand this comment. Maybe something like this instead?

"Avoid triggering filters.behaviorChanged multiple times when multiple filter
changes happen at the same time."

[Sebastian Noack, 2016/03/23 16:44:47]

Done.

-  // not trigger multiple times if multiple filter changes occur simultaneously.
+  // Avoid triggering filters.behaviorChanged multiple times
+  // when multiple filter hanges happen at the same time.
   if (ignoreFilterNotifications)
     return;
 
   if (action != "load")
   {
     let parts = action.split(".");
     let [category, event] = parts;
     if (category == "subscription")
     {
       if (event != "added"  &&
@@ skipping 29 matching lines @@
   }
 
   ignoreFilterNotifications = true;
   setTimeout(() =>
   {
     ignoreFilterNotifications = false;
     ext.webRequest.handlerBehaviorChanged();
     FilterNotifier.triggerListeners("filter.behaviorChanged");
   });
 });