Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#6423 closed change (fixed)

Observe only relevant mutation types in elemHideEmulation.js

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

https://codereview.adblockplus.org/29585589/

Description

Background

Currently in lib/content/elemHideEmulation.js (adblockpluscore) we observe all types of DOM mutations. We can improve the performance here by observing only those types of mutations that are relevant to the document. For example, if only the filter -abp-contains(Hello) applies to the document, there's no need to listen for attribute changes; similarly if the only filter is -abp-has(#hello), there's no need to observe character data changes.

What to change

Add properties like dependsOnAttributes and dependsOnCharacterData to the selector classes and observe the specific types of DOM mutations only if the respective values are true.

Change History (8)

comment:1 Changed 21 months ago by mjethani

  • Review URL(s) modified (diff)

comment:2 Changed 21 months ago by abpbot

A commit referencing this issue has landed:
Issue 6423 - Observe only relevant mutation types

comment:3 Changed 21 months ago by mjethani

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

comment:4 Changed 21 months ago by mjethani

@kzar I have closed this as fixed. Is the milestone also supposed to be Adblock-Plus-for-Chrome-Opera-Firefox-next?

comment:5 Changed 21 months ago by kzar

No since this change is to adblockpluscore. The dependency update issue which includes this change would have the milestone (and the testing hints).

comment:6 Changed 21 months ago by mjethani

@kzar thanks, I suspected as much.

Now since this change isn't strictly necessary, should there be a dependency update issue for this one specifically, or does it get included whenever there's a dependency update for some other reason?

If it is the latter, that dependency update issue would cover testing hints for this change as well?

comment:7 Changed 21 months ago by kzar

Well it's up to you if you want to update the dependency now or not.

Yes, that's why we have to be careful when filing dependency update issues to look at all the touched files and write useful instructions so that any potentially broken functionality is checked. That's why I'm always nagging people about it, since regressions slip through otherwise.

comment:8 Changed 21 months ago by mjethani

@kzar makes sense, thanks.

Note: See TracTickets for help on using tickets.