Opened on 05/11/2018 at 05:00:24 PM

Closed on 08/29/2019 at 05:43:52 PM

#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):

Description (last modified by mjethani)


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 and, the element <div class="foo">Ad</div> should be hidden on but should remain visible on Excluding domains in a filter should work too; for example, given only the filter,, the element <div class="foo">Ad</div> should be hidden on but remain visible on

Attachments (0)

Change History (19)

comment:1 follow-up: Changed on 05/11/2018 at 05:03:49 PM by mjethani

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

Right now a filter like, will still apply on even though it shouldn't.

comment:2 Changed on 05/13/2018 at 02:13:51 PM by mjethani

  • Description modified (diff)

comment:3 in reply to: ↑ 1 Changed on 05/13/2018 at 05:21:39 PM by mjethani

Replying to mjethani:

Right now a filter like, will still apply on even though it shouldn't.

My bad, this already works.

comment:4 Changed on 05/14/2018 at 08:46:25 AM by arthur

  • Cc arthur added

comment:5 Changed on 05/14/2018 at 05:04:59 PM 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 on 05/17/2018 at 05:52:37 AM by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed on 05/17/2018 at 05:53:29 AM by mjethani

  • Blocked By 6652 removed

comment:8 Changed on 05/17/2018 at 05:53:47 AM by mjethani

  • Blocking 6538 added

comment:9 Changed on 05/17/2018 at 06:05:53 AM by mjethani

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

comment:10 Changed on 05/17/2018 at 06:09:02 AM 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 on 05/17/2018 at 11:52:18 AM by kzar

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

comment:12 Changed on 06/04/2018 at 08:26:45 PM by jsonesen

  • Status changed from new to reviewing

comment:13 Changed on 06/05/2018 at 04:42:32 AM by mjethani

  • Owner set to mjethani

comment:14 Changed on 07/26/2018 at 01:59:53 PM 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 on 08/08/2018 at 04:09:00 PM by mjethani

  • Blocking 6842 added

comment:16 Changed on 08/15/2018 at 07:27:47 PM by abpbot

comment:17 Changed on 08/28/2018 at 08:24:00 AM by mjethani

  • Description modified (diff)

comment:18 Changed on 10/24/2018 at 07:48:02 AM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

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

comment:19 Changed on 08/29/2019 at 05:43:52 PM 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.

Add Comment

Modify Ticket

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