Issue 6843 - Log snippet filter hits

lib/contentFiltering.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 195 matching lines @@
         return;
 
       throw error;
     });
   }
   catch (error)
   {
     // See the comment in the catch block associated with the call to
     // tabs.insertCSS for why we catch any error here and simply
     // return a rejected promise.
     return Promise.reject(error);

[Sebastian Noack, 2018/08/27 17:37:34]

It appears I'm missing something here. But what is the difference
between "p" and "p.catch(e => Promise.reject(e))"?

[hub, 2018/08/27 19:45:24]

Promise.catch(f) with call Promise.then(undefined, f) under the hood (this is
the MDN documentation)

However, here, we are in a plain exception catch(), so we don't have a promise.

[Manish Jethani, 2018/08/27 20:07:54]

If the call to `tabs.executeScript` throws, the function won't return a promise,
it'll return undefined; we need to return a "thenable" here (and it seems
appropriate to return a rejected promise here as it makes it consistent between
Firefox and Chrome, see comment in `addStyleSheet` for the background).

[Sebastian Noack, 2018/08/27 20:15:11]

Acknowledged. I misread the code and thought we are in catch() call of a
promise, not catch-block here.

   }
 }
 
 port.on("content.applyFilters", (message, sender) =>
 {
   let selectors = [];
   let emulatedPatterns = [];
   let trace = HitLogger.hasListener(sender.page.id);
   let inline = !userStyleSheetsSupported;
 
   let {elemhide, snippets} = message.filterTypes ||
                              {elemhide: true, snippets: true};
 
   if (!checkWhitelisted(sender.page, sender.frame, null,
                         RegExpFilter.typeMap.DOCUMENT))
   {
     let docDomain = extractHostFromFrame(sender.frame);
 
     if (snippets)
     {
       for (let filter of Snippets.getFiltersForDomain(docDomain))
       {
-        executeScript(filter.script, sender.page.id, sender.frame.id)
-        .then(() =>

[Sebastian Noack, 2018/08/27 17:37:34]

Nit: Wrapping here appears unnecessary.

[hub, 2018/08/27 19:45:24]

without wrapping the `>` reach column 80 so I felt it was better this way.

done

[Sebastian Noack, 2018/08/27 20:15:11]

Yes, the line may have up to 80 characters, not more, exactly 80 is fine.

+        executeScript(filter.script, sender.page.id, sender.frame.id).then(() =>
         {
           let tabIds = [sender.page.id];
           if (filter)
             FilterNotifier.emit("filter.hitCount", filter, 0, 0, tabIds);
 
           logRequest(tabIds, {
             url: sender.frame.url.href,
             type: "SNIPPET",
             docDomain
           }, filter);
@@ skipping 38 matching lines @@
   updateFrameStyles(sender.page.id, sender.frame.id, message.selectors,
                     message.groupName, message.appendOnly);
 });
 
 fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"})
 .then(response => response.ok ? response.text() : "")
 .then(text =>
 {
   snippetsLibrarySource = text;
 });