Issue 5781 - Merge messages for regular and emulated element hiding filters

lib/cssInjection.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 26 matching lines @@
     cssOrigin: "user",
     frameId,
     matchAboutBlank: true,
     runAt: "document_start"
   });
 }
 
 port.on("elemhide.getSelectors", (msg, sender) =>
 {
   let selectors = [];
-  let emulated = [];

[Manish Jethani, 2017/09/26 11:50:18]

How about we call this emulationPatterns, similar to how it's referred to in the
content script? "emulated" being an adjective, it feels more like a boolean
value to me than an array of objects.

[Sebastian Noack, 2017/09/26 21:50:40]

Done.

+  let emulatedPatterns = [];
   let trace = devtools && devtools.hasPanel(sender.page);
   let inject = !userStyleSheetsSupported;
 
   if (!checkWhitelisted(sender.page, sender.frame,
                         RegExpFilter.typeMap.DOCUMENT |
                         RegExpFilter.typeMap.ELEMHIDE))
   {
     let hostname = extractHostFromFrame(sender.frame);

[Manish Jethani, 2017/09/26 11:50:18]

I notice that this is a change in behavior, the old one just uses
sender.frame.url.hostname, maybe we should add this information to the Trac
issue.

[Sebastian Noack, 2017/09/26 21:50:40]

You are right, and extractHostFromFrame() should be used, the old behavior was
wrong. The difference is that if the message got sent from within a frame with
dynamically constructed content, the URL will be "about:blank" and the hostname
will be "", essentially only matching generic filters (emulation filters cannot
be generic). Instead we must fall back to the parent frame's hostname, this is
what extractHostFromFrame() is doing. I updated the issue.

     let specificOnly = checkWhitelisted(sender.page, sender.frame,
                                         RegExpFilter.typeMap.GENERICHIDE);
 
     selectors = ElemHide.getSelectorsForDomain(
       hostname,
       specificOnly ? ElemHide.SPECIFIC_ONLY : ElemHide.ALL_MATCHING
     );
 
     for (let filter of ElemHideEmulation.getRulesForDomain(hostname))
-      emulated.push({selector: filter.selector, text: filter.text});

[Manish Jethani, 2017/09/26 11:50:18]

Just an idea, this could be written idiomatically as:

  emulationPatterns = ElemHideEmulation.getRulesForDomain(hostname)
                      .map(({selector, text}) => ({selector, text}));

[Sebastian Noack, 2017/09/26 21:50:40]

This will result in less efficient code for two reasons:

1. We create yet another object (array), even if there are no emulation filters.
That is also true for regular element hiding, but avoiding it there isn't
possible without additional boilerplate, which probably isn't worth it.
2. map() has to call from native code back into JS for each item, and the JIT
compiler cannot inline these calls.

There is no need to go crazy about micro optimizations here, but the more
efficient approach is IMO also more readable, but defiantly not any less.

[Manish Jethani, 2017/09/26 22:45:18]

Fair enough.

I just checked on both Node.js and Chrome and map does in fact take about twice
as long to execute, while on Firefox it performs just as well as let...of.

+      emulatedPatterns.push({selector: filter.selector, text: filter.text});
   }
 
   if (!inject && selectors.length > 0)
     hideElements(sender.page.id, sender.frame.id, selectors);
 
-  let response = {trace, inject, emulated};

[Manish Jethani, 2017/09/26 11:50:18]

Maybe not as part of this very change, but we should consider not passing the
emulation patterns to the content script at all unless necessary. If user style
sheets are supported (which will soon be true for Chrome as well!), we should
just inject the selectors here and then pass the emulation patterns to the
content script only if trace is true.

  if (!inject && (selectors.length > 0 || emulationPatterns.length > 0)
    hideElements(sender.page.id, sender.frame.id, selectors, emulationPatterns);

  ...
  if (trace || inject)
  {
    response.selectors = selectors;
    response.emulationPatterns = emulationPatterns;
  }

This would also simplify the code in the content script, plus we could get rid
of "elemhide.injectSelectors"

[Sebastian Noack, 2017/09/26 21:50:40]

It seems you misunderstand the concept of emulation filters. These filters
provide non-standard CSS features implemented by our content scripts, therefore
the content script must process them regardless whether user stylesheets are
supported or not. In some cases the content script will transfrom them (based on
what is in the DOM) to compatible selectors which are then sent back to the
background page through elemhide.injectSelectors message if user stylesheets are
supported, while more advanced emulation filters will cause the elements to be
hidden directly through the DOM.

[Manish Jethani, 2017/09/26 22:45:18]

Yes, it seems I misunderstood. I was thinking we could just do "new
ElemHideEmulation(...).apply(...)" in the background page, but apparently that
won't work.

+  let response = {trace, inject, emulatedPatterns};
   if (trace || inject)
     response.selectors = selectors;
 
   return response;
 });
 
 port.on("elemhide.injectSelectors", (msg, sender) =>
 {
   hideElements(sender.page.id, sender.frame.id, msg.selectors);
 });