Opened 3 months ago

Closed 3 months ago

Last modified 8 weeks ago

#6781 closed change (fixed)

Implement basic support for snippet filters

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: circumvention
Cc: kzar, hfiguiere, rscott, Ross Blocked By: #6689
Blocking: #6538, #6782, #6790, #6791, #6798, #6803 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29737558/
https://codereview.adblockplus.org/29761597/
https://codereview.adblockplus.org/29761612/
https://codereview.adblockplus.org/29824589/
https://codereview.adblockplus.org/29827633/
https://codereview.adblockplus.org/29829569/
https://codereview.adblockplus.org/29837555/

Description (last modified by mjethani)

Background

#6538 describes support for a new feature called "Snippets" in the Core module. We want to ship a basic version of this initially and develop it further as we learn from experience.

The initial implementation should have the following limitations:

  • Similar to element hiding emulation, if there are too many snippet filters then it won't perform well (this will be resolved by #6665)
  • It will not be possible to specify exceptions for snippet filters
  • Only snippet filters from our own "circumvention" list will be parsed, such filters from other lists will be ignored
  • The snippets library will be shipped statically with the extension and will not be updated periodically
  • Generic snippet filters will be allowed (#6797)

What to change

In lib/filterClasses.js:

  • Rename ElemHideBase to ContentFilter and the selector property to body. In the ContentFilter constructor, do not do any CSS escaping.
  • Create a new class ElemHideBase that extends from ContentFilter. Add a selector property getter that return the value of the superclass's body property, after applying CSS escaping.
  • Rename elemhideRegExp to contentRegExp.
  • Create a new SnippetFilter class that extends from ContentFilter. Add a script property getter that returns the value of the superclass's body property.

Create a new lib/snippets.js file that contains a new Snippets module, modeled on ElemHideEmulation in lib/elemHideEmulation.js. It should have a getScriptsForDomain(domain) function that returns all the snippet scripts that apply on the given domain, ignoring any exceptions.

In lib/filterListener.js, add and remove any snippet filters (instances of SnippetFilter) to the new Snippets module.

In the new lib/snippets.js:

  • Implement a parseScript(script) function that returns an array of arrays containing the "commands" (snippet calls) to execute, with the arguments to each command, after parsing the given script based on the specification in #6538.
  • Implement a compileScript(script, libraries) function that takes a script and a list of libraries (in source form) and returns executable JavaScript code, which includes the library sources followed by calls to one or more snippet functions with zero or more arguments based on the given script.

Add lib/content/snippets.js as the initial snippets library containing one simple function that logs its arguments to the console.

In lib/filterListener.js, add a snippet filter only if it belongs to a subscription of type circumvention (check the type property of the Subscription object (#6689)).

In lib/content/snippets.js, implement an API such that a snippet can be executed in either the content script's context (default) or the document's context. This would ideally be a function that wraps the snippet into an "injector" that is then exported in place of the snippet. When the injector is called, it should forward the call to the snippet, but in the form of a script injected directly into the document. The technique for injecting the script into the document should be copied from inject.preload.js in the adblockpluschrome repo.

Hints for testers

These changes affect element hiding and element hiding emulation. Make sure these features work as they did before. In particular, check that CSS escaping (#4684) works.

For Snippets, make sure that snippet filters, as described in #6538, are being parsed, compiled, and executed (via the web extension) correctly. This may be done using the log snippet, which logs its arguments to the DevTools console for the page. For example, the filter example.com#$#log 'Hello, world!' should log the string "Hello, world!" to the console on example.com. Make sure quoting and escaping within the snippet filter, as described in #6538, work correctly.

Also make sure that snippet filters are accepted only from the "circumvention" filter list (#6689) or the user's own filters. Any snippet filters in any other list (e.g. EasyList) should be ignored silently.

Known issues

#6811 is a known issue and will be addressed in a future version.

Change History (24)

comment:1 Changed 3 months ago by mjethani

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

comment:2 Changed 3 months ago by mjethani

  • Summary changed from Implement basic version of the Snippets feature to Implement basic support for snippet filters

comment:3 Changed 3 months ago by mjethani

  • Blocking 6782 added

comment:4 Changed 3 months ago by hfiguiere

  • Cc hfiguiere added

comment:5 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:6 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 3 months ago by mjethani

  • Sensitive set

comment:8 Changed 3 months ago by mjethani

  • Blocking 6790 added

comment:9 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)

comment:10 Changed 3 months ago by mjethani

  • Blocking 6791 added

comment:11 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 3 months ago by mjethani

  • Blocking 6798 added

comment:13 Changed 3 months ago by mjethani

  • Blocked By 6689 added
  • Description modified (diff)

comment:14 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 3 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:17 Changed 3 months ago by kzar

  • Description modified (diff)

Thanks, this is a really well written issue.

The testing instructions are good too. The only thing I could see to add was a concrete example of how to use the log snippet (did I get that right?).

comment:18 Changed 3 months ago by mjethani

  • Description modified (diff)

I've updated the example slightly, thanks for adding it.

comment:19 Changed 3 months ago by mjethani

  • Review URL(s) modified (diff)

comment:20 Changed 3 months ago by mjethani

  • Blocking 6803 added

comment:21 Changed 3 months ago by kzar

(Last week.)

A commit referencing this issue has landed:
Issue 6538, 6781 - Minimize access to ElemHideBase's selector property

comment:22 Changed 3 months ago by mjethani

It seems all the commits are logged in #6538 because it appears first in the commit message.

comment:23 Changed 2 months ago by mjethani

  • Cc rscott Ross added
  • Description modified (diff)

comment:24 Changed 8 weeks ago by sebastian

  • Sensitive unset
Note: See TracTickets for help on using tickets.