[adblockpluschrome] Issue 3596 - Added support for CSS property filters to devtools panel

lib/devtools.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
-let {

[Sebastian Noack, 2017/01/11 16:20:41]

I think I would prefer to wrap this line like this:

  let {RegExpFilter, WhitelistFilter,
       ElemHideFilter, ElemHideEmulationFilter} = require("filterClasses");

Or like that:

  let {RegExpFilter,
       WhitelistFilter,
       ElemHideFilter,
       ElemHideEmulationFilter} = require("filterClasses");

But I guess this is mostly personal preference, though it is more concise and
looks a little more consistent with the surrounding one-line destructuring
assignments. So let me know if you disagree.

[wspee, 2017/01/12 13:39:58]

Done.

[Sebastian Noack, 2017/02/09 11:07:38]

It seems you haven't changed that code. But since you said "Done", it seems you
don't disagree, but it's rather an accident that this code still looks the same?

[wspee, 2017/02/10 10:46:46]

Done.

-  RegExpFilter,
-  WhitelistFilter,
-  ElemHideFilter,
-  ElemHideEmulationFilter
-} = require("filterClasses");
-
-let {SpecialSubscription} = require("subscriptionClasses");
-let {FilterStorage} = require("filterStorage");
-let {defaultMatcher} = require("matcher");
-let {FilterNotifier} = require("filterNotifier");
-let {extractHostFromFrame} = require("url");
-let {port} = require("messaging");
+const {RegExpFilter,
+       WhitelistFilter,
+       ElemHideFilter,
+       ElemHideEmulationFilter} = require("filterClasses");
+
+const {SpecialSubscription} = require("subscriptionClasses");
+const {FilterStorage} = require("filterStorage");
+const {defaultMatcher} = require("matcher");
+const {FilterNotifier} = require("filterNotifier");
+const {extractHostFromFrame} = require("url");
+const {port} = require("messaging");
 
 const nonRequestTypes = ["DOCUMENT", "ELEMHIDE", "GENERICBLOCK", "GENERICHIDE"];
 
 // Mapping of inspected tabs to their devpanel page
 // and recorded items. We can't use a PageMap here,
 // because data must persist after navigation/reload.
 let panels = Object.create(null);
 
 function hasPanels()
 {
@@ skipping 126 matching lines @@
   if (panel)
   {
     for (let subscription of FilterStorage.subscriptions)
     {
       if (subscription.disabled)
         continue;
 
       for (let filter of subscription.filters)
       {
         if (!(filter instanceof ElemHideFilter) &&
             !(filter instanceof ElemHideEmulationFilter))

[Sebastian Noack, 2017/01/11 16:20:41]

This logic could be slightly simplified (one less logical operation):

  if (!(filter instanceof ElemHideFilter ||
        filter instanceof ElemHideEmulationFilter))

I'm not sure which reads more natural though. I leave it up to you.

[wspee, 2017/01/12 13:39:58]

Done.

           continue;
         if (selectors.indexOf(filter.selector) == -1)
           continue;
         if (!filter.isActiveOnDomain(docDomain))
           continue;
 
         addRecord(panel, {type: "ELEMHIDE", docDomain: docDomain}, filter);
       }
     }
   }
@@ skipping 194 matching lines @@
   panels[inspectedTabId] = {port: port, records: []};
 });
 
 port.on("devtools.traceElemHide", (message, sender) =>
 {
   logHiddenElements(
     sender.page, message.selectors,
     extractHostFromFrame(sender.frame)
   );
 });