Opened 7 months ago

Last modified 7 weeks ago

#6665 reviewing change

Share functionality and optimizations between ElemHide and ElemHideEmulation

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: kzar, hfiguiere, arthur Blocked By:
Blocking: #6538, #6842 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29783618/
https://codereview.adblockplus.org/29784555/

Description (last modified by mjethani)

Background

We are doing some optimizations to element hiding emulation to make it perform better (hide elements as soon as they appear, don't waste CPU, etc.) (see #6437). We have also recently done some optimizations to element hiding selector lookups, while some others are on the way (see #6562, #6652). It may be possible now to merge the code in lib/elemHide.js and lib/elemHideEmulation.js so that ElemHideEmulation can have all the functionality of ElemHide, including the optimizations.

Once this is done, both ElemHide and ElemHideEmulation as well as the upcoming Snippets module (#6538) will be able to do fast lookups of CSS selectors, emulation rules, and snippet scripts.

What to change

Split out code that is related to exceptions into its own lib/elemHideExceptions.js file and export the functionality as a new ElemHideExceptions module.

In lib/elemHide.js, separate out code that is common to both ElemHide and ElemHideEmulation, including the data structures, into a new ElemHideTemplate object. Make both ElemHide and ElemHideEmulation instances of this "class" or "template", with their own functions added on top (e.g. ElemHide.getSelectorsForDomain and ElemHideEmulation.getRulesForDomain).

Both ElemHide and ElemHideEmulation should use the same logic via ElemHideTemplate but have their own copies of the data structures that are used for bookkeeping and optimization. Exceptions should be shared between the two via the new ElemHideExceptions object.

Hints for testers

After changeset 3fa9d74b5e3f, element hiding exceptions should work correctly as before. For example, given two filters example.com##.foo and www.example.com#@#.foo, the element <div class="foo">Ad</div> should be hidden on example.com but should remain visible on www.example.com. Excluding domains in a filter should work too; for example, given only the filter example.com,~www.example.com##.foo, the element <div class="foo">Ad</div> should be hidden on example.com but remain visible on www.example.com.

Change History (18)

comment:1 follow-up: Changed 7 months ago by mjethani

@kzar, @hfiguiere does this sound like a good idea?

Right now a filter like ~cool.example.com,example.com#?#div:-abp-contains(Ad) will still apply on cool.example.com even though it shouldn't.

comment:2 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:3 in reply to: ↑ 1 Changed 7 months ago by mjethani

Replying to mjethani:

Right now a filter like ~cool.example.com,example.com#?#div:-abp-contains(Ad) will still apply on cool.example.com even though it shouldn't.

My bad, this already works.

comment:4 Changed 7 months ago by arthur

  • Cc arthur added

comment:5 Changed 7 months ago by kzar

It sounds like a good idea to me, if we can reuse ElemHide.getSelectorsForDomain for the emulation hiding filters then why not? It's fast and well tested. That said I'm not as familiar with the emulation hiding code, so I'm not sure what this would look like in practice.

comment:6 Changed 7 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 7 months ago by mjethani

  • Blocked By 6652 removed

comment:8 Changed 7 months ago by mjethani

  • Blocking 6538 added

comment:9 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Merge ElemHideEmulation into ElemHide to Share functionality and optimizations between ElemHide and ElemHideEmulation

comment:10 Changed 7 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

@kzar since you agree that doing at least some work towards this is a good idea, I'm setting this to Ready. I also think that it would be better to do this first so the Snippets implementation can benefit from it.

comment:11 Changed 7 months ago by kzar

Yes, you're free to triage issues in the Core module now.

comment:12 Changed 6 months ago by jsonesen

  • Status changed from new to reviewing

comment:13 Changed 6 months ago by mjethani

  • Owner set to mjethani

comment:14 Changed 5 months ago by mjethani

This is the only issue that will keep us from making full use of element hiding emulation, we should make it a priority.

comment:15 Changed 4 months ago by mjethani

  • Blocking 6842 added

comment:16 Changed 4 months ago by abpbot

comment:17 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:18 Changed 7 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.