Use underscore prefixes for internal properties

[mjethani]

Background

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

[TBD]

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.

[jsonesen]

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

[abpbot]

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

[abpbot]

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

[jha2125]

spam

[sebastian]

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