Opened 3 years ago

Closed 21 months ago

#5142 closed change (fixed)

[emscripten] Convert element hiding container to C++

Reported by: trev Assignee: hfiguiere
Priority: P3 Milestone:
Module: Core Keywords:
Cc: oleksandr Blocked By:
Blocking: #4122, #5144 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29587914/

Description (last modified by trev)

Background

See #4122 for the rationale. For maximal performance, all operations involving filter text should stay within Emscripten - including elemHide container.

What to change

  • Implement ElemHide namespace (alternatively: class with default instance, like in #5141) exposing methods clear, add and remove to manage list of filters.
  • Methods getUnconditionalSelectors and getSelectorsForDomain should return a _ElemHide_SelectorList object which is internally a list of filter references. This object should expose a selectorCount property as well as selectorAt and filterKeyAt methods. This approach makes getUnconditionalFilterKeys method unnecessary.
  • Method getFilterByKey should allow retrieving a filter by its key. So the key needs to have the following properties:
    • it should uniquely identify a filter
    • invalid keys should be rejected
    • conclusion from the key to the filter (or from multiple keys to the relation between the corresponding filters) shouldn't be possible
    • ideally, keys shouldn't require additional data storage but rather be calculated from existing data using a randomized secret value
  • Methods getSelectors and getException are unused and shouldn't be reimplemented.

Notes

Implementing filter keys is optional given that these are currently only used by the legacy Firefox codebase.

Change History (9)

comment:1 Changed 3 years ago by trev

  • Description modified (diff)

comment:2 Changed 3 years ago by trev

  • Description modified (diff)

comment:3 Changed 3 years ago by trev

  • Blocking 4122 added

comment:4 Changed 3 years ago by trev

  • Blocking 5144 added

comment:5 Changed 2 years ago by hfiguiere

  • Owner set to hfiguiere

comment:6 Changed 2 years ago by oleksandr

  • Cc oleksandr added

comment:7 Changed 2 years ago by hfiguiere

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

comment:8 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 5142 - Convert Element Hiding to C++

comment:9 Changed 21 months ago by hfiguiere

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.