Opened 2 years ago

Closed 13 months ago

Last modified 12 months ago

#6940 closed change (rejected)

Use underscore prefixes for internal properties

Reported by: mjethani Assignee:
Priority: P3 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: hfiguiere, jsonesen, kzar, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

Description (last modified by mjethani)


In lib/matcher.js, the Matcher class has an internal property called keywordByFilter. Any other code using instances of this class has no business accessing this property; in fact, CombinedMatcher, which is in the same source file, doesn't even access this property itself but rather uses the getKeywordForFilter method. So clearly this property is supposed to be "private" or internal to the class.

Since we're moving to ES2015 classes (#6741), we should also update the code to clearly mark internal properties as internal in some way. There was an idea about using private symbols for this (#6424), which was rejected in the browser extension and would not be feasible for core anyway due to performance. Instead we should just stick with the common convention of prefixing the name of the property with an underscore (_).

There are good reasons for keeping an object's internal state private to the object itself (e.g. #6916).

An internal property of an object should:

  1. Have a name beginning with an underscore (_)
  2. If a JSDoc comment is present, it must include the @private tag
  3. The property should not be accessed from outside the object's own constructor and methods (we might have to update our ESLint config for this)

Some properties must be accessed directly from within the same source file for performance reasons, yet they are not meant to be used from other code; in such cases, let's just mark the property @protected instead and use no underscore.

What to change


Integration notes

With changeset 7126b8cc2fad, the blacklist and whitelist properties of CombinedMatcher are now "private". They can still be accessed as before using their new names starting with an underscore, but if there's any need to do this, please file an issue and let us know the use case and we'll try to provide a proper API for whatever you're trying to do. After changeset e8254db792d0 it should no longer be necessary to access the whitelist property directly since the implementation of CombinedMatcher's matchesAny method is now optimized for $document, $elemhide, $generichide, and $genericblock.

In adblockpluschrome, please update lib/whitelisting.js and lib/filterComposer.js to use the matchesAny method of defaultMatcher directly.

Hints for testers

With changeset 7126b8cc2fad we have renamed some internal properties, but all of this is covered by unit tests. There is no change in the implementation (so there's nothing to test). The dependency update for adblockpluschrome will require some changes to the code there, please see the relevant dependency update ticket for testing hints.

Change History (12)

comment:1 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:2 Changed 2 years ago by jsonesen

  • Owner set to jsonesen

comment:3 Changed 2 years ago by jsonesen

  • Owner jsonesen deleted

I get the feeling this is more of a meta ticket and may include multiple commits, so I unassigned myself.

Last edited 2 years ago by jsonesen (previous) (diff)

comment:4 Changed 2 years ago by jsonesen

  • Review URL(s) modified (diff)

comment:5 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 6940 - Use underscore prefixes in lib/matcher.js

comment:6 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:7 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:8 Changed 23 months ago by abpbot

Last edited 23 months ago by jsonesen (previous) (diff)

comment:9 Changed 20 months ago by abpbot

A commit referencing this issue has landed:
Issue 6940 - Use underscore prefixes in lib/matcher.js

comment:10 Changed 19 months ago by ukacar

  • Verified working set

comment:11 Changed 14 months ago by jha2125


Last edited 12 months ago by kzar (previous) (diff)

comment:12 Changed 13 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new 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.