Opened 17 months ago

Closed 3 weeks ago

#6665 closed change (rejected)

Share functionality and optimizations between ElemHide and ElemHideEmulation

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
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 (19)

comment:1 follow-up: Changed 17 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 16 months ago by mjethani

  • Description modified (diff)

comment:3 in reply to: ↑ 1 Changed 16 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 16 months ago by arthur

  • Cc arthur added

comment:5 Changed 16 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 16 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 16 months ago by mjethani

  • Blocked By 6652 removed

comment:8 Changed 16 months ago by mjethani

  • Blocking 6538 added

comment:9 Changed 16 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 16 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 16 months ago by kzar

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

comment:12 Changed 16 months ago by jsonesen

  • Status changed from new to reviewing

comment:13 Changed 16 months ago by mjethani

  • Owner set to mjethani

comment:14 Changed 14 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 14 months ago by mjethani

  • Blocking 6842 added

comment:16 Changed 13 months ago by abpbot

comment:17 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:18 Changed 11 months 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

comment:19 Changed 3 weeks ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reviewing to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.