Issue 6402 - Split filter hit / request logging out into own API

lib/hitLogger.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 20 matching lines @@
 ];
 
 let eventEmitter = new EventEmitter();
 
 /**
  * @namespace
  * @static
  */
 let HitLogger = exports.HitLogger = {
   /**
    * Adds a listener for requests, filter hits etc related to the tab.

[Sebastian Noack, 2018/05/02 16:11:33]

Can we please document/specify that the calling code is responsible for removing
any listener themselves and that the listener is not automatically removed when
the tab is closed/navigates/etc?

[kzar, 2018/05/03 15:56:52]

Done.

    *
+   * Note: Calling code is responsible for removing the listener again,
+   *       it will not be automatically removed when the tab is closed.
+   *
    * @param {number} tabId
    * @param {function} listener
    */
   addListener: eventEmitter.on.bind(eventEmitter),
 
   /**
    * Removes a listener for the tab.
    *
    * @param {number} tabId
    * @param {function} listener
    */
   removeListener: eventEmitter.off.bind(eventEmitter),
 
   /**
    * Checks whether a tab is being inspected by anything.
    *
    * @param {number} tabId
    * @return {boolean}
    */
   hasListener(tabId)

[Sebastian Noack, 2018/05/02 16:11:33]

Does this method needs to be public (i.e. in the HitLogger object)? It seems it
is only used for some internal optimizations inside this module?

[kzar, 2018/05/03 15:56:52]

Yea, since it's also used in lib/cssInjection.js.

   {
     let listeners = eventEmitter._listeners.get(tabId);
     return listeners && listeners.length > 0;
   }
 };
 
 /**
  * Logs a request associated with a tab or multiple tabs.
  *
  * @param {number[]} tabIds
  *   The tabIds associated with the request
  * @param {Object} request
  *   The request to log
  * @param {string} request.url
  *   The URL of the request
  * @param {string} request.type
  *  The request type
  * @param {string} request.docDomain
- *  The IDN-decoded hostname of the document
+ *  The hostname of the document
  * @param {boolean} request.thirdParty
  *   Whether the origin of the request and document differs
  * @param {?string} request.sitekey
  *   The active sitekey if there is any
  * @param {?boolean} request.specificOnly
  *   Whether generic filters should be ignored
  * @param {?BlockingFilter} filter
  *  The matched filter or null if there is no match
  */
-exports.logRequest = function(tabIds, request, filter)

[Sebastian Noack, 2018/05/02 16:11:33]

Any particular reason why you pass in the request information in the internal
data structure used by the hitLogger? It seems to just add boilerplate to the
calling code and requires to create an additional object no matter what?

[kzar, 2018/05/03 15:56:52]

Well I did it this way since Manish suggested it[1]. FWIW I didn't mind too much
either way but I probably prefer it how it is now, passing in the request
Object.

[1] -
https://codereview.adblockplus.org/29705719/diff/29751602/lib/hitLogger.js#ne...

[Sebastian Noack, 2018/05/03 16:40:58]

Well, the main argument for creating the request object on demand, rather than
passing it in externally, is to avoid creating the object if there is no
listener registered (i.e. in the common case). Manish, what is your reasoning
here, for doing it the other way around?

[Manish Jethani, 2018/05/05 01:26:09]

No reasoning other than it makes the code easy to reason about. My counter
question would be, what is the reason to avoid creating the object? Note that
we're not checking whether there's a listener now because it's no longer needed.
If there's any performance benefit to avoid creating an object, I would be in
favor of it. We would also have to include the cost of checking whether there's
a listener in that calculation. Right now the code seems fairly straightforward.

[Sebastian Noack, 2018/05/05 12:56:23]

Fair enough. I'm still not entirely convinced that this change made the code
more readable (when I look at the calling code), but you made a point, that with
the added abstraction, creating the request object on demand will require
additional complexity, and the benefits of creating the object on demand might
be cancelled out. So LGTM if both of you prefer to leave it like that.

+exports.logRequest = (tabIds, request, filter) =>
 {
   for (let tabId of tabIds)
     eventEmitter.emit(tabId, request, filter);
 };
 
 /**
  * Logs active element hiding filters for a tab.
  *
  * @param {number}   tabId      The ID of the tab, the elements were hidden in
  * @param {string[]} selectors  The selectors of applied ElemHideFilters
  * @param {string[]} filters    The text of applied ElemHideEmulationFilters
- * @param {string}   docDomain  The IDN-decoded hostname of the document
+ * @param {string}   docDomain  The hostname of the document
  */
 function logHiddenElements(tabId, selectors, filters, docDomain)
 {
   if (HitLogger.hasListener(tabId))
   {
     for (let subscription of FilterStorage.subscriptions)
     {
       if (subscription.disabled)
         continue;
 
@@ skipping 15 matching lines @@
 }
 
 /**
  * Logs a whitelisting filter that disables (some kind of)
  * blocking for a particular document.
  *
  * @param {number}       tabId     The tabId the whitelisting is active for
  * @param {string}       url       The url of the whitelisted document
  * @param {number}       typeMask  The bit mask of whitelisting types checked
  *                                 for
- * @param {string}       docDomain The IDN-decoded hostname of the parent
- *                                 document
+ * @param {string}       docDomain The hostname of the parent document
  * @param {WhitelistFilter} filter The matched whitelisting filter
  */
-exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain,

[Sebastian Noack, 2018/05/02 16:11:33]

Nit: It seems in other modules we use arrow function syntax for exported
functions. As a side effect this might also avoid wrapping the line here.

[kzar, 2018/05/03 15:56:51]

Done.

-                                          filter)
+exports.logWhitelistedDocument = (tabId, url, typeMask, docDomain, filter) =>
 {
   if (HitLogger.hasListener(tabId))
   {
     for (let type of nonRequestTypes)
     {
       if (typeMask & filter.contentType & RegExpFilter.typeMap[type])
         eventEmitter.emit(tabId, {url, type, docDomain}, filter);
     }
   }
 };
 
 port.on("hitLogger.traceElemHide", (message, sender) =>
 {
   logHiddenElements(

[Sebastian Noack, 2018/05/02 16:11:33]

I wonder it would make more sense to just inline the logic here, since
logHiddenElements() is only called once and not exported.

[kzar, 2018/05/03 15:56:52]

I prefer it how it is, I think it's easier to reason with this way.

     sender.page.id, message.selectors, message.filters,
     extractHostFromFrame(sender.frame)
   );
 });