Opened on 04/14/2017 at 07:40:24 PM

Closed on 01/29/2018 at 02:07:38 PM

#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.

Attachments (0)

Change History (9)

comment:1 Changed on 04/14/2017 at 07:41:22 PM by trev

  • Description modified (diff)

comment:2 Changed on 04/14/2017 at 08:05:23 PM by trev

  • Description modified (diff)

comment:3 Changed on 04/14/2017 at 08:05:50 PM by trev

  • Blocking 4122 added

comment:4 Changed on 04/15/2017 at 08:25:56 AM by trev

  • Blocking 5144 added

comment:5 Changed on 10/16/2017 at 07:05:51 PM by hfiguiere

  • Owner set to hfiguiere

comment:6 Changed on 10/19/2017 at 09:54:12 AM by oleksandr

  • Cc oleksandr added

comment:7 Changed on 10/25/2017 at 01:20:11 AM by hfiguiere

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

comment:8 Changed on 01/29/2018 at 02:06:09 PM by abpbot

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

comment:9 Changed on 01/29/2018 at 02:07:38 PM by hfiguiere

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

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from hfiguiere.
 
Note: See TracTickets for help on using tickets.