Opened 16 months ago

Closed 14 months ago

Last modified 12 months ago

#6402 closed change (fixed)

Split out request / hit logging from devtools code into separate API

Reported by: kzar Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-3.2-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: wspee, greiner, saroyanm, sebastian, mapx, sergz Blocked By:
Blocking: #5093, #6386 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29705719/

Description (last modified by kzar)

Background

With issue #154 we added our own Adblock Plus pane to the developer tools which lists requests and filter hits for a page. It's useful for filter list authors to understand what's going on.

The issue reporter for the legacy Firefox extension provided similar information, we'd like to do the same for the WebExt extension #6386.

Since there are now two consumers for the information that we're logging for the developer tools pane it makes sense to split out the logging part into a separate API.

What to change

Create the module lib/hitLogger.js in adblockpluschrome with an API something like this:

exports.nonRequestTypes = [
  "DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE", "CSP"
];

/**
 * @namespace
 * @static
 */
exports.HitLogger = {
  /**
   * Adds a listener for requests, filter hits etc related to the tab.
   *
   * 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(tabId, listener)

  /**
   * Removes a listener for the tab.
   *
   * @param {number} tabId
   * @param {function} listener
   */
  removeListener(tabId, listener)

  /**
   * Checks whether a tab is being inspected by anything.
   *
   * @param {number} tabId
   * @return {boolean}
   */
  hasListener(tabId)
}

/**
 * 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 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)

/**
 * 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 hostname of the parent document
 * @param {WhitelistFilter} filter The matched whitelisting filter
 */
exports.logWhitelistedDocument = function(tabId, url, typeMask, docDomain, filter)

port.on("hitLogger.traceElemHide", (message, sender) =>
{
   ...
});

Listeners should be called back with a request and filter:

listener(request, filter).

Where a request might look something like:

{"pageId":123,"url":"https://example.com/example.jpg","type":"IMAGE","docDomain":"www.reddit.com","thirdParty":true,"sitekey":null,"specificOnly":false}

and a filter could be an element hiding, whitelisting or blocking filter - or null if there is no filter associate e.g. we're logging a request rather than a filter hit.

Hints for testers

  • Ensure that the Adblock Plus developer tools pane still works as before. Test as much functionality as possible, for example blocking a request and whitelisting it again from the developer tools pane.
  • Ensure element hiding still workings on Microsoft Edge.

Change History (31)

comment:1 Changed 16 months ago by kzar

Please confirm this looks OK everyone and I'll mark it ready. In the mean time I'm starting an implementation.

comment:2 Changed 16 months ago by kzar

  • Component changed from Unknown to Platform

comment:3 Changed 16 months ago by kzar

  • Description modified (diff)

comment:4 Changed 16 months ago by kzar

  • Description modified (diff)

comment:5 follow-up: Changed 16 months ago by greiner

Looking good. Sounds like the only thing that's not part of this is the endpoint with which we'd connect to the background page. If so, that's great because it'd allow us to simply extend message responder.

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.
Being able to differentiate between those would also be helpful for #5093 BTW.

comment:6 in reply to: ↑ 5 Changed 16 months ago by kzar

Replying to greiner:

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.
Being able to differentiate between those would also be helpful for #5093 BTW.

Honestly I'm not sure yet, I think once I have a proof of concept working I'll have a better idea.

comment:7 follow-up: Changed 16 months ago by mapx

So, still using devtools ? If you want the normal users use such feature you could consider having a logger in a separate tab (and have a keyboard shortcut to open it). You already have the uBo example which log all the requests even those "behind the scene", you can implement all the search features you want and finally have the same logger for all browsers.

comment:8 Changed 16 months ago by mapx

  • Cc mapx added

comment:9 in reply to: ↑ 7 Changed 16 months ago by kzar

Replying to mapx:

So, still using devtools ? If you want the normal users use such feature you could consider having a logger in a separate tab (and have a keyboard shortcut to open it). You already have the uBo example which log all the requests even those "behind the scene", you can implement all the search features you want and finally have the same logger for all browsers.

That is something else mapx, but FWIW the work in this issue might potentially help with it.

comment:10 Changed 16 months ago by kzar

  • Description modified (diff)

comment:11 Changed 16 months ago by greiner

@mapx I've created #6404 so that we can gather what's required for such a page. Hopefully, we should be able to extend the existing DevTools pane to also work as a standalone page to avoid any unnecessary duplication.

Depending on how that page turns out we may then also want to investigate the added value the integration into the DevTools window provides and whether such a standalone page would render it redundant.

comment:12 Changed 16 months ago by kzar

  • Description modified (diff)

comment:13 Changed 16 months ago by kzar

  • Description modified (diff)

comment:14 Changed 16 months ago by kzar

  • Description modified (diff)

comment:15 Changed 16 months ago by kzar

  • Description modified (diff)
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:16 Changed 16 months ago by kzar

  • Description modified (diff)

comment:17 Changed 16 months ago by kzar

  • Description modified (diff)

comment:18 Changed 16 months ago by kzar

  • Description modified (diff)

Would we also get requests from background tabs or only the ones the extension is able to associate with the pageId that we listen to? It'd probably be helpful to listen to such requests independently because I don't know whether we want to include those in issue reports whereas it makes sense to show them in the the DevTools pane.

So you'll get both, but the pageId is now given for requests so it should simply be a matter of checking if that's -1 or not.

Last edited 16 months ago by kzar (previous) (diff)

comment:19 Changed 16 months ago by sergz

  • Cc sergz added

comment:20 Changed 16 months ago by agiammarchi

  • Blocking 5093 added

comment:21 follow-up: Changed 15 months ago by kzar

  • Ready unset

The API is going to change quite a bit so the issue description is no longer accurate. I'll update that when I'm ready, but in the mean time watch the discussion in the codereview if you're interested.

comment:22 Changed 15 months ago by kzar

  • Description modified (diff)

So you'll get both, but ​the pageId is now given for requests so it should simply be a matter of checking if that's -1 or not.

Note: This is no longer true, Sebastian's made some changes recently (#6543) to figure out the associated tabs for these requests.

comment:23 Changed 15 months ago by kzar

  • Description modified (diff)

comment:24 Changed 15 months ago by kzar

  • Description modified (diff)
  • Ready set

comment:25 Changed 15 months ago by kzar

  • Description modified (diff)

comment:26 in reply to: ↑ 21 ; follow-up: Changed 15 months ago by kzar

Replying to kzar:

The API is going to change quite a bit so the issue description is no longer accurate...

OK I've updated it again now, should be a bit more accurate. Does it look OK to you Thomas?

comment:27 in reply to: ↑ 26 Changed 15 months ago by greiner

Replying to kzar:

OK I've updated it again now, should be a bit more accurate. Does it look OK to you Thomas?

Yep. I was only worried about background tabs previously since those could've been relevant for an issue report but given that we can now associate each of those with the given tab and send all relevant requests to the issue reporter, my concern is no longer valid.

comment:28 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6402 - Split filter hit / request logging out into own API

comment:29 Changed 14 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:30 Changed 14 months ago by kzar

  • Description modified (diff)

comment:31 Changed 12 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. DevTools and Issue Reporter still work as expected.

ABP 3.1.0.2069
Chrome 67 / 64 / 49 / Windows 7
Firefox 60 / 55 / 51 / Windows 7
Opera 52 / 45 / 38 / Windows 7

ABP 0.9.11.2052
Edge 42.17134.1.0 / Windows 10

Note: See TracTickets for help on using tickets.